Re: confusing code....whats the point of this construct ?
valdis.kletni...@vt.edu writes: > On Wed, 11 Mar 2015 18:37:32 -, 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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
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 pgpKXlKzWxjks.pgp Description: PGP signature ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: confusing code....whats the point of this construct ?
valdis.kletni...@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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
RE: confusing code....whats the point of this construct ?
-Original Message- From: valdis.kletni...@vt.edu [mailto:valdis.kletni...@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 codewhats the point of this construct ? On Wed, 11 Mar 2015 18:37:32 -, 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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: confusing code....whats the point of this construct ?
On Wed, 11 Mar 2015 18:37:32 -, 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. pgp3aFNq2myUk.pgp Description: PGP signature ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: confusing code....whats the point of this construct ?
On Wed, 11 Mar 2015, Jeff Haran wrote: > -Original Message- > From: kernelnewbies-boun...@kernelnewbies.org > [mailto:kernelnewbies-boun...@kernelnewbies.org] On Behalf Of > valdis.kletni...@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() ret = wait_event_timeout(mgr->tx_waitq, check_txmsg_state(mgr, txmsg), (4 * HZ)); which I find is much simpler to understand than the "inline" code in ath10k_flush(). thx! hofrat ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
RE: confusing code....whats the point of this construct ?
On March 11, 2015 2:37:32 PM EDT, Jeff Haran wrote: >-Original Message- >From: kernelnewbies-boun...@kernelnewbies.org >[mailto:kernelnewbies-boun...@kernelnewbies.org] On Behalf Of >valdis.kletni...@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. ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
RE: confusing code....whats the point of this construct ?
-Original Message- From: kernelnewbies-boun...@kernelnewbies.org [mailto:kernelnewbies-boun...@kernelnewbies.org] On Behalf Of valdis.kletni...@vt.edu Sent: Wednesday, March 11, 2015 10:00 AM To: Nicholas Mc Guire Cc: Bj??rn Mork; kernelnewbies@kernelnewbies.org Subject: Re: confusing codewhats 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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: confusing code....whats the point of this construct ?
On Wed, 11 Mar 2015, Bj??rn Mork wrote: > valdis.kletni...@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_SIDE
Re: confusing code....whats the point of this construct ?
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.kletni...@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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: confusing code....whats the point of this construct ?
On Wed, 11 Mar 2015, valdis.kletni...@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 ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies
Re: confusing code....whats the point of this construct ?
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... pgpwpqWJn4iCd.pgp Description: PGP signature ___ Kernelnewbies mailing list Kernelnewbies@kernelnewbies.org http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies