netdev - Re: [Intel-wired-lan] [PATCH v2 1/4] i40e: clean zero-copy XDP Tx ring on shutdown/reset

Open Source and information security mailing list archives

Message-ID: <CAJ+HfNgYwWo1Cmx81SGSRAPEtVzKvogaWSL17vPuorYpx-U-Lw@mail.gmail.com>
Date:   Fri, 21 Sep 2018 09:35:51 +0200
From:   Björn Töpel <bjorn.topel@...il.com>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>,
        intel-wired-lan <intel-wired-lan@...ts.osuosl.org>
Cc:     Netdev <netdev@...r.kernel.org>,
        Björn Töpel <bjorn.topel@...el.com>,
        Magnus Karlsson <magnus.karlsson@...il.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>,
        Daniel Borkmann <daniel@...earbox.net>, ast@...nel.org
Subject: Re: [Intel-wired-lan] [PATCH v2 1/4] i40e: clean zero-copy XDP Tx
 ring on shutdown/reset

Jeff,

Den fre 7 sep. 2018 kl 10:29 skrev Björn Töpel <bjorn.topel@...il.com>:
>
> From: Björn Töpel <bjorn.topel@...el.com>
>
> When the zero-copy enabled XDP Tx ring is torn down, due to
> configuration changes, outstandning frames on the hardware descriptor
> ring are queued on the completion ring.
>
> The completion ring has a back-pressure mechanism that will guarantee
> that there is sufficient space on the ring.
>
> Signed-off-by: Björn Töpel <bjorn.topel@...el.com>
> ---
>  drivers/net/ethernet/intel/i40e/i40e_txrx.c   | 17 +++++++----
>  .../ethernet/intel/i40e/i40e_txrx_common.h    |  2 ++
>  drivers/net/ethernet/intel/i40e/i40e_xsk.c    | 30 +++++++++++++++++++
>  3 files changed, 43 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx.c b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> index 37bd4e50ccde..7f85d4ba8b54 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx.c
> @@ -636,13 +636,18 @@ void i40e_clean_tx_ring(struct i40e_ring *tx_ring)
>         unsigned long bi_size;
>         u16 i;
>
> -       /* ring already cleared, nothing to do */
> -       if (!tx_ring->tx_bi)
> -               return;
> +       if (ring_is_xdp(tx_ring) && tx_ring->xsk_umem) {
> +               i40e_xsk_clean_tx_ring(tx_ring);
> +       } else {
> +               /* ring already cleared, nothing to do */
> +               if (!tx_ring->tx_bi)
> +                       return;
>
> -       /* Free all the Tx ring sk_buffs */
> -       for (i = 0; i < tx_ring->count; i++)
> -               i40e_unmap_and_free_tx_resource(tx_ring, &tx_ring->tx_bi[i]);
> +               /* Free all the Tx ring sk_buffs */
> +               for (i = 0; i < tx_ring->count; i++)
> +                       i40e_unmap_and_free_tx_resource(tx_ring,
> +                                                       &tx_ring->tx_bi[i]);
> +       }
>
>         bi_size = sizeof(struct i40e_tx_buffer) * tx_ring->count;
>         memset(tx_ring->tx_bi, 0, bi_size);
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_txrx_common.h b/drivers/net/ethernet/intel/i40e/i40e_txrx_common.h
> index b5afd479a9c5..29c68b29d36f 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_txrx_common.h
> +++ b/drivers/net/ethernet/intel/i40e/i40e_txrx_common.h
> @@ -87,4 +87,6 @@ static inline void i40e_arm_wb(struct i40e_ring *tx_ring,
>         }
>  }
>
> +void i40e_xsk_clean_tx_ring(struct i40e_ring *tx_ring);
> +
>  #endif /* I40E_TXRX_COMMON_ */
> diff --git a/drivers/net/ethernet/intel/i40e/i40e_xsk.c b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> index 2ebfc78bbd09..99116277c4d2 100644
> --- a/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> +++ b/drivers/net/ethernet/intel/i40e/i40e_xsk.c
> @@ -830,3 +830,33 @@ int i40e_xsk_async_xmit(struct net_device *dev, u32 queue_id)
>
>         return 0;
>  }
> +
> +/**
> + * i40e_xsk_clean_xdp_ring - Clean the XDP Tx ring on shutdown
> + * @xdp_ring: XDP Tx ring
> + **/
> +void i40e_xsk_clean_tx_ring(struct i40e_ring *tx_ring)
> +{
> +       u16 ntc = tx_ring->next_to_clean, ntu = tx_ring->next_to_use;
> +       struct xdp_umem *umem = tx_ring->xsk_umem;
> +       struct i40e_tx_buffer *tx_bi;
> +       u32 xsk_frames = 0;
> +
> +       while (ntc != ntu) {
> +               tx_bi = &tx_ring->tx_bi[ntc];
> +
> +               if (tx_bi->xdpf)
> +                       i40e_clean_xdp_tx_buffer(tx_ring, tx_bi);
> +               else
> +                       xsk_frames++;
> +
> +               tx_bi->xdpf = NULL;
> +
> +               ntc++;
> +               if (ntc > tx_ring->count)

This is an off-by-one error, and should be:
if (ntc == tx_ring->count)

Can you fix it up, or should I respin the patch?

Thanks!
Björn

> +                       ntc = 0;
> +       }
> +
> +       if (xsk_frames)
> +               xsk_umem_complete_tx(umem, xsk_frames);
> +}
> --
> 2.17.1
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@...osl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

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