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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 13 Dec 2017 18:34:27 -0800
From:   Jakub Kicinski <jakub.kicinski@...ronome.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        dsahern@...il.com, oss-drivers@...ronome.com,
        Simon Horman <simon.horman@...ronome.com>,
        netdev@...r.kernel.org, gospo@...adcom.com, bjorn.topel@...el.com,
        michael.chan@...adcom.com
Subject: Re: [bpf-next V1-RFC PATCH 08/14] nfp: setup xdp_rxq_info

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@...ronome.com
> Cc: Jakub Kicinski <jakub.kicinski@...ronome.com>
> Cc: Simon Horman <simon.horman@...ronome.com>
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.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...