Now that all the users of the page pool members in struct page have been
gone, the members can be removed from struct page.
However, since struct netmem_desc might still use the space in struct
page, the size of struct netmem_desc should be checked, until struct
netmem_desc has its own instance from slab, to avoid conficting with
other members within struct page.
Remove the page pool members in struct page and add a static checker for
the size.
Signed-off-by: Byungchul Park <byungchul@sk.com>
---
include/linux/mm_types.h | 11 -----------
include/net/netmem.h | 28 +++++-----------------------
2 files changed, 5 insertions(+), 34 deletions(-)
diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 873e820e1521..5a7864eb9d76 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -119,17 +119,6 @@ struct page {
*/
unsigned long private;
};
- struct { /* page_pool used by netstack */
- unsigned long _pp_mapping_pad;
- /**
- * @pp_magic: magic value to avoid recycling non
- * page_pool allocated pages.
- */
- unsigned long pp_magic;
- struct page_pool *pp;
- unsigned long dma_addr;
- atomic_long_t pp_ref_count;
- };
struct { /* Tail pages of compound page */
unsigned long compound_head; /* Bit zero is set */
};
diff --git a/include/net/netmem.h b/include/net/netmem.h
index c63a7e20f5f3..257c22398d7a 100644
--- a/include/net/netmem.h
+++ b/include/net/netmem.h
@@ -77,30 +77,12 @@ struct net_iov_area {
unsigned long base_virtual;
};
-/* These fields in struct page are used by the page_pool and net stack:
- *
- * struct {
- * unsigned long _pp_mapping_pad;
- * unsigned long pp_magic;
- * struct page_pool *pp;
- * unsigned long dma_addr;
- * atomic_long_t pp_ref_count;
- * };
- *
- * We mirror the page_pool fields here so the page_pool can access these fields
- * without worrying whether the underlying fields belong to a page or net_iov.
- *
- * The non-net stack fields of struct page are private to the mm stack and must
- * never be mirrored to net_iov.
+/* XXX: The page pool fields in struct page have been removed but they
+ * might still use the space in struct page. Thus, the size of struct
+ * netmem_desc should be under control until struct netmem_desc has its
+ * own instance from slab.
*/
-#define NET_IOV_ASSERT_OFFSET(pg, iov) \
- static_assert(offsetof(struct page, pg) == \
- offsetof(struct net_iov, iov))
-NET_IOV_ASSERT_OFFSET(pp_magic, pp_magic);
-NET_IOV_ASSERT_OFFSET(pp, pp);
-NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr);
-NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
-#undef NET_IOV_ASSERT_OFFSET
+static_assert(sizeof(struct netmem_desc) <= offsetof(struct page, _refcount));
static inline struct net_iov_area *net_iov_owner(const struct net_iov *niov)
{
--
2.17.1On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
>
> Now that all the users of the page pool members in struct page have been
> gone, the members can be removed from struct page.
>
> However, since struct netmem_desc might still use the space in struct
> page, the size of struct netmem_desc should be checked, until struct
> netmem_desc has its own instance from slab, to avoid conficting with
> other members within struct page.
>
> Remove the page pool members in struct page and add a static checker for
> the size.
>
> Signed-off-by: Byungchul Park <byungchul@sk.com>
> ---
> include/linux/mm_types.h | 11 -----------
> include/net/netmem.h | 28 +++++-----------------------
> 2 files changed, 5 insertions(+), 34 deletions(-)
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 873e820e1521..5a7864eb9d76 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -119,17 +119,6 @@ struct page {
> */
> unsigned long private;
> };
> - struct { /* page_pool used by netstack */
> - unsigned long _pp_mapping_pad;
> - /**
> - * @pp_magic: magic value to avoid recycling non
> - * page_pool allocated pages.
> - */
> - unsigned long pp_magic;
> - struct page_pool *pp;
> - unsigned long dma_addr;
> - atomic_long_t pp_ref_count;
> - };
> struct { /* Tail pages of compound page */
> unsigned long compound_head; /* Bit zero is set */
> };
> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index c63a7e20f5f3..257c22398d7a 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -77,30 +77,12 @@ struct net_iov_area {
> unsigned long base_virtual;
> };
>
> -/* These fields in struct page are used by the page_pool and net stack:
> - *
> - * struct {
> - * unsigned long _pp_mapping_pad;
> - * unsigned long pp_magic;
> - * struct page_pool *pp;
> - * unsigned long dma_addr;
> - * atomic_long_t pp_ref_count;
> - * };
> - *
> - * We mirror the page_pool fields here so the page_pool can access these fields
> - * without worrying whether the underlying fields belong to a page or net_iov.
> - *
> - * The non-net stack fields of struct page are private to the mm stack and must
> - * never be mirrored to net_iov.
> +/* XXX: The page pool fields in struct page have been removed but they
> + * might still use the space in struct page. Thus, the size of struct
> + * netmem_desc should be under control until struct netmem_desc has its
> + * own instance from slab.
> */
> -#define NET_IOV_ASSERT_OFFSET(pg, iov) \
> - static_assert(offsetof(struct page, pg) == \
> - offsetof(struct net_iov, iov))
> -NET_IOV_ASSERT_OFFSET(pp_magic, pp_magic);
> -NET_IOV_ASSERT_OFFSET(pp, pp);
> -NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr);
> -NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
> -#undef NET_IOV_ASSERT_OFFSET
> +static_assert(sizeof(struct netmem_desc) <= offsetof(struct page, _refcount));
>
Removing these asserts is actually a bit dangerous. Functions like
netmem_or_pp_magic() rely on the fact that the offsets are the same
between struct page and struct net_iov to access these fields without
worrying about the type of the netmem. What we do in these helpers is
we we clear the least significant bit of the netmem, and then access
the field. This works only because we verified at build time that the
offset is the same.
I think we have 3 options here:
1. Keep the asserts as-is, then in the follow up patch where we remove
netmem_desc from struct page, we update the asserts to make sure
struct page and struct net_iov can grab the netmem_desc in a uniform
way.
2. We remove the asserts, but all the helpers that rely on
__netmem_clear_lsb need to be modified to do custom handling of
net_iov vs page. Something like:
static inline void netmem_or_pp_magic(netmem_ref netmem, unsigned long pp_magic)
{
if (netmem_is_net_iov(netmem)
netmem_to_net_iov(netmem)->pp_magic |= pp_magic;
else
netmem_to_page(netmem)->pp_magic |= pp_magic;
}
Option #2 requires extra checks, which may affect the performance
reported by page_pool_bench_simple that I pointed you to before.
3. We could swap out all the individual asserts for one assert, if
both page and net_iov have a netmem_desc subfield. This will also need
to be reworked when netmem_desc is eventually moved out of struct page
and is slab allocated:
NET_IOV_ASSERT_OFFSET(netmem_desc, netmem_desc);
--
Thanks,
Mina
On Fri, May 23, 2025 at 10:55:54AM -0700, Mina Almasry wrote:
> On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> >
> > Now that all the users of the page pool members in struct page have been
> > gone, the members can be removed from struct page.
> >
> > However, since struct netmem_desc might still use the space in struct
> > page, the size of struct netmem_desc should be checked, until struct
> > netmem_desc has its own instance from slab, to avoid conficting with
> > other members within struct page.
> >
> > Remove the page pool members in struct page and add a static checker for
> > the size.
> >
> > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > ---
> > include/linux/mm_types.h | 11 -----------
> > include/net/netmem.h | 28 +++++-----------------------
> > 2 files changed, 5 insertions(+), 34 deletions(-)
> >
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index 873e820e1521..5a7864eb9d76 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -119,17 +119,6 @@ struct page {
> > */
> > unsigned long private;
> > };
> > - struct { /* page_pool used by netstack */
> > - unsigned long _pp_mapping_pad;
> > - /**
> > - * @pp_magic: magic value to avoid recycling non
> > - * page_pool allocated pages.
> > - */
> > - unsigned long pp_magic;
> > - struct page_pool *pp;
> > - unsigned long dma_addr;
> > - atomic_long_t pp_ref_count;
> > - };
> > struct { /* Tail pages of compound page */
> > unsigned long compound_head; /* Bit zero is set */
> > };
> > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > index c63a7e20f5f3..257c22398d7a 100644
> > --- a/include/net/netmem.h
> > +++ b/include/net/netmem.h
> > @@ -77,30 +77,12 @@ struct net_iov_area {
> > unsigned long base_virtual;
> > };
> >
> > -/* These fields in struct page are used by the page_pool and net stack:
> > - *
> > - * struct {
> > - * unsigned long _pp_mapping_pad;
> > - * unsigned long pp_magic;
> > - * struct page_pool *pp;
> > - * unsigned long dma_addr;
> > - * atomic_long_t pp_ref_count;
> > - * };
> > - *
> > - * We mirror the page_pool fields here so the page_pool can access these fields
> > - * without worrying whether the underlying fields belong to a page or net_iov.
> > - *
> > - * The non-net stack fields of struct page are private to the mm stack and must
> > - * never be mirrored to net_iov.
> > +/* XXX: The page pool fields in struct page have been removed but they
> > + * might still use the space in struct page. Thus, the size of struct
> > + * netmem_desc should be under control until struct netmem_desc has its
> > + * own instance from slab.
> > */
> > -#define NET_IOV_ASSERT_OFFSET(pg, iov) \
> > - static_assert(offsetof(struct page, pg) == \
> > - offsetof(struct net_iov, iov))
> > -NET_IOV_ASSERT_OFFSET(pp_magic, pp_magic);
> > -NET_IOV_ASSERT_OFFSET(pp, pp);
> > -NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr);
> > -NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
> > -#undef NET_IOV_ASSERT_OFFSET
> > +static_assert(sizeof(struct netmem_desc) <= offsetof(struct page, _refcount));
> >
>
> Removing these asserts is actually a bit dangerous. Functions like
> netmem_or_pp_magic() rely on the fact that the offsets are the same
> between struct page and struct net_iov to access these fields without
Worth noting this patch removes the page pool fields from struct page.
However, yes, I will keep necessary assertions with some changes applied
so that it can work even after removing the page pool fields like:
NET_IOV_ASSERT_OFFSET(lru, pp_magic);
NET_IOV_ASSERT_OFFSET(mapping, _pp_mapping_pad);
> worrying about the type of the netmem. What we do in these helpers is
> we we clear the least significant bit of the netmem, and then access
> the field. This works only because we verified at build time that the
> offset is the same.
>
> I think we have 3 options here:
>
> 1. Keep the asserts as-is, then in the follow up patch where we remove
> netmem_desc from struct page, we update the asserts to make sure
> struct page and struct net_iov can grab the netmem_desc in a uniform
Ah. It's worth noting that I'm removing the page pool fields all the
way from strcut page, instead of placing a place-holder that I did in
RFC as Matthew requested.
> way.
>
> 2. We remove the asserts, but all the helpers that rely on
> __netmem_clear_lsb need to be modified to do custom handling of
> net_iov vs page. Something like:
>
> static inline void netmem_or_pp_magic(netmem_ref netmem, unsigned long pp_magic)
> {
> if (netmem_is_net_iov(netmem)
> netmem_to_net_iov(netmem)->pp_magic |= pp_magic;
> else
> netmem_to_page(netmem)->pp_magic |= pp_magic;
struct page should not have pp_magic field once the page pool fields are
gone.
Byungchul
> }
>
> Option #2 requires extra checks, which may affect the performance
> reported by page_pool_bench_simple that I pointed you to before.
>
> 3. We could swap out all the individual asserts for one assert, if
> both page and net_iov have a netmem_desc subfield. This will also need
> to be reworked when netmem_desc is eventually moved out of struct page
> and is slab allocated:
>
> NET_IOV_ASSERT_OFFSET(netmem_desc, netmem_desc);
>
> --
> Thanks,
> Mina
On 5/26/25 02:37, Byungchul Park wrote:
> On Fri, May 23, 2025 at 10:55:54AM -0700, Mina Almasry wrote:
>> On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
>>>
>>> Now that all the users of the page pool members in struct page have been
>>> gone, the members can be removed from struct page.
>>>
>>> However, since struct netmem_desc might still use the space in struct
>>> page, the size of struct netmem_desc should be checked, until struct
>>> netmem_desc has its own instance from slab, to avoid conficting with
>>> other members within struct page.
>>>
>>> Remove the page pool members in struct page and add a static checker for
>>> the size.
>>>
>>> Signed-off-by: Byungchul Park <byungchul@sk.com>
>>> ---
>>> include/linux/mm_types.h | 11 -----------
>>> include/net/netmem.h | 28 +++++-----------------------
>>> 2 files changed, 5 insertions(+), 34 deletions(-)
>>>
>>> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
>>> index 873e820e1521..5a7864eb9d76 100644
>>> --- a/include/linux/mm_types.h
>>> +++ b/include/linux/mm_types.h
>>> @@ -119,17 +119,6 @@ struct page {
>>> */
>>> unsigned long private;
>>> };
>>> - struct { /* page_pool used by netstack */
>>> - unsigned long _pp_mapping_pad;
>>> - /**
>>> - * @pp_magic: magic value to avoid recycling non
>>> - * page_pool allocated pages.
>>> - */
>>> - unsigned long pp_magic;
>>> - struct page_pool *pp;
>>> - unsigned long dma_addr;
>>> - atomic_long_t pp_ref_count;
>>> - };
>>> struct { /* Tail pages of compound page */
>>> unsigned long compound_head; /* Bit zero is set */
>>> };
>>> diff --git a/include/net/netmem.h b/include/net/netmem.h
>>> index c63a7e20f5f3..257c22398d7a 100644
>>> --- a/include/net/netmem.h
>>> +++ b/include/net/netmem.h
>>> @@ -77,30 +77,12 @@ struct net_iov_area {
>>> unsigned long base_virtual;
>>> };
>>>
>>> -/* These fields in struct page are used by the page_pool and net stack:
>>> - *
>>> - * struct {
>>> - * unsigned long _pp_mapping_pad;
>>> - * unsigned long pp_magic;
>>> - * struct page_pool *pp;
>>> - * unsigned long dma_addr;
>>> - * atomic_long_t pp_ref_count;
>>> - * };
>>> - *
>>> - * We mirror the page_pool fields here so the page_pool can access these fields
>>> - * without worrying whether the underlying fields belong to a page or net_iov.
>>> - *
>>> - * The non-net stack fields of struct page are private to the mm stack and must
>>> - * never be mirrored to net_iov.
>>> +/* XXX: The page pool fields in struct page have been removed but they
>>> + * might still use the space in struct page. Thus, the size of struct
>>> + * netmem_desc should be under control until struct netmem_desc has its
>>> + * own instance from slab.
>>> */
>>> -#define NET_IOV_ASSERT_OFFSET(pg, iov) \
>>> - static_assert(offsetof(struct page, pg) == \
>>> - offsetof(struct net_iov, iov))
>>> -NET_IOV_ASSERT_OFFSET(pp_magic, pp_magic);
>>> -NET_IOV_ASSERT_OFFSET(pp, pp);
>>> -NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr);
>>> -NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
>>> -#undef NET_IOV_ASSERT_OFFSET
>>> +static_assert(sizeof(struct netmem_desc) <= offsetof(struct page, _refcount));
>>>
>>
>> Removing these asserts is actually a bit dangerous. Functions like
>> netmem_or_pp_magic() rely on the fact that the offsets are the same
>> between struct page and struct net_iov to access these fields without
>
> Worth noting this patch removes the page pool fields from struct page.
static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
{
return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
}
static inline atomic_long_t *netmem_get_pp_ref_count_ref(netmem_ref netmem)
{
return &__netmem_clear_lsb(netmem)->pp_ref_count;
}
That's a snippet of code after applying the series. So, let's say we
take a page, it's casted to netmem, then the netmem (as it was before)
is casted to net_iov. Before it relied on net_iov and the pp's part of
the page having the same layout, which was checked by static asserts,
but now, unless I'm mistaken, it's aligned in the exactly same way but
points to a seemingly random offset of the page. We should not be doing
that.
Just to be clear, I think casting pages to struct net_iov *, as it
currently is, is quite ugly, but that's something netmem_desc and this
effort can help with.
What you likely want to do is:
Patch 1:
struct page {
unsigned long flags;
union {
struct_group_tagged(netmem_desc, netmem_desc) {
// same layout as before
...
struct page_pool *pp;
...
};
}
}
struct net_iov {
unsigned long flags_padding;
union {
struct {
// same layout as in page + build asserts;
...
struct page_pool *pp;
...
};
struct netmem_desc desc;
};
};
struct netmem_desc *page_to_netmem_desc(struct page *page)
{
return &page->netmem_desc;
}
struct netmem_desc *netmem_to_desc(netmem_t netmem)
{
if (netmem_is_page(netmem))
return page_to_netmem_desc(netmem_to_page(netmem);
return &netmem_to_niov(netmem)->desc;
}
The compiler should be able to optimise the branch in netmem_to_desc(),
but we might need to help it a bit.
Then, patch 2 ... N convert page pool and everyone else accessing
those page fields directly to netmem_to_desc / etc.
And the final patch replaces the struct group in the page with a
new field:
struct netmem_desc {
struct page_pool *pp;
...
};
struct page {
unsigned long flags_padding;
union {
struct netmem_desc desc;
...
};
};
net_iov will drop its union in a later series to avoid conflicts.
btw, I don't think you need to convert page pool to netmem for this
to happen, so that can be done in a separate unrelated series. It's
18 patches, and netdev usually requires it to be no more than 15.
--
Pavel Begunkov
On Mon, May 26, 2025 at 05:58:10PM +0100, Pavel Begunkov wrote:
> On 5/26/25 02:37, Byungchul Park wrote:
> > On Fri, May 23, 2025 at 10:55:54AM -0700, Mina Almasry wrote:
> > > On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@sk.com> wrote:
> > > >
> > > > Now that all the users of the page pool members in struct page have been
> > > > gone, the members can be removed from struct page.
> > > >
> > > > However, since struct netmem_desc might still use the space in struct
> > > > page, the size of struct netmem_desc should be checked, until struct
> > > > netmem_desc has its own instance from slab, to avoid conficting with
> > > > other members within struct page.
> > > >
> > > > Remove the page pool members in struct page and add a static checker for
> > > > the size.
> > > >
> > > > Signed-off-by: Byungchul Park <byungchul@sk.com>
> > > > ---
> > > > include/linux/mm_types.h | 11 -----------
> > > > include/net/netmem.h | 28 +++++-----------------------
> > > > 2 files changed, 5 insertions(+), 34 deletions(-)
> > > >
> > > > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > > > index 873e820e1521..5a7864eb9d76 100644
> > > > --- a/include/linux/mm_types.h
> > > > +++ b/include/linux/mm_types.h
> > > > @@ -119,17 +119,6 @@ struct page {
> > > > */
> > > > unsigned long private;
> > > > };
> > > > - struct { /* page_pool used by netstack */
> > > > - unsigned long _pp_mapping_pad;
> > > > - /**
> > > > - * @pp_magic: magic value to avoid recycling non
> > > > - * page_pool allocated pages.
> > > > - */
> > > > - unsigned long pp_magic;
> > > > - struct page_pool *pp;
> > > > - unsigned long dma_addr;
> > > > - atomic_long_t pp_ref_count;
> > > > - };
> > > > struct { /* Tail pages of compound page */
> > > > unsigned long compound_head; /* Bit zero is set */
> > > > };
> > > > diff --git a/include/net/netmem.h b/include/net/netmem.h
> > > > index c63a7e20f5f3..257c22398d7a 100644
> > > > --- a/include/net/netmem.h
> > > > +++ b/include/net/netmem.h
> > > > @@ -77,30 +77,12 @@ struct net_iov_area {
> > > > unsigned long base_virtual;
> > > > };
> > > >
> > > > -/* These fields in struct page are used by the page_pool and net stack:
> > > > - *
> > > > - * struct {
> > > > - * unsigned long _pp_mapping_pad;
> > > > - * unsigned long pp_magic;
> > > > - * struct page_pool *pp;
> > > > - * unsigned long dma_addr;
> > > > - * atomic_long_t pp_ref_count;
> > > > - * };
> > > > - *
> > > > - * We mirror the page_pool fields here so the page_pool can access these fields
> > > > - * without worrying whether the underlying fields belong to a page or net_iov.
> > > > - *
> > > > - * The non-net stack fields of struct page are private to the mm stack and must
> > > > - * never be mirrored to net_iov.
> > > > +/* XXX: The page pool fields in struct page have been removed but they
> > > > + * might still use the space in struct page. Thus, the size of struct
> > > > + * netmem_desc should be under control until struct netmem_desc has its
> > > > + * own instance from slab.
> > > > */
> > > > -#define NET_IOV_ASSERT_OFFSET(pg, iov) \
> > > > - static_assert(offsetof(struct page, pg) == \
> > > > - offsetof(struct net_iov, iov))
> > > > -NET_IOV_ASSERT_OFFSET(pp_magic, pp_magic);
> > > > -NET_IOV_ASSERT_OFFSET(pp, pp);
> > > > -NET_IOV_ASSERT_OFFSET(dma_addr, dma_addr);
> > > > -NET_IOV_ASSERT_OFFSET(pp_ref_count, pp_ref_count);
> > > > -#undef NET_IOV_ASSERT_OFFSET
> > > > +static_assert(sizeof(struct netmem_desc) <= offsetof(struct page, _refcount));
> > > >
> > >
> > > Removing these asserts is actually a bit dangerous. Functions like
> > > netmem_or_pp_magic() rely on the fact that the offsets are the same
> > > between struct page and struct net_iov to access these fields without
> >
> > Worth noting this patch removes the page pool fields from struct page.
>
> static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> {
> return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
> }
>
> static inline atomic_long_t *netmem_get_pp_ref_count_ref(netmem_ref netmem)
> {
> return &__netmem_clear_lsb(netmem)->pp_ref_count;
> }
>
> That's a snippet of code after applying the series. So, let's say we
> take a page, it's casted to netmem, then the netmem (as it was before)
> is casted to net_iov. Before it relied on net_iov and the pp's part of
> the page having the same layout, which was checked by static asserts,
> but now, unless I'm mistaken, it's aligned in the exactly same way but
> points to a seemingly random offset of the page. We should not be doing
> that.
I told it in another thread. My bad. I will fix it.
> Just to be clear, I think casting pages to struct net_iov *, as it
> currently is, is quite ugly, but that's something netmem_desc and this
> effort can help with.
>
> What you likely want to do is:
>
> Patch 1:
>
> struct page {
> unsigned long flags;
> union {
> struct_group_tagged(netmem_desc, netmem_desc) {
> // same layout as before
> ...
> struct page_pool *pp;
> ...
> };
This part will be gone shortly. The matters come from absence of this
part.
> }
> }
>
> struct net_iov {
> unsigned long flags_padding;
> union {
> struct {
> // same layout as in page + build asserts;
> ...
> struct page_pool *pp;
> ...
> };
> struct netmem_desc desc;
> };
> };
>
> struct netmem_desc *page_to_netmem_desc(struct page *page)
> {
> return &page->netmem_desc;
page will not have any netmem things in it after this, that matters.
> }
>
> struct netmem_desc *netmem_to_desc(netmem_t netmem)
> {
> if (netmem_is_page(netmem))
> return page_to_netmem_desc(netmem_to_page(netmem);
> return &netmem_to_niov(netmem)->desc;
> }
>
> The compiler should be able to optimise the branch in netmem_to_desc(),
> but we might need to help it a bit.
>
>
> Then, patch 2 ... N convert page pool and everyone else accessing
> those page fields directly to netmem_to_desc / etc.
>
> And the final patch replaces the struct group in the page with a
> new field:
>
> struct netmem_desc {
> struct page_pool *pp;
> ...
> };
>
> struct page {
> unsigned long flags_padding;
> union {
> struct netmem_desc desc;
^
should be gone.
Byungchul
> ...
> };
> };
>
> net_iov will drop its union in a later series to avoid conflicts.
>
> btw, I don't think you need to convert page pool to netmem for this
> to happen, so that can be done in a separate unrelated series. It's
> 18 patches, and netdev usually requires it to be no more than 15.
>
> --
> Pavel Begunkov
On 5/27/25 02:02, Byungchul Park wrote:
...>> Patch 1:
>>
>> struct page {
>> unsigned long flags;
>> union {
>> struct_group_tagged(netmem_desc, netmem_desc) {
>> // same layout as before
>> ...
>> struct page_pool *pp;
>> ...
>> };
>
> This part will be gone shortly. The matters come from absence of this
> part.
Right, the problem is not having an explicit netmem_desc in struct
page and not using struct netmem_desc in all relevant helpers.
>> struct net_iov {
>> unsigned long flags_padding;
>> union {
>> struct {
>> // same layout as in page + build asserts;
>> ...
>> struct page_pool *pp;
>> ...
>> };
>> struct netmem_desc desc;
>> };
>> };
>>
>> struct netmem_desc *page_to_netmem_desc(struct page *page)
>> {
>> return &page->netmem_desc;
>
> page will not have any netmem things in it after this, that matters.
Ok, the question is where are you going to stash the fields?
We still need space to store them. Are you going to do the
indirection mm folks want?
AFAIK, the plan is that in the end pages will still have
netmem_desc but through an indirection. E.g.
static inline bool page_pool_page_is_pp(struct page *page)
{
return page->page_type == PAGE_PP_NET;
}
struct netmem_desc *page_to_netmem_desc(struct page *page)
{
return page->page_private;
}
--
Pavel Begunkov
On Mon, May 26, 2025 at 10:29 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
>
> On 5/27/25 02:02, Byungchul Park wrote:
> ...>> Patch 1:
> >>
> >> struct page {
> >> unsigned long flags;
> >> union {
> >> struct_group_tagged(netmem_desc, netmem_desc) {
> >> // same layout as before
> >> ...
> >> struct page_pool *pp;
> >> ...
> >> };
> >
> > This part will be gone shortly. The matters come from absence of this
> > part.
>
> Right, the problem is not having an explicit netmem_desc in struct
> page and not using struct netmem_desc in all relevant helpers.
>
> >> struct net_iov {
> >> unsigned long flags_padding;
> >> union {
> >> struct {
> >> // same layout as in page + build asserts;
> >> ...
> >> struct page_pool *pp;
> >> ...
> >> };
> >> struct netmem_desc desc;
> >> };
> >> };
> >>
> >> struct netmem_desc *page_to_netmem_desc(struct page *page)
> >> {
> >> return &page->netmem_desc;
> >
> > page will not have any netmem things in it after this, that matters.
>
> Ok, the question is where are you going to stash the fields?
> We still need space to store them. Are you going to do the
> indirection mm folks want?
>
I think I see some confusion here. I'm not sure indirection is what mm
folks want. The memdesc effort has already been implemented for zpdesc
and ptdesc[1], and the approach they did is very different from this
series. zpdesc and ptdesc have created a struct that mirrors the
entirety of struct page, not a subfield of struct page with
indirection:
https://elixir.bootlin.com/linux/v6.14.3/source/mm/zpdesc.h#L29
I'm now a bit confused, because the code changes in this series do not
match the general approach that zpdesc and ptdesc have done.
Byungchul, is the deviation in approach from zpdesc and ptdecs
intentional? And if so why? Should we follow the zpdesc and ptdesc
lead and implement a new struct that mirrors the entirety of struct
page?
[1] https://kernelnewbies.org/MatthewWilcox/Memdescs/Path
--
Thanks,
Mina
On 5/27/25 18:38, Mina Almasry wrote:
...>>>> struct netmem_desc *page_to_netmem_desc(struct page *page)
>>>> {
>>>> return &page->netmem_desc;
>>>
>>> page will not have any netmem things in it after this, that matters.
>>
>> Ok, the question is where are you going to stash the fields?
>> We still need space to store them. Are you going to do the
>> indirection mm folks want?
>>
>
> I think I see some confusion here. I'm not sure indirection is what mm
> folks want. The memdesc effort has already been implemented for zpdesc
To the best of my knowledge, it is. What you're looking at should be
a temporary state before all other users are converted, after which
mm will shrink the page in a single patch / small series.
> and ptdesc[1], and the approach they did is very different from this
> series. zpdesc and ptdesc have created a struct that mirrors the
> entirety of struct page, not a subfield of struct page with
> indirection:
>
> https://elixir.bootlin.com/linux/v6.14.3/source/mm/zpdesc.h#L29
>
> I'm now a bit confused, because the code changes in this series do not
> match the general approach that zpdesc and ptdesc have done.
In my estimation, the only bits that mm needs for a clean final
patch is a new struct with use case specific fields (i.e. netmem_desc),
a helper converting a page to it, and that everyone uses the helper
to access the fields. I'd argue a temporary placeholder in struct
page is an easier approach than separate overlays, but either is
fine to me.
> Byungchul, is the deviation in approach from zpdesc and ptdecs
> intentional? And if so why? Should we follow the zpdesc and ptdesc
> lead and implement a new struct that mirrors the entirety of struct
> page?
>
> [1] https://kernelnewbies.org/MatthewWilcox/Memdescs/Path
--
Pavel Begunkov
On Tue, May 27, 2025 at 10:38:43AM -0700, Mina Almasry wrote:
> On Mon, May 26, 2025 at 10:29 PM Pavel Begunkov <asml.silence@gmail.com> wrote:
> >
> > On 5/27/25 02:02, Byungchul Park wrote:
> > ...>> Patch 1:
> > >>
> > >> struct page {
> > >> unsigned long flags;
> > >> union {
> > >> struct_group_tagged(netmem_desc, netmem_desc) {
> > >> // same layout as before
> > >> ...
> > >> struct page_pool *pp;
> > >> ...
> > >> };
> > >
> > > This part will be gone shortly. The matters come from absence of this
> > > part.
> >
> > Right, the problem is not having an explicit netmem_desc in struct
> > page and not using struct netmem_desc in all relevant helpers.
> >
> > >> struct net_iov {
> > >> unsigned long flags_padding;
> > >> union {
> > >> struct {
> > >> // same layout as in page + build asserts;
> > >> ...
> > >> struct page_pool *pp;
> > >> ...
> > >> };
> > >> struct netmem_desc desc;
> > >> };
> > >> };
> > >>
> > >> struct netmem_desc *page_to_netmem_desc(struct page *page)
> > >> {
> > >> return &page->netmem_desc;
> > >
> > > page will not have any netmem things in it after this, that matters.
> >
> > Ok, the question is where are you going to stash the fields?
> > We still need space to store them. Are you going to do the
> > indirection mm folks want?
> >
>
> I think I see some confusion here. I'm not sure indirection is what mm
> folks want. The memdesc effort has already been implemented for zpdesc
> and ptdesc[1], and the approach they did is very different from this
> series. zpdesc and ptdesc have created a struct that mirrors the
It's struct netmem_desc. Just introducing struct netmem_desc that looks
exact same as struct net_iov, is ugly.
> entirety of struct page, not a subfield of struct page with
> indirection:
I think you got confused.
At the beginning, I tried to place a place-holder:
https://lore.kernel.org/all/20250512125103.GC45370@system.software.com/
But changed the direction as Matthew requested:
https://lore.kernel.org/all/aCK6J2YtA7vi1Kjz@casper.infradead.org/
So now, I will go with the same direction as the others. I will share
the updates version with the assert issues fixed.
Byungchul
>
> https://elixir.bootlin.com/linux/v6.14.3/source/mm/zpdesc.h#L29
>
> I'm now a bit confused, because the code changes in this series do not
> match the general approach that zpdesc and ptdesc have done.
> Byungchul, is the deviation in approach from zpdesc and ptdecs
> intentional? And if so why? Should we follow the zpdesc and ptdesc
> lead and implement a new struct that mirrors the entirety of struct
> page?
>
> [1] https://kernelnewbies.org/MatthewWilcox/Memdescs/Path
>
> --
> Thanks,
> Mina
On Tue, May 27, 2025 at 10:02:26AM +0900, Byungchul Park wrote:
> On Mon, May 26, 2025 at 05:58:10PM +0100, Pavel Begunkov wrote:
> > struct net_iov {
> > unsigned long flags_padding;
> > union {
> > struct {
> > // same layout as in page + build asserts;
> > ...
> > struct page_pool *pp;
> > ...
> > };
> > struct netmem_desc desc;
> > };
> > };
> >
> > struct netmem_desc *page_to_netmem_desc(struct page *page)
> > {
> > return &page->netmem_desc;
>
> page will not have any netmem things in it after this, that matters.
^
this patch series
Byungchul
>
> > }
> >
> > struct netmem_desc *netmem_to_desc(netmem_t netmem)
> > {
> > if (netmem_is_page(netmem))
> > return page_to_netmem_desc(netmem_to_page(netmem);
> > return &netmem_to_niov(netmem)->desc;
> > }
> >
> > The compiler should be able to optimise the branch in netmem_to_desc(),
> > but we might need to help it a bit.
> >
> >
> > Then, patch 2 ... N convert page pool and everyone else accessing
> > those page fields directly to netmem_to_desc / etc.
> >
> > And the final patch replaces the struct group in the page with a
> > new field:
> >
> > struct netmem_desc {
> > struct page_pool *pp;
> > ...
> > };
> >
> > struct page {
> > unsigned long flags_padding;
> > union {
> > struct netmem_desc desc;
> ^
> should be gone.
>
> Byungchul
> > ...
> > };
> > };
> >
> > net_iov will drop its union in a later series to avoid conflicts.
> >
> > btw, I don't think you need to convert page pool to netmem for this
> > to happen, so that can be done in a separate unrelated series. It's
> > 18 patches, and netdev usually requires it to be no more than 15.
> >
> > --
> > Pavel Begunkov
On Mon, May 26, 2025 at 9:57 AM Pavel Begunkov <asml.silence@gmail.com> wrote:
> >> Removing these asserts is actually a bit dangerous. Functions like
> >> netmem_or_pp_magic() rely on the fact that the offsets are the same
> >> between struct page and struct net_iov to access these fields without
> >
> > Worth noting this patch removes the page pool fields from struct page.
>
> static inline struct net_iov *__netmem_clear_lsb(netmem_ref netmem)
> {
> return (struct net_iov *)((__force unsigned long)netmem & ~NET_IOV);
> }
>
> static inline atomic_long_t *netmem_get_pp_ref_count_ref(netmem_ref netmem)
> {
> return &__netmem_clear_lsb(netmem)->pp_ref_count;
> }
>
> That's a snippet of code after applying the series. So, let's say we
> take a page, it's casted to netmem, then the netmem (as it was before)
> is casted to net_iov. Before it relied on net_iov and the pp's part of
> the page having the same layout, which was checked by static asserts,
> but now, unless I'm mistaken, it's aligned in the exactly same way but
> points to a seemingly random offset of the page. We should not be doing
> that.
>
Agreed.
> Just to be clear, I think casting pages to struct net_iov *, as it
> currently is, is quite ugly, but that's something netmem_desc and this
> effort can help with.
>
Agreed it's quite ugly. It was done in the name of optimizing the page
pool benchmark to the extreme as far as I can remember. We could use
page pool benchmark numbers on this series to make sure these new
changes aren't regressing the fast path.
--
Thanks,
Mina
Hi Byungchul,
kernel test robot noticed the following build errors:
[auto build test ERROR on f44092606a3f153bb7e6b277006b1f4a5b914cfc]
url: https://github.com/intel-lab-lkp/linux/commits/Byungchul-Park/netmem-introduce-struct-netmem_desc-struct_group_tagged-ed-on-struct-net_iov/20250523-112806
base: f44092606a3f153bb7e6b277006b1f4a5b914cfc
patch link: https://lore.kernel.org/r/20250523032609.16334-19-byungchul%40sk.com
patch subject: [PATCH 18/18] mm, netmem: remove the page pool members in struct page
config: x86_64-rhel-9.4-kunit (https://download.01.org/0day-ci/archive/20250524/202505240152.9ODpQBK0-lkp@intel.com/config)
compiler: gcc-12 (Debian 12.2.0-14) 12.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250524/202505240152.9ODpQBK0-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202505240152.9ODpQBK0-lkp@intel.com/
All errors (new ones prefixed by >>):
In file included from include/linux/net/intel/libie/rx.h:7,
from drivers/net/ethernet/intel/iavf/iavf_txrx.c:5:
include/net/libeth/rx.h: In function 'libeth_rx_sync_for_cpu':
include/net/libeth/rx.h:140:40: error: 'struct page' has no member named 'pp'
140 | page_pool_dma_sync_for_cpu(page->pp, page, fqe->offset, len);
| ^~
drivers/net/ethernet/intel/iavf/iavf_txrx.c: In function 'iavf_add_rx_frag':
>> drivers/net/ethernet/intel/iavf/iavf_txrx.c:1200:33: error: 'struct page' has no member named 'pp'
1200 | u32 hr = rx_buffer->page->pp->p.offset;
| ^~
drivers/net/ethernet/intel/iavf/iavf_txrx.c: In function 'iavf_build_skb':
drivers/net/ethernet/intel/iavf/iavf_txrx.c:1217:33: error: 'struct page' has no member named 'pp'
1217 | u32 hr = rx_buffer->page->pp->p.offset;
| ^~
--
In file included from drivers/net/ethernet/intel/idpf/idpf_txrx.c:4:
include/net/libeth/rx.h: In function 'libeth_rx_sync_for_cpu':
include/net/libeth/rx.h:140:40: error: 'struct page' has no member named 'pp'
140 | page_pool_dma_sync_for_cpu(page->pp, page, fqe->offset, len);
| ^~
drivers/net/ethernet/intel/idpf/idpf_txrx.c: In function 'idpf_rx_page_rel':
>> drivers/net/ethernet/intel/idpf/idpf_txrx.c:389:45: error: 'struct page' has no member named 'pp'
389 | page_pool_put_full_page(rx_buf->page->pp, rx_buf->page, false);
| ^~
drivers/net/ethernet/intel/idpf/idpf_txrx.c: In function 'idpf_rx_add_frag':
drivers/net/ethernet/intel/idpf/idpf_txrx.c:3254:30: error: 'struct page' has no member named 'pp'
3254 | u32 hr = rx_buf->page->pp->p.offset;
| ^~
drivers/net/ethernet/intel/idpf/idpf_txrx.c: In function 'idpf_rx_hsplit_wa':
drivers/net/ethernet/intel/idpf/idpf_txrx.c:3286:64: error: 'struct page' has no member named 'pp'
3286 | dst = page_address(hdr->page) + hdr->offset + hdr->page->pp->p.offset;
| ^~
drivers/net/ethernet/intel/idpf/idpf_txrx.c:3287:64: error: 'struct page' has no member named 'pp'
3287 | src = page_address(buf->page) + buf->offset + buf->page->pp->p.offset;
| ^~
drivers/net/ethernet/intel/idpf/idpf_txrx.c: In function 'idpf_rx_build_skb':
drivers/net/ethernet/intel/idpf/idpf_txrx.c:3305:27: error: 'struct page' has no member named 'pp'
3305 | u32 hr = buf->page->pp->p.offset;
| ^~
--
In file included from drivers/net/wireless/mediatek/mt76/mt76x2/../mt76x02.h:12,
from drivers/net/wireless/mediatek/mt76/mt76x2/mt76x2.h:23,
from drivers/net/wireless/mediatek/mt76/mt76x2/eeprom.c:9:
drivers/net/wireless/mediatek/mt76/mt76x2/../mt76.h: In function 'mt76_put_page_pool_buf':
>> drivers/net/wireless/mediatek/mt76/mt76x2/../mt76.h:1788:37: error: 'struct page' has no member named 'pp'
1788 | page_pool_put_full_page(page->pp, page, allow_direct);
| ^~
vim +1200 drivers/net/ethernet/intel/iavf/iavf_txrx.c
7f12ad741a4870 drivers/net/ethernet/intel/i40evf/i40e_txrx.c Greg Rose 2013-12-21 1184
ab9ad98eb5f95b drivers/net/ethernet/intel/i40evf/i40e_txrx.c Jesse Brandeburg 2016-04-18 1185 /**
56184e01c00d6d drivers/net/ethernet/intel/iavf/iavf_txrx.c Jesse Brandeburg 2018-09-14 1186 * iavf_add_rx_frag - Add contents of Rx buffer to sk_buff
ab9ad98eb5f95b drivers/net/ethernet/intel/i40evf/i40e_txrx.c Jesse Brandeburg 2016-04-18 1187 * @skb: sk_buff to place the data into
5fa4caff59f251 drivers/net/ethernet/intel/iavf/iavf_txrx.c Alexander Lobakin 2024-04-18 1188 * @rx_buffer: buffer containing page to add
a0cfc3130eef54 drivers/net/ethernet/intel/i40evf/i40e_txrx.c Alexander Duyck 2017-03-14 1189 * @size: packet length from rx_desc
ab9ad98eb5f95b drivers/net/ethernet/intel/i40evf/i40e_txrx.c Jesse Brandeburg 2016-04-18 1190 *
ab9ad98eb5f95b drivers/net/ethernet/intel/i40evf/i40e_txrx.c Jesse Brandeburg 2016-04-18 1191 * This function will add the data contained in rx_buffer->page to the skb.
fa2343e9034ce6 drivers/net/ethernet/intel/i40evf/i40e_txrx.c Alexander Duyck 2017-03-14 1192 * It will just attach the page as a frag to the skb.
ab9ad98eb5f95b drivers/net/ethernet/intel/i40evf/i40e_txrx.c Jesse Brandeburg 2016-04-18 1193 *
fa2343e9034ce6 drivers/net/ethernet/intel/i40evf/i40e_txrx.c Alexander Duyck 2017-03-14 1194 * The function will then update the page offset.
ab9ad98eb5f95b drivers/net/ethernet/intel/i40evf/i40e_txrx.c Jesse Brandeburg 2016-04-18 1195 **/
5fa4caff59f251 drivers/net/ethernet/intel/iavf/iavf_txrx.c Alexander Lobakin 2024-04-18 1196 static void iavf_add_rx_frag(struct sk_buff *skb,
5fa4caff59f251 drivers/net/ethernet/intel/iavf/iavf_txrx.c Alexander Lobakin 2024-04-18 1197 const struct libeth_fqe *rx_buffer,
a0cfc3130eef54 drivers/net/ethernet/intel/i40evf/i40e_txrx.c Alexander Duyck 2017-03-14 1198 unsigned int size)
ab9ad98eb5f95b drivers/net/ethernet/intel/i40evf/i40e_txrx.c Jesse Brandeburg 2016-04-18 1199 {
5fa4caff59f251 drivers/net/ethernet/intel/iavf/iavf_txrx.c Alexander Lobakin 2024-04-18 @1200 u32 hr = rx_buffer->page->pp->p.offset;
efa14c3985828d drivers/net/ethernet/intel/iavf/iavf_txrx.c Mitch Williams 2019-05-14 1201
fa2343e9034ce6 drivers/net/ethernet/intel/i40evf/i40e_txrx.c Alexander Duyck 2017-03-14 1202 skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags, rx_buffer->page,
5fa4caff59f251 drivers/net/ethernet/intel/iavf/iavf_txrx.c Alexander Lobakin 2024-04-18 1203 rx_buffer->offset + hr, size, rx_buffer->truesize);
9a064128fc8489 drivers/net/ethernet/intel/i40evf/i40e_txrx.c Alexander Duyck 2017-03-14 1204 }
9a064128fc8489 drivers/net/ethernet/intel/i40evf/i40e_txrx.c Alexander Duyck 2017-03-14 1205
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
© 2016 - 2025 Red Hat, Inc.