netdev - Re: [bpf-next V2 PATCH 2/8] i40e: implement flush flag for ndo_xdp

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]

Message-ID: <9c934312-5ba2-ccff-193a-8cbc17473324@iogearbox.net>
Date:   Mon, 4 Jun 2018 15:11:02 +0200
From:   Daniel Borkmann <daniel@...earbox.net>
To:     Jesper Dangaard Brouer <brouer@...hat.com>, netdev@...r.kernel.org,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>
Cc:     liu.song.a23@...il.com, songliubraving@...com,
        John Fastabend <john.fastabend@...il.com>
Subject: Re: [bpf-next V2 PATCH 2/8] i40e: implement flush flag for
 ndo_xdp_xmit

On 05/31/2018 10:59 AM, Jesper Dangaard Brouer wrote:
> When passed the XDP_XMIT_FLUSH flag i40e_xdp_xmit now performs the
> same kind of ring tail update as in i40e_xdp_flush.  The advantage is
> that all the necessary checks have been performed and xdp_ring can be
> updated, instead of having to perform the exact same steps/checks in
> i40e_xdp_flush
> 
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c |   10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index c0451d6e0790..5f01e4ce9c92 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -3676,6 +3676,7 @@ int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
>  	struct i40e_netdev_priv *np = netdev_priv(dev);
>  	unsigned int queue_index = smp_processor_id();
>  	struct i40e_vsi *vsi = np->vsi;
> +	struct i40e_ring *xdp_ring;
>  	int drops = 0;
>  	int i;
>  
> @@ -3685,20 +3686,25 @@ int i40e_xdp_xmit(struct net_device *dev, int n, struct xdp_frame **frames,
>  	if (!i40e_enabled_xdp_vsi(vsi) || queue_index >= vsi->num_queue_pairs)
>  		return -ENXIO;
>  
> -	if (unlikely(flags & ~XDP_XMIT_FLAGS_NONE))
> +	if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
>  		return -EINVAL;
>  
> +	xdp_ring = vsi->xdp_rings[queue_index];
> +
>  	for (i = 0; i < n; i++) {
>  		struct xdp_frame *xdpf = frames[i];
>  		int err;
>  
> -		err = i40e_xmit_xdp_ring(xdpf, vsi->xdp_rings[queue_index]);
> +		err = i40e_xmit_xdp_ring(xdpf, xdp_ring);
>  		if (err != I40E_XDP_TX) {
>  			xdp_return_frame_rx_napi(xdpf);
>  			drops++;
>  		}
>  	}
>  
> +	if (unlikely(flags & XDP_XMIT_FLUSH))
> +		i40e_xdp_ring_update_tail(xdp_ring);

In addition to Alexei's feedback, I'd remove the unlikely() on the flush from here and the
ixgbe one like you did on the rest of the drivers in the series, just let CPU decide.

For the invalid flags case it's totally fine and in fact you could probably do this for all
three cases where you bail out in the beginning of i40e_xdp_xmit() and won't able able to
send anything anyway:

        if (test_bit(__I40E_VSI_DOWN, vsi->state))
                return -ENETDOWN;

        if (!i40e_enabled_xdp_vsi(vsi) || queue_index >= vsi->num_queue_pairs)
                return -ENXIO;

        if (unlikely(flags & ~XDP_XMIT_FLAGS_MASK))
                return -EINVAL;

Thanks,
Daniel