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

[Mina Almasry]

On Sat, Dec 16, 2023 at 5:45 PM David Ahern <dsahern@xxxxxxxxxx> wrote:
>
> On 12/16/23 2:10 PM, Mina Almasry wrote:
> > On Fri, Dec 15, 2023 at 6:52 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote:
> >>
> >> On Wed, 13 Dec 2023 18:05:25 -0800 Mina Almasry wrote:
> >>> +struct netmem {
> >>> +     union {
> >>> +             struct page page;
> >>> +
> >>> +             /* Stub to prevent compiler implicitly converting from page*
> >>> +              * to netmem_t* and vice versa.
> >>> +              *
> >>> +              * Other memory type(s) net stack would like to support
> >>> +              * can be added to this union.
> >>> +              */
> >>> +             void *addr;
> >>> +     };
> >>> +};
> >>
> >> My mind went to something like:
> >>
> >> typedef unsigned long __bitwise netmem_ref;
> >>
> >> instead. struct netmem does not exist, it's a handle which has
> >> to be converted to a real type using a helper.
> >
> > 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@xxxxxxxxxxxxxxxxxxxx/
> >
> > 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?

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. I honestly could go either way on this. The union
provides some self documenting code and avoids casts. The
implementation without the union obfuscates the type and makes it much
more opaque.

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.

-- 
Thanks,
Mina