| Message ID | 151316400161.14967.13516011331263133183.stgit@firesoul |
|---|---|
| State | RFC, archived |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | xdp: new XDP rx-queue info concept | expand |
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 :) > /** > 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? 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...
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).
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; /** 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 @@ -1608,11 +1608,13 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget) unsigned int true_bufsz; struct sk_buff *skb; int pkts_polled = 0; + struct xdp_buff xdp; int idx; rcu_read_lock(); xdp_prog = READ_ONCE(dp->xdp_prog); true_bufsz = xdp_prog ? PAGE_SIZE : dp->fl_bufsz; + xdp.rxq = &rx_ring->xdp_rxq; tx_ring = r_vec->xdp_ring; while (pkts_polled < budget) { @@ -1703,7 +1705,6 @@ static int nfp_net_rx(struct nfp_net_rx_ring *rx_ring, int budget) dp->bpf_offload_xdp) && !meta.portid) { void *orig_data = rxbuf->frag + pkt_off; unsigned int dma_off; - struct xdp_buff xdp; int act; xdp.data_hard_start = rxbuf->frag + NFP_NET_RX_BUF_HEADROOM; @@ -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,
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> --- drivers/net/ethernet/netronome/nfp/nfp_net.h | 5 ++++- .../net/ethernet/netronome/nfp/nfp_net_common.c | 10 +++++++++- 2 files changed, 13 insertions(+), 2 deletions(-)