diff mbox series

[net-next] page_pool: Rename frag_users to frag_cnt

Message ID 20231215073119.543560-1-ilias.apalodimas@linaro.org
State New
Headers
Series [net-next] page_pool: Rename frag_users to frag_cnt |

Commit Message

Ilias Apalodimas Dec. 15, 2023, 7:31 a.m. UTC 
  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/20231212044614.42733-2-liangchen.linux@gmail.com/
Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
---
 include/net/page_pool.h | 2 +-
 net/core/page_pool.c    | 8 ++++----
 2 files changed, 5 insertions(+), 5 deletions(-)

--
2.37.2

Comments

Ilias Apalodimas Dec. 15, 2023, 7:43 a.m. UTC | #1 
On Fri, 15 Dec 2023 at 09:31, Ilias Apalodimas
<ilias.apalodimas@linaro.org> 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/20231212044614.42733-2-liangchen.linux@gmail.com/
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  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
>
Yunsheng Lin Dec. 15, 2023, 11:09 a.m. UTC | #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/20231212044614.42733-2-liangchen.linux@gmail.com/
> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> ---
>  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
> 
> .
>
Ilias Apalodimas Dec. 15, 2023, 11:58 a.m. UTC | #3 
Hi Yunsheng,

On Fri, 15 Dec 2023 at 13:10, Yunsheng Lin <linyunsheng@huawei.com> 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/20231212044614.42733-2-liangchen.linux@gmail.com/
> > Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > ---
> >  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
Ilias Apalodimas Dec. 20, 2023, 7:56 a.m. UTC | #4 
Hi Yunsheng,

On Fri, 15 Dec 2023 at 14:34, Yunsheng Lin <linyunsheng@huawei.com> wrote:
>
> On 2023/12/15 19:58, Ilias Apalodimas wrote:
> > Hi Yunsheng,
> >
> > On Fri, 15 Dec 2023 at 13:10, Yunsheng Lin <linyunsheng@huawei.com> 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/20231212044614.42733-2-liangchen.linux@gmail.com/
> >>> Signed-off-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>> ---
> >>>  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
Ilias Apalodimas Dec. 21, 2023, 6:37 a.m. UTC | #5 
Hi Yunsheng,

On Thu, 21 Dec 2023 at 04:07, Yunsheng Lin <linyunsheng@huawei.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.

/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
> > .
> >
diff mbox series

Patch

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;