| Message ID | 20231215073119.543560-1-ilias.apalodimas@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] page_pool: Rename frag_users to frag_cnt | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/apply | fail | Patch does not apply to net-next |
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 >
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 > > . >
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
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
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
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 <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 > > . > >
On 2023/12/21 14:37, Ilias Apalodimas wrote: > 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. > 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 <linyunsheng@huawei.com> wrote: > > On 2023/12/21 14:37, Ilias Apalodimas wrote: > > 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. > > > > 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.
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;
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