Re: [bpf-next V1-RFC PATCH 08/14] nfp: setup xdp_rxq_info

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

On Wed, 13 Dec 2017 18:34:27 -0800
Jakub Kicinski <jakub.kicinski@netronome.com> wrote:

> On Wed, 13 Dec 2017 12:20:01 +0100, Jesper Dangaard Brouer wrote:
> > Driver hook points for xdp_rxq_info:
> >  * init+reg: nfp_net_rx_ring_alloc
> >  * unreg   : nfp_net_rx_ring_free
> > 
> > In struct nfp_net_rx_ring moved member @size into a hole on 64-bit.
> > Thus, the size remaines the same after adding member @xdp_rxq.
> > 
> > Cc: oss-drivers@netronome.com
> > Cc: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Cc: Simon Horman <simon.horman@netronome.com>
> > Signed-off-by: Jesper Dangaard Brouer <brouer@redhat.com>  
> 
> > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net.h b/drivers/net/ethernet/netronome/nfp/nfp_net.h
> > index 3801c52098d5..0e564cfabe7e 100644
> > --- a/drivers/net/ethernet/netronome/nfp/nfp_net.h
> > +++ b/drivers/net/ethernet/netronome/nfp/nfp_net.h
> > @@ -47,6 +47,7 @@
> >  #include <linux/netdevice.h>
> >  #include <linux/pci.h>
> >  #include <linux/io-64-nonatomic-hi-lo.h>
> > +#include <net/xdp.h>
> >  
> >  #include "nfp_net_ctrl.h"
> >  
> > @@ -350,6 +351,7 @@ struct nfp_net_rx_buf {
> >   * @rxds:       Virtual address of FL/RX ring in host memory
> >   * @dma:        DMA address of the FL/RX ring
> >   * @size:       Size, in bytes, of the FL/RX ring (needed to free)
> > + * @xdp_rxq:    RX-ring info avail for XDP
> >   */
> >  struct nfp_net_rx_ring {
> >  	struct nfp_net_r_vector *r_vec;
> > @@ -361,13 +363,14 @@ struct nfp_net_rx_ring {
> >  	u32 idx;
> >  
> >  	int fl_qcidx;
> > +	unsigned int size;
> >  	u8 __iomem *qcp_fl;
> >  
> >  	struct nfp_net_rx_buf *rxbufs;
> >  	struct nfp_net_rx_desc *rxds;
> >  
> >  	dma_addr_t dma;
> > -	unsigned int size;
> > +	struct xdp_rxq_info xdp_rxq;
> >  } ____cacheline_aligned;  
> 
> The @size member is not in the hole on purpose.  IIRC all the members
> up to @dma are in the first cacheline.  All things which are not
> needed on the fast path are after @dma.  IOW @size is not used on the
> fast path and the hole is for fast path stuff :)

Yes, I did notice @size was not used on fast-path, but it didn't hurt
to move it up.  I was just excited to see I could add this without
increasing the rx_ring struct size.

I'm more and more considering Ahern's suggestion of returning an err,
and if I do so, I also want to do proper allocation of xdp_rxq_info,
which means this will be converted into a pointer instead (and thus
much smaller effect on rx_ring size).


> >  /**
> > diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> > index ad3e9f6a61e5..6474aecd0451 100644
> > --- a/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> > +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_common.c
> > @@ -2252,6 +2253,7 @@ static void nfp_net_rx_ring_free(struct nfp_net_rx_ring *rx_ring)
> >  	struct nfp_net_r_vector *r_vec = rx_ring->r_vec;
> >  	struct nfp_net_dp *dp = &r_vec->nfp_net->dp;
> >  
> > +	xdp_rxq_info_unreg(&rx_ring->xdp_rxq);
> >  	kfree(rx_ring->rxbufs);
> >  
> >  	if (rx_ring->rxds)
> > @@ -2277,6 +2279,12 @@ nfp_net_rx_ring_alloc(struct nfp_net_dp *dp, struct nfp_net_rx_ring *rx_ring)
> >  {
> >  	int sz;
> >  
> > +	/* XDP RX-queue info */
> > +	xdp_rxq_info_init(&rx_ring->xdp_rxq);
> > +	rx_ring->xdp_rxq.dev = dp->netdev;
> > +	rx_ring->xdp_rxq.queue_index = rx_ring->idx;
> > +	xdp_rxq_info_reg(&rx_ring->xdp_rxq);
> > +
> >  	rx_ring->cnt = dp->rxd_cnt;
> >  	rx_ring->size = sizeof(*rx_ring->rxds) * rx_ring->cnt;
> >  	rx_ring->rxds = dma_zalloc_coherent(dp->dev, rx_ring->size,  
> 
> The nfp driver implements the prepare/commit for reallocating rings.  
> I don't think it matters now, but there can be 2 sets of rings with the
> same ID allocated during reconfiguration (see nfp_net_ring_reconfig()).
> Maybe place the register/unregister in nfp_net_open_stack() and
> nfp_net_close_stack() respectively?

Going over the your driver code again, I do think I handle this
correctly in nfp_net_rx_ring_free() / nfp_net_rx_ring_alloc().

Your calls nfp_net_open_stack() / nfp_net_close_stack(), doesn't
support failing, which conflicts with Ahern's suggestion.

As I explained, in another reply, I do want to support having 2 sets
of rings during reconfiguration, as many drivers do this.  This is also
the reason I cannot use net_device->_rx[] area.

> Perhaps that won't be necessary, only cleaner :)  I'm not sure how is
> the redirect between drivers intended to work WRT freeing rings and
> unloading drivers while packets fly...

I do have a plan for handling in-flight packets when driver is being
unloaded... that is the reason for having the unreg call. (Sorry, I
should have included you in that offlist discussion).

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