| Message ID | 20210609103326.278782-7-toke@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | BPF |
| Headers | show |
| Series | Clean up and document RCU-based object protection for XDP_REDIRECT | expand |
| Context | Check | Description |
|---|---|---|
| netdev/cover_letter | success | Link |
| netdev/fixes_present | success | Link |
| netdev/patch_count | fail | Series longer than 15 patches |
| netdev/tree_selection | success | Clearly marked for bpf-next |
| netdev/subject_prefix | success | Link |
| netdev/cc_maintainers | warning | 10 maintainers not CCed: yhs@fb.com kpsingh@kernel.org daniel@iogearbox.net andrii@kernel.org hawk@kernel.org ast@kernel.org john.fastabend@gmail.com songliubraving@fb.com davem@davemloft.net kuba@kernel.org |
| netdev/source_inline | success | Was 0 now: 0 |
| netdev/verify_signedoff | success | Link |
| netdev/module_param | success | Was 0 now: 0 |
| netdev/build_32bit | success | Errors and warnings before: 0 this patch: 0 |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/verify_fixes | success | Link |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 9 lines checked |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 0 this patch: 0 |
| netdev/header_inline | success | Link |
On Wed, Jun 09, 2021 at 12:33:15PM +0200, Toke Høiland-Jørgensen wrote: > The bnxt driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP > program invocations. However, the actual lifetime of the objects referred > by the XDP program invocation is longer, all the way through to the call to > xdp_do_flush(), making the scope of the rcu_read_lock() too small. This > turns out to be harmless because it all happens in a single NAPI poll > cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock() > misleading. > > Rather than extend the scope of the rcu_read_lock(), just get rid of it > entirely. With the addition of RCU annotations to the XDP_REDIRECT map > types that take bh execution into account, lockdep even understands this to > be safe, so there's really no reason to keep it around. And same for the rest of these removals. Someone might be very happy to have that comment at some later date, and that someone just might be you. ;-) Thanx, Paul > Cc: Michael Chan <michael.chan@broadcom.com> > Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> > --- > drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > index ec9564e584e0..bee6e091a997 100644 > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c > @@ -138,9 +138,7 @@ bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons, > xdp_prepare_buff(&xdp, *data_ptr - offset, offset, *len, false); > orig_data = xdp.data; > > - rcu_read_lock(); > act = bpf_prog_run_xdp(xdp_prog, &xdp); > - rcu_read_unlock(); > > tx_avail = bnxt_tx_avail(bp, txr); > /* If the tx ring is not full, we must not update the rx producer yet > -- > 2.31.1 >
"Paul E. McKenney" <paulmck@kernel.org> writes: > On Wed, Jun 09, 2021 at 12:33:15PM +0200, Toke Høiland-Jørgensen wrote: >> The bnxt driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP >> program invocations. However, the actual lifetime of the objects referred >> by the XDP program invocation is longer, all the way through to the call to >> xdp_do_flush(), making the scope of the rcu_read_lock() too small. This >> turns out to be harmless because it all happens in a single NAPI poll >> cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock() >> misleading. >> >> Rather than extend the scope of the rcu_read_lock(), just get rid of it >> entirely. With the addition of RCU annotations to the XDP_REDIRECT map >> types that take bh execution into account, lockdep even understands this to >> be safe, so there's really no reason to keep it around. > > And same for the rest of these removals. Someone might be very happy > to have that comment at some later date, and that someone just might > be you. ;-) Bah, why do you have to go and make sensible suggestions like that? ;) Will wait for Martin's review and add this in a v2. BTW, is it OK to include your patch in the series like this, or should I rather request that your tree be merged into bpf-next? -Toke
diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c index ec9564e584e0..bee6e091a997 100644 --- a/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c @@ -138,9 +138,7 @@ bool bnxt_rx_xdp(struct bnxt *bp, struct bnxt_rx_ring_info *rxr, u16 cons, xdp_prepare_buff(&xdp, *data_ptr - offset, offset, *len, false); orig_data = xdp.data; - rcu_read_lock(); act = bpf_prog_run_xdp(xdp_prog, &xdp); - rcu_read_unlock(); tx_avail = bnxt_tx_avail(bp, txr); /* If the tx ring is not full, we must not update the rx producer yet
The bnxt driver has rcu_read_lock()/rcu_read_unlock() pairs around XDP program invocations. However, the actual lifetime of the objects referred by the XDP program invocation is longer, all the way through to the call to xdp_do_flush(), making the scope of the rcu_read_lock() too small. This turns out to be harmless because it all happens in a single NAPI poll cycle (and thus under local_bh_disable()), but it makes the rcu_read_lock() misleading. Rather than extend the scope of the rcu_read_lock(), just get rid of it entirely. With the addition of RCU annotations to the XDP_REDIRECT map types that take bh execution into account, lockdep even understands this to be safe, so there's really no reason to keep it around. Cc: Michael Chan <michael.chan@broadcom.com> Signed-off-by: Toke Høiland-Jørgensen <toke@redhat.com> --- drivers/net/ethernet/broadcom/bnxt/bnxt_xdp.c | 2 -- 1 file changed, 2 deletions(-)