On Thu, 25 Jun 2020 23:28:59 +0200
Daniel Borkmann [off-list ref] wrote:

quoted

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.

quoted

+			} 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