To simplify struct page, the effort to seperate its own descriptor from
struct page is required and the work for page pool is on going.
To achieve that, all the code should avoid accessing page pool members
of struct page directly, but use safe APIs for the purpose.
Use netmem_is_pp() instead of directly accessing page->pp_magic in
page_pool_page_is_pp().
Signed-off-by: Byungchul Park <byungchul@sk.com>
---
include/linux/mm.h | 5 +----
net/core/page_pool.c | 5 +++++
2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/include/linux/mm.h b/include/linux/mm.h
index 8dc012e84033..3f7c80fb73ce 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
#define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
#ifdef CONFIG_PAGE_POOL
-static inline bool page_pool_page_is_pp(struct page *page)
-{
- return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
-}
+bool page_pool_page_is_pp(struct page *page);
#else
static inline bool page_pool_page_is_pp(struct page *page)
{
diff --git a/net/core/page_pool.c b/net/core/page_pool.c
index 1071cb3d63e5..37e667e6ca33 100644
--- a/net/core/page_pool.c
+++ b/net/core/page_pool.c
@@ -1284,3 +1284,8 @@ void net_mp_niov_clear_page_pool(struct net_iov *niov)
page_pool_clear_pp_info(netmem);
}
+
+bool page_pool_page_is_pp(struct page *page)
+{
+ return netmem_is_pp(page_to_netmem(page));
+}
--
2.17.1On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
>
> To simplify struct page, the effort to seperate its own descriptor from
> struct page is required and the work for page pool is on going.
>
> To achieve that, all the code should avoid accessing page pool members
> of struct page directly, but use safe APIs for the purpose.
>
> Use netmem_is_pp() instead of directly accessing page->pp_magic in
> page_pool_page_is_pp().
>
> Signed-off-by: Byungchul Park <byungchul@sk.com>
> ---
> include/linux/mm.h | 5 +----
> net/core/page_pool.c | 5 +++++
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 8dc012e84033..3f7c80fb73ce 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
>
> #ifdef CONFIG_PAGE_POOL
> -static inline bool page_pool_page_is_pp(struct page *page)
> -{
> - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
> -}
I vote for keeping this function as-is (do not convert it to netmem),
and instead modify it to access page->netmem_desc->pp_magic.
The reason is that page_pool_is_pp() is today only called from code
paths we have a page and not a netmem. Casting the page to a netmem
which will cast it back to a page pretty much is a waste of cpu
cycles. The page_pool is a place where we count cycles and we have
benchmarks to verify performance (I pointed you to
page_pool_bench_simple on the RFC).
So lets avoid the cpu cycles if possible.
--
Thanks,
Mina
On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
> On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> >
> > To simplify struct page, the effort to seperate its own descriptor from
> > struct page is required and the work for page pool is on going.
> >
> > To achieve that, all the code should avoid accessing page pool members
> > of struct page directly, but use safe APIs for the purpose.
> >
> > Use netmem_is_pp() instead of directly accessing page->pp_magic in
> > page_pool_page_is_pp().
> >
> > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > ---
> > include/linux/mm.h | 5 +----
> > net/core/page_pool.c | 5 +++++
> > 2 files changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 8dc012e84033..3f7c80fb73ce 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
> >
> > #ifdef CONFIG_PAGE_POOL
> > -static inline bool page_pool_page_is_pp(struct page *page)
> > -{
> > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
> > -}
>
> I vote for keeping this function as-is (do not convert it to netmem),
> and instead modify it to access page->netmem_desc->pp_magic.
Once the page pool fields are removed from struct page, struct page will
have neither struct netmem_desc nor the fields..
So it's unevitable to cast it to netmem_desc in order to refer to
pp_magic. Again, pp_magic is no longer associated to struct page.
Thoughts?
Byungchul
> The reason is that page_pool_is_pp() is today only called from code
> paths we have a page and not a netmem. Casting the page to a netmem
> which will cast it back to a page pretty much is a waste of cpu
> cycles. The page_pool is a place where we count cycles and we have
> benchmarks to verify performance (I pointed you to
> page_pool_bench_simple on the RFC).
>
> So lets avoid the cpu cycles if possible.
>
> --
> Thanks,
> Mina
On 5/26/25 03:23, Byungchul Park wrote:
> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
>> On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
>>>
>>> To simplify struct page, the effort to seperate its own descriptor from
>>> struct page is required and the work for page pool is on going.
>>>
>>> To achieve that, all the code should avoid accessing page pool members
>>> of struct page directly, but use safe APIs for the purpose.
>>>
>>> Use netmem_is_pp() instead of directly accessing page->pp_magic in
>>> page_pool_page_is_pp().
>>>
>>> Signed-off-by: Byungchul Park <byungchul@sk.com>
>>> ---
>>> include/linux/mm.h | 5 +----
>>> net/core/page_pool.c | 5 +++++
>>> 2 files changed, 6 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>> index 8dc012e84033..3f7c80fb73ce 100644
>>> --- a/include/linux/mm.h
>>> +++ b/include/linux/mm.h
>>> @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
>>> #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
>>>
>>> #ifdef CONFIG_PAGE_POOL
>>> -static inline bool page_pool_page_is_pp(struct page *page)
>>> -{
>>> - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
>>> -}
>>
>> I vote for keeping this function as-is (do not convert it to netmem),
>> and instead modify it to access page->netmem_desc->pp_magic.
>
> Once the page pool fields are removed from struct page, struct page will
> have neither struct netmem_desc nor the fields..
>
> So it's unevitable to cast it to netmem_desc in order to refer to
> pp_magic. Again, pp_magic is no longer associated to struct page.
>
> Thoughts?
Once the indirection / page shrinking is realized, the page is
supposed to have a type field, isn't it? And all pp_magic trickery
will be replaced with something like
page_pool_page_is_pp() { return page->type == PAGE_TYPE_PP; }
--
Pavel Begunkov
On Wed, May 28, 2025 at 08:51:47AM +0100, Pavel Begunkov wrote:
> On 5/26/25 03:23, Byungchul Park wrote:
> > On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
> > > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> > > >
> > > > To simplify struct page, the effort to seperate its own descriptor from
> > > > struct page is required and the work for page pool is on going.
> > > >
> > > > To achieve that, all the code should avoid accessing page pool members
> > > > of struct page directly, but use safe APIs for the purpose.
> > > >
> > > > Use netmem_is_pp() instead of directly accessing page->pp_magic in
> > > > page_pool_page_is_pp().
> > > >
> > > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > > > ---
> > > > include/linux/mm.h | 5 +----
> > > > net/core/page_pool.c | 5 +++++
> > > > 2 files changed, 6 insertions(+), 4 deletions(-)
> > > >
> > > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > > index 8dc012e84033..3f7c80fb73ce 100644
> > > > --- a/include/linux/mm.h
> > > > +++ b/include/linux/mm.h
> > > > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> > > > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
> > > >
> > > > #ifdef CONFIG_PAGE_POOL
> > > > -static inline bool page_pool_page_is_pp(struct page *page)
> > > > -{
> > > > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
> > > > -}
> > >
> > > I vote for keeping this function as-is (do not convert it to netmem),
> > > and instead modify it to access page->netmem_desc->pp_magic.
> >
> > Once the page pool fields are removed from struct page, struct page will
> > have neither struct netmem_desc nor the fields..
> >
> > So it's unevitable to cast it to netmem_desc in order to refer to
> > pp_magic. Again, pp_magic is no longer associated to struct page.
> >
> > Thoughts?
>
> Once the indirection / page shrinking is realized, the page is
> supposed to have a type field, isn't it? And all pp_magic trickery
> will be replaced with something like
>
> page_pool_page_is_pp() { return page->type == PAGE_TYPE_PP; }
Agree, but we need a temporary solution until then. I will use the
following way for now:
https://lore.kernel.org/all/20250528051452.GB59539@system.software.com/
Byungchul
>
>
> --
> Pavel Begunkov
On 5/28/25 09:14, Byungchul Park wrote:
> On Wed, May 28, 2025 at 08:51:47AM +0100, Pavel Begunkov wrote:
>> On 5/26/25 03:23, Byungchul Park wrote:
>>> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
>>>> On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
>>>>>
>>>>> To simplify struct page, the effort to seperate its own descriptor from
>>>>> struct page is required and the work for page pool is on going.
>>>>>
>>>>> To achieve that, all the code should avoid accessing page pool members
>>>>> of struct page directly, but use safe APIs for the purpose.
>>>>>
>>>>> Use netmem_is_pp() instead of directly accessing page->pp_magic in
>>>>> page_pool_page_is_pp().
>>>>>
>>>>> Signed-off-by: Byungchul Park <byungchul@sk.com>
>>>>> ---
>>>>> include/linux/mm.h | 5 +----
>>>>> net/core/page_pool.c | 5 +++++
>>>>> 2 files changed, 6 insertions(+), 4 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>>> index 8dc012e84033..3f7c80fb73ce 100644
>>>>> --- a/include/linux/mm.h
>>>>> +++ b/include/linux/mm.h
>>>>> @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
>>>>> #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
>>>>>
>>>>> #ifdef CONFIG_PAGE_POOL
>>>>> -static inline bool page_pool_page_is_pp(struct page *page)
>>>>> -{
>>>>> - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
>>>>> -}
>>>>
>>>> I vote for keeping this function as-is (do not convert it to netmem),
>>>> and instead modify it to access page->netmem_desc->pp_magic.
>>>
>>> Once the page pool fields are removed from struct page, struct page will
>>> have neither struct netmem_desc nor the fields..
>>>
>>> So it's unevitable to cast it to netmem_desc in order to refer to
>>> pp_magic. Again, pp_magic is no longer associated to struct page.
>>>
>>> Thoughts?
>>
>> Once the indirection / page shrinking is realized, the page is
>> supposed to have a type field, isn't it? And all pp_magic trickery
>> will be replaced with something like
>>
>> page_pool_page_is_pp() { return page->type == PAGE_TYPE_PP; }
>
> Agree, but we need a temporary solution until then. I will use the
> following way for now:
The question is what is the problem that you need another temporary
solution? If, for example, we go the placeholder way, page_pool_page_is_pp()
can continue using page->netmem_desc->pp_magic as before, and mm folks
will fix it up to page->type when it's time for that. And the compiler
will help by failing compilation if forgotten. You should be able to do
the same with the overlay option.
And, AFAIU, they want to remove/move the lru field in the same way?
In which case we'll get the same problem and need to re-alias it to
something else.
--
Pavel Begunkov
On Wed, May 28, 2025 at 10:07:52AM +0100, Pavel Begunkov wrote:
> On 5/28/25 09:14, Byungchul Park wrote:
> > On Wed, May 28, 2025 at 08:51:47AM +0100, Pavel Begunkov wrote:
> > > On 5/26/25 03:23, Byungchul Park wrote:
> > > > On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
> > > > > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> > > > > >
> > > > > > To simplify struct page, the effort to seperate its own descriptor from
> > > > > > struct page is required and the work for page pool is on going.
> > > > > >
> > > > > > To achieve that, all the code should avoid accessing page pool members
> > > > > > of struct page directly, but use safe APIs for the purpose.
> > > > > >
> > > > > > Use netmem_is_pp() instead of directly accessing page->pp_magic in
> > > > > > page_pool_page_is_pp().
> > > > > >
> > > > > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > > > > > ---
> > > > > > include/linux/mm.h | 5 +----
> > > > > > net/core/page_pool.c | 5 +++++
> > > > > > 2 files changed, 6 insertions(+), 4 deletions(-)
> > > > > >
> > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > > > > index 8dc012e84033..3f7c80fb73ce 100644
> > > > > > --- a/include/linux/mm.h
> > > > > > +++ b/include/linux/mm.h
> > > > > > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> > > > > > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
> > > > > >
> > > > > > #ifdef CONFIG_PAGE_POOL
> > > > > > -static inline bool page_pool_page_is_pp(struct page *page)
> > > > > > -{
> > > > > > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
> > > > > > -}
> > > > >
> > > > > I vote for keeping this function as-is (do not convert it to netmem),
> > > > > and instead modify it to access page->netmem_desc->pp_magic.
> > > >
> > > > Once the page pool fields are removed from struct page, struct page will
> > > > have neither struct netmem_desc nor the fields..
> > > >
> > > > So it's unevitable to cast it to netmem_desc in order to refer to
> > > > pp_magic. Again, pp_magic is no longer associated to struct page.
> > > >
> > > > Thoughts?
> > >
> > > Once the indirection / page shrinking is realized, the page is
> > > supposed to have a type field, isn't it? And all pp_magic trickery
> > > will be replaced with something like
> > >
> > > page_pool_page_is_pp() { return page->type == PAGE_TYPE_PP; }
> >
> > Agree, but we need a temporary solution until then. I will use the
> > following way for now:
>
> The question is what is the problem that you need another temporary
> solution? If, for example, we go the placeholder way, page_pool_page_is_pp()
I prefer using the place-holder, but Matthew does not. I explained it:
https://lore.kernel.org/all/20250528013145.GB2986@system.software.com/
Now, I'm going with the same way as the other approaches e.g. ptdesc.
Byungchul
> can continue using page->netmem_desc->pp_magic as before, and mm folks
> will fix it up to page->type when it's time for that. And the compiler
> will help by failing compilation if forgotten. You should be able to do
> the same with the overlay option.
>
> And, AFAIU, they want to remove/move the lru field in the same way?
> In which case we'll get the same problem and need to re-alias it to
> something else.
>
> --
> Pavel Begunkov
On 5/28/25 10:14, Byungchul Park wrote:
> On Wed, May 28, 2025 at 10:07:52AM +0100, Pavel Begunkov wrote:
>> On 5/28/25 09:14, Byungchul Park wrote:
>>> On Wed, May 28, 2025 at 08:51:47AM +0100, Pavel Begunkov wrote:
>>>> On 5/26/25 03:23, Byungchul Park wrote:
>>>>> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
>>>>>> On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
>>>>>>>
>>>>>>> To simplify struct page, the effort to seperate its own descriptor from
>>>>>>> struct page is required and the work for page pool is on going.
>>>>>>>
>>>>>>> To achieve that, all the code should avoid accessing page pool members
>>>>>>> of struct page directly, but use safe APIs for the purpose.
>>>>>>>
>>>>>>> Use netmem_is_pp() instead of directly accessing page->pp_magic in
>>>>>>> page_pool_page_is_pp().
>>>>>>>
>>>>>>> Signed-off-by: Byungchul Park <byungchul@sk.com>
>>>>>>> ---
>>>>>>> include/linux/mm.h | 5 +----
>>>>>>> net/core/page_pool.c | 5 +++++
>>>>>>> 2 files changed, 6 insertions(+), 4 deletions(-)
>>>>>>>
>>>>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>>>>> index 8dc012e84033..3f7c80fb73ce 100644
>>>>>>> --- a/include/linux/mm.h
>>>>>>> +++ b/include/linux/mm.h
>>>>>>> @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
>>>>>>> #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
>>>>>>>
>>>>>>> #ifdef CONFIG_PAGE_POOL
>>>>>>> -static inline bool page_pool_page_is_pp(struct page *page)
>>>>>>> -{
>>>>>>> - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
>>>>>>> -}
>>>>>>
>>>>>> I vote for keeping this function as-is (do not convert it to netmem),
>>>>>> and instead modify it to access page->netmem_desc->pp_magic.
>>>>>
>>>>> Once the page pool fields are removed from struct page, struct page will
>>>>> have neither struct netmem_desc nor the fields..
>>>>>
>>>>> So it's unevitable to cast it to netmem_desc in order to refer to
>>>>> pp_magic. Again, pp_magic is no longer associated to struct page.
>>>>>
>>>>> Thoughts?
>>>>
>>>> Once the indirection / page shrinking is realized, the page is
>>>> supposed to have a type field, isn't it? And all pp_magic trickery
>>>> will be replaced with something like
>>>>
>>>> page_pool_page_is_pp() { return page->type == PAGE_TYPE_PP; }
>>>
>>> Agree, but we need a temporary solution until then. I will use the
>>> following way for now:
>>
>> The question is what is the problem that you need another temporary
>> solution? If, for example, we go the placeholder way, page_pool_page_is_pp()
>
> I prefer using the place-holder, but Matthew does not. I explained it:
>
> https://lore.kernel.org/all/20250528013145.GB2986@system.software.com/
>
> Now, I'm going with the same way as the other approaches e.g. ptdesc.
Sure, but that doesn't change my point
--
Pavel Begunkov
On Wed, May 28, 2025 at 10:20:29AM +0100, Pavel Begunkov wrote:
> On 5/28/25 10:14, Byungchul Park wrote:
> > On Wed, May 28, 2025 at 10:07:52AM +0100, Pavel Begunkov wrote:
> > > On 5/28/25 09:14, Byungchul Park wrote:
> > > > On Wed, May 28, 2025 at 08:51:47AM +0100, Pavel Begunkov wrote:
> > > > > On 5/26/25 03:23, Byungchul Park wrote:
> > > > > > On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
> > > > > > > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> > > > > > > >
> > > > > > > > To simplify struct page, the effort to seperate its own descriptor from
> > > > > > > > struct page is required and the work for page pool is on going.
> > > > > > > >
> > > > > > > > To achieve that, all the code should avoid accessing page pool members
> > > > > > > > of struct page directly, but use safe APIs for the purpose.
> > > > > > > >
> > > > > > > > Use netmem_is_pp() instead of directly accessing page->pp_magic in
> > > > > > > > page_pool_page_is_pp().
> > > > > > > >
> > > > > > > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > > > > > > > ---
> > > > > > > > include/linux/mm.h | 5 +----
> > > > > > > > net/core/page_pool.c | 5 +++++
> > > > > > > > 2 files changed, 6 insertions(+), 4 deletions(-)
> > > > > > > >
> > > > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > > > > > > index 8dc012e84033..3f7c80fb73ce 100644
> > > > > > > > --- a/include/linux/mm.h
> > > > > > > > +++ b/include/linux/mm.h
> > > > > > > > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> > > > > > > > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
> > > > > > > >
> > > > > > > > #ifdef CONFIG_PAGE_POOL
> > > > > > > > -static inline bool page_pool_page_is_pp(struct page *page)
> > > > > > > > -{
> > > > > > > > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
> > > > > > > > -}
> > > > > > >
> > > > > > > I vote for keeping this function as-is (do not convert it to netmem),
> > > > > > > and instead modify it to access page->netmem_desc->pp_magic.
> > > > > >
> > > > > > Once the page pool fields are removed from struct page, struct page will
> > > > > > have neither struct netmem_desc nor the fields..
> > > > > >
> > > > > > So it's unevitable to cast it to netmem_desc in order to refer to
> > > > > > pp_magic. Again, pp_magic is no longer associated to struct page.
> > > > > >
> > > > > > Thoughts?
> > > > >
> > > > > Once the indirection / page shrinking is realized, the page is
> > > > > supposed to have a type field, isn't it? And all pp_magic trickery
> > > > > will be replaced with something like
> > > > >
> > > > > page_pool_page_is_pp() { return page->type == PAGE_TYPE_PP; }
> > > >
> > > > Agree, but we need a temporary solution until then. I will use the
> > > > following way for now:
> > >
> > > The question is what is the problem that you need another temporary
> > > solution? If, for example, we go the placeholder way, page_pool_page_is_pp()
> >
> > I prefer using the place-holder, but Matthew does not. I explained it:
> >
> > https://lore.kernel.org/all/20250528013145.GB2986@system.software.com/
> >
> > Now, I'm going with the same way as the other approaches e.g. ptdesc.
>
> Sure, but that doesn't change my point
What's your point? The other appoaches do not use place-holders. I
don't get your point.
As I told you, I will introduce a new struct, netmem_desc, instead of
struct_group_tagged() on struct net_iov, and modify the static assert on
the offsets to keep the important fields between struct page and
netmem_desc.
Then, is that following your point? Or could you explain your point in
more detail? Did you say other points than these?
Byungchul
>
> --
> Pavel Begunkov
On 5/28/25 10:33, Byungchul Park wrote:
> On Wed, May 28, 2025 at 10:20:29AM +0100, Pavel Begunkov wrote:
>> On 5/28/25 10:14, Byungchul Park wrote:
>>> On Wed, May 28, 2025 at 10:07:52AM +0100, Pavel Begunkov wrote:
>>>> On 5/28/25 09:14, Byungchul Park wrote:
>>>>> On Wed, May 28, 2025 at 08:51:47AM +0100, Pavel Begunkov wrote:
>>>>>> On 5/26/25 03:23, Byungchul Park wrote:
>>>>>>> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
>>>>>>>> On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
>>>>>>>>>
>>>>>>>>> To simplify struct page, the effort to seperate its own descriptor from
>>>>>>>>> struct page is required and the work for page pool is on going.
>>>>>>>>>
>>>>>>>>> To achieve that, all the code should avoid accessing page pool members
>>>>>>>>> of struct page directly, but use safe APIs for the purpose.
>>>>>>>>>
>>>>>>>>> Use netmem_is_pp() instead of directly accessing page->pp_magic in
>>>>>>>>> page_pool_page_is_pp().
>>>>>>>>>
>>>>>>>>> Signed-off-by: Byungchul Park <byungchul@sk.com>
>>>>>>>>> ---
>>>>>>>>> include/linux/mm.h | 5 +----
>>>>>>>>> net/core/page_pool.c | 5 +++++
>>>>>>>>> 2 files changed, 6 insertions(+), 4 deletions(-)
>>>>>>>>>
>>>>>>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>>>>>>> index 8dc012e84033..3f7c80fb73ce 100644
>>>>>>>>> --- a/include/linux/mm.h
>>>>>>>>> +++ b/include/linux/mm.h
>>>>>>>>> @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
>>>>>>>>> #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
>>>>>>>>>
>>>>>>>>> #ifdef CONFIG_PAGE_POOL
>>>>>>>>> -static inline bool page_pool_page_is_pp(struct page *page)
>>>>>>>>> -{
>>>>>>>>> - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
>>>>>>>>> -}
>>>>>>>>
>>>>>>>> I vote for keeping this function as-is (do not convert it to netmem),
>>>>>>>> and instead modify it to access page->netmem_desc->pp_magic.
>>>>>>>
>>>>>>> Once the page pool fields are removed from struct page, struct page will
>>>>>>> have neither struct netmem_desc nor the fields..
>>>>>>>
>>>>>>> So it's unevitable to cast it to netmem_desc in order to refer to
>>>>>>> pp_magic. Again, pp_magic is no longer associated to struct page.
>>>>>>>
>>>>>>> Thoughts?
>>>>>>
>>>>>> Once the indirection / page shrinking is realized, the page is
>>>>>> supposed to have a type field, isn't it? And all pp_magic trickery
>>>>>> will be replaced with something like
>>>>>>
>>>>>> page_pool_page_is_pp() { return page->type == PAGE_TYPE_PP; }
>>>>>
>>>>> Agree, but we need a temporary solution until then. I will use the
>>>>> following way for now:
>>>>
>>>> The question is what is the problem that you need another temporary
>>>> solution? If, for example, we go the placeholder way, page_pool_page_is_pp()
>>>
>>> I prefer using the place-holder, but Matthew does not. I explained it:
>>>
>>> https://lore.kernel.org/all/20250528013145.GB2986@system.software.com/
>>>
>>> Now, I'm going with the same way as the other approaches e.g. ptdesc.
>>
>> Sure, but that doesn't change my point
>
> What's your point? The other appoaches do not use place-holders. I
> don't get your point.
>
> As I told you, I will introduce a new struct, netmem_desc, instead of
> struct_group_tagged() on struct net_iov, and modify the static assert on
> the offsets to keep the important fields between struct page and
> netmem_desc.
>
> Then, is that following your point? Or could you explain your point in
> more detail? Did you say other points than these?
Then please read the message again first. I was replying to th
aliasing with "lru", and even at the place you cut the message it
says "for example", which was followed by "You should be able to
do the same with the overlay option.".
You can still continue to use pp_magic placed in the netmem_desc
until mm gets rid of it in favour of page->type. I hear that you're
saying it's temporary, but it's messy and there is nothing more
persistent than a "temporary solution", who knows where the final
conversion is going to happen.
--
Pavel Begunkov
On Wed, May 28, 2025 at 10:51:29AM +0100, Pavel Begunkov wrote:
> On 5/28/25 10:33, Byungchul Park wrote:
> > On Wed, May 28, 2025 at 10:20:29AM +0100, Pavel Begunkov wrote:
> > > On 5/28/25 10:14, Byungchul Park wrote:
> > > > On Wed, May 28, 2025 at 10:07:52AM +0100, Pavel Begunkov wrote:
> > > > > On 5/28/25 09:14, Byungchul Park wrote:
> > > > > > On Wed, May 28, 2025 at 08:51:47AM +0100, Pavel Begunkov wrote:
> > > > > > > On 5/26/25 03:23, Byungchul Park wrote:
> > > > > > > > On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
> > > > > > > > > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> > > > > > > > > >
> > > > > > > > > > To simplify struct page, the effort to seperate its own descriptor from
> > > > > > > > > > struct page is required and the work for page pool is on going.
> > > > > > > > > >
> > > > > > > > > > To achieve that, all the code should avoid accessing page pool members
> > > > > > > > > > of struct page directly, but use safe APIs for the purpose.
> > > > > > > > > >
> > > > > > > > > > Use netmem_is_pp() instead of directly accessing page->pp_magic in
> > > > > > > > > > page_pool_page_is_pp().
> > > > > > > > > >
> > > > > > > > > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > > > > > > > > > ---
> > > > > > > > > > include/linux/mm.h | 5 +----
> > > > > > > > > > net/core/page_pool.c | 5 +++++
> > > > > > > > > > 2 files changed, 6 insertions(+), 4 deletions(-)
> > > > > > > > > >
> > > > > > > > > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > > > > > > > > index 8dc012e84033..3f7c80fb73ce 100644
> > > > > > > > > > --- a/include/linux/mm.h
> > > > > > > > > > +++ b/include/linux/mm.h
> > > > > > > > > > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> > > > > > > > > > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
> > > > > > > > > >
> > > > > > > > > > #ifdef CONFIG_PAGE_POOL
> > > > > > > > > > -static inline bool page_pool_page_is_pp(struct page *page)
> > > > > > > > > > -{
> > > > > > > > > > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
> > > > > > > > > > -}
> > > > > > > > >
> > > > > > > > > I vote for keeping this function as-is (do not convert it to netmem),
> > > > > > > > > and instead modify it to access page->netmem_desc->pp_magic.
> > > > > > > >
> > > > > > > > Once the page pool fields are removed from struct page, struct page will
> > > > > > > > have neither struct netmem_desc nor the fields..
> > > > > > > >
> > > > > > > > So it's unevitable to cast it to netmem_desc in order to refer to
> > > > > > > > pp_magic. Again, pp_magic is no longer associated to struct page.
> > > > > > > >
> > > > > > > > Thoughts?
> > > > > > >
> > > > > > > Once the indirection / page shrinking is realized, the page is
> > > > > > > supposed to have a type field, isn't it? And all pp_magic trickery
> > > > > > > will be replaced with something like
> > > > > > >
> > > > > > > page_pool_page_is_pp() { return page->type == PAGE_TYPE_PP; }
> > > > > >
> > > > > > Agree, but we need a temporary solution until then. I will use the
> > > > > > following way for now:
> > > > >
> > > > > The question is what is the problem that you need another temporary
> > > > > solution? If, for example, we go the placeholder way, page_pool_page_is_pp()
> > > >
> > > > I prefer using the place-holder, but Matthew does not. I explained it:
> > > >
> > > > https://lore.kernel.org/all/20250528013145.GB2986@system.software.com/
> > > >
> > > > Now, I'm going with the same way as the other approaches e.g. ptdesc.
> > >
> > > Sure, but that doesn't change my point
> >
> > What's your point? The other appoaches do not use place-holders. I
> > don't get your point.
> >
> > As I told you, I will introduce a new struct, netmem_desc, instead of
> > struct_group_tagged() on struct net_iov, and modify the static assert on
> > the offsets to keep the important fields between struct page and
> > netmem_desc.
> >
> > Then, is that following your point? Or could you explain your point in
> > more detail? Did you say other points than these?
>
> Then please read the message again first. I was replying to th
> aliasing with "lru", and even at the place you cut the message it
> says "for example", which was followed by "You should be able to
> do the same with the overlay option.".
With struct_group_tagged() on struct net_iov, no idea about how to.
However, it's doable with a new separate struct, struct netmem_desc.
I will.
Byungchul
>
> You can still continue to use pp_magic placed in the netmem_desc
> until mm gets rid of it in favour of page->type. I hear that you're
> saying it's temporary, but it's messy and there is nothing more
> persistent than a "temporary solution", who knows where the final
> conversion is going to happen.
>
> --
> Pavel Begunkov
>
On 5/28/25 11:44, Byungchul Park wrote:
> On Wed, May 28, 2025 at 10:51:29AM +0100, Pavel Begunkov wrote:
>> On 5/28/25 10:33, Byungchul Park wrote:
>>> On Wed, May 28, 2025 at 10:20:29AM +0100, Pavel Begunkov wrote:
>>>> On 5/28/25 10:14, Byungchul Park wrote:
>>>>> On Wed, May 28, 2025 at 10:07:52AM +0100, Pavel Begunkov wrote:
>>>>>> On 5/28/25 09:14, Byungchul Park wrote:
>>>>>>> On Wed, May 28, 2025 at 08:51:47AM +0100, Pavel Begunkov wrote:
>>>>>>>> On 5/26/25 03:23, Byungchul Park wrote:
>>>>>>>>> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
>>>>>>>>>> On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
>>>>>>>>>>>
>>>>>>>>>>> To simplify struct page, the effort to seperate its own descriptor from
>>>>>>>>>>> struct page is required and the work for page pool is on going.
>>>>>>>>>>>
>>>>>>>>>>> To achieve that, all the code should avoid accessing page pool members
>>>>>>>>>>> of struct page directly, but use safe APIs for the purpose.
>>>>>>>>>>>
>>>>>>>>>>> Use netmem_is_pp() instead of directly accessing page->pp_magic in
>>>>>>>>>>> page_pool_page_is_pp().
>>>>>>>>>>>
>>>>>>>>>>> Signed-off-by: Byungchul Park <byungchul@sk.com>
>>>>>>>>>>> ---
>>>>>>>>>>> include/linux/mm.h | 5 +----
>>>>>>>>>>> net/core/page_pool.c | 5 +++++
>>>>>>>>>>> 2 files changed, 6 insertions(+), 4 deletions(-)
>>>>>>>>>>>
>>>>>>>>>>> diff --git a/include/linux/mm.h b/include/linux/mm.h
>>>>>>>>>>> index 8dc012e84033..3f7c80fb73ce 100644
>>>>>>>>>>> --- a/include/linux/mm.h
>>>>>>>>>>> +++ b/include/linux/mm.h
>>>>>>>>>>> @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
>>>>>>>>>>> #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
>>>>>>>>>>>
>>>>>>>>>>> #ifdef CONFIG_PAGE_POOL
>>>>>>>>>>> -static inline bool page_pool_page_is_pp(struct page *page)
>>>>>>>>>>> -{
>>>>>>>>>>> - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
>>>>>>>>>>> -}
>>>>>>>>>>
>>>>>>>>>> I vote for keeping this function as-is (do not convert it to netmem),
>>>>>>>>>> and instead modify it to access page->netmem_desc->pp_magic.
>>>>>>>>>
>>>>>>>>> Once the page pool fields are removed from struct page, struct page will
>>>>>>>>> have neither struct netmem_desc nor the fields..
>>>>>>>>>
>>>>>>>>> So it's unevitable to cast it to netmem_desc in order to refer to
>>>>>>>>> pp_magic. Again, pp_magic is no longer associated to struct page.
>>>>>>>>>
>>>>>>>>> Thoughts?
>>>>>>>>
>>>>>>>> Once the indirection / page shrinking is realized, the page is
>>>>>>>> supposed to have a type field, isn't it? And all pp_magic trickery
>>>>>>>> will be replaced with something like
>>>>>>>>
>>>>>>>> page_pool_page_is_pp() { return page->type == PAGE_TYPE_PP; }
>>>>>>>
>>>>>>> Agree, but we need a temporary solution until then. I will use the
>>>>>>> following way for now:
>>>>>>
>>>>>> The question is what is the problem that you need another temporary
>>>>>> solution? If, for example, we go the placeholder way, page_pool_page_is_pp()
>>>>>
>>>>> I prefer using the place-holder, but Matthew does not. I explained it:
>>>>>
>>>>> https://lore.kernel.org/all/20250528013145.GB2986@system.software.com/
>>>>>
>>>>> Now, I'm going with the same way as the other approaches e.g. ptdesc.
>>>>
>>>> Sure, but that doesn't change my point
>>>
>>> What's your point? The other appoaches do not use place-holders. I
>>> don't get your point.
>>>
>>> As I told you, I will introduce a new struct, netmem_desc, instead of
>>> struct_group_tagged() on struct net_iov, and modify the static assert on
>>> the offsets to keep the important fields between struct page and
>>> netmem_desc.
>>>
>>> Then, is that following your point? Or could you explain your point in
>>> more detail? Did you say other points than these?
>>
>> Then please read the message again first. I was replying to th
>> aliasing with "lru", and even at the place you cut the message it
>> says "for example", which was followed by "You should be able to
>> do the same with the overlay option.".
>
> With struct_group_tagged() on struct net_iov, no idea about how to.
> However, it's doable with a new separate struct, struct netmem_desc.
static inline bool page_pool_page_is_pp(struct page *page)
{
pp_magic = page_to_netdesc(page)->pp_magic;
return pp_magic == ...;
}
page_to_netdesc() is either casting directly in case of full page
overlays, or "&page->netdesc" for the placeholder option.
--
Pavel Begunkov
On Mon, May 26, 2025 at 11:23:07AM +0900, Byungchul Park wrote:
> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
> > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> > >
> > > To simplify struct page, the effort to seperate its own descriptor from
> > > struct page is required and the work for page pool is on going.
> > >
> > > To achieve that, all the code should avoid accessing page pool members
> > > of struct page directly, but use safe APIs for the purpose.
> > >
> > > Use netmem_is_pp() instead of directly accessing page->pp_magic in
> > > page_pool_page_is_pp().
> > >
> > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > > ---
> > > include/linux/mm.h | 5 +----
> > > net/core/page_pool.c | 5 +++++
> > > 2 files changed, 6 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index 8dc012e84033..3f7c80fb73ce 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> > > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
> > >
> > > #ifdef CONFIG_PAGE_POOL
> > > -static inline bool page_pool_page_is_pp(struct page *page)
> > > -{
> > > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
> > > -}
> >
> > I vote for keeping this function as-is (do not convert it to netmem),
> > and instead modify it to access page->netmem_desc->pp_magic.
>
> Once the page pool fields are removed from struct page, struct page will
> have neither struct netmem_desc nor the fields..
>
> So it's unevitable to cast it to netmem_desc in order to refer to
> pp_magic. Again, pp_magic is no longer associated to struct page.
Options that come across my mind are:
1. use lru field of struct page instead, with appropriate comment but
looks so ugly.
2. instead of a full word for the magic, use a bit of flags or use
the private field for that purpose.
3. do not check magic number for page pool.
4. more?
Byungchul
>
> Thoughts?
>
> Byungchul
>
> > The reason is that page_pool_is_pp() is today only called from code
> > paths we have a page and not a netmem. Casting the page to a netmem
> > which will cast it back to a page pretty much is a waste of cpu
> > cycles. The page_pool is a place where we count cycles and we have
> > benchmarks to verify performance (I pointed you to
> > page_pool_bench_simple on the RFC).
> >
> > So lets avoid the cpu cycles if possible.
> >
> > --
> > Thanks,
> > Mina
Byungchul Park <byungchul@sk.com> writes:
> On Mon, May 26, 2025 at 11:23:07AM +0900, Byungchul Park wrote:
>> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
>> > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
>> > >
>> > > To simplify struct page, the effort to seperate its own descriptor from
>> > > struct page is required and the work for page pool is on going.
>> > >
>> > > To achieve that, all the code should avoid accessing page pool members
>> > > of struct page directly, but use safe APIs for the purpose.
>> > >
>> > > Use netmem_is_pp() instead of directly accessing page->pp_magic in
>> > > page_pool_page_is_pp().
>> > >
>> > > Signed-off-by: Byungchul Park <byungchul@sk.com>
>> > > ---
>> > > include/linux/mm.h | 5 +----
>> > > net/core/page_pool.c | 5 +++++
>> > > 2 files changed, 6 insertions(+), 4 deletions(-)
>> > >
>> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
>> > > index 8dc012e84033..3f7c80fb73ce 100644
>> > > --- a/include/linux/mm.h
>> > > +++ b/include/linux/mm.h
>> > > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
>> > > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
>> > >
>> > > #ifdef CONFIG_PAGE_POOL
>> > > -static inline bool page_pool_page_is_pp(struct page *page)
>> > > -{
>> > > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
>> > > -}
>> >
>> > I vote for keeping this function as-is (do not convert it to netmem),
>> > and instead modify it to access page->netmem_desc->pp_magic.
>>
>> Once the page pool fields are removed from struct page, struct page will
>> have neither struct netmem_desc nor the fields..
>>
>> So it's unevitable to cast it to netmem_desc in order to refer to
>> pp_magic. Again, pp_magic is no longer associated to struct page.
>
> Options that come across my mind are:
>
> 1. use lru field of struct page instead, with appropriate comment but
> looks so ugly.
> 2. instead of a full word for the magic, use a bit of flags or use
> the private field for that purpose.
> 3. do not check magic number for page pool.
> 4. more?
I'm not sure I understand Mina's concern about CPU cycles from casting.
The casting is a compile-time thing, which shouldn't affect run-time
performance as long as the check is kept as an inline function. So it's
"just" a matter of exposing struct netmem_desc to mm.h so it can use it
in the inline definition. Unless I'm missing something?
-Toke
On Mon, May 26, 2025 at 10:40:30AM +0200, Toke Høiland-Jørgensen wrote:
> Byungchul Park <byungchul@sk.com> writes:
>
> > On Mon, May 26, 2025 at 11:23:07AM +0900, Byungchul Park wrote:
> >> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
> >> > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> >> > >
> >> > > To simplify struct page, the effort to seperate its own descriptor from
> >> > > struct page is required and the work for page pool is on going.
> >> > >
> >> > > To achieve that, all the code should avoid accessing page pool members
> >> > > of struct page directly, but use safe APIs for the purpose.
> >> > >
> >> > > Use netmem_is_pp() instead of directly accessing page->pp_magic in
> >> > > page_pool_page_is_pp().
> >> > >
> >> > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> >> > > ---
> >> > > include/linux/mm.h | 5 +----
> >> > > net/core/page_pool.c | 5 +++++
> >> > > 2 files changed, 6 insertions(+), 4 deletions(-)
> >> > >
> >> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> >> > > index 8dc012e84033..3f7c80fb73ce 100644
> >> > > --- a/include/linux/mm.h
> >> > > +++ b/include/linux/mm.h
> >> > > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> >> > > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
> >> > >
> >> > > #ifdef CONFIG_PAGE_POOL
> >> > > -static inline bool page_pool_page_is_pp(struct page *page)
> >> > > -{
> >> > > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
> >> > > -}
> >> >
> >> > I vote for keeping this function as-is (do not convert it to netmem),
> >> > and instead modify it to access page->netmem_desc->pp_magic.
> >>
> >> Once the page pool fields are removed from struct page, struct page will
> >> have neither struct netmem_desc nor the fields..
> >>
> >> So it's unevitable to cast it to netmem_desc in order to refer to
> >> pp_magic. Again, pp_magic is no longer associated to struct page.
> >
> > Options that come across my mind are:
> >
> > 1. use lru field of struct page instead, with appropriate comment but
> > looks so ugly.
> > 2. instead of a full word for the magic, use a bit of flags or use
> > the private field for that purpose.
> > 3. do not check magic number for page pool.
> > 4. more?
>
> I'm not sure I understand Mina's concern about CPU cycles from casting.
> The casting is a compile-time thing, which shouldn't affect run-time
I didn't mention it but yes.
> performance as long as the check is kept as an inline function. So it's
> "just" a matter of exposing struct netmem_desc to mm.h so it can use it
Then.. we should expose net_iov as well, but I'm afraid it looks weird.
Do you think it's okay?
As I told in another thread, embedding strcut netmem_desc into struct
net_iov will require a huge single patch altering all the users of
struct net_iov.
Byungchul
> in the inline definition. Unless I'm missing something?
>
> -Toke
Byungchul Park <byungchul@sk.com> writes:
> On Mon, May 26, 2025 at 10:40:30AM +0200, Toke Høiland-Jørgensen wrote:
>> Byungchul Park <byungchul@sk.com> writes:
>>
>> > On Mon, May 26, 2025 at 11:23:07AM +0900, Byungchul Park wrote:
>> >> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
>> >> > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
>> >> > >
>> >> > > To simplify struct page, the effort to seperate its own descriptor from
>> >> > > struct page is required and the work for page pool is on going.
>> >> > >
>> >> > > To achieve that, all the code should avoid accessing page pool members
>> >> > > of struct page directly, but use safe APIs for the purpose.
>> >> > >
>> >> > > Use netmem_is_pp() instead of directly accessing page->pp_magic in
>> >> > > page_pool_page_is_pp().
>> >> > >
>> >> > > Signed-off-by: Byungchul Park <byungchul@sk.com>
>> >> > > ---
>> >> > > include/linux/mm.h | 5 +----
>> >> > > net/core/page_pool.c | 5 +++++
>> >> > > 2 files changed, 6 insertions(+), 4 deletions(-)
>> >> > >
>> >> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
>> >> > > index 8dc012e84033..3f7c80fb73ce 100644
>> >> > > --- a/include/linux/mm.h
>> >> > > +++ b/include/linux/mm.h
>> >> > > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
>> >> > > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
>> >> > >
>> >> > > #ifdef CONFIG_PAGE_POOL
>> >> > > -static inline bool page_pool_page_is_pp(struct page *page)
>> >> > > -{
>> >> > > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
>> >> > > -}
>> >> >
>> >> > I vote for keeping this function as-is (do not convert it to netmem),
>> >> > and instead modify it to access page->netmem_desc->pp_magic.
>> >>
>> >> Once the page pool fields are removed from struct page, struct page will
>> >> have neither struct netmem_desc nor the fields..
>> >>
>> >> So it's unevitable to cast it to netmem_desc in order to refer to
>> >> pp_magic. Again, pp_magic is no longer associated to struct page.
>> >
>> > Options that come across my mind are:
>> >
>> > 1. use lru field of struct page instead, with appropriate comment but
>> > looks so ugly.
>> > 2. instead of a full word for the magic, use a bit of flags or use
>> > the private field for that purpose.
>> > 3. do not check magic number for page pool.
>> > 4. more?
>>
>> I'm not sure I understand Mina's concern about CPU cycles from casting.
>> The casting is a compile-time thing, which shouldn't affect run-time
>
> I didn't mention it but yes.
>
>> performance as long as the check is kept as an inline function. So it's
>> "just" a matter of exposing struct netmem_desc to mm.h so it can use it
>
> Then.. we should expose net_iov as well, but I'm afraid it looks weird.
> Do you think it's okay?
Well, it'll be ugly, I grant you that :)
Hmm, so another idea could be to add the pp_magic field to the inner
union that the lru field is in, and keep the page_pool_page_is_pp()
as-is. Then add an assert for offsetof(struct page, pp_magic) ==
offsetof(netmem_desc, pp_magic) on the netmem side, which can be removed
once the two structs no longer shadow each other?
That way you can still get rid of the embedded page_pool struct in
struct page, and the pp_magic field will just be a transition thing
until things are completely separated...
-Toke
On Mon, May 26, 2025 at 11:54:33AM +0200, Toke Høiland-Jørgensen wrote:
> Byungchul Park <byungchul@sk.com> writes:
>
> > On Mon, May 26, 2025 at 10:40:30AM +0200, Toke Høiland-Jørgensen wrote:
> >> Byungchul Park <byungchul@sk.com> writes:
> >>
> >> > On Mon, May 26, 2025 at 11:23:07AM +0900, Byungchul Park wrote:
> >> >> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
> >> >> > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> >> >> > >
> >> >> > > To simplify struct page, the effort to seperate its own descriptor from
> >> >> > > struct page is required and the work for page pool is on going.
> >> >> > >
> >> >> > > To achieve that, all the code should avoid accessing page pool members
> >> >> > > of struct page directly, but use safe APIs for the purpose.
> >> >> > >
> >> >> > > Use netmem_is_pp() instead of directly accessing page->pp_magic in
> >> >> > > page_pool_page_is_pp().
> >> >> > >
> >> >> > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> >> >> > > ---
> >> >> > > include/linux/mm.h | 5 +----
> >> >> > > net/core/page_pool.c | 5 +++++
> >> >> > > 2 files changed, 6 insertions(+), 4 deletions(-)
> >> >> > >
> >> >> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> >> >> > > index 8dc012e84033..3f7c80fb73ce 100644
> >> >> > > --- a/include/linux/mm.h
> >> >> > > +++ b/include/linux/mm.h
> >> >> > > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> >> >> > > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
> >> >> > >
> >> >> > > #ifdef CONFIG_PAGE_POOL
> >> >> > > -static inline bool page_pool_page_is_pp(struct page *page)
> >> >> > > -{
> >> >> > > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
> >> >> > > -}
> >> >> >
> >> >> > I vote for keeping this function as-is (do not convert it to netmem),
> >> >> > and instead modify it to access page->netmem_desc->pp_magic.
> >> >>
> >> >> Once the page pool fields are removed from struct page, struct page will
> >> >> have neither struct netmem_desc nor the fields..
> >> >>
> >> >> So it's unevitable to cast it to netmem_desc in order to refer to
> >> >> pp_magic. Again, pp_magic is no longer associated to struct page.
> >> >
> >> > Options that come across my mind are:
> >> >
> >> > 1. use lru field of struct page instead, with appropriate comment but
> >> > looks so ugly.
> >> > 2. instead of a full word for the magic, use a bit of flags or use
> >> > the private field for that purpose.
> >> > 3. do not check magic number for page pool.
> >> > 4. more?
> >>
> >> I'm not sure I understand Mina's concern about CPU cycles from casting.
> >> The casting is a compile-time thing, which shouldn't affect run-time
> >
> > I didn't mention it but yes.
> >
> >> performance as long as the check is kept as an inline function. So it's
> >> "just" a matter of exposing struct netmem_desc to mm.h so it can use it
> >
> > Then.. we should expose net_iov as well, but I'm afraid it looks weird.
> > Do you think it's okay?
>
> Well, it'll be ugly, I grant you that :)
>
> Hmm, so another idea could be to add the pp_magic field to the inner
> union that the lru field is in, and keep the page_pool_page_is_pp()
> as-is. Then add an assert for offsetof(struct page, pp_magic) ==
> offsetof(netmem_desc, pp_magic) on the netmem side, which can be removed
> once the two structs no longer shadow each other?
>
> That way you can still get rid of the embedded page_pool struct in
> struct page, and the pp_magic field will just be a transition thing
> until things are completely separated...
Or what about to do that as mm folks did in page_is_pfmemalloc()?
static inline bool page_pool_page_is_pp(struct page *page)
{
/*
* XXX: The space of page->lru.next is used as pp_magic in
* struct netmem_desc overlaying on struct page temporarily.
* This API will be unneeded shortly. Let's use the ugly but
* temporal way to access pp_magic until struct netmem_desc has
* its own instance.
*/
return (((unsigned long)page->lru.next) & PP_MAGIC_MASK) == PP_SIGNATURE;
}
Byungchul
>
> -Toke
Byungchul Park <byungchul@sk.com> writes:
> On Mon, May 26, 2025 at 11:54:33AM +0200, Toke Høiland-Jørgensen wrote:
>> Byungchul Park <byungchul@sk.com> writes:
>>
>> > On Mon, May 26, 2025 at 10:40:30AM +0200, Toke Høiland-Jørgensen wrote:
>> >> Byungchul Park <byungchul@sk.com> writes:
>> >>
>> >> > On Mon, May 26, 2025 at 11:23:07AM +0900, Byungchul Park wrote:
>> >> >> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
>> >> >> > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
>> >> >> > >
>> >> >> > > To simplify struct page, the effort to seperate its own descriptor from
>> >> >> > > struct page is required and the work for page pool is on going.
>> >> >> > >
>> >> >> > > To achieve that, all the code should avoid accessing page pool members
>> >> >> > > of struct page directly, but use safe APIs for the purpose.
>> >> >> > >
>> >> >> > > Use netmem_is_pp() instead of directly accessing page->pp_magic in
>> >> >> > > page_pool_page_is_pp().
>> >> >> > >
>> >> >> > > Signed-off-by: Byungchul Park <byungchul@sk.com>
>> >> >> > > ---
>> >> >> > > include/linux/mm.h | 5 +----
>> >> >> > > net/core/page_pool.c | 5 +++++
>> >> >> > > 2 files changed, 6 insertions(+), 4 deletions(-)
>> >> >> > >
>> >> >> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
>> >> >> > > index 8dc012e84033..3f7c80fb73ce 100644
>> >> >> > > --- a/include/linux/mm.h
>> >> >> > > +++ b/include/linux/mm.h
>> >> >> > > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
>> >> >> > > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
>> >> >> > >
>> >> >> > > #ifdef CONFIG_PAGE_POOL
>> >> >> > > -static inline bool page_pool_page_is_pp(struct page *page)
>> >> >> > > -{
>> >> >> > > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
>> >> >> > > -}
>> >> >> >
>> >> >> > I vote for keeping this function as-is (do not convert it to netmem),
>> >> >> > and instead modify it to access page->netmem_desc->pp_magic.
>> >> >>
>> >> >> Once the page pool fields are removed from struct page, struct page will
>> >> >> have neither struct netmem_desc nor the fields..
>> >> >>
>> >> >> So it's unevitable to cast it to netmem_desc in order to refer to
>> >> >> pp_magic. Again, pp_magic is no longer associated to struct page.
>> >> >
>> >> > Options that come across my mind are:
>> >> >
>> >> > 1. use lru field of struct page instead, with appropriate comment but
>> >> > looks so ugly.
>> >> > 2. instead of a full word for the magic, use a bit of flags or use
>> >> > the private field for that purpose.
>> >> > 3. do not check magic number for page pool.
>> >> > 4. more?
>> >>
>> >> I'm not sure I understand Mina's concern about CPU cycles from casting.
>> >> The casting is a compile-time thing, which shouldn't affect run-time
>> >
>> > I didn't mention it but yes.
>> >
>> >> performance as long as the check is kept as an inline function. So it's
>> >> "just" a matter of exposing struct netmem_desc to mm.h so it can use it
>> >
>> > Then.. we should expose net_iov as well, but I'm afraid it looks weird.
>> > Do you think it's okay?
>>
>> Well, it'll be ugly, I grant you that :)
>>
>> Hmm, so another idea could be to add the pp_magic field to the inner
>> union that the lru field is in, and keep the page_pool_page_is_pp()
>> as-is. Then add an assert for offsetof(struct page, pp_magic) ==
>> offsetof(netmem_desc, pp_magic) on the netmem side, which can be removed
>> once the two structs no longer shadow each other?
>>
>> That way you can still get rid of the embedded page_pool struct in
>> struct page, and the pp_magic field will just be a transition thing
>> until things are completely separated...
>
> Or what about to do that as mm folks did in page_is_pfmemalloc()?
>
> static inline bool page_pool_page_is_pp(struct page *page)
> {
> /*
> * XXX: The space of page->lru.next is used as pp_magic in
> * struct netmem_desc overlaying on struct page temporarily.
> * This API will be unneeded shortly. Let's use the ugly but
> * temporal way to access pp_magic until struct netmem_desc has
> * its own instance.
> */
> return (((unsigned long)page->lru.next) & PP_MAGIC_MASK) == PP_SIGNATURE;
> }
Sure, that can work as a temporary solution (maybe with a static assert
somewhere that pp_magic and lru have the same offsetof())?
-Toke
On Wed, May 28, 2025 at 09:35:03AM +0200, Toke Høiland-Jørgensen wrote:
> Byungchul Park <byungchul@sk.com> writes:
>
> > On Mon, May 26, 2025 at 11:54:33AM +0200, Toke Høiland-Jørgensen wrote:
> >> Byungchul Park <byungchul@sk.com> writes:
> >>
> >> > On Mon, May 26, 2025 at 10:40:30AM +0200, Toke Høiland-Jørgensen wrote:
> >> >> Byungchul Park <byungchul@sk.com> writes:
> >> >>
> >> >> > On Mon, May 26, 2025 at 11:23:07AM +0900, Byungchul Park wrote:
> >> >> >> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
> >> >> >> > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> >> >> >> > >
> >> >> >> > > To simplify struct page, the effort to seperate its own descriptor from
> >> >> >> > > struct page is required and the work for page pool is on going.
> >> >> >> > >
> >> >> >> > > To achieve that, all the code should avoid accessing page pool members
> >> >> >> > > of struct page directly, but use safe APIs for the purpose.
> >> >> >> > >
> >> >> >> > > Use netmem_is_pp() instead of directly accessing page->pp_magic in
> >> >> >> > > page_pool_page_is_pp().
> >> >> >> > >
> >> >> >> > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> >> >> >> > > ---
> >> >> >> > > include/linux/mm.h | 5 +----
> >> >> >> > > net/core/page_pool.c | 5 +++++
> >> >> >> > > 2 files changed, 6 insertions(+), 4 deletions(-)
> >> >> >> > >
> >> >> >> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> >> >> >> > > index 8dc012e84033..3f7c80fb73ce 100644
> >> >> >> > > --- a/include/linux/mm.h
> >> >> >> > > +++ b/include/linux/mm.h
> >> >> >> > > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> >> >> >> > > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
> >> >> >> > >
> >> >> >> > > #ifdef CONFIG_PAGE_POOL
> >> >> >> > > -static inline bool page_pool_page_is_pp(struct page *page)
> >> >> >> > > -{
> >> >> >> > > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
> >> >> >> > > -}
> >> >> >> >
> >> >> >> > I vote for keeping this function as-is (do not convert it to netmem),
> >> >> >> > and instead modify it to access page->netmem_desc->pp_magic.
> >> >> >>
> >> >> >> Once the page pool fields are removed from struct page, struct page will
> >> >> >> have neither struct netmem_desc nor the fields..
> >> >> >>
> >> >> >> So it's unevitable to cast it to netmem_desc in order to refer to
> >> >> >> pp_magic. Again, pp_magic is no longer associated to struct page.
> >> >> >
> >> >> > Options that come across my mind are:
> >> >> >
> >> >> > 1. use lru field of struct page instead, with appropriate comment but
> >> >> > looks so ugly.
> >> >> > 2. instead of a full word for the magic, use a bit of flags or use
> >> >> > the private field for that purpose.
> >> >> > 3. do not check magic number for page pool.
> >> >> > 4. more?
> >> >>
> >> >> I'm not sure I understand Mina's concern about CPU cycles from casting.
> >> >> The casting is a compile-time thing, which shouldn't affect run-time
> >> >
> >> > I didn't mention it but yes.
> >> >
> >> >> performance as long as the check is kept as an inline function. So it's
> >> >> "just" a matter of exposing struct netmem_desc to mm.h so it can use it
> >> >
> >> > Then.. we should expose net_iov as well, but I'm afraid it looks weird.
> >> > Do you think it's okay?
> >>
> >> Well, it'll be ugly, I grant you that :)
> >>
> >> Hmm, so another idea could be to add the pp_magic field to the inner
> >> union that the lru field is in, and keep the page_pool_page_is_pp()
> >> as-is. Then add an assert for offsetof(struct page, pp_magic) ==
> >> offsetof(netmem_desc, pp_magic) on the netmem side, which can be removed
> >> once the two structs no longer shadow each other?
> >>
> >> That way you can still get rid of the embedded page_pool struct in
> >> struct page, and the pp_magic field will just be a transition thing
> >> until things are completely separated...
> >
> > Or what about to do that as mm folks did in page_is_pfmemalloc()?
> >
> > static inline bool page_pool_page_is_pp(struct page *page)
> > {
> > /*
> > * XXX: The space of page->lru.next is used as pp_magic in
> > * struct netmem_desc overlaying on struct page temporarily.
> > * This API will be unneeded shortly. Let's use the ugly but
> > * temporal way to access pp_magic until struct netmem_desc has
> > * its own instance.
> > */
> > return (((unsigned long)page->lru.next) & PP_MAGIC_MASK) == PP_SIGNATURE;
> > }
>
> Sure, that can work as a temporary solution (maybe with a static assert
> somewhere that pp_magic and lru have the same offsetof())?
Sure. I will do that as I posted in the cover letter:
https://lore.kernel.org/all/20250528022911.73453-1-byungchul@sk.com/
Byungchul
>
> -Toke
On Mon, May 26, 2025 at 11:54:33AM +0200, Toke Høiland-Jørgensen wrote:
> Byungchul Park <byungchul@sk.com> writes:
>
> > On Mon, May 26, 2025 at 10:40:30AM +0200, Toke Høiland-Jørgensen wrote:
> >> Byungchul Park <byungchul@sk.com> writes:
> >>
> >> > On Mon, May 26, 2025 at 11:23:07AM +0900, Byungchul Park wrote:
> >> >> On Fri, May 23, 2025 at 10:21:17AM -0700, Mina Almasry wrote:
> >> >> > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> >> >> > >
> >> >> > > To simplify struct page, the effort to seperate its own descriptor from
> >> >> > > struct page is required and the work for page pool is on going.
> >> >> > >
> >> >> > > To achieve that, all the code should avoid accessing page pool members
> >> >> > > of struct page directly, but use safe APIs for the purpose.
> >> >> > >
> >> >> > > Use netmem_is_pp() instead of directly accessing page->pp_magic in
> >> >> > > page_pool_page_is_pp().
> >> >> > >
> >> >> > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> >> >> > > ---
> >> >> > > include/linux/mm.h | 5 +----
> >> >> > > net/core/page_pool.c | 5 +++++
> >> >> > > 2 files changed, 6 insertions(+), 4 deletions(-)
> >> >> > >
> >> >> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> >> >> > > index 8dc012e84033..3f7c80fb73ce 100644
> >> >> > > --- a/include/linux/mm.h
> >> >> > > +++ b/include/linux/mm.h
> >> >> > > @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> >> >> > > #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
> >> >> > >
> >> >> > > #ifdef CONFIG_PAGE_POOL
> >> >> > > -static inline bool page_pool_page_is_pp(struct page *page)
> >> >> > > -{
> >> >> > > - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
> >> >> > > -}
> >> >> >
> >> >> > I vote for keeping this function as-is (do not convert it to netmem),
> >> >> > and instead modify it to access page->netmem_desc->pp_magic.
> >> >>
> >> >> Once the page pool fields are removed from struct page, struct page will
> >> >> have neither struct netmem_desc nor the fields..
> >> >>
> >> >> So it's unevitable to cast it to netmem_desc in order to refer to
> >> >> pp_magic. Again, pp_magic is no longer associated to struct page.
> >> >
> >> > Options that come across my mind are:
> >> >
> >> > 1. use lru field of struct page instead, with appropriate comment but
> >> > looks so ugly.
> >> > 2. instead of a full word for the magic, use a bit of flags or use
> >> > the private field for that purpose.
> >> > 3. do not check magic number for page pool.
> >> > 4. more?
> >>
> >> I'm not sure I understand Mina's concern about CPU cycles from casting.
> >> The casting is a compile-time thing, which shouldn't affect run-time
> >
> > I didn't mention it but yes.
> >
> >> performance as long as the check is kept as an inline function. So it's
> >> "just" a matter of exposing struct netmem_desc to mm.h so it can use it
> >
> > Then.. we should expose net_iov as well, but I'm afraid it looks weird.
> > Do you think it's okay?
>
> Well, it'll be ugly, I grant you that :)
>
> Hmm, so another idea could be to add the pp_magic field to the inner
> union that the lru field is in, and keep the page_pool_page_is_pp()
> as-is. Then add an assert for offsetof(struct page, pp_magic) ==
> offsetof(netmem_desc, pp_magic) on the netmem side, which can be removed
> once the two structs no longer shadow each other?
It would work, but still that's what I wanted to avoid.
To Matthew and mm folks,
Does it look okay?
Byungchul
>
> That way you can still get rid of the embedded page_pool struct in
> struct page, and the pp_magic field will just be a transition thing
> until things are completely separated...
>
> -Toke
Byungchul Park <byungchul@sk.com> writes:
> To simplify struct page, the effort to seperate its own descriptor from
> struct page is required and the work for page pool is on going.
>
> To achieve that, all the code should avoid accessing page pool members
> of struct page directly, but use safe APIs for the purpose.
>
> Use netmem_is_pp() instead of directly accessing page->pp_magic in
> page_pool_page_is_pp().
>
> Signed-off-by: Byungchul Park <byungchul@sk.com>
> ---
> include/linux/mm.h | 5 +----
> net/core/page_pool.c | 5 +++++
> 2 files changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 8dc012e84033..3f7c80fb73ce 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -4312,10 +4312,7 @@ int arch_lock_shadow_stack_status(struct task_struct *t, unsigned long status);
> #define PP_MAGIC_MASK ~(PP_DMA_INDEX_MASK | 0x3UL)
>
> #ifdef CONFIG_PAGE_POOL
> -static inline bool page_pool_page_is_pp(struct page *page)
> -{
> - return (page->pp_magic & PP_MAGIC_MASK) == PP_SIGNATURE;
> -}
> +bool page_pool_page_is_pp(struct page *page);
Here you're turning an inline function into a function call, which has
performance implications. Please try to avoid that.
-Toke
© 2016 - 2025 Red Hat, Inc.