Regression with wait_event_timeout in next-20140226

Regression with wait_event_timeout in next-20140226

[Gregory CLEMENT]

Hi,

while testing next-20140226 I got an issue with the function
wait_event_timeout. When this function timed out instead of returning
0, it returned the value of the timeout passed in parameter. I found
that reverting "sched/wait: Suppress Sparse 'variable shadowing'
warning" fixed this regression.

I got this issue in the driver drivers/i2c/busses/i2c-mv64xxx.c.


Thanks,

Gregory

-- 
Gregory Clement, Free Electrons
Kernel, drivers, real-time and embedded Linux
development, consulting, training and support.
http://free-electrons.com

Re: Regression with wait_event_timeout in next-20140226

[Peter Zijlstra]

On Wed, Feb 26, 2014 at 05:35:19PM +0100, Gregory CLEMENT wrote:
> Hi,
> 
> while testing next-20140226 I got an issue with the function
> wait_event_timeout. When this function timed out instead of returning
> 0, it returned the value of the timeout passed in parameter. I found
> that reverting "sched/wait: Suppress Sparse 'variable shadowing'
> warning" fixed this regression.
> 
> I got this issue in the driver drivers/i2c/busses/i2c-mv64xxx.c.

Ah indeed. We actually rely on the shadowing for ___wait_cond_timeout().

We further used the __ret variable in __wait_event_timeout()'s cmd
argument: __ret = schedule_timeout(__ret). That now explicitly uses the
wrong __ret.

Yeah, we need to pull that patch.

Re: Regression with wait_event_timeout in next-20140226

[Andrew Morton]

On Wed, 26 Feb 2014 17:50:43 +0100 Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Feb 26, 2014 at 05:35:19PM +0100, Gregory CLEMENT wrote:
> > Hi,
> > 
> > while testing next-20140226 I got an issue with the function
> > wait_event_timeout. When this function timed out instead of returning
> > 0, it returned the value of the timeout passed in parameter. I found
> > that reverting "sched/wait: Suppress Sparse 'variable shadowing'
> > warning" fixed this regression.
> > 
> > I got this issue in the driver drivers/i2c/busses/i2c-mv64xxx.c.
> 
> Ah indeed. We actually rely on the shadowing for ___wait_cond_timeout().
> 
> We further used the __ret variable in __wait_event_timeout()'s cmd
> argument: __ret = schedule_timeout(__ret). That now explicitly uses the
> wrong __ret.
> 
> Yeah, we need to pull that patch.


Is there anything we can do to make all this clearer?  Simply using a
distinctive variable name ("__wait_var__"?) in place of __ret (and
documenting it) would help a lot.

Some __ret's are long and some are int.  Maybe that's a glitch, maybe
it's because some __ret's are used for inter-macro communications and
some are not, which just makes things worse.

I started to do a patch, got all confused and gave up.  We've made
quite a tangly mess in there, alas.

Re: Regression with wait_event_timeout in next-20140226

[Peter Zijlstra]

On Wed, Feb 26, 2014 at 02:25:34PM -0800, Andrew Morton wrote:
> Is there anything we can do to make all this clearer?  Simply using a
> distinctive variable name ("__wait_var__"?) in place of __ret (and
> documenting it) would help a lot.

% s/\<__ret\>/__wait_var__/g should get you mostly there I suppose :-)

Although I'm not entirely sure __wait_var__ is a better name.

> Some __ret's are long and some are int.  Maybe that's a glitch, 

No that's on purpose.

The longs are needed to hold the timeout values, we truncate to an int
where we only need to return errors.

> maybe
> it's because some __ret's are used for inter-macro communications and
> some are not, which just makes things worse.

The timeout related ones are the worst. The others aren't nearly as bad.

> I started to do a patch, got all confused and gave up.  We've made
> quite a tangly mess in there, alas.

Hehe, yes, made a lot of duplicated code go away though. Maybe we
compressed too much, dunno.

Re: Regression with wait_event_timeout in next-20140226

[Stephen Rothwell]

[-- Attachment #1: Type: text/plain, Size: 1018 bytes --]

Hi Peter,

On Wed, 26 Feb 2014 17:50:43 +0100 Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Feb 26, 2014 at 05:35:19PM +0100, Gregory CLEMENT wrote:
> > 
> > while testing next-20140226 I got an issue with the function
> > wait_event_timeout. When this function timed out instead of returning
> > 0, it returned the value of the timeout passed in parameter. I found
> > that reverting "sched/wait: Suppress Sparse 'variable shadowing'
> > warning" fixed this regression.
> > 
> > I got this issue in the driver drivers/i2c/busses/i2c-mv64xxx.c.
> 
> Ah indeed. We actually rely on the shadowing for ___wait_cond_timeout().
> 
> We further used the __ret variable in __wait_event_timeout()'s cmd
> argument: __ret = schedule_timeout(__ret). That now explicitly uses the
> wrong __ret.
> 
> Yeah, we need to pull that patch.

I have reverted that commit from linux-next for today pending some other
resolution.

-- 
Cheers,
Stephen Rothwell                    sfr@canb.auug.org.au

[-- Attachment #2: Type: application/pgp-signature, Size: 836 bytes --]

Re: Regression with wait_event_timeout in next-20140226

[Peter Zijlstra]

On Wed, Feb 26, 2014 at 02:25:34PM -0800, Andrew Morton wrote:
> Is there anything we can do to make all this clearer?  Simply using a
> distinctive variable name ("__wait_var__"?) in place of __ret (and
> documenting it) would help a lot.
> 
> Some __ret's are long and some are int.  Maybe that's a glitch, maybe
> it's because some __ret's are used for inter-macro communications and
> some are not, which just makes things worse.
> 
> I started to do a patch, got all confused and gave up.  We've made
> quite a tangly mess in there, alas.

Something like so?

---
Subject: wait: Explain the shadowing and type inconsistencies
From: Peter Zijlstra <peterz@infradead.org>
Date: Wed Apr  9 12:50:34 CEST 2014

Stick in a comment before someone else tries to fix the sparse warning
this generates.

Requested-by: Andrew Morton <akpm@linux-foundation.org>
Signed-off-by: Peter Zijlstra <peterz@infradead.org>
Link: http://lkml.kernel.org/n/tip-o2ro6f3vkxklni0bc8f7m68s@git.kernel.org
---
 include/linux/wait.h |   14 +++++++++++++-
 1 file changed, 13 insertions(+), 1 deletion(-)

--- a/include/linux/wait.h
+++ b/include/linux/wait.h
@@ -191,11 +191,23 @@ wait_queue_head_t *bit_waitqueue(void *,
 	(!__builtin_constant_p(state) ||				\
 		state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE)	\
 
+/*
+ * The below macro ___wait_event() has an explicit shadow of the __ret
+ * variable when used from the wait_event_*() macros.
+ *
+ * This is so that both can use the ___wait_cond_timeout() construct
+ * to wrap the condition.
+ *
+ * The type inconsistency of the wait_event_*() __ret variable is also
+ * on purpose; we use long where we can return timeout values and int
+ * otherwise.
+ */
+
 #define ___wait_event(wq, condition, state, exclusive, ret, cmd)	\
 ({									\
 	__label__ __out;						\
 	wait_queue_t __wait;						\
-	long __ret = ret;						\
+	long __ret = ret;	/* explicit shadow */			\
 									\
 	INIT_LIST_HEAD(&__wait.task_list);				\
 	if (exclusive)							\

Re: Regression with wait_event_timeout in next-20140226

[Andrew Morton]

On Wed, 9 Apr 2014 13:16:38 +0200 Peter Zijlstra <peterz@infradead.org> wrote:

> On Wed, Feb 26, 2014 at 02:25:34PM -0800, Andrew Morton wrote:
> > Is there anything we can do to make all this clearer?  Simply using a
> > distinctive variable name ("__wait_var__"?) in place of __ret (and
> > documenting it) would help a lot.
> > 
> > Some __ret's are long and some are int.  Maybe that's a glitch, maybe
> > it's because some __ret's are used for inter-macro communications and
> > some are not, which just makes things worse.
> > 
> > I started to do a patch, got all confused and gave up.  We've made
> > quite a tangly mess in there, alas.
> 
> Something like so?
> 
> ---
> Subject: wait: Explain the shadowing and type inconsistencies
> From: Peter Zijlstra <peterz@infradead.org>
> Date: Wed Apr  9 12:50:34 CEST 2014
> 
> Stick in a comment before someone else tries to fix the sparse warning
> this generates.
> 
> Requested-by: Andrew Morton <akpm@linux-foundation.org>
> Signed-off-by: Peter Zijlstra <peterz@infradead.org>
> Link: http://lkml.kernel.org/n/tip-o2ro6f3vkxklni0bc8f7m68s@git.kernel.org
> ---
>  include/linux/wait.h |   14 +++++++++++++-
>  1 file changed, 13 insertions(+), 1 deletion(-)
> 
> --- a/include/linux/wait.h
> +++ b/include/linux/wait.h
> @@ -191,11 +191,23 @@ wait_queue_head_t *bit_waitqueue(void *,
>  	(!__builtin_constant_p(state) ||				\
>  		state == TASK_INTERRUPTIBLE || state == TASK_KILLABLE)	\
>  
> +/*
> + * The below macro ___wait_event() has an explicit shadow of the __ret
> + * variable when used from the wait_event_*() macros.
> + *
> + * This is so that both can use the ___wait_cond_timeout() construct
> + * to wrap the condition.
> + *
> + * The type inconsistency of the wait_event_*() __ret variable is also
> + * on purpose; we use long where we can return timeout values and int
> + * otherwise.
> + */
> +
>  #define ___wait_event(wq, condition, state, exclusive, ret, cmd)	\
>  ({									\
>  	__label__ __out;						\
>  	wait_queue_t __wait;						\
> -	long __ret = ret;						\
> +	long __ret = ret;	/* explicit shadow */			\
>  									\
>  	INIT_LIST_HEAD(&__wait.task_list);				\
>  	if (exclusive)							\

Looks nice to me, thanks.