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

[Pavel Begunkov <asml.silence@gmail.com>]

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