Re: [RFC PATCH net-next v1 2/4] net: introduce abstraction for network memory

[Jakub Kicinski <kuba@kernel.org>]

On Sun, 17 Dec 2023 00:14:59 -0800 Mina Almasry wrote:
> > > Sure thing I can do that. Is it better to do something like:
> > >
> > > struct netmem_ref;
> > >
> > > like in this patch:
> > >
> > > https://lore.kernel.org/linux-mm/20221108194139.57604-1-torvalds@linux-foundation.org/
> > >
> > > Asking because checkpatch tells me not to add typedefs to the kernel,
> > > but checkpatch can be ignored if you think it's OK.
> > >
> > > Also with this approach I can't use container_of and I need to do a
> > > cast, I assume that's fine.
> > >  
> >
> > Isn't that the whole point of this set - to introduce a new data type
> > and avoid casts?  

I don't see how we can avoid casts if the type of the referenced object
is encoded on the low bits of the pointer. If we had a separate member
we could so something like:

struct netmem_ref {
	enum netmem_type type;
	union {
		struct page *p;
		struct page_pool_iov *pi;
	};
};

barring crazy things with endian-aware bitfields, we need at least one
cast.

> My understanding here the requirements from Jason are:
> 
> 1. Never pass a non-page to an mm api.
> 2. If a mangle a pointer to indicate it's not a page, then I must not
> call it mm's struct page*, I must add a new type.
> 
> I think both requirements are met regardless of whether
> netmem_to_page() is implemented using union/container_of or straight
> casts. folios implemented something similar being unioned with struct
> page to avoid casts. 

Folios overlay a real struct page. It's completely different.

> I honestly could go either way on this. The union
> provides some self documenting code and avoids casts. 

Maybe you guys know some trick to mask out the bottom bit :S

> The implementation without the union obfuscates the type and makes it much
> more opaque.

Some would say that that's the damn point of the wrapping..

You don't want non-core code futzing with the inside of the struct.

> I finished addressing the rest of the comments and I have this series
> and the next devmem TCP series ready to go, so I fired v2 of this
> patchset. If one feels strongly about this let me know and I will
> re-spin.

You didn't address my feedback :|

struct netmem which contains struct page by value is almost as bad
as passing around pretend struct page pointers.