Since [0] got merged, it's clear that 'pp_ref_count' is used to track
the number of users for each page. On struct_page though we have
a member called 'frag_users'. Despite of what the name suggests this is
not the number of users. It instead represents the number of fragments of
the current page. When we have a single page this is set to one. When we
split the page this is set to the actual number of frags and later used
in page_pool_drain_frag() to infer the real number of users.
So let's rename it to something that matches the description above
[0]
Link: https://lore.kernel.org/netdev/[email protected]/
Signed-off-by: Ilias Apalodimas <[email protected]>
---
include/net/page_pool.h | 2 +-
net/core/page_pool.c | 8 ++++----
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/include/net/page_pool.h b/include/net/page_pool.h
index 813c93499f20..957cd84bb3f4 100644
--- a/include/net/page_pool.h
+++ b/include/net/page_pool.h
@@ -158,7 +158,7 @@ struct page_pool {
u32 pages_state_hold_cnt;
unsigned int frag_offset;
struct page *frag_page;
- long frag_users;
+ long frag_cnt;
#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;
/* Some user is still using the page frag */
if (likely(page_pool_defrag_page(page, drain_count)))
@@ -678,7 +678,7 @@ static struct page *page_pool_drain_frag(struct page_pool *pool,
static void page_pool_free_frag(struct page_pool *pool)
{
- long drain_count = BIAS_MAX - pool->frag_users;
+ long drain_count = BIAS_MAX - pool->frag_cnt;
struct page *page = pool->frag_page;
pool->frag_page = NULL;
@@ -721,14 +721,14 @@ struct page *page_pool_alloc_frag(struct page_pool *pool,
pool->frag_page = page;
frag_reset:
- pool->frag_users = 1;
+ pool->frag_cnt = 1;
*offset = 0;
pool->frag_offset = size;
page_pool_fragment_page(page, BIAS_MAX);
return page;
}
- pool->frag_users++;
+ pool->frag_cnt++;
pool->frag_offset = *offset + size;
alloc_stat_inc(pool, fast);
return page;
--
2.37.2
On Fri, 15 Dec 2023 at 09:31, Ilias Apalodimas
<[email protected]> wrote:
>
> Since [0] got merged, it's clear that 'pp_ref_count' is used to track
> the number of users for each page. On struct_page though we have
> a member called 'frag_users'. Despite of what the name suggests this is
> not the number of users. It instead represents the number of fragments of
> the current page. When we have a single page this is set to one.
Replying to myself here, but this is a typo. On single pages, we set
pp_ref_count to one, not this.
> When we
> split the page this is set to the actual number of frags and later used
> in page_pool_drain_frag() to infer the real number of users.
>
> So let's rename it to something that matches the description above
>
> [0]
> Link: https://lore.kernel.org/netdev/[email protected]/
> Signed-off-by: Ilias Apalodimas <[email protected]>
> ---
> include/net/page_pool.h | 2 +-
> net/core/page_pool.c | 8 ++++----
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index 813c93499f20..957cd84bb3f4 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -158,7 +158,7 @@ struct page_pool {
> u32 pages_state_hold_cnt;
> unsigned int frag_offset;
> struct page *frag_page;
> - long frag_users;
> + long frag_cnt;
>
> #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;
>
> /* Some user is still using the page frag */
> if (likely(page_pool_defrag_page(page, drain_count)))
> @@ -678,7 +678,7 @@ static struct page *page_pool_drain_frag(struct page_pool *pool,
>
> static void page_pool_free_frag(struct page_pool *pool)
> {
> - long drain_count = BIAS_MAX - pool->frag_users;
> + long drain_count = BIAS_MAX - pool->frag_cnt;
> struct page *page = pool->frag_page;
>
> pool->frag_page = NULL;
> @@ -721,14 +721,14 @@ struct page *page_pool_alloc_frag(struct page_pool *pool,
> pool->frag_page = page;
>
> frag_reset:
> - pool->frag_users = 1;
> + pool->frag_cnt = 1;
> *offset = 0;
> pool->frag_offset = size;
> page_pool_fragment_page(page, BIAS_MAX);
> return page;
> }
>
> - pool->frag_users++;
> + pool->frag_cnt++;
> pool->frag_offset = *offset + size;
> alloc_stat_inc(pool, fast);
> return page;
> --
> 2.37.2
>
On 2023/12/15 15:31, Ilias Apalodimas wrote:
> Since [0] got merged, it's clear that 'pp_ref_count' is used to track
> the number of users for each page. On struct_page though we have
> a member called 'frag_users'. Despite of what the name suggests this is
> not the number of users. It instead represents the number of fragments of
> the current page. When we have a single page this is set to one. When we
> split the page this is set to the actual number of frags and later used
> in page_pool_drain_frag() to infer the real number of users.
>
> So let's rename it to something that matches the description above
>
> [0]
> Link: https://lore.kernel.org/netdev/[email protected]/
> Signed-off-by: Ilias Apalodimas <[email protected]>
> ---
> include/net/page_pool.h | 2 +-
> net/core/page_pool.c | 8 ++++----
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> index 813c93499f20..957cd84bb3f4 100644
> --- a/include/net/page_pool.h
> +++ b/include/net/page_pool.h
> @@ -158,7 +158,7 @@ struct page_pool {
> u32 pages_state_hold_cnt;
> unsigned int frag_offset;
> struct page *frag_page;
> - long frag_users;
> + long frag_cnt;
I would rename it to something like refcnt_bais to mirror the pagecnt_bias
in struct page_frag_cache.
>
> #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;
or
remove it and use pool->refcnt_bais directly.
>
> /* Some user is still using the page frag */
> if (likely(page_pool_defrag_page(page, drain_count)))
> @@ -678,7 +678,7 @@ static struct page *page_pool_drain_frag(struct page_pool *pool,
>
> static void page_pool_free_frag(struct page_pool *pool)
> {
> - long drain_count = BIAS_MAX - pool->frag_users;
> + long drain_count = BIAS_MAX - pool->frag_cnt;
Same here.
> struct page *page = pool->frag_page;
>
> pool->frag_page = NULL;
> @@ -721,14 +721,14 @@ struct page *page_pool_alloc_frag(struct page_pool *pool,
> pool->frag_page = page;
>
> frag_reset:
> - pool->frag_users = 1;
> + pool->frag_cnt = 1;
pool->refcnt_bais = BIAS_MAX - 1;
> *offset = 0;
> pool->frag_offset = size;
> page_pool_fragment_page(page, BIAS_MAX);
> return page;
> }
>
> - pool->frag_users++;
> + pool->frag_cnt++;
pool->refcnt_bais--;
> pool->frag_offset = *offset + size;
> alloc_stat_inc(pool, fast);
> return page;
> --
> 2.37.2
>
> .
>
Hi Yunsheng,
On Fri, 15 Dec 2023 at 13:10, Yunsheng Lin <[email protected]> wrote:
>
> On 2023/12/15 15:31, Ilias Apalodimas wrote:
> > Since [0] got merged, it's clear that 'pp_ref_count' is used to track
> > the number of users for each page. On struct_page though we have
> > a member called 'frag_users'. Despite of what the name suggests this is
> > not the number of users. It instead represents the number of fragments of
> > the current page. When we have a single page this is set to one. When we
> > split the page this is set to the actual number of frags and later used
> > in page_pool_drain_frag() to infer the real number of users.
> >
> > So let's rename it to something that matches the description above
> >
> > [0]
> > Link: https://lore.kernel.org/netdev/[email protected]/
> > Signed-off-by: Ilias Apalodimas <[email protected]>
> > ---
> > include/net/page_pool.h | 2 +-
> > net/core/page_pool.c | 8 ++++----
> > 2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> > index 813c93499f20..957cd84bb3f4 100644
> > --- a/include/net/page_pool.h
> > +++ b/include/net/page_pool.h
> > @@ -158,7 +158,7 @@ struct page_pool {
> > u32 pages_state_hold_cnt;
> > unsigned int frag_offset;
> > struct page *frag_page;
> > - long frag_users;
> > + long frag_cnt;
>
> I would rename it to something like refcnt_bais to mirror the pagecnt_bias
> in struct page_frag_cache.
Sure
>
> >
> > #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
long drain_count = BIAS_MAX - pool->refcnt_bias;
>
> or
>
> remove it and use pool->refcnt_bais directly.
I don't see any reason for inverting the logic. The bias is the number
of refs that should be accounted for during allocation. I'll just
stick with the rename
>
> >
> > /* Some user is still using the page frag */
> > if (likely(page_pool_defrag_page(page, drain_count)))
> > @@ -678,7 +678,7 @@ static struct page *page_pool_drain_frag(struct page_pool *pool,
> >
> > static void page_pool_free_frag(struct page_pool *pool)
> > {
> > - long drain_count = BIAS_MAX - pool->frag_users;
> > + long drain_count = BIAS_MAX - pool->frag_cnt;
>
> Same here.
>
> > struct page *page = pool->frag_page;
> >
> > pool->frag_page = NULL;
> > @@ -721,14 +721,14 @@ struct page *page_pool_alloc_frag(struct page_pool *pool,
> > pool->frag_page = page;
> >
> > frag_reset:
> > - pool->frag_users = 1;
> > + pool->frag_cnt = 1;
>
> pool->refcnt_bais = BIAS_MAX - 1;
>
> > *offset = 0;
> > pool->frag_offset = size;
> > page_pool_fragment_page(page, BIAS_MAX);
> > return page;
> > }
> >
> > - pool->frag_users++;
> > + pool->frag_cnt++;
>
> pool->refcnt_bais--;
>
> > pool->frag_offset = *offset + size;
> > alloc_stat_inc(pool, fast);
> > return page;
> > --
> > 2.37.2
> >
> > .
> >
Thanks
/Ilias
On 2023/12/15 19:58, Ilias Apalodimas wrote:
> Hi Yunsheng,
>
> On Fri, 15 Dec 2023 at 13:10, Yunsheng Lin <[email protected]> wrote:
>>
>> On 2023/12/15 15:31, Ilias Apalodimas wrote:
>>> Since [0] got merged, it's clear that 'pp_ref_count' is used to track
>>> the number of users for each page. On struct_page though we have
>>> a member called 'frag_users'. Despite of what the name suggests this is
>>> not the number of users. It instead represents the number of fragments of
>>> the current page. When we have a single page this is set to one. When we
>>> split the page this is set to the actual number of frags and later used
>>> in page_pool_drain_frag() to infer the real number of users.
>>>
>>> So let's rename it to something that matches the description above
>>>
>>> [0]
>>> Link: https://lore.kernel.org/netdev/[email protected]/
>>> Signed-off-by: Ilias Apalodimas <[email protected]>
>>> ---
>>> include/net/page_pool.h | 2 +-
>>> net/core/page_pool.c | 8 ++++----
>>> 2 files changed, 5 insertions(+), 5 deletions(-)
>>>
>>> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
>>> index 813c93499f20..957cd84bb3f4 100644
>>> --- a/include/net/page_pool.h
>>> +++ b/include/net/page_pool.h
>>> @@ -158,7 +158,7 @@ struct page_pool {
>>> u32 pages_state_hold_cnt;
>>> unsigned int frag_offset;
>>> struct page *frag_page;
>>> - long frag_users;
>>> + long frag_cnt;
>>
>> I would rename it to something like refcnt_bais to mirror the pagecnt_bias
>> in struct page_frag_cache.
>
> Sure
>
>>
>>>
>>> #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
Hi Yunsheng,
On Fri, 15 Dec 2023 at 14:34, Yunsheng Lin <[email protected]> wrote:
>
> On 2023/12/15 19:58, Ilias Apalodimas wrote:
> > Hi Yunsheng,
> >
> > On Fri, 15 Dec 2023 at 13:10, Yunsheng Lin <[email protected]> wrote:
> >>
> >> On 2023/12/15 15:31, Ilias Apalodimas wrote:
> >>> Since [0] got merged, it's clear that 'pp_ref_count' is used to track
> >>> the number of users for each page. On struct_page though we have
> >>> a member called 'frag_users'. Despite of what the name suggests this is
> >>> not the number of users. It instead represents the number of fragments of
> >>> the current page. When we have a single page this is set to one. When we
> >>> split the page this is set to the actual number of frags and later used
> >>> in page_pool_drain_frag() to infer the real number of users.
> >>>
> >>> So let's rename it to something that matches the description above
> >>>
> >>> [0]
> >>> Link: https://lore.kernel.org/netdev/[email protected]/
> >>> Signed-off-by: Ilias Apalodimas <[email protected]>
> >>> ---
> >>> include/net/page_pool.h | 2 +-
> >>> net/core/page_pool.c | 8 ++++----
> >>> 2 files changed, 5 insertions(+), 5 deletions(-)
> >>>
> >>> diff --git a/include/net/page_pool.h b/include/net/page_pool.h
> >>> index 813c93499f20..957cd84bb3f4 100644
> >>> --- a/include/net/page_pool.h
> >>> +++ b/include/net/page_pool.h
> >>> @@ -158,7 +158,7 @@ struct page_pool {
> >>> u32 pages_state_hold_cnt;
> >>> unsigned int frag_offset;
> >>> struct page *frag_page;
> >>> - long frag_users;
> >>> + long frag_cnt;
> >>
> >> I would rename it to something like refcnt_bais to mirror the pagecnt_bias
> >> in struct page_frag_cache.
> >
> > Sure
> >
> >>
> >>>
> >>> #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
with having a common approach. I'll send the rename again shortly, and
I can send the logic invert a bit later (or feel free to send it,
since it was your idea).
Thanks
/Ilias
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
> with having a common approach. I'll send the rename again shortly, and
> I can send the logic invert a bit later (or feel free to send it,
> since it was your idea).
>
> Thanks
> /Ilias
> .
>
Hi Yunsheng,
On Thu, 21 Dec 2023 at 04:07, Yunsheng Lin <[email protected]> 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.
/Ilias
>
> > with having a common approach. I'll send the rename again shortly, and
> > I can send the logic invert a bit later (or feel free to send it,
> > since it was your idea).
> >
> > Thanks
> > /Ilias
> > .
> >
On 2023/12/21 14:37, Ilias Apalodimas wrote:
> Hi Yunsheng,
>
> On Thu, 21 Dec 2023 at 04:07, Yunsheng Lin <[email protected]> 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.
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 fragmemts?
>
Hi Yunsheng,
On Thu, 21 Dec 2023 at 10:00, Yunsheng Lin <[email protected]> wrote:
>
> On 2023/12/21 14:37, Ilias Apalodimas wrote:
> > Hi Yunsheng,
> >
> > On Thu, 21 Dec 2023 at 04:07, Yunsheng Lin <[email protected]> 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
>
> >
On 2023/12/21 16:24, Ilias Apalodimas wrote:
>
>> 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
It is fine by me.
And I can send the logic inversion patch if v2 is merged.