On Thu, 25 Jun 2020 23:28:59 +0200
Daniel Borkmann <daniel@xxxxxxxxxxxxx> wrote:
> > diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> > index 4e4cd240f07b..c0b2f265ccb2 100644
> > --- a/kernel/bpf/cpumap.c
> > +++ b/kernel/bpf/cpumap.c
> > @@ -240,7 +240,7 @@ static int cpu_map_bpf_prog_run_xdp(struct bpf_cpu_map_entry *rcpu,
> > xdp_set_return_frame_no_direct();
> > xdp.rxq = &rxq;
> >
> > - rcu_read_lock();
> > + rcu_read_lock_bh();
> >
> > prog = READ_ONCE(rcpu->prog);
> > for (i = 0; i < n; i++) {
> > @@ -266,6 +266,16 @@ static int cpu_map_bpf_prog_run_xdp(struct bpf_cpu_map_entry *rcpu,
> > stats->pass++;
> > }
> > break;
> > + case XDP_REDIRECT:
> > + err = xdp_do_redirect(xdpf->dev_rx, &xdp,
> > + prog);
> > + if (unlikely(err)) {
> > + xdp_return_frame(xdpf);
> > + stats->drop++;
I consider if this should be a redir_err counter.
> > + } else {
> > + stats->redirect++;
> > + }
>
> Could we do better with all the accounting and do this from /inside/ BPF tracing prog
> instead (otherwise too bad we need to have it here even if the tracepoint is disabled)?
I'm on-the-fence with this one...
First of all the BPF-prog cannot see the return code of xdp_do_redirect.
So, it cannot give the correct/needed stats without this counter. It
would basically report the redirects as successful redirects. (This is
actually a re-occuring support issue, when end-users misconfigure
xdp_redirect sample and think they get good performance, even-though
packets are dropped).
Specifically for XDP_REDIRECT we need to update some state anyhow, such
that we know to call xdp_do_flush_map(). Thus removing the counter
would not gain much performance wise.
--
Best regards,
Jesper Dangaard Brouer
MSc.CS, Principal Kernel Engineer at Red Hat
LinkedIn: http://www.linkedin.com/in/brouer
