Messages in this thread | | | Date | Tue, 27 Sep 2022 21:17:26 +0100 | Subject | Re: [PATCH net-next 0/4] shrink struct ubuf_info | From | Pavel Begunkov <> |
| |
On 9/27/22 20:59, Paolo Abeni wrote: > On Tue, 2022-09-27 at 19:48 +0100, Pavel Begunkov wrote: >> On 9/27/22 18:56, Paolo Abeni wrote: >>> On Tue, 2022-09-27 at 18:16 +0100, Pavel Begunkov wrote: >>>> 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. >>> >>> Uhmm... I underlook the fact we must preserve the current accessors for >>> the common fields. >>> >>> I guess something like the following could do (completely untested, >>> hopefully should illustrate the idea): >>> >>> struct ubuf_info { >>> struct_group_tagged(ubuf_info_common, common, >>> void (*callback)(struct sk_buff *, struct ubuf_info *, >>> bool zerocopy_success); >>> refcount_t refcnt; >>> u8 flags; >>> ); >>> >>> union { >>> struct { >>> unsigned long desc; >>> void *ctx; >>> }; >>> struct { >>> u32 id; >>> u16 len; >>> u16 zerocopy:1; >>> u32 bytelen; >>> }; >>> }; >>> >>> struct mmpin { >>> struct user_struct *user; >>> unsigned int num_pg; >>> } mmp; >>> }; >>> >>> Then you should be able to: >>> - access ubuf_info->callback, >>> - access the same field via ubuf_info->common.callback >>> - declare variables as 'struct ubuf_info_commom' with appropriate >>> contents. >>> >>> WDYT? >> >> Interesting, I didn't think about struct_group, this would >> let to split patches better and would limit non-core changes. >> But if the plan is to convert the core helpers to >> ubuf_info_common, than I think it's still messier than changing >> ubuf providers only. >> >> I can do the exercise, but I don't really see what is the goal. >> Let me ask this, if we forget for a second how diffs look, >> do you care about which pair is going to be in the end? > > Uhm... I proposed this initially with the goal of remove non fuctional > changes from a patch that was hard to digest for me (4/4). So it's > about diffstat to me ;)
Ah, got it
> On the flip side the change suggested would probably not be as > straighforward as I would hope for. > >> ubuf_info_common/ubuf_info vs ubuf_info/ubuf_info_msgzc? > > The specific names used are not much relevant. > >> Are there you concerned about naming or is there more to it? > > I feel like this series is potentially dangerous, but I could not spot > bugs into the code. I would have felt more relaxed eariler in the devel > cycle.
union { struct { unsigned long desc; void *ctx; }; struct { u32 id; u16 len; u16 zerocopy:1; u32 bytelen; }; };
btw, nobody would frivolously change ->zerocopy anyway as it's in a union. Even without the series we're absolutely screwed if someone does that. If anything it adds a way to get rid of it:
1) Make vhost and xen use their own structures with right types. 2) kill unused struct {ctx, desc} for MSG_ZEROCOPY
-- Pavel Begunkov
| |