| Message ID | 20230116174013.3272728-1-willemdebruijn.kernel@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net] selftests/net: toeplitz: fix race on tpacket_v3 block close | expand |
On Mon, 2023-01-16 at 12:40 -0500, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@google.com> > > Avoid race between process wakeup and tpacket_v3 block timeout. > > The test waits for cfg_timeout_msec for packets to arrive. Packets > arrive in tpacket_v3 rings, which pass packets ("frames") to the > process in batches ("blocks"). The sk waits for req3.tp_retire_blk_tov > msec to release a block. > > Set the block timeout lower than the process waiting time, else > the process may find that no block has been released by the time it > scans the socket list. Convert to a ring of more than one, smaller, > blocks with shorter timeouts. Blocks must be page aligned, so >= 64KB. > > Somewhat awkward while () notation dictated by checkpatch: no empty > braces allowed, nor statement on the same line as the condition. You might look at using a do/while approach rather than just a straight while. I believe that is the pattern used at various points throughout the kernel when you do nothing between the braces. I know we have instances of "do {} while (0)" throughout the kernel so that might be a way to go.
On Tue, Jan 17, 2023 at 11:31 AM Alexander H Duyck <alexander.duyck@gmail.com> wrote: > > On Mon, 2023-01-16 at 12:40 -0500, Willem de Bruijn wrote: > > From: Willem de Bruijn <willemb@google.com> > > > > Avoid race between process wakeup and tpacket_v3 block timeout. > > > > The test waits for cfg_timeout_msec for packets to arrive. Packets > > arrive in tpacket_v3 rings, which pass packets ("frames") to the > > process in batches ("blocks"). The sk waits for req3.tp_retire_blk_tov > > msec to release a block. > > > > Set the block timeout lower than the process waiting time, else > > the process may find that no block has been released by the time it > > scans the socket list. Convert to a ring of more than one, smaller, > > blocks with shorter timeouts. Blocks must be page aligned, so >= 64KB. > > > > Somewhat awkward while () notation dictated by checkpatch: no empty > > braces allowed, nor statement on the same line as the condition. > > You might look at using a do/while approach rather than just a straight > while. I believe that is the pattern used at various points throughout > the kernel when you do nothing between the braces. I know we have > instances of "do {} while (0)" throughout the kernel so that might be a > way to go. Thanks. I tried a couple of approaches. None of them seemed particularly clean. I include do {} while (condition) in that list. In this case, I think the patchwork error is a false positive and while (condition) {} is idiomatic. The rule it triggers is to prevent excess braces for single line branches. Alternative might be while (condition); That triggers a rule that tries to prevent "if (condition) action;" on the same line. I found the current solution the least bad of the allowed options. But can definitely respin using "do {} while" if preferred.
On 1/16/2023 9:40 AM, Willem de Bruijn wrote: > From: Willem de Bruijn <willemb@google.com> > > Avoid race between process wakeup and tpacket_v3 block timeout. > > The test waits for cfg_timeout_msec for packets to arrive. Packets > arrive in tpacket_v3 rings, which pass packets ("frames") to the > process in batches ("blocks"). The sk waits for req3.tp_retire_blk_tov > msec to release a block. > > Set the block timeout lower than the process waiting time, else > the process may find that no block has been released by the time it > scans the socket list. Convert to a ring of more than one, smaller, > blocks with shorter timeouts. Blocks must be page aligned, so >= 64KB. > > Somewhat awkward while () notation dictated by checkpatch: no empty > braces allowed, nor statement on the same line as the condition. > > Fixes: 5ebfb4cc3048 ("selftests/net: toeplitz test") > Signed-off-by: Willem de Bruijn <willemb@google.com> > --- > tools/testing/selftests/net/toeplitz.c | 13 ++++++++----- > 1 file changed, 8 insertions(+), 5 deletions(-) > > diff --git a/tools/testing/selftests/net/toeplitz.c b/tools/testing/selftests/net/toeplitz.c > index 90026a27eac0c..66f7f6568643a 100644 > --- a/tools/testing/selftests/net/toeplitz.c > +++ b/tools/testing/selftests/net/toeplitz.c > @@ -215,7 +215,7 @@ static char *recv_frame(const struct ring_state *ring, char *frame) > } > > /* A single TPACKET_V3 block can hold multiple frames */ > -static void recv_block(struct ring_state *ring) > +static bool recv_block(struct ring_state *ring) > { > struct tpacket_block_desc *block; > char *frame; > @@ -223,7 +223,7 @@ static void recv_block(struct ring_state *ring) > > block = (void *)(ring->mmap + ring->idx * ring_block_sz); > if (!(block->hdr.bh1.block_status & TP_STATUS_USER)) > - return; > + return false; > > frame = (char *)block; > frame += block->hdr.bh1.offset_to_first_pkt; > @@ -235,6 +235,8 @@ static void recv_block(struct ring_state *ring) > > block->hdr.bh1.block_status = TP_STATUS_KERNEL; > ring->idx = (ring->idx + 1) % ring_block_nr; > + > + return true; > } > > /* simple test: sleep once unconditionally and then process all rings */ > @@ -245,7 +247,8 @@ static void process_rings(void) > usleep(1000 * cfg_timeout_msec); > > for (i = 0; i < num_cpus; i++) > - recv_block(&rings[i]); > + while (recv_block(&rings[i])) > + ; I'd rather have one of while (recv_block(&rings[i])); or while (recv_block(&rings[i])) {} or even (but less preferred: do {} (while (recv_block(&rings[i])); instead of this ; on its own line. Even if this violates checkpatch attempts to catch other bad style this is preferable to the lone ';' on its own line. If necessary we can/should change checkpatch to allow the idiomatic approach.
> On 1/16/2023 9:40 AM, Willem de Bruijn wrote: > > From: Willem de Bruijn <willemb@google.com> > > > > Avoid race between process wakeup and tpacket_v3 block timeout. > > > > The test waits for cfg_timeout_msec for packets to arrive. Packets > > arrive in tpacket_v3 rings, which pass packets ("frames") to the > > process in batches ("blocks"). The sk waits for req3.tp_retire_blk_tov > > msec to release a block. > > > > Set the block timeout lower than the process waiting time, else > > the process may find that no block has been released by the time it > > scans the socket list. Convert to a ring of more than one, smaller, > > blocks with shorter timeouts. Blocks must be page aligned, so >= 64KB. > > > > Somewhat awkward while () notation dictated by checkpatch: no empty > > braces allowed, nor statement on the same line as the condition. > > > > Fixes: 5ebfb4cc3048 ("selftests/net: toeplitz test") > > Signed-off-by: Willem de Bruijn <willemb@google.com> > > --- > > tools/testing/selftests/net/toeplitz.c | 13 ++++++++----- > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/tools/testing/selftests/net/toeplitz.c b/tools/testing/selftests/net/toeplitz.c > > index 90026a27eac0c..66f7f6568643a 100644 > > --- a/tools/testing/selftests/net/toeplitz.c > > +++ b/tools/testing/selftests/net/toeplitz.c > > @@ -215,7 +215,7 @@ static char *recv_frame(const struct ring_state *ring, char *frame) > > } > > > > /* A single TPACKET_V3 block can hold multiple frames */ > > -static void recv_block(struct ring_state *ring) > > +static bool recv_block(struct ring_state *ring) > > { > > struct tpacket_block_desc *block; > > char *frame; > > @@ -223,7 +223,7 @@ static void recv_block(struct ring_state *ring) > > > > block = (void *)(ring->mmap + ring->idx * ring_block_sz); > > if (!(block->hdr.bh1.block_status & TP_STATUS_USER)) > > - return; > > + return false; > > > > frame = (char *)block; > > frame += block->hdr.bh1.offset_to_first_pkt; > > @@ -235,6 +235,8 @@ static void recv_block(struct ring_state *ring) > > > > block->hdr.bh1.block_status = TP_STATUS_KERNEL; > > ring->idx = (ring->idx + 1) % ring_block_nr; > > + > > + return true; > > } > > > > /* simple test: sleep once unconditionally and then process all rings */ > > @@ -245,7 +247,8 @@ static void process_rings(void) > > usleep(1000 * cfg_timeout_msec); > > > > for (i = 0; i < num_cpus; i++) > > - recv_block(&rings[i]); > > + while (recv_block(&rings[i])) > > + ; > > I'd rather have one of > > while (recv_block(&rings[i])); > > or > > while (recv_block(&rings[i])) {} > > or even (but less preferred: > > do {} (while (recv_block(&rings[i])); > > instead of this ; on its own line. > > Even if this violates checkpatch attempts to catch other bad style this > is preferable to the lone ';' on its own line. > > If necessary we can/should change checkpatch to allow the idiomatic > approach. Let me send a v2 with the do {} while construct. Cc:ed the checkpatch maintainers for visibility.
On Tue, 2023-01-17 at 14:15 -0500, Willem de Bruijn wrote: > > On 1/16/2023 9:40 AM, Willem de Bruijn wrote: > > > From: Willem de Bruijn <willemb@google.com> > > > > > > Avoid race between process wakeup and tpacket_v3 block timeout. > > > > > > The test waits for cfg_timeout_msec for packets to arrive. Packets > > > arrive in tpacket_v3 rings, which pass packets ("frames") to the > > > process in batches ("blocks"). The sk waits for > > > req3.tp_retire_blk_tov > > > msec to release a block. > > > > > > Set the block timeout lower than the process waiting time, else > > > the process may find that no block has been released by the time > > > it > > > scans the socket list. Convert to a ring of more than one, > > > smaller, > > > blocks with shorter timeouts. Blocks must be page aligned, so >= > > > 64KB. > > > > > > Somewhat awkward while () notation dictated by checkpatch: no > > > empty > > > braces allowed, nor statement on the same line as the condition. > > > > > > Fixes: 5ebfb4cc3048 ("selftests/net: toeplitz test") > > > Signed-off-by: Willem de Bruijn <willemb@google.com> > > > --- > > > tools/testing/selftests/net/toeplitz.c | 13 ++++++++----- > > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > > > diff --git a/tools/testing/selftests/net/toeplitz.c > > > b/tools/testing/selftests/net/toeplitz.c > > > index 90026a27eac0c..66f7f6568643a 100644 > > > --- a/tools/testing/selftests/net/toeplitz.c > > > +++ b/tools/testing/selftests/net/toeplitz.c > > > @@ -215,7 +215,7 @@ static char *recv_frame(const struct > > > ring_state *ring, char *frame) > > > } > > > > > > /* A single TPACKET_V3 block can hold multiple frames */ > > > -static void recv_block(struct ring_state *ring) > > > +static bool recv_block(struct ring_state *ring) > > > { > > > struct tpacket_block_desc *block; > > > char *frame; > > > @@ -223,7 +223,7 @@ static void recv_block(struct ring_state > > > *ring) > > > > > > block = (void *)(ring->mmap + ring->idx * ring_block_sz); > > > if (!(block->hdr.bh1.block_status & TP_STATUS_USER)) > > > - return; > > > + return false; > > > > > > frame = (char *)block; > > > frame += block->hdr.bh1.offset_to_first_pkt; > > > @@ -235,6 +235,8 @@ static void recv_block(struct ring_state > > > *ring) > > > > > > block->hdr.bh1.block_status = TP_STATUS_KERNEL; > > > ring->idx = (ring->idx + 1) % ring_block_nr; > > > + > > > + return true; > > > } > > > > > > /* simple test: sleep once unconditionally and then process all > > > rings */ > > > @@ -245,7 +247,8 @@ static void process_rings(void) > > > usleep(1000 * cfg_timeout_msec); > > > > > > for (i = 0; i < num_cpus; i++) > > > - recv_block(&rings[i]); > > > + while (recv_block(&rings[i])) > > > + ; > > > > I'd rather have one of > > > > while (recv_block(&rings[i])); > > > > or > > > > while (recv_block(&rings[i])) {} > > > > or even (but less preferred: > > > > do {} (while (recv_block(&rings[i])); > > > > instead of this ; on its own line. > > > > Even if this violates checkpatch attempts to catch other bad style > > this > > is preferable to the lone ';' on its own line. > > > > If necessary we can/should change checkpatch to allow the idiomatic > > approach. To me it's a 'Don't care'. There are many hundreds of these in the kernel and there is no valid reason to require a particular style. $ git grep -P '^\t+;\s*$' -- '*.c' | wc -l 871
On Tue, Jan 17, 2023 at 7:54 PM Joe Perches <joe@perches.com> wrote: > > On Tue, 2023-01-17 at 14:15 -0500, Willem de Bruijn wrote: > > > On 1/16/2023 9:40 AM, Willem de Bruijn wrote: > > > > From: Willem de Bruijn <willemb@google.com> > > > > > > > > Avoid race between process wakeup and tpacket_v3 block timeout. > > > > > > > > The test waits for cfg_timeout_msec for packets to arrive. Packets > > > > arrive in tpacket_v3 rings, which pass packets ("frames") to the > > > > process in batches ("blocks"). The sk waits for > > > > req3.tp_retire_blk_tov > > > > msec to release a block. > > > > > > > > Set the block timeout lower than the process waiting time, else > > > > the process may find that no block has been released by the time > > > > it > > > > scans the socket list. Convert to a ring of more than one, > > > > smaller, > > > > blocks with shorter timeouts. Blocks must be page aligned, so >= > > > > 64KB. > > > > > > > > Somewhat awkward while () notation dictated by checkpatch: no > > > > empty > > > > braces allowed, nor statement on the same line as the condition. > > > > > > > > Fixes: 5ebfb4cc3048 ("selftests/net: toeplitz test") > > > > Signed-off-by: Willem de Bruijn <willemb@google.com> > > > > --- > > > > tools/testing/selftests/net/toeplitz.c | 13 ++++++++----- > > > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > > > > > diff --git a/tools/testing/selftests/net/toeplitz.c > > > > b/tools/testing/selftests/net/toeplitz.c > > > > index 90026a27eac0c..66f7f6568643a 100644 > > > > --- a/tools/testing/selftests/net/toeplitz.c > > > > +++ b/tools/testing/selftests/net/toeplitz.c > > > > @@ -215,7 +215,7 @@ static char *recv_frame(const struct > > > > ring_state *ring, char *frame) > > > > } > > > > > > > > /* A single TPACKET_V3 block can hold multiple frames */ > > > > -static void recv_block(struct ring_state *ring) > > > > +static bool recv_block(struct ring_state *ring) > > > > { > > > > struct tpacket_block_desc *block; > > > > char *frame; > > > > @@ -223,7 +223,7 @@ static void recv_block(struct ring_state > > > > *ring) > > > > > > > > block = (void *)(ring->mmap + ring->idx * ring_block_sz); > > > > if (!(block->hdr.bh1.block_status & TP_STATUS_USER)) > > > > - return; > > > > + return false; > > > > > > > > frame = (char *)block; > > > > frame += block->hdr.bh1.offset_to_first_pkt; > > > > @@ -235,6 +235,8 @@ static void recv_block(struct ring_state > > > > *ring) > > > > > > > > block->hdr.bh1.block_status = TP_STATUS_KERNEL; > > > > ring->idx = (ring->idx + 1) % ring_block_nr; > > > > + > > > > + return true; > > > > } > > > > > > > > /* simple test: sleep once unconditionally and then process all > > > > rings */ > > > > @@ -245,7 +247,8 @@ static void process_rings(void) > > > > usleep(1000 * cfg_timeout_msec); > > > > > > > > for (i = 0; i < num_cpus; i++) > > > > - recv_block(&rings[i]); > > > > + while (recv_block(&rings[i])) > > > > + ; > > > > > > I'd rather have one of > > > > > > while (recv_block(&rings[i])); > > > > > > or > > > > > > while (recv_block(&rings[i])) {} > > > > > > or even (but less preferred: > > > > > > do {} (while (recv_block(&rings[i])); > > > > > > instead of this ; on its own line. > > > > > > Even if this violates checkpatch attempts to catch other bad style > > > this > > > is preferable to the lone ';' on its own line. > > > > > > If necessary we can/should change checkpatch to allow the idiomatic > > > approach. > > To me it's a 'Don't care'. > > There are many hundreds of these in the kernel and there is no > valid reason to require a particular style. > > $ git grep -P '^\t+;\s*$' -- '*.c' | wc -l > 871 Interesting, thanks. I had to send a v2 by now anyway, so changed to a "do {} while" as evidently people found that more palatable.
diff --git a/tools/testing/selftests/net/toeplitz.c b/tools/testing/selftests/net/toeplitz.c index 90026a27eac0c..66f7f6568643a 100644 --- a/tools/testing/selftests/net/toeplitz.c +++ b/tools/testing/selftests/net/toeplitz.c @@ -215,7 +215,7 @@ static char *recv_frame(const struct ring_state *ring, char *frame) } /* A single TPACKET_V3 block can hold multiple frames */ -static void recv_block(struct ring_state *ring) +static bool recv_block(struct ring_state *ring) { struct tpacket_block_desc *block; char *frame; @@ -223,7 +223,7 @@ static void recv_block(struct ring_state *ring) block = (void *)(ring->mmap + ring->idx * ring_block_sz); if (!(block->hdr.bh1.block_status & TP_STATUS_USER)) - return; + return false; frame = (char *)block; frame += block->hdr.bh1.offset_to_first_pkt; @@ -235,6 +235,8 @@ static void recv_block(struct ring_state *ring) block->hdr.bh1.block_status = TP_STATUS_KERNEL; ring->idx = (ring->idx + 1) % ring_block_nr; + + return true; } /* simple test: sleep once unconditionally and then process all rings */ @@ -245,7 +247,8 @@ static void process_rings(void) usleep(1000 * cfg_timeout_msec); for (i = 0; i < num_cpus; i++) - recv_block(&rings[i]); + while (recv_block(&rings[i])) + ; fprintf(stderr, "count: pass=%u nohash=%u fail=%u\n", frames_received - frames_nohash - frames_error, @@ -257,12 +260,12 @@ static char *setup_ring(int fd) struct tpacket_req3 req3 = {0}; void *ring; - req3.tp_retire_blk_tov = cfg_timeout_msec; + req3.tp_retire_blk_tov = cfg_timeout_msec / 8; req3.tp_feature_req_word = TP_FT_REQ_FILL_RXHASH; req3.tp_frame_size = 2048; req3.tp_frame_nr = 1 << 10; - req3.tp_block_nr = 2; + req3.tp_block_nr = 16; req3.tp_block_size = req3.tp_frame_size * req3.tp_frame_nr; req3.tp_block_size /= req3.tp_block_nr;