Re: [PATCH 18/18] mm, netmem: remove the page pool members in struct page

[Pavel Begunkov <asml.silence@gmail.com>]

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