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

2015-03-12 Thread Bjørn Mork
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 ?

2015-03-12 Thread Valdis . Kletnieks
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 ?

2015-03-12 Thread Bjørn Mork
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 ?

2015-03-11 Thread Jeff Haran
-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 ?

2015-03-11 Thread Valdis . Kletnieks
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 ?

2015-03-11 Thread Nicholas Mc Guire
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 ?

2015-03-11 Thread Nicholas Krause


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 ?

2015-03-11 Thread Jeff Haran
-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 ?

2015-03-11 Thread Nicholas Mc Guire
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 ?

2015-03-11 Thread 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.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 ?

2015-03-11 Thread Nicholas Mc Guire
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 ?

2015-03-11 Thread Valdis . Kletnieks
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