On Thu, May 22, 2025 at 8:26 PM Byungchul Park <byungchul@xxxxxx> wrote:
>
> To simplify struct page, the page pool members of struct page should be
> moved to other, allowing these members to be removed from struct page.
>
> Introduce a network memory descriptor to store the members, struct
> netmem_desc, reusing struct net_iov that already mirrored struct page.
>
> While at it, relocate _pp_mapping_pad to group struct net_iov's fields.
>
> Signed-off-by: Byungchul Park <byungchul@xxxxxx>
> ---
> include/linux/mm_types.h | 2 +-
> include/net/netmem.h | 43 +++++++++++++++++++++++++++++++++-------
> 2 files changed, 37 insertions(+), 8 deletions(-)
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 56d07edd01f9..873e820e1521 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -120,13 +120,13 @@ 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 _pp_mapping_pad;
Like Toke says, moving this to the beginning of this struct is not
allowed. The first 3 bits of pp_magic are overlaid with page->lru so
the pp makes sure not to use them. _pp_mapping_pad is overlaid with
page->mapping, so the pp makes sure not to use it. AFAICT, this moving
of _pp_mapping_pad is not necessary for this patch. I think just drop
it.
> unsigned long dma_addr;
> atomic_long_t pp_ref_count;
> };
> diff --git a/include/net/netmem.h b/include/net/netmem.h
> index 386164fb9c18..08e9d76cdf14 100644
> --- a/include/net/netmem.h
> +++ b/include/net/netmem.h
> @@ -31,12 +31,41 @@ enum net_iov_type {
> };
>
> struct net_iov {
> - enum net_iov_type type;
> - unsigned long pp_magic;
> - struct page_pool *pp;
> - struct net_iov_area *owner;
> - unsigned long dma_addr;
> - atomic_long_t pp_ref_count;
> + /*
> + * XXX: Now that struct netmem_desc overlays on struct page,
> + * struct_group_tagged() should cover all of them. However,
> + * a separate struct netmem_desc should be declared and embedded,
> + * once struct netmem_desc is no longer overlayed but it has its
> + * own instance from slab. The final form should be:
> + *
> + * struct netmem_desc {
> + * unsigned long pp_magic;
> + * struct page_pool *pp;
> + * unsigned long dma_addr;
> + * atomic_long_t pp_ref_count;
> + * };
> + *
> + * struct net_iov {
> + * enum net_iov_type type;
> + * struct net_iov_area *owner;
> + * struct netmem_desc;
> + * };
> + */
I'm unclear on why moving to this format is a TODO for the future. Why
isn't this state in the comment the state in the code? I think I gave
the same code snippet on the RFC, but here again:
struct netmem_desc {
/**
* @pp_magic: magic value to avoid recycling non
* page_pool allocated pages.
*/
unsigned long pp_magic;
struct page_pool *pp;
unsigned long _pp_mapping_pad;
unsigned long dma_addr;
atomic_long_t pp_ref_count;
};
(Roughly):
struct page {
...
struct { /* page_pool used by netstack */
struct netmem_desc;
};
...
};
struct net_iov {
enum net_iov_type type;
struct netmem_desc;
struct net_iov_area *owner;
}
AFAICT, this should work..?
--
Thanks,
Mina
