Re: [bpf-next V1-RFC PATCH 12/14] xdp: generic XDP handling of xdp_rxq_info

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

On Wed, 13 Dec 2017 14:50:07 -0800
Saeed Mahameed <saeedm@mellanox.com> wrote:

> On 12/13/2017 3:20 AM, Jesper Dangaard Brouer wrote:
> > Hook points for xdp_rxq_info:
> >   * init+reg: netif_alloc_rx_queues
> >   * unreg   : netif_free_rx_queues
> > 
> > The net_device have some members (num_rx_queues + real_num_rx_queues)
> > and data-area (dev->_rx with struct netdev_rx_queue's) that were
> > primarily used for exporting information about RPS (CONFIG_RPS) queues
> > to sysfs (CONFIG_SYSFS).
> > 
> > For generic XDP extend struct netdev_rx_queue with the xdp_rxq_info,
> > and remove some of the CONFIG_SYSFS ifdefs.
> > 
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>
> > ---
> >   include/linux/netdevice.h |    2 ++
> >   net/core/dev.c            |   60 ++++++++++++++++++++++++++++++++++++++-------
> >   2 files changed, 52 insertions(+), 10 deletions(-)
> > 
> > diff --git a/include/linux/netdevice.h b/include/linux/netdevice.h
> > index cc4ce7456e38..43595b037872 100644
> > --- a/include/linux/netdevice.h
> > +++ b/include/linux/netdevice.h
> > @@ -44,6 +44,7 @@
> >   #include <net/dcbnl.h>
> >   #endif
> >   #include <net/netprio_cgroup.h>
> > +#include <net/xdp.h>
> >   
> >   #include <linux/netdev_features.h>
> >   #include <linux/neighbour.h>
> > @@ -686,6 +687,7 @@ struct netdev_rx_queue {
> >   #endif
> >   	struct kobject			kobj;
> >   	struct net_device		*dev;
> > +	struct xdp_rxq_info		xdp_rxq;
> >   } ____cacheline_aligned_in_smp;
> >     
> 
> Instead of duplicating this xdp_rxq_info and have 2 instances of it for 
> drivers that do support XDP (the generic one and the driver internal 
> xdp_rxq_info), drivers can use the generic netdev_rx_queue.xdp_rxq to 
> register their own xdp_rxq_info.
> I suggest the following API for drivers to use:
> 
> xdp_rxq_info_reg(netdev, rxq_index)
> {
> 	rxqueue = dev->_rx + rxq_index;
> 	xdp_rxq = rxqueue.xdp_rxq;
>          xdp_rxq_info_init(xdp_rxq);
> 	xdp_rxq.dev = netdev;
> 	xdp_rxq.queue_index = rxq_index;
> }
> 
> xdp_rxq_info_unreg(netdev, rxq_index)
> {
> ...
> }
> 
> This way you can avoid the xdp_rxq_info structure management by the 
> drivers them selves and reduce duplicated code to init, fill the 
> xdp_rxq_info per driver.

Having the xdp_rxq_info struct memory associated with the net_device,
and not the drivers RX-rings, make it harder for the drivers to use
this API.  Because drivers (e.g. i40e) to minimize "downtime" when
e.g. changing numbers of queues (ethtool set_channels) allocate and
setup new (RX+TX+XDP_TX)-rings _before_ taking down current RX-rings.
Thus, you have the problem of driver wanting to register two
xdp_rxq_info's at the same time, before unregistering the old.

Another issue I've seen is that, drivers do funny tricks, and you
cannot always assuming that rxq_index would be valid and bind directly
to dev->_rx + rxq_index.  E.g. in the i40e driver they have
vsi->num_queue_pairs they iterate over, and one of the
vsi->rx_rings[i] is a special flow-director (FDIR) ring that comes in
the end of the rx_rings[] array but internally have queue_index zero.

I was also hoping to unifying/integrating the generic-XDP REDIRECT
code path, by assigning the net_device stored xdp_rxq_info another
qtype "SKB-MODE-TYPE".  Allowing us to share more of the native-XDP
redirect-code path with SKB-XDP frames from generic-XDP.

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer