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

[Ilias Apalodimas]

Hi Yunsheng,

On Thu, 21 Dec 2023 at 10:00, Yunsheng Lin <linyunsheng@xxxxxxxxxx> wrote:
>
> On 2023/12/21 14:37, Ilias Apalodimas wrote:
> > Hi Yunsheng,
> >
> > On Thu, 21 Dec 2023 at 04:07, Yunsheng Lin <linyunsheng@xxxxxxxxxx> 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

>
> >