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

On Mon, Dec 18, 2023 at 2:06 PM Jakub Kicinski [off-list ref] wrote:

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

quoted

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.

quoted

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.

quoted

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

quoted

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.

quoted

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.

Sorry about that. I misread your original request as 'here is
something else you can do if you want', not something that you feel is
critical. Honestly I missed the subtlety and the approaches seemed
roughly equivalent to me. I will respin after the 24hr cooldown.

-- 
Thanks,
Mina