* [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc
@ 2019-06-25 18:23 Nathan Chancellor
2019-06-25 18:32 ` Nick Desaulniers
` (3 more replies)
0 siblings, 4 replies; 6+ messages in thread
From: Nathan Chancellor @ 2019-06-25 18:23 UTC (permalink / raw)
To: Björn Töpel, Magnus Karlsson, David S. Miller,
Alexei Starovoitov, Daniel Borkmann, Jakub Kicinski,
Jesper Dangaard Brouer, John Fastabend
Cc: netdev, bpf, xdp-newbies, linux-kernel, clang-built-linux,
Nick Desaulniers, Nathan Huckleberry, Nathan Chancellor
Clang warns:
In file included from net/xdp/xsk_queue.c:10:
net/xdp/xsk_queue.h:292:2: warning: expression result unused
[-Wunused-value]
WRITE_ONCE(q->ring->producer, q->prod_tail);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
include/linux/compiler.h:284:6: note: expanded from macro 'WRITE_ONCE'
__u.__val; \
~~~ ^~~~~
1 warning generated.
The q->prod_tail assignment has a comma at the end, not a semi-colon.
Fix that so clang no longer warns and everything works as expected.
Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support")
Link: https://github.com/ClangBuiltLinux/linux/issues/544
Signed-off-by: Nathan Chancellor <natechancellor@gmail.com>
---
net/xdp/xsk_queue.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h
index 88b9ae24658d..cba4a640d5e8 100644
--- a/net/xdp/xsk_queue.h
+++ b/net/xdp/xsk_queue.h
@@ -288,7 +288,7 @@ static inline void xskq_produce_flush_desc(struct xsk_queue *q)
/* Order producer and data */
smp_wmb(); /* B, matches C */
- q->prod_tail = q->prod_head,
+ q->prod_tail = q->prod_head;
WRITE_ONCE(q->ring->producer, q->prod_tail);
}
--
2.22.0
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc 2019-06-25 18:23 [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc Nathan Chancellor @ 2019-06-25 18:32 ` Nick Desaulniers 2019-06-25 19:46 ` Jonathan Lemon ` (2 subsequent siblings) 3 siblings, 0 replies; 6+ messages in thread From: Nick Desaulniers @ 2019-06-25 18:32 UTC (permalink / raw) To: Nathan Chancellor Cc: Björn Töpel, Magnus Karlsson, David S. Miller, Alexei Starovoitov, Daniel Borkmann, Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend, netdev, bpf, xdp-newbies, LKML, clang-built-linux, Nathan Huckleberry On Tue, Jun 25, 2019 at 11:24 AM Nathan Chancellor <natechancellor@gmail.com> wrote: > > Clang warns: > > In file included from net/xdp/xsk_queue.c:10: > net/xdp/xsk_queue.h:292:2: warning: expression result unused > [-Wunused-value] > WRITE_ONCE(q->ring->producer, q->prod_tail); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/compiler.h:284:6: note: expanded from macro 'WRITE_ONCE' > __u.__val; \ > ~~~ ^~~~~ > 1 warning generated. > > The q->prod_tail assignment has a comma at the end, not a semi-colon. > Fix that so clang no longer warns and everything works as expected. oh no! -Wunderhanded-C-contest-style-use-of-comma-operator strikes again! Great find and thanks for the fix. Acked-by: Nick Desaulniers <ndesaulniers@google.com> Björn and Alexei should carefully review. Because WRITE_ONCE is a macro that expands to a GNU C statement expression, which returns the last statement, this code was previously assigning q->prod_tail to itself, now it's assigning it to q->prod_head. I assume that was not intentional, but am unfamiliar with the code. > > Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support") > Link: https://github.com/ClangBuiltLinux/linux/issues/544 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > --- > net/xdp/xsk_queue.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h > index 88b9ae24658d..cba4a640d5e8 100644 > --- a/net/xdp/xsk_queue.h > +++ b/net/xdp/xsk_queue.h > @@ -288,7 +288,7 @@ static inline void xskq_produce_flush_desc(struct xsk_queue *q) > /* Order producer and data */ > smp_wmb(); /* B, matches C */ > > - q->prod_tail = q->prod_head, > + q->prod_tail = q->prod_head; > WRITE_ONCE(q->ring->producer, q->prod_tail); > } > > -- > 2.22.0 > -- Thanks, ~Nick Desaulniers ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc 2019-06-25 18:23 [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc Nathan Chancellor 2019-06-25 18:32 ` Nick Desaulniers @ 2019-06-25 19:46 ` Jonathan Lemon 2019-06-25 20:49 ` Björn Töpel 2019-06-26 1:13 ` Song Liu 2019-06-26 14:30 ` Daniel Borkmann 3 siblings, 1 reply; 6+ messages in thread From: Jonathan Lemon @ 2019-06-25 19:46 UTC (permalink / raw) To: Nathan Chancellor Cc: Björn Töpel, Magnus Karlsson, David S. Miller, Alexei Starovoitov, Daniel Borkmann, Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend, netdev, bpf, xdp-newbies, linux-kernel, clang-built-linux, Nick Desaulniers, Nathan Huckleberry On 25 Jun 2019, at 11:23, Nathan Chancellor wrote: > Clang warns: > > In file included from net/xdp/xsk_queue.c:10: > net/xdp/xsk_queue.h:292:2: warning: expression result unused > [-Wunused-value] > WRITE_ONCE(q->ring->producer, q->prod_tail); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/compiler.h:284:6: note: expanded from macro 'WRITE_ONCE' > __u.__val; \ > ~~~ ^~~~~ > 1 warning generated. > > The q->prod_tail assignment has a comma at the end, not a semi-colon. > Fix that so clang no longer warns and everything works as expected. > > Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support") > Link: https://github.com/ClangBuiltLinux/linux/issues/544 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> Nice find. Acked-by: Jonathan Lemon <jonathan.lemon@gmail.com> > --- > net/xdp/xsk_queue.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h > index 88b9ae24658d..cba4a640d5e8 100644 > --- a/net/xdp/xsk_queue.h > +++ b/net/xdp/xsk_queue.h > @@ -288,7 +288,7 @@ static inline void xskq_produce_flush_desc(struct > xsk_queue *q) > /* Order producer and data */ > smp_wmb(); /* B, matches C */ > > - q->prod_tail = q->prod_head, > + q->prod_tail = q->prod_head; > WRITE_ONCE(q->ring->producer, q->prod_tail); > } > > -- > 2.22.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc 2019-06-25 19:46 ` Jonathan Lemon @ 2019-06-25 20:49 ` Björn Töpel 0 siblings, 0 replies; 6+ messages in thread From: Björn Töpel @ 2019-06-25 20:49 UTC (permalink / raw) To: Jonathan Lemon Cc: Nathan Chancellor, Björn Töpel, Magnus Karlsson, David S. Miller, Alexei Starovoitov, Daniel Borkmann, Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend, Netdev, bpf, Xdp, LKML, clang-built-linux, Nick Desaulniers, Nathan Huckleberry On Tue, 25 Jun 2019 at 22:04, Jonathan Lemon <jonathan.lemon@gmail.com> wrote: > > > > On 25 Jun 2019, at 11:23, Nathan Chancellor wrote: > > > Clang warns: > > > > In file included from net/xdp/xsk_queue.c:10: > > net/xdp/xsk_queue.h:292:2: warning: expression result unused > > [-Wunused-value] > > WRITE_ONCE(q->ring->producer, q->prod_tail); > > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > include/linux/compiler.h:284:6: note: expanded from macro 'WRITE_ONCE' > > __u.__val; \ > > ~~~ ^~~~~ > > 1 warning generated. > > > > The q->prod_tail assignment has a comma at the end, not a semi-colon. > > Fix that so clang no longer warns and everything works as expected. > > > > Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support") > > Link: https://github.com/ClangBuiltLinux/linux/issues/544 > > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> > > Nice find. > > Acked-by: Jonathan Lemon <jonathan.lemon@gmail.com> > Yikes. Yes, nice find, indeed. Acked-by: Björn Töpel <bjorn.topel@intel.com> The broader question is "Why does it work at all?", which is an "oh no" moment. The problematic functions are xsk_flush() and xsk_generic_rcv, where xskq_produce_flush_desc() is inlined. On the test machine, the GCC version is: $ gcc --version gcc (Ubuntu 7.4.0-1ubuntu1~18.04) 7.4.0 Copyright (C) 2017 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. I when I diff the output, both .lst and .o: $ diff -u old.lst new.lst --- old.lst 2019-06-25 22:10:57.709591605 +0200 +++ new.lst 2019-06-25 22:10:35.301359865 +0200 @@ -2480,7 +2480,7 @@ 1566: 48 8b 87 e0 02 00 00 mov 0x2e0(%rdi),%rax { 156d: 48 89 e5 mov %rsp,%rbp - q->prod_tail = q->prod_head, + q->prod_tail = q->prod_head; 1570: 8b 50 18 mov 0x18(%rax),%edx 1573: 89 50 1c mov %edx,0x1c(%rax) WRITE_ONCE(q->ring->producer, q->prod_tail); @@ -2649,7 +2649,7 @@ 16fb: 83 40 24 01 addl $0x1,0x24(%rax) xskq_produce_flush_desc(xs->rx); 16ff: 49 8b 86 e0 02 00 00 mov 0x2e0(%r14),%rax - q->prod_tail = q->prod_head, + q->prod_tail = q->prod_head; 1706: 8b 50 18 mov 0x18(%rax),%edx xs->sk.sk_data_ready(&xs->sk); 1709: 4c 89 f7 mov %r14,%rdi $ diff -u <(gdb -batch -ex 'file old.o' -ex 'disassemble xsk_flush') <(gdb -batch -ex 'file new.o' -ex 'disassemble xsk_flush') && echo "Whew" Whew $ diff -u <(gdb -batch -ex 'file old.o' -ex 'disassemble xsk_generic_rcv') <(gdb -batch -ex 'file new.o' -ex 'disassemble xsk_generic_rcv') && echo "Whew" Whew struct xsk_queue { u64 chunk_mask; /* 0 0x8 */ u64 size; /* 0x8 0x8 */ u32 ring_mask; /* 0x10 0x4 */ u32 nentries; /* 0x14 0x4 */ u32 prod_head; /* 0x18 0x4 */ u32 prod_tail; /* 0x1c 0x4 */ u32 cons_head; /* 0x20 0x4 */ u32 cons_tail; /* 0x24 0x4 */ struct xdp_ring * ring; /* 0x28 0x8 */ u64 invalid_descs; /* 0x30 0x8 */ /* size: 56, cachelines: 1, members: 10 */ /* last cacheline: 56 bytes */ }; So, it appears that the generated code is equal, both in xsk_flush() and xsk_generic_rcv() where flush was inlined. I'll be digging into more GCC versions, and observe the generated code. Regardless, this was a really good find. Thank you very much! Clang is added to my kernel build workflow from now on... Björn > > > --- > > net/xdp/xsk_queue.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h > > index 88b9ae24658d..cba4a640d5e8 100644 > > --- a/net/xdp/xsk_queue.h > > +++ b/net/xdp/xsk_queue.h > > @@ -288,7 +288,7 @@ static inline void xskq_produce_flush_desc(struct > > xsk_queue *q) > > /* Order producer and data */ > > smp_wmb(); /* B, matches C */ > > > > - q->prod_tail = q->prod_head, > > + q->prod_tail = q->prod_head; > > WRITE_ONCE(q->ring->producer, q->prod_tail); > > } > > > > -- > > 2.22.0 ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc 2019-06-25 18:23 [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc Nathan Chancellor 2019-06-25 18:32 ` Nick Desaulniers 2019-06-25 19:46 ` Jonathan Lemon @ 2019-06-26 1:13 ` Song Liu 2019-06-26 14:30 ` Daniel Borkmann 3 siblings, 0 replies; 6+ messages in thread From: Song Liu @ 2019-06-26 1:13 UTC (permalink / raw) To: Nathan Chancellor Cc: Björn Töpel, Magnus Karlsson, David S. Miller, Alexei Starovoitov, Daniel Borkmann, Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend, Networking, bpf, xdp-newbies, open list, clang-built-linux, Nick Desaulniers, Nathan Huckleberry On Tue, Jun 25, 2019 at 12:54 PM Nathan Chancellor <natechancellor@gmail.com> wrote: > > Clang warns: > > In file included from net/xdp/xsk_queue.c:10: > net/xdp/xsk_queue.h:292:2: warning: expression result unused > [-Wunused-value] > WRITE_ONCE(q->ring->producer, q->prod_tail); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/compiler.h:284:6: note: expanded from macro 'WRITE_ONCE' > __u.__val; \ > ~~~ ^~~~~ > 1 warning generated. > > The q->prod_tail assignment has a comma at the end, not a semi-colon. > Fix that so clang no longer warns and everything works as expected. > > Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support") > Link: https://github.com/ClangBuiltLinux/linux/issues/544 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> Cc: <stable@vger.kernel.org> # v4.18+ Acked-by: Song Liu <songliubraving@fb.com> Thanks for the fix! > --- > net/xdp/xsk_queue.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/net/xdp/xsk_queue.h b/net/xdp/xsk_queue.h > index 88b9ae24658d..cba4a640d5e8 100644 > --- a/net/xdp/xsk_queue.h > +++ b/net/xdp/xsk_queue.h > @@ -288,7 +288,7 @@ static inline void xskq_produce_flush_desc(struct xsk_queue *q) > /* Order producer and data */ > smp_wmb(); /* B, matches C */ > > - q->prod_tail = q->prod_head, > + q->prod_tail = q->prod_head; > WRITE_ONCE(q->ring->producer, q->prod_tail); > } > > -- > 2.22.0 > ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc 2019-06-25 18:23 [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc Nathan Chancellor ` (2 preceding siblings ...) 2019-06-26 1:13 ` Song Liu @ 2019-06-26 14:30 ` Daniel Borkmann 3 siblings, 0 replies; 6+ messages in thread From: Daniel Borkmann @ 2019-06-26 14:30 UTC (permalink / raw) To: Nathan Chancellor, Björn Töpel, Magnus Karlsson, David S. Miller, Alexei Starovoitov, Jakub Kicinski, Jesper Dangaard Brouer, John Fastabend Cc: netdev, bpf, xdp-newbies, linux-kernel, clang-built-linux, Nick Desaulniers, Nathan Huckleberry On 06/25/2019 08:23 PM, Nathan Chancellor wrote: > Clang warns: > > In file included from net/xdp/xsk_queue.c:10: > net/xdp/xsk_queue.h:292:2: warning: expression result unused > [-Wunused-value] > WRITE_ONCE(q->ring->producer, q->prod_tail); > ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > include/linux/compiler.h:284:6: note: expanded from macro 'WRITE_ONCE' > __u.__val; \ > ~~~ ^~~~~ > 1 warning generated. > > The q->prod_tail assignment has a comma at the end, not a semi-colon. > Fix that so clang no longer warns and everything works as expected. > > Fixes: c497176cb2e4 ("xsk: add Rx receive functions and poll support") > Link: https://github.com/ClangBuiltLinux/linux/issues/544 > Signed-off-by: Nathan Chancellor <natechancellor@gmail.com> Applied, thanks! ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-06-26 14:30 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-06-25 18:23 [PATCH] xsk: Properly terminate assignment in xskq_produce_flush_desc Nathan Chancellor 2019-06-25 18:32 ` Nick Desaulniers 2019-06-25 19:46 ` Jonathan Lemon 2019-06-25 20:49 ` Björn Töpel 2019-06-26 1:13 ` Song Liu 2019-06-26 14:30 ` Daniel Borkmann
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).