On Mon, Dec 18, 2023 at 2:06 PM Jakub Kicinski <kuba@xxxxxxxxxx> wrote: > > 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@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? > > 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. 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
