On Wed, 13 Dec 2017 14:50:07 -0800
Saeed Mahameed <sae...@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 <bro...@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
