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.
