Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <85cccb780608e830024fc82a8e4f703031646f4e.camel@redhat.com>
Subject: Re: [PATCH net-next 0/4] shrink struct ubuf_info
From:   Paolo Abeni <pabeni@redhat.com>
To:     Pavel Begunkov <asml.silence@gmail.com>, netdev@vger.kernel.org
Cc:     "David S . Miller" <davem@davemloft.net>,
        Jakub Kicinski <kuba@kernel.org>, linux-kernel@vger.kernel.org,
        xen-devel@lists.xenproject.org, Wei Liu <wei.liu@kernel.org>,
        Paul Durrant <paul@xen.org>, kvm@vger.kernel.org,
        virtualization@lists.linux-foundation.org,
        "Michael S . Tsirkin" <mst@redhat.com>,
        Jason Wang <jasowang@redhat.com>
Date:   Tue, 27 Sep 2022 19:56:04 +0200
In-Reply-To: <bbb212f6-0165-0747-d99d-b49acbb02a80@gmail.com>
References: <cover.1663892211.git.asml.silence@gmail.com>
         <7fef56880d40b9d83cc99317df9060c4e7cdf919.camel@redhat.com>
         <021d8ea4-891c-237d-686e-64cecc2cc842@gmail.com>
         <bbb212f6-0165-0747-d99d-b49acbb02a80@gmail.com>
Content-Type: text/plain; charset="UTF-8"
User-Agent: Evolution 3.42.4 (3.42.4-2.fc35) 
MIME-Version: 1.0
Content-Transfer-Encoding: 8bit
Precedence: bulk

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?

Thanks,

Paolo