All of lore.kernel.org
 help / color / mirror / Atom feed
From: Yunsheng Lin <linyunsheng@huawei.com>
To: Alexander Duyck <alexander.duyck@gmail.com>
Cc: David Miller <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Russell King - ARM Linux <linux@armlinux.org.uk>,
	Marcin Wojtas <mw@semihalf.com>, <linuxarm@openeuler.org>,
	<yisen.zhuang@huawei.com>, "Salil Mehta" <salil.mehta@huawei.com>,
	<thomas.petazzoni@bootlin.com>, <hawk@kernel.org>,
	Ilias Apalodimas <ilias.apalodimas@linaro.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	Daniel Borkmann <daniel@iogearbox.net>,
	"John Fastabend" <john.fastabend@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Peter Zijlstra <peterz@infradead.org>,
	"Will Deacon" <will@kernel.org>,
	Matthew Wilcox <willy@infradead.org>,
	"Vlastimil Babka" <vbabka@suse.cz>, <fenghua.yu@intel.com>,
	<guro@fb.com>, Peter Xu <peterx@redhat.com>,
	Feng Tang <feng.tang@intel.com>, Jason Gunthorpe <jgg@ziepe.ca>,
	Matteo Croce <mcroce@microsoft.com>,
	Hugh Dickins <hughd@google.com>,
	Jonathan Lemon <jonathan.lemon@gmail.com>,
	"Alexander Lobakin" <alobakin@pm.me>,
	Willem de Bruijn <willemb@google.com>, <wenxu@ucloud.cn>,
	Cong Wang <cong.wang@bytedance.com>,
	Kevin Hao <haokexin@gmail.com>, <nogikh@google.com>,
	Marco Elver <elver@google.com>, Yonghong Song <yhs@fb.com>,
	<kpsingh@kernel.org>, <andrii@kernel.org>,
	"Martin KaFai Lau" <kafai@fb.com>, <songliubraving@fb.com>,
	Netdev <netdev@vger.kernel.org>,
	LKML <linux-kernel@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Subject: Re: [PATCH rfc v6 2/4] page_pool: add interface to manipulate frag count in page pool
Date: Fri, 23 Jul 2021 19:12:56 +0800	[thread overview]
Message-ID: <41283c5f-2f58-7fa7-e8fe-a91207a57353@huawei.com> (raw)
In-Reply-To: <CAKgT0UcBgo0Ex=x514qGeLvppJr-0vqx9ZngAFDTwugjtKUrOA@mail.gmail.com>

On 2021/7/22 23:18, Alexander Duyck wrote:
>>>
>>>> You are right that that may cover up the reference count errors. How about
>>>> something like below:
>>>>
>>>> static inline long page_pool_atomic_sub_frag_count_return(struct page *page,
>>>>                                                           long nr)
>>>> {
>>>> #ifdef CONFIG_DEBUG_PAGE_REF
>>>>         long ret = atomic_long_sub_return(nr, &page->pp_frag_count);
>>>>
>>>>         WARN_ON(ret < 0);
>>>>
>>>>         return ret;
>>>> #else
>>>>         if (atomic_long_read(&page->pp_frag_count) == nr)
>>>>                 return 0;
>>>>
>>>>         return atomic_long_sub_return(nr, &page->pp_frag_count);
>>>> #end
>>>> }
>>>>
>>>> Or any better suggestion?
>>>
>>> So the one thing I might change would be to make it so that you only
>>> do the atomic_long_read if nr is a constant via __builtin_constant_p.
>>> That way you would be performing the comparison in
>>> __page_pool_put_page and in the cases of freeing or draining the
>>> page_frags you would be using the atomic_long_sub_return which should
>>> be paths where you would not expect it to match or that are slowpath
>>> anyway.
>>>
>>> Also I would keep the WARN_ON in both paths just to be on the safe side.
>>
>> If I understand it correctly, we should change it as below, right?
>>
>> static inline long page_pool_atomic_sub_frag_count_return(struct page *page,
>>                                                           long nr)
>> {
>>         long ret;
>>
>>         /* 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
>>          * not expect it to match or that are slowpath anyway.
>>          */
>>         if (__builtin_constant_p(nr) &&
>>             atomic_long_read(&page->pp_frag_count) == nr)
>>                 return 0;
>>
>>         ret = atomic_long_sub_return(nr, &page->pp_frag_count);
>>         WARN_ON(ret < 0);
>>         return ret;
>> }
> 
> Yes, that is what I had in mind.
> 
> One thought I had for a future optimization is that we could look at
> reducing the count by 1 so that we could essentially combine the
> non-frag and frag cases.Then instead of testing for 1 we would test
> for 0 at thee start of the function and test for < 0 to decide if we
> want to free it or not instead of testing for 0. With that we can
> essentially reduce the calls to the WARN_ON since we should only have
> one case where we actually return a value < 0, and we can then check
> to see if we overshot -1 which would be the WARN_ON case.
> 
> With that a value of 0 instead of 1 would indicate page frag is not in
> use for the page *AND/OR* that the page has reached the state where
> there are no other frags present so the page can be recycled. In
> effect it would allow us to mix page frags and no frags within the
> same pool. The added bonus would be we could get rid of the check for
> PP_FLAG_PAGE_FRAG flag check in the __page_pool_put_page function and
> replace it with a check for PAGE_POOL_DMA_USE_PP_FRAG_COUNT since we
> cannot read frag_count in that case.

Let's leave it for a future optimization.
I am not sure if there is use case to support both frag page and non-frag
page for the same page pool. If there is, maybe we can use "page->pp_frag_count
> 0" to indicate that the page is frag page, and "page->pp_frag_count == 0"
to indicate that the page is non-frag page, so that we can support frag page and
non-frag page for the same page pool instead of disabling non-frag page support
when PP_FLAG_PAGE_FRAG flag is set, which might be conflit with the above
optimization?


Also, I am prototyping the tx recycling based on page pool in order to see
if there is any value supporting the tx recycling.
As the busypoll has enable the one-to-one relation between NAPI and sock,
and there is one-to-one relation between NAPI and page pool, perhaps it make
senses that we use page pool to recycle the tx page too?

There are possibly below problems when doing that as I am aware of now:
1. busypoll is for rx, and tx may not be using the same queue as rx even if
   there are *technically* the same flow, so I am not sure it is ok to use
   busypoll infrastructure to get the page pool ptr for a specific sock.

2. There may be multi socks using the same page pool ptr to allocate page for
   multi flow, so we can not assume the same NAPI polling protection as rx,
   which might mean we can only use the recyclable page from pool->ring under the
   r->consumer_lock protection.

3. Right now tcp_sendmsg_locked() use sk_page_frag_refill() to refill the page
   frag for tcp xmit, when implementing a similar sk_page_pool_frag_refill()
   based on page pool, I found that tcp coalesce in tcp_mtu_probe() and
   tcp fragment in tso_fragment() might mess with the page_ref_count directly.

As the above the problem I am aware of(I believe there are other problems I am not
aware of yet), I am not sure if the tcp tx page recycling based on page pool is
doable or not, I would like to hear about your opinion about tcp tx recycling support
based on page pool first, in case it is a dead end to support that.

> .
> 

  reply	other threads:[~2021-07-23 11:13 UTC|newest]

Thread overview: 17+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-07-20  3:35 [PATCH rfc v6 0/4] add frag page support " Yunsheng Lin
2021-07-20  3:35 ` [PATCH rfc v6 1/4] page_pool: keep pp info as long as page pool owns the page Yunsheng Lin
2021-07-20  3:35 ` [PATCH rfc v6 2/4] page_pool: add interface to manipulate frag count in page pool Yunsheng Lin
2021-07-20 15:43   ` Alexander Duyck
2021-07-21  8:15     ` Yunsheng Lin
2021-07-21 14:06       ` Alexander Duyck
2021-07-22  8:07         ` Yunsheng Lin
2021-07-22 15:18           ` Alexander Duyck
2021-07-23 11:12             ` Yunsheng Lin [this message]
2021-07-23 16:08               ` Alexander Duyck
2021-07-24 13:07                 ` Yunsheng Lin
2021-07-25 16:49                   ` Alexander Duyck
2021-07-27  7:54                     ` Yunsheng Lin
2021-07-27 18:38                       ` Alexander Duyck
2021-08-02  9:17                         ` Yunsheng Lin
2021-07-20  3:35 ` [PATCH rfc v6 3/4] page_pool: add frag page recycling support " Yunsheng Lin
2021-07-20  3:35 ` [PATCH rfc v6 4/4] net: hns3: support skb's frag page recycling based on " 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=41283c5f-2f58-7fa7-e8fe-a91207a57353@huawei.com \
    --to=linyunsheng@huawei.com \
    --cc=akpm@linux-foundation.org \
    --cc=alexander.duyck@gmail.com \
    --cc=alobakin@pm.me \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=cong.wang@bytedance.com \
    --cc=daniel@iogearbox.net \
    --cc=davem@davemloft.net \
    --cc=elver@google.com \
    --cc=feng.tang@intel.com \
    --cc=fenghua.yu@intel.com \
    --cc=guro@fb.com \
    --cc=haokexin@gmail.com \
    --cc=hawk@kernel.org \
    --cc=hughd@google.com \
    --cc=ilias.apalodimas@linaro.org \
    --cc=jgg@ziepe.ca \
    --cc=john.fastabend@gmail.com \
    --cc=jonathan.lemon@gmail.com \
    --cc=kafai@fb.com \
    --cc=kpsingh@kernel.org \
    --cc=kuba@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux@armlinux.org.uk \
    --cc=linuxarm@openeuler.org \
    --cc=mcroce@microsoft.com \
    --cc=mw@semihalf.com \
    --cc=netdev@vger.kernel.org \
    --cc=nogikh@google.com \
    --cc=peterx@redhat.com \
    --cc=peterz@infradead.org \
    --cc=salil.mehta@huawei.com \
    --cc=songliubraving@fb.com \
    --cc=thomas.petazzoni@bootlin.com \
    --cc=vbabka@suse.cz \
    --cc=wenxu@ucloud.cn \
    --cc=will@kernel.org \
    --cc=willemb@google.com \
    --cc=willy@infradead.org \
    --cc=yhs@fb.com \
    --cc=yisen.zhuang@huawei.com \
    --subject='Re: [PATCH rfc v6 2/4] page_pool: add interface to manipulate frag count in page pool' \
    /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

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.