Message-ID: <bbb212f6-0165-0747-d99d-b49acbb02a80@gmail.com>
Date:   Tue, 27 Sep 2022 18:16:19 +0100
From:   Pavel Begunkov <asml.silence@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org
Cc:     "David S . Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, linux-kernel@...r.kernel.org,
        xen-devel@...ts.xenproject.org, Wei Liu <wei.liu@...nel.org>,
        Paul Durrant <paul@....org>, kvm@...r.kernel.org,
        virtualization@...ts.linux-foundation.org,
        "Michael S . Tsirkin" <mst@...hat.com>,
        Jason Wang <jasowang@...hat.com>
Subject: Re: [PATCH net-next 0/4] shrink struct ubuf_info

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. And then I'd still need to touch all
users to do ubuf_info -> ubuf_info_common conversion and all in a
single commit to not break build.

If it's about naming, I can add a tree-wide renaming patch on top.

Paolo, I'd appreciate if you let know whether you're fine with it
or not, I don't want the series to get stuck. For bug concerns,
all places touching those optional fields are converted to
ubuf_info_msgzc, and I wouldn't say 4/4 is so bad.

-- 
Pavel Begunkov

Powered by blists - more mailing lists