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

On Sun, 17 Dec 2023 00:14:59 -0800 Mina Almasry wrote:

quoted

quoted

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/ (local)

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.