Messages in this thread | | | Date | Tue, 27 Sep 2022 22:02:37 +0100 | Subject | Re: [PATCH net-next 0/4] shrink struct ubuf_info | From | Pavel Begunkov <> |
| |
On 9/27/22 21:23, Paolo Abeni wrote: > On Tue, 2022-09-27 at 21:17 +0100, Pavel Begunkov wrote: >> 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 > > Ok, the above sounds reasonable. Additionally I've spent the last > surviving neuron on my side to on this series, and it looks sane, so... > > Acked-by: Paolo Abeni <pabeni@redhat.com>
Great, thanks for taking a look!
-- Pavel Begunkov
| |