confusing code....whats the point of this construct ?

confusing code....whats the point of this construct ?

[Nicholas Mc Guire]

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

confusing code....whats the point of this construct ?

[Valdis.Kletnieks at vt.edu]

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...
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150311/9e0390e5/attachment.bin 

confusing code....whats the point of this construct ?

[Nicholas Mc Guire]

On Wed, 11 Mar 2015, Valdis.Kletnieks at 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

confusing code....whats the point of this construct ?

[Bjørn Mork]

Valdis.Kletnieks at 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

confusing code....whats the point of this construct ?

[Malte Vesper]

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 at 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 at kernelnewbies.org
> http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150311/8d5ad704/attachment.html 

confusing code....whats the point of this construct ?

[Nicholas Mc Guire]

On Wed, 11 Mar 2015, Bj??rn Mork wrote:

> Valdis.Kletnieks at 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

confusing code....whats the point of this construct ?

[Valdis.Kletnieks at vt.edu]

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....
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150311/ed4cdeae/attachment.bin 

confusing code....whats the point of this construct ?

[Jeff Haran]

-----Original Message-----
From: kernelnewbies-bounces@kernelnewbies.org [mailto:kernelnewbies-bounces at kernelnewbies.org] On Behalf Of Valdis.Kletnieks at vt.edu
Sent: Wednesday, March 11, 2015 10:00 AM
To: Nicholas Mc Guire
Cc: Bj??rn Mork; kernelnewbies at 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

confusing code....whats the point of this construct ?

[Nicholas Krause]

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 at kernelnewbies.org] On Behalf Of
>Valdis.Kletnieks at vt.edu
>Sent: Wednesday, March 11, 2015 10:00 AM
>To: Nicholas Mc Guire
>Cc: Bj??rn Mork; kernelnewbies at 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 at kernelnewbies.org
>http://lists.kernelnewbies.org/mailman/listinfo/kernelnewbies

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

confusing code....whats the point of this construct ?

[Nicholas Mc Guire]

On Wed, 11 Mar 2015, Jeff Haran wrote:

> -----Original Message-----
> From: kernelnewbies-bounces at kernelnewbies.org [mailto:kernelnewbies-bounces at kernelnewbies.org] On Behalf Of Valdis.Kletnieks at vt.edu
> Sent: Wednesday, March 11, 2015 10:00 AM
> To: Nicholas Mc Guire
> Cc: Bj??rn Mork; kernelnewbies at 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 

confusing code....whats the point of this construct ?

[Valdis.Kletnieks at vt.edu]

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.

-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 848 bytes
Desc: not available
Url : http://lists.kernelnewbies.org/pipermail/kernelnewbies/attachments/20150311/0de6ac06/attachment.bin 

confusing code....whats the point of this construct ?

[Jeff Haran]

-----Original Message-----
From: Valdis.Kletnieks@vt.edu [mailto:Valdis.Kletnieks at vt.edu] 
Sent: Wednesday, March 11, 2015 11:58 AM
To: Jeff Haran
Cc: Nicholas Mc Guire; Bj??rn Mork; kernelnewbies at 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

confusing code....whats the point of this construct ?

[Bjørn Mork]

Valdis.Kletnieks at 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