linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)

linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)

[Stephen Rothwell]

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

Hi all,

After merging the final tree, today's linux-next build (arm defconfig)
produced this warning:

fs/direct-io.c: In function 'sb_init_dio_done_wq':
fs/direct-io.c:557:2: warning: value computed is not used [-Wunused-value]

This is:

	cmpxchg(&sb->s_dio_done_wq, NULL, wq);

Introduced by commit 7b7a8665edd8 ("direct-io: Implement generic deferred
AIO completions").

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

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

Re: linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)

[Geert Uytterhoeven]

On Fri, Sep 6, 2013 at 9:19 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> After merging the final tree, today's linux-next build (arm defconfig)
> produced this warning:
>
> fs/direct-io.c: In function 'sb_init_dio_done_wq':
> fs/direct-io.c:557:2: warning: value computed is not used [-Wunused-value]
>
> This is:
>
>         cmpxchg(&sb->s_dio_done_wq, NULL, wq);
>
> Introduced by commit 7b7a8665edd8 ("direct-io: Implement generic deferred
> AIO completions").

This happens for include/asm-generic/cmpxchg.h and several other
arch-specific implementations that cast the return value of cmpxchg()
like

#define cmpxchg(ptr, o, n)   ((__typeof__(*(ptr)))__cmpxchg(....

If the caller of cmpxchg() doesn't use the return value, we get a
compiler warning,
at least with some versions of gcc.

Any idea how to fix this once and for good?

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)

[Olof Johansson]

On Fri, Sep 06, 2013 at 10:52:52AM +0200, Geert Uytterhoeven wrote:
> On Fri, Sep 6, 2013 at 9:19 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > After merging the final tree, today's linux-next build (arm defconfig)
> > produced this warning:
> >
> > fs/direct-io.c: In function 'sb_init_dio_done_wq':
> > fs/direct-io.c:557:2: warning: value computed is not used [-Wunused-value]
> >
> > This is:
> >
> >         cmpxchg(&sb->s_dio_done_wq, NULL, wq);
> >
> > Introduced by commit 7b7a8665edd8 ("direct-io: Implement generic deferred
> > AIO completions").
> 
> This happens for include/asm-generic/cmpxchg.h and several other
> arch-specific implementations that cast the return value of cmpxchg()
> like
> 
> #define cmpxchg(ptr, o, n)   ((__typeof__(*(ptr)))__cmpxchg(....
> 
> If the caller of cmpxchg() doesn't use the return value, we get a
> compiler warning,
> at least with some versions of gcc.
> 
> Any idea how to fix this once and for good?

Should it be fixed? Chances are that the caller needs to do actions
depending on if the change happened, and checking the value afterwards
is inherently racy.

For this specific fs/direct-io.c case it seems to be safe since the
workqueue is only ever set and never cleared, but it might still be a
good idea to do:


---8<----------8<----------8<----------8<----------8<----------8<----------8<--

>From 7fdfa2da727aa9153a7df919c240abfe4f564d7a Mon Sep 17 00:00:00 2001
From: Olof Johansson <olof@lixom.net>
Date: Sun, 8 Sep 2013 20:12:57 -0700
Subject: [PATCH] direct-io: Use return from cmpxchg to decide of assignment happened

Not using the return value can in the generic case be racy, so it's better to
use the expected way of using that instead of comparing later.

This also resolved the warning caused on ARM and other architectures:

fs/direct-io.c: In function 'sb_init_dio_done_wq':
fs/direct-io.c:557:2: warning: value computed is not used [-Wunused-value]

Signed-off-by: Olof Johansson <olof@lixom.net>
---
 fs/direct-io.c |    5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/fs/direct-io.c b/fs/direct-io.c
index 1782023..0e04142 100644
--- a/fs/direct-io.c
+++ b/fs/direct-io.c
@@ -544,6 +544,7 @@ static inline int dio_bio_reap(struct dio *dio, struct dio_submit *sdio)
  */
 static int sb_init_dio_done_wq(struct super_block *sb)
 {
+	struct workqueue_struct *old;
 	struct workqueue_struct *wq = alloc_workqueue("dio/%s",
 						      WQ_MEM_RECLAIM, 0,
 						      sb->s_id);
@@ -552,9 +553,9 @@ static int sb_init_dio_done_wq(struct super_block *sb)
 	/*
 	 * This has to be atomic as more DIOs can race to create the workqueue
 	 */
-	cmpxchg(&sb->s_dio_done_wq, NULL, wq);
+	old = cmpxchg(&sb->s_dio_done_wq, NULL, wq);
 	/* Someone created workqueue before us? Free ours... */
-	if (wq != sb->s_dio_done_wq)
+	if (old)
 		destroy_workqueue(wq);
 	return 0;
 }
-- 
1.7.10.4

Re: linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)

[Jan Kara]

On Sun 08-09-13 20:21:54, Olof Johansson wrote:
> On Fri, Sep 06, 2013 at 10:52:52AM +0200, Geert Uytterhoeven wrote:
> > On Fri, Sep 6, 2013 at 9:19 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> > > After merging the final tree, today's linux-next build (arm defconfig)
> > > produced this warning:
> > >
> > > fs/direct-io.c: In function 'sb_init_dio_done_wq':
> > > fs/direct-io.c:557:2: warning: value computed is not used [-Wunused-value]
> > >
> > > This is:
> > >
> > >         cmpxchg(&sb->s_dio_done_wq, NULL, wq);
> > >
> > > Introduced by commit 7b7a8665edd8 ("direct-io: Implement generic deferred
> > > AIO completions").
> > 
> > This happens for include/asm-generic/cmpxchg.h and several other
> > arch-specific implementations that cast the return value of cmpxchg()
> > like
> > 
> > #define cmpxchg(ptr, o, n)   ((__typeof__(*(ptr)))__cmpxchg(....
> > 
> > If the caller of cmpxchg() doesn't use the return value, we get a
> > compiler warning,
> > at least with some versions of gcc.
> > 
> > Any idea how to fix this once and for good?
> 
> Should it be fixed? Chances are that the caller needs to do actions
> depending on if the change happened, and checking the value afterwards
> is inherently racy.
> 
> For this specific fs/direct-io.c case it seems to be safe since the
> workqueue is only ever set and never cleared, but it might still be a
> good idea to do:
  I'm not against this change - feel free to add my:
Reviewed-by: Jan Kara <jack@suse.cz>

  to it and merge it. However I maintain there are valid usecases where you
do not care about the return value so warning about it doesn't seem right.
OTOH thinking about it some more I agree we have other precedents where
sometimes-correct-often-bugs constructs are warned about and I can see how
people can consider cmpxchg() to be that case. But in that case we should:
  a) be consistent among architectures about the warning
  b) comment at cmpxchg definition that you are supposed to check its
     return value. If you really know what you are doing, you can cast the
     return value to (void) and comment why it's safe.

								Honza
> 
> 
> ---8<----------8<----------8<----------8<----------8<----------8<----------8<--
> 
> From 7fdfa2da727aa9153a7df919c240abfe4f564d7a Mon Sep 17 00:00:00 2001
> From: Olof Johansson <olof@lixom.net>
> Date: Sun, 8 Sep 2013 20:12:57 -0700
> Subject: [PATCH] direct-io: Use return from cmpxchg to decide of assignment happened
> 
> Not using the return value can in the generic case be racy, so it's better to
> use the expected way of using that instead of comparing later.
> 
> This also resolved the warning caused on ARM and other architectures:
> 
> fs/direct-io.c: In function 'sb_init_dio_done_wq':
> fs/direct-io.c:557:2: warning: value computed is not used [-Wunused-value]
> 
> Signed-off-by: Olof Johansson <olof@lixom.net>
> ---
>  fs/direct-io.c |    5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/direct-io.c b/fs/direct-io.c
> index 1782023..0e04142 100644
> --- a/fs/direct-io.c
> +++ b/fs/direct-io.c
> @@ -544,6 +544,7 @@ static inline int dio_bio_reap(struct dio *dio, struct dio_submit *sdio)
>   */
>  static int sb_init_dio_done_wq(struct super_block *sb)
>  {
> +	struct workqueue_struct *old;
>  	struct workqueue_struct *wq = alloc_workqueue("dio/%s",
>  						      WQ_MEM_RECLAIM, 0,
>  						      sb->s_id);
> @@ -552,9 +553,9 @@ static int sb_init_dio_done_wq(struct super_block *sb)
>  	/*
>  	 * This has to be atomic as more DIOs can race to create the workqueue
>  	 */
> -	cmpxchg(&sb->s_dio_done_wq, NULL, wq);
> +	old = cmpxchg(&sb->s_dio_done_wq, NULL, wq);
>  	/* Someone created workqueue before us? Free ours... */
> -	if (wq != sb->s_dio_done_wq)
> +	if (old)
>  		destroy_workqueue(wq);
>  	return 0;
>  }
> -- 
> 1.7.10.4
> 
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR

Re: linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)

[Olof Johansson]

On Mon, Sep 9, 2013 at 7:39 AM, Jan Kara <jack@suse.cz> wrote:
> On Sun 08-09-13 20:21:54, Olof Johansson wrote:
>> On Fri, Sep 06, 2013 at 10:52:52AM +0200, Geert Uytterhoeven wrote:
>> > On Fri, Sep 6, 2013 at 9:19 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>> > > After merging the final tree, today's linux-next build (arm defconfig)
>> > > produced this warning:
>> > >
>> > > fs/direct-io.c: In function 'sb_init_dio_done_wq':
>> > > fs/direct-io.c:557:2: warning: value computed is not used [-Wunused-value]
>> > >
>> > > This is:
>> > >
>> > >         cmpxchg(&sb->s_dio_done_wq, NULL, wq);
>> > >
>> > > Introduced by commit 7b7a8665edd8 ("direct-io: Implement generic deferred
>> > > AIO completions").
>> >
>> > This happens for include/asm-generic/cmpxchg.h and several other
>> > arch-specific implementations that cast the return value of cmpxchg()
>> > like
>> >
>> > #define cmpxchg(ptr, o, n)   ((__typeof__(*(ptr)))__cmpxchg(....
>> >
>> > If the caller of cmpxchg() doesn't use the return value, we get a
>> > compiler warning,
>> > at least with some versions of gcc.
>> >
>> > Any idea how to fix this once and for good?
>>
>> Should it be fixed? Chances are that the caller needs to do actions
>> depending on if the change happened, and checking the value afterwards
>> is inherently racy.
>>
>> For this specific fs/direct-io.c case it seems to be safe since the
>> workqueue is only ever set and never cleared, but it might still be a
>> good idea to do:
>   I'm not against this change - feel free to add my:
> Reviewed-by: Jan Kara <jack@suse.cz>
>
>   to it and merge it. However I maintain there are valid usecases where you
> do not care about the return value so warning about it doesn't seem right.

Yes, similar to (some) __must_check-annotated functions.

> OTOH thinking about it some more I agree we have other precedents where
> sometimes-correct-often-bugs constructs are warned about and I can see how
> people can consider cmpxchg() to be that case. But in that case we should:
>   a) be consistent among architectures about the warning
>   b) comment at cmpxchg definition that you are supposed to check its
>      return value. If you really know what you are doing, you can cast the
>      return value to (void) and comment why it's safe.

Unfortunately cmpxchg() is a #define since it needs to use __typeof__.
Marking it __must_check isn't feasible. I'm open for better
suggestions.


-Olof

Re: linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)

[Jan Kara]

On Mon 09-09-13 09:45:52, Olof Johansson wrote:
> On Mon, Sep 9, 2013 at 7:39 AM, Jan Kara <jack@suse.cz> wrote:
> > On Sun 08-09-13 20:21:54, Olof Johansson wrote:
> >> On Fri, Sep 06, 2013 at 10:52:52AM +0200, Geert Uytterhoeven wrote:
> >> > On Fri, Sep 6, 2013 at 9:19 AM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >> > > After merging the final tree, today's linux-next build (arm defconfig)
> >> > > produced this warning:
> >> > >
> >> > > fs/direct-io.c: In function 'sb_init_dio_done_wq':
> >> > > fs/direct-io.c:557:2: warning: value computed is not used [-Wunused-value]
> >> > >
> >> > > This is:
> >> > >
> >> > >         cmpxchg(&sb->s_dio_done_wq, NULL, wq);
> >> > >
> >> > > Introduced by commit 7b7a8665edd8 ("direct-io: Implement generic deferred
> >> > > AIO completions").
> >> >
> >> > This happens for include/asm-generic/cmpxchg.h and several other
> >> > arch-specific implementations that cast the return value of cmpxchg()
> >> > like
> >> >
> >> > #define cmpxchg(ptr, o, n)   ((__typeof__(*(ptr)))__cmpxchg(....
> >> >
> >> > If the caller of cmpxchg() doesn't use the return value, we get a
> >> > compiler warning,
> >> > at least with some versions of gcc.
> >> >
> >> > Any idea how to fix this once and for good?
> >>
> >> Should it be fixed? Chances are that the caller needs to do actions
> >> depending on if the change happened, and checking the value afterwards
> >> is inherently racy.
> >>
> >> For this specific fs/direct-io.c case it seems to be safe since the
> >> workqueue is only ever set and never cleared, but it might still be a
> >> good idea to do:
> >   I'm not against this change - feel free to add my:
> > Reviewed-by: Jan Kara <jack@suse.cz>
> >
> >   to it and merge it. However I maintain there are valid usecases where you
> > do not care about the return value so warning about it doesn't seem right.
> 
> Yes, similar to (some) __must_check-annotated functions.
> 
> > OTOH thinking about it some more I agree we have other precedents where
> > sometimes-correct-often-bugs constructs are warned about and I can see how
> > people can consider cmpxchg() to be that case. But in that case we should:
> >   a) be consistent among architectures about the warning
> >   b) comment at cmpxchg definition that you are supposed to check its
> >      return value. If you really know what you are doing, you can cast the
> >      return value to (void) and comment why it's safe.
> 
> Unfortunately cmpxchg() is a #define since it needs to use __typeof__.
> Marking it __must_check isn't feasible. I'm open for better
> suggestions.
  Well, if you explicitely type return value of cmpxchg() to its type like
some architectures do, then recent gccs will complain when the return value
isn't used. But I agree this probably isn't very futureproof.

								Honza
-- 
Jan Kara <jack@suse.cz>
SUSE Labs, CR