Message-ID: <CAKgT0Ue+VvnzNUuKiO1XFW6w3Ka9=SSfGBP_KpkbvR6uzqvg5A@mail.gmail.com>
Date:   Thu, 6 Jul 2023 07:47:03 -0700
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Alexander Lobakin <aleksander.lobakin@...el.com>
Cc:     "David S. Miller" <davem@...emloft.net>,
        Eric Dumazet <edumazet@...gle.com>,
        Jakub Kicinski <kuba@...nel.org>,
        Paolo Abeni <pabeni@...hat.com>,
        Paul Menzel <pmenzel@...gen.mpg.de>,
        Jesper Dangaard Brouer <hawk@...nel.org>,
        Larysa Zaremba <larysa.zaremba@...el.com>,
        netdev@...r.kernel.org, Alexander Duyck <alexanderduyck@...com>,
        Ilias Apalodimas <ilias.apalodimas@...aro.org>,
        linux-kernel@...r.kernel.org,
        Yunsheng Lin <linyunsheng@...wei.com>,
        Michal Kubiak <michal.kubiak@...el.com>,
        intel-wired-lan@...ts.osuosl.org,
        David Christensen <drc@...ux.vnet.ibm.com>
Subject: Re: [Intel-wired-lan] [PATCH RFC net-next v4 3/9] iavf: drop page
 splitting and recycling

On Wed, Jul 5, 2023 at 8:57 AM Alexander Lobakin
<aleksander.lobakin@...el.com> wrote:
>
> As an intermediate step, remove all page splitting/recyclig code. Just

Spelling issue: "recycling"

> always allocate a new page and don't touch its refcount, so that it gets
> freed by the core stack later.
> Same for the "in-place" recycling, i.e. when an unused buffer gets
> assigned to a first needs-refilling descriptor. In some cases, this
> was leading to moving up to 63 &iavf_rx_buf structures around the ring
> on a per-field basis -- not something wanted on hotpath.
> The change allows to greatly simplify certain parts of the code:
>
> Function: add/remove: 0/2 grow/shrink: 1/3 up/down: 3/-494 (-491)
>
> Although the array of &iavf_rx_buf is barely used now and could be
> replaced with just page pointer array, don't touch it now to not
> complicate replacing it with libie Rx buffer struct later on.
> No surprise perf loses up to 30% here, but that regression will go
> away once PP lands.
>
> Signed-off-by: Alexander Lobakin <aleksander.lobakin@...el.com>
> ---
>  drivers/net/ethernet/intel/iavf/iavf_txrx.c | 151 ++------------------
>  drivers/net/ethernet/intel/iavf/iavf_txrx.h |   8 --
>  2 files changed, 13 insertions(+), 146 deletions(-)
>
> diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.c b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
> index a85b270fc769..789b10815d7f 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_txrx.c
> +++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.c
> @@ -723,7 +723,7 @@ static void iavf_clean_rx_ring(struct iavf_ring *rx_ring)
>                                      DMA_FROM_DEVICE,
>                                      IAVF_RX_DMA_ATTR);
>
> -               __page_frag_cache_drain(rx_bi->page, rx_bi->pagecnt_bias);
> +               __free_pages(rx_bi->page, iavf_rx_pg_order(rx_ring));
>
>                 rx_bi->page = NULL;
>                 rx_bi->page_offset = 0;
> @@ -735,7 +735,6 @@ static void iavf_clean_rx_ring(struct iavf_ring *rx_ring)
>         /* Zero out the descriptor ring */
>         memset(rx_ring->desc, 0, rx_ring->size);
>
> -       rx_ring->next_to_alloc = 0;
>         rx_ring->next_to_clean = 0;
>         rx_ring->next_to_use = 0;
>  }
> @@ -791,7 +790,6 @@ int iavf_setup_rx_descriptors(struct iavf_ring *rx_ring)
>                 goto err;
>         }
>
> -       rx_ring->next_to_alloc = 0;
>         rx_ring->next_to_clean = 0;
>         rx_ring->next_to_use = 0;
>
> @@ -811,9 +809,6 @@ static inline void iavf_release_rx_desc(struct iavf_ring *rx_ring, u32 val)
>  {
>         rx_ring->next_to_use = val;
>
> -       /* update next to alloc since we have filled the ring */
> -       rx_ring->next_to_alloc = val;
> -
>         /* Force memory writes to complete before letting h/w
>          * know there are new descriptors to fetch.  (Only
>          * applicable for weak-ordered memory model archs,
> @@ -837,12 +832,6 @@ static bool iavf_alloc_mapped_page(struct iavf_ring *rx_ring,
>         struct page *page = bi->page;
>         dma_addr_t dma;
>
> -       /* since we are recycling buffers we should seldom need to alloc */
> -       if (likely(page)) {
> -               rx_ring->rx_stats.page_reuse_count++;
> -               return true;
> -       }
> -
>         /* alloc new page for storage */
>         page = dev_alloc_pages(iavf_rx_pg_order(rx_ring));
>         if (unlikely(!page)) {
> @@ -869,9 +858,6 @@ static bool iavf_alloc_mapped_page(struct iavf_ring *rx_ring,
>         bi->page = page;
>         bi->page_offset = IAVF_SKB_PAD;
>
> -       /* initialize pagecnt_bias to 1 representing we fully own page */
> -       bi->pagecnt_bias = 1;
> -
>         return true;
>  }
>
> @@ -1103,91 +1089,6 @@ static bool iavf_cleanup_headers(struct iavf_ring *rx_ring, struct sk_buff *skb)
>         return false;
>  }
>
> -/**
> - * iavf_reuse_rx_page - page flip buffer and store it back on the ring
> - * @rx_ring: rx descriptor ring to store buffers on
> - * @old_buff: donor buffer to have page reused
> - *
> - * Synchronizes page for reuse by the adapter
> - **/
> -static void iavf_reuse_rx_page(struct iavf_ring *rx_ring,
> -                              struct iavf_rx_buffer *old_buff)
> -{
> -       struct iavf_rx_buffer *new_buff;
> -       u16 nta = rx_ring->next_to_alloc;
> -
> -       new_buff = &rx_ring->rx_bi[nta];
> -
> -       /* update, and store next to alloc */
> -       nta++;
> -       rx_ring->next_to_alloc = (nta < rx_ring->count) ? nta : 0;
> -
> -       /* transfer page from old buffer to new buffer */
> -       new_buff->dma           = old_buff->dma;
> -       new_buff->page          = old_buff->page;
> -       new_buff->page_offset   = old_buff->page_offset;
> -       new_buff->pagecnt_bias  = old_buff->pagecnt_bias;
> -}
> -
> -/**
> - * iavf_can_reuse_rx_page - Determine if this page can be reused by
> - * the adapter for another receive
> - *
> - * @rx_buffer: buffer containing the page
> - *
> - * If page is reusable, rx_buffer->page_offset is adjusted to point to
> - * an unused region in the page.
> - *
> - * For small pages, @truesize will be a constant value, half the size
> - * of the memory at page.  We'll attempt to alternate between high and
> - * low halves of the page, with one half ready for use by the hardware
> - * and the other half being consumed by the stack.  We use the page
> - * ref count to determine whether the stack has finished consuming the
> - * portion of this page that was passed up with a previous packet.  If
> - * the page ref count is >1, we'll assume the "other" half page is
> - * still busy, and this page cannot be reused.
> - *
> - * For larger pages, @truesize will be the actual space used by the
> - * received packet (adjusted upward to an even multiple of the cache
> - * line size).  This will advance through the page by the amount
> - * actually consumed by the received packets while there is still
> - * space for a buffer.  Each region of larger pages will be used at
> - * most once, after which the page will not be reused.
> - *
> - * In either case, if the page is reusable its refcount is increased.
> - **/
> -static bool iavf_can_reuse_rx_page(struct iavf_rx_buffer *rx_buffer)
> -{
> -       unsigned int pagecnt_bias = rx_buffer->pagecnt_bias;
> -       struct page *page = rx_buffer->page;
> -
> -       /* Is any reuse possible? */
> -       if (!dev_page_is_reusable(page))
> -               return false;
> -
> -#if (PAGE_SIZE < 8192)
> -       /* if we are only owner of page we can reuse it */
> -       if (unlikely((page_count(page) - pagecnt_bias) > 1))
> -               return false;
> -#else
> -#define IAVF_LAST_OFFSET \
> -       (SKB_WITH_OVERHEAD(PAGE_SIZE) - IAVF_RXBUFFER_2048)
> -       if (rx_buffer->page_offset > IAVF_LAST_OFFSET)
> -               return false;
> -#endif
> -
> -       /* If we have drained the page fragment pool we need to update
> -        * the pagecnt_bias and page count so that we fully restock the
> -        * number of references the driver holds.
> -        */
> -       if (unlikely(!pagecnt_bias)) {
> -               page_ref_add(page, USHRT_MAX);
> -               rx_buffer->pagecnt_bias = USHRT_MAX;
> -       }
> -
> -       return true;
> -}
> -
>  /**
>   * iavf_add_rx_frag - Add contents of Rx buffer to sk_buff
>   * @rx_ring: rx descriptor ring to transact packets on
> @@ -1216,13 +1117,6 @@ static void iavf_add_rx_frag(struct iavf_ring *rx_ring,
>
>         skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_buffer->page,
>                         rx_buffer->page_offset, size, truesize);
> -
> -       /* page is being used so we must update the page offset */
> -#if (PAGE_SIZE < 8192)
> -       rx_buffer->page_offset ^= truesize;
> -#else
> -       rx_buffer->page_offset += truesize;
> -#endif
>  }
>
>  /**
> @@ -1250,9 +1144,6 @@ static struct iavf_rx_buffer *iavf_get_rx_buffer(struct iavf_ring *rx_ring,
>                                       size,
>                                       DMA_FROM_DEVICE);
>
> -       /* We have pulled a buffer for use, so decrement pagecnt_bias */
> -       rx_buffer->pagecnt_bias--;
> -
>         return rx_buffer;
>  }
>
> @@ -1293,23 +1184,15 @@ static struct sk_buff *iavf_build_skb(struct iavf_ring *rx_ring,
>         skb_reserve(skb, IAVF_SKB_PAD);
>         __skb_put(skb, size);
>
> -       /* buffer is used by skb, update page_offset */
> -#if (PAGE_SIZE < 8192)
> -       rx_buffer->page_offset ^= truesize;
> -#else
> -       rx_buffer->page_offset += truesize;
> -#endif
> -
>         return skb;
>  }
>
>  /**
> - * iavf_put_rx_buffer - Clean up used buffer and either recycle or free
> + * iavf_put_rx_buffer - Unmap used buffer
>   * @rx_ring: rx descriptor ring to transact packets on
>   * @rx_buffer: rx buffer to pull data from
>   *
> - * This function will clean up the contents of the rx_buffer.  It will
> - * either recycle the buffer or unmap it and free the associated resources.
> + * This function will unmap the buffer after it's written by HW.
>   */
>  static void iavf_put_rx_buffer(struct iavf_ring *rx_ring,
>                                struct iavf_rx_buffer *rx_buffer)
> @@ -1317,21 +1200,10 @@ static void iavf_put_rx_buffer(struct iavf_ring *rx_ring,
>         if (!rx_buffer)
>                 return;
>
> -       if (iavf_can_reuse_rx_page(rx_buffer)) {
> -               /* hand second half of page back to the ring */
> -               iavf_reuse_rx_page(rx_ring, rx_buffer);
> -               rx_ring->rx_stats.page_reuse_count++;
> -       } else {
> -               /* we are not reusing the buffer so unmap it */
> -               dma_unmap_page_attrs(rx_ring->dev, rx_buffer->dma,
> -                                    iavf_rx_pg_size(rx_ring),
> -                                    DMA_FROM_DEVICE, IAVF_RX_DMA_ATTR);
> -               __page_frag_cache_drain(rx_buffer->page,
> -                                       rx_buffer->pagecnt_bias);
> -       }
> -
> -       /* clear contents of buffer_info */
> -       rx_buffer->page = NULL;
> +       /* we are not reusing the buffer so unmap it */
> +       dma_unmap_page_attrs(rx_ring->dev, rx_buffer->dma,
> +                            iavf_rx_pg_size(rx_ring),
> +                            DMA_FROM_DEVICE, IAVF_RX_DMA_ATTR);

Rather than reorder all this I would just do the dma_unmap_page_attrs
and then leave the assignment of NULL to rx_buffer->page. It should
make this a bit easier to clean up the code below.

>  }
>
>  /**
> @@ -1431,15 +1303,18 @@ static int iavf_clean_rx_irq(struct iavf_ring *rx_ring, int budget)
>                 else
>                         skb = iavf_build_skb(rx_ring, rx_buffer, size);
>
> +               iavf_put_rx_buffer(rx_ring, rx_buffer);
> +

This should stay below where it was.

>                 /* exit if we failed to retrieve a buffer */
>                 if (!skb) {
>                         rx_ring->rx_stats.alloc_buff_failed++;
> -                       if (rx_buffer && size)
> -                               rx_buffer->pagecnt_bias++;
> +                       __free_pages(rx_buffer->page,
> +                                    iavf_rx_pg_order(rx_ring));
> +                       rx_buffer->page = NULL;
>                         break;
>                 }

This code was undoing the iavf_get_rx_buffer decrement of pagecnt_bias
and then bailing since we have halted forward progress due to an skb
allocation failure. As such we should just be removing the if
statement and the increment of pagecnt_bias.

>
> -               iavf_put_rx_buffer(rx_ring, rx_buffer);
> +               rx_buffer->page = NULL;
>                 cleaned_count++;
>
>                 if (iavf_is_non_eop(rx_ring, rx_desc, skb))

If iavf_put_rx_buffer just does the unmap and assignment of NULL then
it could just be left here as is.

> diff --git a/drivers/net/ethernet/intel/iavf/iavf_txrx.h b/drivers/net/ethernet/intel/iavf/iavf_txrx.h
> index 4b412f7662e4..2170a77f8c8d 100644
> --- a/drivers/net/ethernet/intel/iavf/iavf_txrx.h
> +++ b/drivers/net/ethernet/intel/iavf/iavf_txrx.h
> @@ -266,12 +266,7 @@ struct iavf_tx_buffer {
>  struct iavf_rx_buffer {
>         dma_addr_t dma;
>         struct page *page;
> -#if (BITS_PER_LONG > 32) || (PAGE_SIZE >= 65536)
>         __u32 page_offset;
> -#else
> -       __u16 page_offset;
> -#endif
> -       __u16 pagecnt_bias;
>  };
>
>  struct iavf_queue_stats {
> @@ -293,8 +288,6 @@ struct iavf_rx_queue_stats {
>         u64 non_eop_descs;
>         u64 alloc_page_failed;
>         u64 alloc_buff_failed;
> -       u64 page_reuse_count;
> -       u64 realloc_count;
>  };
>
>  enum iavf_ring_state_t {
> @@ -374,7 +367,6 @@ struct iavf_ring {
>         struct iavf_q_vector *q_vector; /* Backreference to associated vector */
>
>         struct rcu_head rcu;            /* to avoid race on free */
> -       u16 next_to_alloc;
>         struct sk_buff *skb;            /* When iavf_clean_rx_ring_irq() must
>                                          * return before it sees the EOP for
>                                          * the current packet, we save that skb
> --
> 2.41.0
>
> _______________________________________________
> Intel-wired-lan mailing list
> Intel-wired-lan@...osl.org
> https://lists.osuosl.org/mailman/listinfo/intel-wired-lan

Powered by blists - more mailing lists