Messages in this thread | | | Date | Tue, 27 Sep 2022 18:16:19 +0100 | Subject | Re: [PATCH net-next 0/4] shrink struct ubuf_info | From | Pavel Begunkov <> |
| |
On 9/27/22 15:28, Pavel Begunkov wrote: > Hello Paolo, > > On 9/27/22 14:49, Paolo Abeni wrote: >> Hello, >> >> On Fri, 2022-09-23 at 17:39 +0100, Pavel Begunkov wrote: >>> struct ubuf_info is large but not all fields are needed for all >>> cases. We have limited space in io_uring for it and large ubuf_info >>> prevents some struct embedding, even though we use only a subset >>> of the fields. It's also not very clean trying to use this typeless >>> extra space. >>> >>> Shrink struct ubuf_info to only necessary fields used in generic paths, >>> namely ->callback, ->refcnt and ->flags, which take only 16 bytes. And >>> make MSG_ZEROCOPY and some other users to embed it into a larger struct >>> ubuf_info_msgzc mimicking the former ubuf_info. >>> >>> Note, xen/vhost may also have some cleaning on top by creating >>> new structs containing ubuf_info but with proper types. >> >> That sounds a bit scaring to me. If I read correctly, every uarg user >> should check 'uarg->callback == msg_zerocopy_callback' before accessing >> any 'extend' fields. > > Providers of ubuf_info access those fields via callbacks and so already > know the actual structure used. The net core, on the opposite, should > keep it encapsulated and not touch them at all. > > The series lists all places where we use extended fields just on the > merit of stripping the structure of those fields and successfully > building it. The only user in net/ipv{4,6}/* is MSG_ZEROCOPY, which > again uses callbacks. > > Sounds like the right direction for me. There is a couple of > places where it might get type safer, i.e. adding types instead > of void* in for struct tun_msg_ctl and getting rid of one macro > hiding types in xen. But seems more like TODO for later. > >> AFAICS the current code sometimes don't do the >> explicit test because the condition is somewhat implied, which in turn >> is quite hard to track. >> >> clearing uarg->zerocopy for the 'wrong' uarg was armless and undetected >> before this series, and after will trigger an oops.. > > And now we don't have this field at all to access, considering that > nobody blindly casts it. > >> There is some noise due to uarg -> uarg_zc renaming which make the >> series harder to review. Have you considered instead keeping the old >> name and introducing a smaller 'struct ubuf_info_common'? the overall >> code should be mostly the same, but it will avoid the above mentioned >> noise. > > I don't think there will be less noise this way, but let me try > and see if I can get rid of some churn.
It doesn't look any better for me
TL;DR; This series converts only 3 users: tap, xen and MSG_ZEROCOPY and doesn't touch core code. If we do ubuf_info_common though I'd need to convert lots of places in skbuff.c and multiple places across tcp/udp, which is much worse. And then I'd still need to touch all users to do ubuf_info -> ubuf_info_common conversion and all in a single commit to not break build.
If it's about naming, I can add a tree-wide renaming patch on top.
Paolo, I'd appreciate if you let know whether you're fine with it or not, I don't want the series to get stuck. For bug concerns, all places touching those optional fields are converted to ubuf_info_msgzc, and I wouldn't say 4/4 is so bad.
-- Pavel Begunkov
| |