Re: [PATCH net-next 0/4] shrink struct ubuf_info

Pavel Begunkov
2022-09-27T20:19:28Z

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