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 at redhat.com>