confusing code....whats the point of this construct ?
HI ! Trying to understand the intent of this code construct drivers/net/wireless/ath/ath10k/mac.c:ath10k_flush() <snip> ret = wait_event_timeout(ar->htt.empty_tx_wq, ({ bool empty; spin_lock_bh(&ar->htt.tx_lock); empty = (ar->htt.num_pending_tx == 0); spin_unlock_bh(&ar->htt.tx_lock); skip = (ar->state == ATH10K_STATE_WEDGED) || test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags); (empty || skip); }), ATH10K_FLUSH_TIMEOUT_HZ); <snip> FYI include/linux/wait.h:wait_event_timeout() <snip> * wait_event_timeout - sleep until a condition gets true or a timeout elapses * @wq: the waitqueue to wait on * @condition: a C expression for the event to wait for * @timeout: timeout, in jiffies <snip> So the wait_event_timeout condition here ends up being (empty || skip) but what is the point of puting this code into the parameter list of wait_event_timeout() ? Would it not be equivalent to: bool empty; ... spin_lock_bh(&ar->htt.tx_lock); empty = (ar->htt.num_pending_tx == 0); spin_unlock_bh(&ar->htt.tx_lock); skip = (ar->state == ATH10K_STATE_WEDGED) || test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags); ret = wait_event_timeout(ar->htt.empty_tx_wq, (empty || skip), ATH10K_FLUSH_TIMEOUT_HZ); What am I missing here ? thx! hofrat
On Wed, 11 Mar 2015 15:17:44 +0100, Nicholas Mc Guire said:
So the wait_event_timeout condition here ends up being (empty || skip) but what is the point of puting this code into the parameter list of wait_event_timeout() ?
Would it not be equivalent to:
bool empty; ...
spin_lock_bh(&ar->htt.tx_lock); empty = (ar->htt.num_pending_tx == 0); spin_unlock_bh(&ar->htt.tx_lock);
skip = (ar->state == ATH10K_STATE_WEDGED) || test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags);
ret = wait_event_timeout(ar->htt.empty_tx_wq, (empty || skip), ATH10K_FLUSH_TIMEOUT_HZ);
What am I missing here ?
Umm... a Signed-off-by: and formatting it as an actual patch? :) Seriously - you're right, it's ugly code that needs fixing...
On Wed, 11 Mar 2015, Valdis.Kletnieks@vt.edu wrote:
On Wed, 11 Mar 2015 15:17:44 +0100, Nicholas Mc Guire said:
So the wait_event_timeout condition here ends up being (empty || skip) but what is the point of puting this code into the parameter list of wait_event_timeout() ?
Would it not be equivalent to:
bool empty; ...
spin_lock_bh(&ar->htt.tx_lock); empty = (ar->htt.num_pending_tx == 0); spin_unlock_bh(&ar->htt.tx_lock);
skip = (ar->state == ATH10K_STATE_WEDGED) || test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags);
ret = wait_event_timeout(ar->htt.empty_tx_wq, (empty || skip), ATH10K_FLUSH_TIMEOUT_HZ);
What am I missing here ?
Umm... a Signed-off-by: and formatting it as an actual patch? :)
Seriously - you're right, it's ugly code that needs fixing...
thats what I thought too but it seemed to be intentional so I was just confused if it were some strange side-effect that I had not understood. thanks for the clarification ! hofrat
Valdis.Kletnieks@vt.edu writes:
On Wed, 11 Mar 2015 15:17:44 +0100, Nicholas Mc Guire said:
So the wait_event_timeout condition here ends up being (empty || skip) but what is the point of puting this code into the parameter list of wait_event_timeout() ?
Would it not be equivalent to:
bool empty; ...
spin_lock_bh(&ar->htt.tx_lock); empty = (ar->htt.num_pending_tx == 0); spin_unlock_bh(&ar->htt.tx_lock);
skip = (ar->state == ATH10K_STATE_WEDGED) || test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags);
ret = wait_event_timeout(ar->htt.empty_tx_wq, (empty || skip), ATH10K_FLUSH_TIMEOUT_HZ);
What am I missing here ?
Umm... a Signed-off-by: and formatting it as an actual patch? :)
Seriously - you're right, it's ugly code that needs fixing...
Huh? The condition needs to be re-evaluated every time the process wakes up. Evaluating it once and then reusing that result is not the same. Something elseis supposed to modify ar->htt.num_pending_tx, ar->state or ar->dev_flags while we are waiting. Bjørn
On Wed, 11 Mar 2015, Bj??rn Mork wrote:
Valdis.Kletnieks@vt.edu writes:
On Wed, 11 Mar 2015 15:17:44 +0100, Nicholas Mc Guire said:
So the wait_event_timeout condition here ends up being (empty || skip) but what is the point of puting this code into the parameter list of wait_event_timeout() ?
Would it not be equivalent to:
bool empty; ...
spin_lock_bh(&ar->htt.tx_lock); empty = (ar->htt.num_pending_tx == 0); spin_unlock_bh(&ar->htt.tx_lock);
skip = (ar->state == ATH10K_STATE_WEDGED) || test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags);
ret = wait_event_timeout(ar->htt.empty_tx_wq, (empty || skip), ATH10K_FLUSH_TIMEOUT_HZ);
What am I missing here ?
Umm... a Signed-off-by: and formatting it as an actual patch? :)
Seriously - you're right, it's ugly code that needs fixing...
Huh?
The condition needs to be re-evaluated every time the process wakes up. Evaluating it once and then reusing that result is not the same. Something elseis supposed to modify ar->htt.num_pending_tx, ar->state or ar->dev_flags while we are waiting.
after picking appart the mac.i file I see what you mean (a bit reformated to make it kind of readable) ret = ({ long __ret = (5*250); do { _cond_resched(); } while (0); if (!({ bool __cond = (({ bool empty; spin_lock_bh(&ar->htt.tx_lock); empty = (ar->htt.num_pending_tx == 0); spin_unlock_bh(&ar->htt.tx_lock); skip = (ar->state == ATH10K_STATE_WEDGED) || (__builtin_constant_p((ATH10K_FLAG_CRASH_FLUSH)) ? constant_test_bit((ATH10K_FLAG_CRASH_FLUSH), (&ar->dev_flags)) : variable_test_bit((ATH10K_FLAG_CRASH_FLUSH), (&ar->dev_flags))); (empty || skip); })); if (__cond && !__ret) __ret = 1; __cond || !__ret; })) __ret = ({ __label__ __out; wait_queue_t __wait; long __ret = (5*250); INIT_LIST_HEAD(&__wait.task_list); if (0) __wait.flags = 0x01; else __wait.flags = 0; for (;;) { long __int = prepare_to_wait_event(&ar->htt.empty_tx_wq, &__wait, 2); if (({ bool __cond = (({ bool empty; spin_lock_bh(&ar->htt.tx_lock); empty = (ar->htt.num_pending_tx == 0); spin_unlock_bh(&ar->htt.tx_lock); skip = (ar->state == ATH10K_STATE_WEDGED) || (__builtin_constant_p((ATH10K_FLAG_CRASH_FLUSH)) ? constant_test_bit((ATH10K_FLAG_CRASH_FLUSH), (&ar->dev_flags)) : variable_test_bit((ATH10K_FLAG_CRASH_FLUSH), (&ar->dev_flags))); (empty || skip); })); i if (__cond && !__ret) __ret = 1; __cond || !__ret; })) break; if ((!__builtin_constant_p(2) || 2 == 1 || 2 == (128 | 2)) && __int) { __ret = __int; if (0) { abort_exclusive_wait(&ar->htt.empty_tx_wq, &__wait, 2, ((void *)0)); goto __out; } break; } __ret = schedule_timeout(__ret); } finish_wait(&ar->htt.empty_tx_wq, &__wait); __out: __ret; }); __ret; }) So the for(;;){ ... } is the part I missed at first. but never the less the code should then pack up the inner block into a static function and pass it to wait_event_timeout rather than putting the basic block into the parameter list e.g. like in drivers/gpu/drm/drm_dp_mst_topology.c:drm_dp_mst_wait_tx_reply() static bool check_txmsg_state(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_sideband_msg_tx *txmsg) { bool ret; mutex_lock(&mgr->qlock); ret = (txmsg->state == DRM_DP_SIDEBAND_TX_RX || txmsg->state == DRM_DP_SIDEBAND_TX_TIMEOUT); mutex_unlock(&mgr->qlock); return ret; } drm_dp_mst_wait_tx_reply() <snip> ret = wait_event_timeout(mgr->tx_waitq, check_txmsg_state(mgr, txmsg), (4 * HZ)); <snip> which is readable and achieves the same purpose. something like the below quick shot: diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c index e8cc19f..7b27d99 100644 --- a/drivers/net/wireless/ath/ath10k/mac.c +++ b/drivers/net/wireless/ath/ath10k/mac.c @@ -4463,11 +4463,25 @@ static int ath10k_set_rts_threshold(struct ieee80211_hw *hw, u32 value) return ret; } +static bool check_htt_state (struct ath10k *ar, bool skip) +{ + bool empty; + + spin_lock_bh(&ar->htt.tx_lock); + empty = (ar->htt.num_pending_tx == 0); + spin_unlock_bh(&ar->htt.tx_lock); + + skip = (ar->state == ATH10K_STATE_WEDGED) || + test_bit(ATH10K_FLAG_CRASH_FLUSH, + &ar->dev_flags); + return (empty || skip); +} + static void ath10k_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif, u32 queues, bool drop) { struct ath10k *ar = hw->priv; - bool skip; + bool skip = false; int ret; /* mac80211 doesn't care if we really xmit queued frames or not @@ -4480,19 +4494,9 @@ static void ath10k_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif, if (ar->state == ATH10K_STATE_WEDGED) goto skip; - ret = wait_event_timeout(ar->htt.empty_tx_wq, ({ - bool empty; - - spin_lock_bh(&ar->htt.tx_lock); - empty = (ar->htt.num_pending_tx == 0); - spin_unlock_bh(&ar->htt.tx_lock); - - skip = (ar->state == ATH10K_STATE_WEDGED) || - test_bit(ATH10K_FLAG_CRASH_FLUSH, - &ar->dev_flags); - - (empty || skip); - }), ATH10K_FLUSH_TIMEOUT_HZ); + ret = wait_event_timeout(ar->htt.empty_tx_wq, + check_htt_state(ar, skip), + ATH10K_FLUSH_TIMEOUT_HZ); if (ret <= 0 || skip) ath10k_warn(ar, "failed to flush transmit queue (skip %i ar-state %i): %i\n", -- 1.7.10.4 not yet sure if its correct though just wrapped it into a function and compile checked it... and the if (ret <= 0 ... also needs fixing as wait_event_timeout returns >= 0 always. thanks for the clarification - think I got it this time. thx! hofrat
On Wed, 11 Mar 2015 17:46:56 +0100, Nicholas Mc Guire said:
ret = ({ long __ret = (5*250); do { _cond_resched(); } while (0); if (!({ bool __cond = (({
Gaak.
ret = wait_event_timeout(mgr->tx_waitq, check_txmsg_state(mgr, txmsg), (4 * HZ));
-EGADS - use of macro as a function violates the Principle of Least Surprise.... I have to wonder how many other places we've got bugs waiting to happen because of this....
-----Original Message----- From: kernelnewbies-bounces@kernelnewbies.org [mailto:kernelnewbies-bounces@kernelnewbies.org] On Behalf Of Valdis.Kletnieks@vt.edu Sent: Wednesday, March 11, 2015 10:00 AM To: Nicholas Mc Guire Cc: Bj??rn Mork; kernelnewbies@kernelnewbies.org Subject: Re: confusing code....whats the point of this construct ? On Wed, 11 Mar 2015 17:46:56 +0100, Nicholas Mc Guire said:
ret = ({ long __ret = (5*250); do { _cond_resched(); } while (0); if (!({ bool __cond = (({
Gaak.
ret = wait_event_timeout(mgr->tx_waitq, check_txmsg_state(mgr, txmsg), (4 * HZ));
-EGADS - use of macro as a function violates the Principle of Least Surprise....
I have to wonder how many other places we've got bugs waiting to happen because of this....
I don't understand the problem here. The caller passes in a condition to be evaluated in a loop. Many times that condition is quite simple (e.g. a counter being non-zero). If it was a function the caller would have to pass in a pointer to a function that does the evaluation, as in: int bar; int foo(void) { return bar; } ... wait_event_interruptible(..., foo, ...); Instead of the much simpler: wait_event_interruptible(..., bar, ...); That latter seems easier to understand and require fewer instructions to be generated since there is no function call overhead. Jeff Haran
On March 11, 2015 2:37:32 PM EDT, Jeff Haran <Jeff.Haran@citrix.com> wrote:
-----Original Message----- From: kernelnewbies-bounces@kernelnewbies.org [mailto:kernelnewbies-bounces@kernelnewbies.org] On Behalf Of Valdis.Kletnieks@vt.edu Sent: Wednesday, March 11, 2015 10:00 AM To: Nicholas Mc Guire Cc: Bj??rn Mork; kernelnewbies@kernelnewbies.org Subject: Re: confusing code....whats the point of this construct ?
On Wed, 11 Mar 2015 17:46:56 +0100, Nicholas Mc Guire said:
ret = ({ long __ret = (5*250); do { _cond_resched(); } while (0); if (!({ bool __cond = (({
Gaak.
ret = wait_event_timeout(mgr->tx_waitq, check_txmsg_state(mgr, txmsg), (4 * HZ));
-EGADS - use of macro as a function violates the Principle of Least Surprise....
I have to wonder how many other places we've got bugs waiting to happen because of this....
I don't understand the problem here. The caller passes in a condition to be evaluated in a loop. Many times that condition is quite simple (e.g. a counter being non-zero). If it was a function the caller would have to pass in a pointer to a function that does the evaluation, as in:
int bar;
int foo(void) { return bar; }
...
wait_event_interruptible(..., foo, ...);
Instead of the much simpler:
wait_event_interruptible(..., bar, ...);
That latter seems easier to understand and require fewer instructions to be generated since there is no function call overhead.
Jeff Haran
Due to complaints about this code., I am requesting a vote for me to send in a patch fixing this terribly ugly code. Please let me know about your results for the vote for me making a patch to fix this code. Nick
Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
-- Sent from my Android device with K-9 Mail. Please excuse my brevity.
On Wed, 11 Mar 2015, Jeff Haran wrote:
-----Original Message----- From: kernelnewbies-bounces@kernelnewbies.org [mailto:kernelnewbies-bounces@kernelnewbies.org] On Behalf Of Valdis.Kletnieks@vt.edu Sent: Wednesday, March 11, 2015 10:00 AM To: Nicholas Mc Guire Cc: Bj??rn Mork; kernelnewbies@kernelnewbies.org Subject: Re: confusing code....whats the point of this construct ?
On Wed, 11 Mar 2015 17:46:56 +0100, Nicholas Mc Guire said:
ret = ({ long __ret = (5*250); do { _cond_resched(); } while (0); if (!({ bool __cond = (({
Gaak.
ret = wait_event_timeout(mgr->tx_waitq, check_txmsg_state(mgr, txmsg), (4 * HZ));
-EGADS - use of macro as a function violates the Principle of Least Surprise....
I have to wonder how many other places we've got bugs waiting to happen because of this....
I don't understand the problem here. The caller passes in a condition to be evaluated in a loop. Many times that condition is quite simple (e.g. a counter being non-zero). If it was a function the caller would have to pass in a pointer to a function that does the evaluation, as in:
int bar;
int foo(void) { return bar; }
...
wait_event_interruptible(..., foo, ...);
Instead of the much simpler:
wait_event_interruptible(..., bar, ...);
That latter seems easier to understand and require fewer instructions to be generated since there is no function call overhead.
for simle conditions that is true and commonly done but for complex conditions that require locking or intermediate variables it is much more readable if packed up in a function like in e.g. see drivers/gpu/drm/drm_dp_mst_topology.c:drm_dp_mst_wait_tx_reply() static bool check_txmsg_state(struct drm_dp_mst_topology_mgr *mgr, struct drm_dp_sideband_msg_tx *txmsg) { bool ret; mutex_lock(&mgr->qlock); ret = (txmsg->state == DRM_DP_SIDEBAND_TX_RX || txmsg->state == DRM_DP_SIDEBAND_TX_TIMEOUT); mutex_unlock(&mgr->qlock); return ret; } drm_dp_mst_wait_tx_reply() <snip> ret = wait_event_timeout(mgr->tx_waitq, check_txmsg_state(mgr, txmsg), (4 * HZ)); <snip> which I find is much simpler to understand than the "inline" code in ath10k_flush(). thx! hofrat
On Wed, 11 Mar 2015 18:37:32 -0000, Jeff Haran said:
I don't understand the problem here. The caller passes in a condition to be evaluated in a loop. Many times that condition is quite simple (e.g. a counter being non-zero). If it was a function the caller would have to pass in a pointer to a function that does the evaluation, as in:
We do pointers to callback functions all the time. We try *really* hard to avoid anonymous lambda functions (which is basically what we have here). The problem here is that there's about 3 zillion ways to screw up the inline version, starting with the compiler optimizing the control variable into a hoisted load outside the loop and causing the test to always fail - note that the macro does *not* use any barriers or volatile or anything like that.
-----Original Message----- From: Valdis.Kletnieks@vt.edu [mailto:Valdis.Kletnieks@vt.edu] Sent: Wednesday, March 11, 2015 11:58 AM To: Jeff Haran Cc: Nicholas Mc Guire; Bj??rn Mork; kernelnewbies@kernelnewbies.org Subject: Re: confusing code....whats the point of this construct ? On Wed, 11 Mar 2015 18:37:32 -0000, Jeff Haran said:
I don't understand the problem here. The caller passes in a condition to be evaluated in a loop. Many times that condition is quite simple (e.g. a counter being non-zero). If it was a function the caller would have to pass in a pointer to a function that does the evaluation, as in:
We do pointers to callback functions all the time. We try *really* hard to avoid anonymous lambda functions (which is basically what we have here).
The problem here is that there's about 3 zillion ways to screw up the inline version, starting with the compiler optimizing the control variable into a >hoisted load outside the loop and causing the test to always fail - note that the macro does *not* use any barriers or volatile or anything like that.
Dealing with those issues has to be a problem for the caller to solve in whatever condition code he decides to pass in, not the implementation of the macro itself. I agree with Nicholas M. that it would have been easier to read had the condition been packaged into an inline function, but if whoever writes that inline function gets the optimization/barriers wrong then its broken regardless of whether it's written inline like it is now or packaged into a separate inline function. As for volatile, I had thought that the usage of volatile in kernel code was generally discouraged. This construct is all over the place in the kernel. Changing theses wait macros into functions would be a big lift. Jeff Haran
Valdis.Kletnieks@vt.edu writes:
On Wed, 11 Mar 2015 18:37:32 -0000, Jeff Haran said:
I don't understand the problem here. The caller passes in a condition to be evaluated in a loop. Many times that condition is quite simple (e.g. a counter being non-zero). If it was a function the caller would have to pass in a pointer to a function that does the evaluation, as in:
We do pointers to callback functions all the time. We try *really* hard to avoid anonymous lambda functions (which is basically what we have here).
The problem here is that there's about 3 zillion ways to screw up the inline version, starting with the compiler optimizing the control variable into a hoisted load outside the loop and causing the test to always fail - note that the macro does *not* use any barriers or volatile or anything like that.
We could go a couple of more rounds on this, but I don't think there is much point. It is sufficent to note that there are different views on the subject. None of them are "right" or "wrong". Use a function if you like. There are probably 3 zillion ways to screw up either way :-) Bjørn
Now I am confused. I thought the code where empty and skip are inside the wait_event_timeout leads to empty beeing evaluated every time that the waiting threads gets awoken. And since some other thread might change /ar->htt.num_pending_tx/ it is necessary to check this every time we get awoken, rather than once before we go to sleep. Also the locking part arround empty seems to support my guess (why sync if you do not have multiple threads accessing a variable). These are only guesses without looking at the surrounding code. Could you please explain why you think it is sufficient to evaluate the condition only once before sleeping on it? (empty || skip) is a constant if you do not update either empty or skip, at least from my point of view. On 11/03/15 14:40, Valdis.Kletnieks@vt.edu wrote:
On Wed, 11 Mar 2015 15:17:44 +0100, Nicholas Mc Guire said:
So the wait_event_timeout condition here ends up being (empty || skip) but what is the point of puting this code into the parameter list of wait_event_timeout() ?
Would it not be equivalent to:
bool empty; ...
spin_lock_bh(&ar->htt.tx_lock); empty = (ar->htt.num_pending_tx == 0); spin_unlock_bh(&ar->htt.tx_lock);
skip = (ar->state == ATH10K_STATE_WEDGED) || test_bit(ATH10K_FLAG_CRASH_FLUSH, &ar->dev_flags);
ret = wait_event_timeout(ar->htt.empty_tx_wq, (empty || skip), ATH10K_FLUSH_TIMEOUT_HZ);
What am I missing here ? Umm... a Signed-off-by: and formatting it as an actual patch? :)
Seriously - you're right, it's ugly code that needs fixing...
_______________________________________________ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
participants (6)
-
Bjørn Mork -
Jeff Haran -
Malte Vesper -
Nicholas Krause -
Nicholas Mc Guire -
Valdis.Kletnieks＠vt.edu