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

Pavel Begunkov
2022-09-27T18:50:02Z

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?
ubuf_info_common/ubuf_info vs ubuf_info/ubuf_info_msgzc?
Are there you concerned about naming or is there more to it?

--
Pavel Begunkov