* linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)
@ 2013-09-06 7:19 Stephen Rothwell
2013-09-06 8:52 ` Geert Uytterhoeven
0 siblings, 1 reply; 6+ messages in thread
From: Stephen Rothwell @ 2013-09-06 7:19 UTC (permalink / raw)
To: Al Viro; +Cc: linux-next, linux-kernel, Christoph Hellwig, Jan Kara
[-- 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 --]
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)
2013-09-06 7:19 linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree) Stephen Rothwell
@ 2013-09-06 8:52 ` Geert Uytterhoeven
2013-09-09 3:21 ` Olof Johansson
0 siblings, 1 reply; 6+ messages in thread
From: Geert Uytterhoeven @ 2013-09-06 8:52 UTC (permalink / raw)
To: Stephen Rothwell
Cc: Al Viro, Linux-Next, linux-kernel@vger.kernel.org,
Christoph Hellwig, Jan Kara
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)
2013-09-06 8:52 ` Geert Uytterhoeven
@ 2013-09-09 3:21 ` Olof Johansson
2013-09-09 14:39 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Olof Johansson @ 2013-09-09 3:21 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Stephen Rothwell, Al Viro, Linux-Next,
linux-kernel@vger.kernel.org, Christoph Hellwig, Jan Kara
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)
2013-09-09 3:21 ` Olof Johansson
@ 2013-09-09 14:39 ` Jan Kara
2013-09-09 16:45 ` Olof Johansson
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kara @ 2013-09-09 14:39 UTC (permalink / raw)
To: Olof Johansson
Cc: Geert Uytterhoeven, Stephen Rothwell, Al Viro, Linux-Next,
linux-kernel@vger.kernel.org, Christoph Hellwig, 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)
2013-09-09 14:39 ` Jan Kara
@ 2013-09-09 16:45 ` Olof Johansson
2013-09-09 19:20 ` Jan Kara
0 siblings, 1 reply; 6+ messages in thread
From: Olof Johansson @ 2013-09-09 16:45 UTC (permalink / raw)
To: Jan Kara
Cc: Geert Uytterhoeven, Stephen Rothwell, Al Viro, Linux-Next,
linux-kernel@vger.kernel.org, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree)
2013-09-09 16:45 ` Olof Johansson
@ 2013-09-09 19:20 ` Jan Kara
0 siblings, 0 replies; 6+ messages in thread
From: Jan Kara @ 2013-09-09 19:20 UTC (permalink / raw)
To: Olof Johansson
Cc: Jan Kara, Geert Uytterhoeven, Stephen Rothwell, Al Viro,
Linux-Next, linux-kernel@vger.kernel.org, Christoph Hellwig
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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2013-09-09 19:20 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2013-09-06 7:19 linux-next: build warning after merge of the final tree (Linus' tree related - vai vfs tree) Stephen Rothwell
2013-09-06 8:52 ` Geert Uytterhoeven
2013-09-09 3:21 ` Olof Johansson
2013-09-09 14:39 ` Jan Kara
2013-09-09 16:45 ` Olof Johansson
2013-09-09 19:20 ` Jan Kara
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox