Re: [PATCH v4 bpf-next 6/9] bpf: cpumap: implement XDP_REDIRECT for eBPF programs attached to map entries

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Thu, 25 Jun 2020 23:28:59 +0200
Daniel Borkmann <daniel@iogearbox.net> 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