netdev - Re: [PATCH net-next] page_pool: Rename frag_users to frag

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]

Message-ID: <CAC_iWjJBcXu=Zz=UtDj1vR-s5+jhFx8GYoYpqOi-bQX7S3XgbA@mail.gmail.com>
Date: Thu, 21 Dec 2023 10:24:13 +0200
From: Ilias Apalodimas <ilias.apalodimas@...aro.org>
To: Yunsheng Lin <linyunsheng@...wei.com>
Cc: netdev@...r.kernel.org, Jesper Dangaard Brouer <hawk@...nel.org>, 
	"David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next] page_pool: Rename frag_users to frag_cnt

Hi Yunsheng,

On Thu, 21 Dec 2023 at 10:00, Yunsheng Lin <linyunsheng@...wei.com> wrote:
>
> On 2023/12/21 14:37, Ilias Apalodimas wrote:
> > Hi Yunsheng,
> >
> > On Thu, 21 Dec 2023 at 04:07, Yunsheng Lin <linyunsheng@...wei.com> wrote:
> >>
> >> On 2023/12/20 15:56, Ilias Apalodimas wrote:
> >>> Hi Yunsheng,
> >>>>>>>  #ifdef CONFIG_PAGE_POOL_STATS
> >>>>>>>       /* these stats are incremented while in softirq context */
> >>>>>>> diff --git a/net/core/page_pool.c b/net/core/page_pool.c
> >>>>>>> index 9b203d8660e4..19a56a52ac8f 100644
> >>>>>>> --- a/net/core/page_pool.c
> >>>>>>> +++ b/net/core/page_pool.c
> >>>>>>> @@ -659,7 +659,7 @@ EXPORT_SYMBOL(page_pool_put_page_bulk);
> >>>>>>>  static struct page *page_pool_drain_frag(struct page_pool *pool,
> >>>>>>>                                        struct page *page)
> >>>>>>>  {
> >>>>>>> -     long drain_count = BIAS_MAX - pool->frag_users;
> >>>>>>> +     long drain_count = BIAS_MAX - pool->frag_cnt;
> >>>>>>
> >>>>>> drain_count = pool->refcnt_bais;
> >>>>>
> >>>>> I think this is a typo right? This still remains
> >>>>
> >>>> It would be better to invert logic too, as it is mirroring:
> >>>>
> >>>> https://elixir.bootlin.com/linux/v6.7-rc5/source/mm/page_alloc.c#L4745
> >>>
> >>> This is still a bit confusing for me since the actual bias is the
> >>> number of fragments that you initially split the page. But I am fine
> >> Acctually there are two bais numbers for a page used by
> >> page_pool_alloc_frag().
> >> the one for page->pp_ref_count, which already use the BIAS_MAX, which
> >> indicates the initial bais number:
> >> https://elixir.bootlin.com/linux/latest/source/net/core/page_pool.c#L779
> >>
> >> Another one for pool->frag_users indicating the runtime bais number, which
> >> need changing when a page is split into more fragments:
> >> https://elixir.bootlin.com/linux/latest/source/net/core/page_pool.c#L776
> >> https://elixir.bootlin.com/linux/latest/source/net/core/page_pool.c#L783
> >
> > I know, and that's exactly what my commit message explains.  Also,
> > that's the reason that the rename was 'frag_cnt' on v1.
> >
>
> Yes, I think we do not need to invert logic when the naming is frag_users
> or frag_cnt.
>

frag_users is kind of wrong, because 'users' basically means refcnt.
frag_cnt on the other hand is clear. It shows the number of fragments
we split the page. But we are in bikeshedding territory now :)

> But if we use 'bias' as part of the name, isn't that more reasonable to set
> both of the bias number to BIAS_MAX initially, and decrement the runtime
> bais number every time the page is split to more fragments?

I think it's a matter of taste and how you interpret BIAS_MAX. In any
case, page_pool_drain_frag() will eventually set the *real* number of
references. But since the code can get complicated I like the idea of
making it identical to the mm subsystem tracking.

Can we just merge v2 and me or you can send the logic inversion
patches right after. They are orthogonal to the rename anyway

Cheers
/Ilias

>
> >