netdev - Re: [bpf-next V5 PATCH 03/15] ixgbe: use xdp_return

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:   Fri, 23 Mar 2018 09:46:25 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Jesper Dangaard Brouer <brouer@...hat.com>
Cc:     Netdev <netdev@...r.kernel.org>,
        BjörnTöpel <bjorn.topel@...el.com>,
        "Karlsson, Magnus" <magnus.karlsson@...el.com>,
        Eugenia Emantayev <eugenia@...lanox.com>,
        Jason Wang <jasowang@...hat.com>,
        John Fastabend <john.fastabend@...il.com>,
        Eran Ben Elisha <eranbe@...lanox.com>,
        Saeed Mahameed <saeedm@...lanox.com>,
        Gal Pressman <galp@...lanox.com>,
        Daniel Borkmann <borkmann@...earbox.net>,
        Alexei Starovoitov <alexei.starovoitov@...il.com>,
        Tariq Toukan <tariqt@...lanox.com>
Subject: Re: [bpf-next V5 PATCH 03/15] ixgbe: use xdp_return_frame API

On Fri, Mar 23, 2018 at 5:18 AM, Jesper Dangaard Brouer
<brouer@...hat.com> wrote:
> Extend struct ixgbe_tx_buffer to store the xdp_mem_info.
>
> Signed-off-by: Jesper Dangaard Brouer <brouer@...hat.com>
> ---
>  drivers/net/ethernet/intel/ixgbe/ixgbe.h      |    1 +
>  drivers/net/ethernet/intel/ixgbe/ixgbe_main.c |    6 ++++--
>  2 files changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> index c1e3a0039ea5..cbc20f199364 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
> @@ -249,6 +249,7 @@ struct ixgbe_tx_buffer {
>         DEFINE_DMA_UNMAP_ADDR(dma);
>         DEFINE_DMA_UNMAP_LEN(len);
>         u32 tx_flags;
> +       struct xdp_mem_info xdp_mem;

Instead of increasing the size of this structure it might make more
sense to find free space somewhere in side of it.

One thing you could probably look at doing is making it so that the
gso_segs, protocol, and tx_flags fields could be put into an anonymous
structure that is then part of a union with the xdp_mem_info
structure. Then you would just have to tweak things slightly so that
the else section for the ring_is_xdp block pulls the code just above
it into that section so that those fields are not read.

You would need to flip the dma, length, and type field ordering but
that shouldn't be much of an issue and it should have no effect on
performance since they are all in the same 16 byte aligned block
anyway.

>  };
>
>  struct ixgbe_rx_buffer {
> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> index 85369423452d..45520eb503ee 100644
> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
> @@ -1207,7 +1207,7 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
>
>                 /* free the skb */
>                 if (ring_is_xdp(tx_ring))
> -                       page_frag_free(tx_buffer->data);
> +                       xdp_return_frame(tx_buffer->data, &tx_buffer->xdp_mem);

Based on my suggestion above this section would have:
    total_packets++;

>                 else
>                         napi_consume_skb(tx_buffer->skb, napi_budget);

This section would pull the lines that read gso_segs and tx_flags down
into this else statement so that the fields go unused.

>
> @@ -5787,7 +5787,7 @@ static void ixgbe_clean_tx_ring(struct ixgbe_ring *tx_ring)
>
>                 /* Free all the Tx ring sk_buffs */
>                 if (ring_is_xdp(tx_ring))
> -                       page_frag_free(tx_buffer->data);
> +                       xdp_return_frame(tx_buffer->data, &tx_buffer->xdp_mem);
>                 else
>                         dev_kfree_skb_any(tx_buffer->skb);
>
> @@ -8351,6 +8351,8 @@ static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter,
>         dma_unmap_len_set(tx_buffer, len, len);
>         dma_unmap_addr_set(tx_buffer, dma, dma);
>         tx_buffer->data = xdp->data;
> +       tx_buffer->xdp_mem = xdp->rxq->mem;
> +

It would work out better if you moved this up to the lines that are
setting the tx_buffer fields. Really you could probably pull the line
that is recording xdp->data up as well.

>         tx_desc->read.buffer_addr = cpu_to_le64(dma);
>
>         /* put descriptor type bits */
>