| Message ID | 20230908135748.794163-5-bigeasy@linutronix.de (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Add missing xdp_do_flush() invocations. | expand |
Hi, On 9/8/2023 9:57 PM, Sebastian Andrzej Siewior wrote: > xdp_do_flush() should be invoked before leaving the NAPI poll function > if XDP-redirect has been performed. > > There are two possible XDP invocations in cpu_map_bpf_prog_run(): > - cpu_map_bpf_prog_run_xdp() > - cpu_map_bpf_prog_run_skb() > > Both functions update stats->redirect if a redirect is performed but > xdp_do_flush() is only invoked after the former. > > Invoke xdp_do_flush() after both functions run and a redirect was > performed. > > Cc: Andrii Nakryiko <andrii@kernel.org> > Cc: Hao Luo <haoluo@google.com> > Cc: Jiri Olsa <jolsa@kernel.org> > Cc: KP Singh <kpsingh@kernel.org> > Cc: Martin KaFai Lau <martin.lau@linux.dev> > Cc: Song Liu <song@kernel.org> > Cc: Stanislav Fomichev <sdf@google.com> > Cc: Yonghong Song <yonghong.song@linux.dev> > Fixes: 11941f8a85362 ("bpf: cpumap: Implement generic cpumap") > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> > --- > kernel/bpf/cpumap.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c > index e42a1bdb7f536..f3ba11b4a732b 100644 > --- a/kernel/bpf/cpumap.c > +++ b/kernel/bpf/cpumap.c > @@ -248,12 +248,12 @@ static int cpu_map_bpf_prog_run(struct bpf_cpu_map_entry *rcpu, void **frames, > > nframes = cpu_map_bpf_prog_run_xdp(rcpu, frames, xdp_n, stats); > > - if (stats->redirect) > - xdp_do_flush(); > - > if (unlikely(!list_empty(list))) > cpu_map_bpf_prog_run_skb(rcpu, list, stats); > > + if (stats->redirect) > + xdp_do_flush(); > + The purpose of xdp_do_flush() is to flush xdp frames stashed in per-cpu cpu_map_flush list into xdp_bulk_queue. But for redirected skbs, these skbs will be directly added into xdp_bulk_queue() in cpu_map_generic_redirect(), so I think xdp_do_flush() is not needed for redirected skbs. > rcu_read_unlock_bh(); /* resched point, may call do_softirq() */ > > return nframes;
On 2023-09-09 10:49:09 [+0800], Hou Tao wrote: > Hi, Hi, > > --- a/kernel/bpf/cpumap.c > > +++ b/kernel/bpf/cpumap.c > > @@ -248,12 +248,12 @@ static int cpu_map_bpf_prog_run(struct bpf_cpu_map_entry *rcpu, void **frames, > > > > nframes = cpu_map_bpf_prog_run_xdp(rcpu, frames, xdp_n, stats); > > > > - if (stats->redirect) > > - xdp_do_flush(); > > - > > if (unlikely(!list_empty(list))) > > cpu_map_bpf_prog_run_skb(rcpu, list, stats); > > > > + if (stats->redirect) > > + xdp_do_flush(); > > + > > The purpose of xdp_do_flush() is to flush xdp frames stashed in per-cpu > cpu_map_flush list into xdp_bulk_queue. But for redirected skbs, these > skbs will be directly added into xdp_bulk_queue() in > cpu_map_generic_redirect(), so I think xdp_do_flush() is not needed for > redirected skbs. Now that I checked the down streams of cpu_map_bpf_prog_run_skb() it does not queue anything to the per-CPU lists like I assumed. You are right, it is no needed. Sorry for the confusion. > > rcu_read_unlock_bh(); /* resched point, may call do_softirq() */ > > > > return nframes; > Sebastian
diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c index e42a1bdb7f536..f3ba11b4a732b 100644 --- a/kernel/bpf/cpumap.c +++ b/kernel/bpf/cpumap.c @@ -248,12 +248,12 @@ static int cpu_map_bpf_prog_run(struct bpf_cpu_map_entry *rcpu, void **frames, nframes = cpu_map_bpf_prog_run_xdp(rcpu, frames, xdp_n, stats); - if (stats->redirect) - xdp_do_flush(); - if (unlikely(!list_empty(list))) cpu_map_bpf_prog_run_skb(rcpu, list, stats); + if (stats->redirect) + xdp_do_flush(); + rcu_read_unlock_bh(); /* resched point, may call do_softirq() */ return nframes;
xdp_do_flush() should be invoked before leaving the NAPI poll function if XDP-redirect has been performed. There are two possible XDP invocations in cpu_map_bpf_prog_run(): - cpu_map_bpf_prog_run_xdp() - cpu_map_bpf_prog_run_skb() Both functions update stats->redirect if a redirect is performed but xdp_do_flush() is only invoked after the former. Invoke xdp_do_flush() after both functions run and a redirect was performed. Cc: Andrii Nakryiko <andrii@kernel.org> Cc: Hao Luo <haoluo@google.com> Cc: Jiri Olsa <jolsa@kernel.org> Cc: KP Singh <kpsingh@kernel.org> Cc: Martin KaFai Lau <martin.lau@linux.dev> Cc: Song Liu <song@kernel.org> Cc: Stanislav Fomichev <sdf@google.com> Cc: Yonghong Song <yonghong.song@linux.dev> Fixes: 11941f8a85362 ("bpf: cpumap: Implement generic cpumap") Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de> --- kernel/bpf/cpumap.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)