All of lore.kernel.org
 help / color / mirror / Atom feed
From: Alexander Duyck <alexander.duyck@gmail.com>
To: Yunsheng Lin <linyunsheng@huawei.com>
Cc: David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>, Netdev <netdev@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>,
	linuxarm@openeuler.org, hawk@kernel.org,
	Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	Jonathan Lemon <jonathan.lemon@gmail.com>,
	Alexander Lobakin <alobakin@pm.me>,
	Willem de Bruijn <willemb@google.com>,
	Cong Wang <cong.wang@bytedance.com>,
	Paolo Abeni <pabeni@redhat.com>, Kevin Hao <haokexin@gmail.com>,
	nogikh@google.com, Marco Elver <elver@google.com>,
	memxor@gmail.com, Eric Dumazet <edumazet@google.com>,
	David Ahern <dsahern@gmail.com>
Subject: Re: [PATCH net-next 1/2] page_pool: support non-split page with PP_FLAG_PAGE_FRAG
Date: Mon, 30 Aug 2021 08:05:17 -0700	[thread overview]
Message-ID: <CAKgT0UfNFw+jwoDr_xx6kX_OoCVgrq2rCSc4zdXRMSZLBmbA8Q@mail.gmail.com> (raw)
In-Reply-To: <1630286290-43714-2-git-send-email-linyunsheng@huawei.com>

On Sun, Aug 29, 2021 at 6:19 PM Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> Currently when PP_FLAG_PAGE_FRAG is set, the caller is not
> expected to call page_pool_alloc_pages() directly because of
> the PP_FLAG_PAGE_FRAG checking in __page_pool_put_page().
>
> The patch removes the above checking to enable non-split page
> support when PP_FLAG_PAGE_FRAG is set.
>
> Signed-off-by: Yunsheng Lin <linyunsheng@huawei.com>
> ---
>  include/net/page_pool.h |  6 ++++++
>  net/core/page_pool.c    | 12 +++++++-----
>  2 files changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index a408240..2ad0706 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -238,6 +238,9 @@ static inline void page_pool_set_dma_addr(struct page *page, dma_addr_t addr)
>
>  static inline void page_pool_set_frag_count(struct page *page, long nr)
>  {
> +       if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT)
> +               return;
> +
>         atomic_long_set(&page->pp_frag_count, nr);
>  }
>
> @@ -246,6 +249,9 @@ static inline long page_pool_atomic_sub_frag_count_return(struct page *page,
>  {
>         long ret;
>
> +       if (PAGE_POOL_DMA_USE_PP_FRAG_COUNT)
> +               return 0;
> +
>         /* As suggested by Alexander, atomic_long_read() may cover up the
>          * reference count errors, so avoid calling atomic_long_read() in
>          * the cases of freeing or draining the page_frags, where we would
> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> index 1a69784..ba9f14d 100644
> --- a/net/core/page_pool.c
> +++ b/net/core/page_pool.c
> @@ -313,11 +313,14 @@ struct page *page_pool_alloc_pages(struct page_pool *pool, gfp_t gfp)
>
>         /* Fast-path: Get a page from cache */
>         page = __page_pool_get_cached(pool);
> -       if (page)
> -               return page;
>
>         /* Slow-path: cache empty, do real allocation */
> -       page = __page_pool_alloc_pages_slow(pool, gfp);
> +       if (!page)
> +               page = __page_pool_alloc_pages_slow(pool, gfp);
> +
> +       if (likely(page))
> +               page_pool_set_frag_count(page, 1);
> +
>         return page;
>  }
>  EXPORT_SYMBOL(page_pool_alloc_pages);
> @@ -426,8 +429,7 @@ __page_pool_put_page(struct page_pool *pool, struct page *page,
>                      unsigned int dma_sync_size, bool allow_direct)
>  {
>         /* It is not the last user for the page frag case */
> -       if (pool->p.flags & PP_FLAG_PAGE_FRAG &&
> -           page_pool_atomic_sub_frag_count_return(page, 1))
> +       if (page_pool_atomic_sub_frag_count_return(page, 1))
>                 return NULL;

Isn't this going to have a negative performance impact on page pool
pages in general? Essentially you are adding an extra atomic operation
for all the non-frag pages.

It would work better if this was doing a check against 1 to determine
if it is okay for this page to be freed here and only if the check
fails then you perform the atomic sub_return.

  reply	other threads:[~2021-08-30 15:05 UTC|newest]

Thread overview: 12+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-30  1:18 [PATCH net-next 0/2] some optimization for page pool Yunsheng Lin
2021-08-30  1:18 ` [PATCH net-next 1/2] page_pool: support non-split page with PP_FLAG_PAGE_FRAG Yunsheng Lin
2021-08-30 15:05   ` Alexander Duyck [this message]
2021-08-31  6:14     ` Yunsheng Lin
2021-08-31 13:43       ` Alexander Duyck
2021-08-30  1:18 ` [PATCH net-next 2/2] skbuff: keep track of pp page when __skb_frag_ref() is called Yunsheng Lin
2021-08-30  4:50   ` kernel test robot
2021-08-30  4:50     ` kernel test robot
2021-08-30 15:14   ` Alexander Duyck
2021-08-31  7:20     ` Yunsheng Lin
2021-08-31 14:30       ` Alexander Duyck
2021-09-01  3:10         ` Yunsheng Lin

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAKgT0UfNFw+jwoDr_xx6kX_OoCVgrq2rCSc4zdXRMSZLBmbA8Q@mail.gmail.com \
    --to=alexander.duyck@gmail.com \
    --cc=alobakin@pm.me \
    --cc=cong.wang@bytedance.com \
    --cc=davem@davemloft.net \
    --cc=dsahern@gmail.com \
    --cc=edumazet@google.com \
    --cc=elver@google.com \
    --cc=haokexin@gmail.com \
    --cc=hawk@kernel.org \
    --cc=ilias.apalodimas@linaro.org \
    --cc=jonathan.lemon@gmail.com \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linuxarm@openeuler.org \
    --cc=linyunsheng@huawei.com \
    --cc=memxor@gmail.com \
    --cc=netdev@vger.kernel.org \
    --cc=nogikh@google.com \
    --cc=pabeni@redhat.com \
    --cc=willemb@google.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.