Re: [PATCH v3 2/2] net: core: support XDP generic on stacked devices.

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

On Thu, 23 May 2019 13:15:44 -0700
Stephen Hemminger <stephen@networkplumber.org> wrote:

> On Thu, 23 May 2019 19:19:40 +0000
> Saeed Mahameed <saeedm@mellanox.com> wrote:
> 
> > On Thu, 2019-05-23 at 10:54 -0700, Stephen Hemminger wrote:  
> > > When a device is stacked like (team, bonding, failsafe or netvsc) the
> > > XDP generic program for the parent device was not called.
> > > 
> > > Move the call to XDP generic inside __netif_receive_skb_core where
> > > it can be done multiple times for stacked case.
> > > 
> > > Suggested-by: Jiri Pirko <jiri@resnulli.us>
> > > Fixes: d445516966dc ("net: xdp: support xdp generic on virtual
> > > devices")
> > > Signed-off-by: Stephen Hemminger <sthemmin@microsoft.com>
> > > ---
> > > v1 - call xdp_generic in netvsc handler
> > > v2 - do xdp_generic in generic rx handler processing
> > > v3 - move xdp_generic call inside the another pass loop
> > > 
> > >  net/core/dev.c | 56 ++++++++++------------------------------------
> > > ----
> > >  1 file changed, 11 insertions(+), 45 deletions(-)
> > > 
> > > diff --git a/net/core/dev.c b/net/core/dev.c
> > > index b6b8505cfb3e..696776e14d00 100644
> > > --- a/net/core/dev.c
> > > +++ b/net/core/dev.c
> > > @@ -4502,23 +4502,6 @@ static int netif_rx_internal(struct sk_buff
> > > *skb)
> > >  
> > >  	trace_netif_rx(skb);
> > >  
> > > -	if (static_branch_unlikely(&generic_xdp_needed_key)) {
> > > -		int ret;
> > > -
> > > -		preempt_disable();
> > > -		rcu_read_lock();
> > > -		ret = do_xdp_generic(rcu_dereference(skb->dev-    
> > > >xdp_prog), skb);    
> > > -		rcu_read_unlock();
> > > -		preempt_enable();
> > > -
> > > -		/* Consider XDP consuming the packet a success from
> > > -		 * the netdev point of view we do not want to count
> > > -		 * this as an error.
> > > -		 */
> > > -		if (ret != XDP_PASS)
> > > -			return NET_RX_SUCCESS;
> > > -	}
> > > -    
> > 
> > Adding Jesper, 
> > 
> > There is a small behavioral change due to this patch, 
> > the XDP program after this patch will run on the RPS CPU, if
> > configured, which could cause some behavioral changes in
> > xdp_redirect_cpu: bpf_redirect_map(cpu_map).
> > 
> > Maybe this is acceptable, but it should be documented, as the current
> > assumption dictates: XDP program runs on the core where the XDP
> > frame/SKB was first seen.  

This does break some assumptions, that I worry about.  I've not
optimized generic XDP much, as this is suppose to be slow-path, but as
you can see in my evaluation[1] generic-XDP do have a performance
potential (XDP drop: native=12Mpps and generic=8.4Mpps), but the
XDP-generic performance dies as soon as we e.g. do XDP_TX
(native=10Mpps and generic=4Mpps).  The reason is lack of bulking.

We could implement the same kind of RX-bulking tricks as we do for
XDP_REDIRECT, where bpf_redirect_map store frames in the map and send
them once NAPI-poll exit via a xdp_do_flush_map().  These tricks
depend on per-CPU data (bpf_redirect_info), thus I cannot see how this
could work if XDP-generic now happens after RPS on a remote CPU.

Notice, that TX bulking at XDP-generic level, is actually rather
simple, as netstack TX path-code support xmit_more via creating a list
of SKBs... Last time I hacked it up, I saw 20%-30% speedup... anyone
motivated to do this?

> 
> Or maybe XDP should just force off RPS (like it does gro)

I guess, we could force off RPS.  But I do see one valid use-case of
combining CPUMAP redirect with RFS (Receive Flow Steering) which is part
of RPS.  Yes, I know we/I *also* have to implement generic-XDP-cpumap
support. But for native-XDP CPUMAP redirect, from 1-2 RX-CPUs into
N-remote CPUs via CPUMAP, and then lets RFS send SKBs to where the
application runs, does make sense to me. (I do have an "assignment" to
implement this in eBPF here[2]).


[1] https://github.com/netoptimizer/prototype-kernel/blob/master/kernel/Documentation/blogposts/xdp25_eval_generic_xdp_tx.rst

[2] https://github.com/xdp-project/xdp-project/blob/master/areas/cpumap.org#cpumap-implement-dynamic-load-balancer-that-is-ooo-safe
-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer