shrink struct ubuf_info

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.

Hello,

Quoted 11 lines · Pavel Begunkov

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.

Quoted 2 lines

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.

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..

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.

Hello Paolo,

Quoted 14 lines · Paolo Abeni

Hello,

Quoted 11 lines · Pavel Begunkov

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 5 lines

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.

Quoted 48 lines · Pavel Begunkov

Hello Paolo,

Quoted 14 lines · Paolo Abeni

Hello,

Quoted 11 lines · Pavel Begunkov

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 58 lines · Pavel Begunkov

Quoted 48 lines · Pavel Begunkov

Hello Paolo,

Quoted 14 lines · Paolo Abeni

Hello,

Quoted 11 lines · Pavel Begunkov

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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?

Quoted 99 lines · Paolo Abeni

Quoted 58 lines · Pavel Begunkov

Quoted 48 lines · Pavel Begunkov

Hello Paolo,

Quoted 14 lines · Paolo Abeni

Hello,

Quoted 11 lines · Pavel Begunkov

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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;
};

Quoted 7 lines

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?

Quoted 114 lines · Pavel Begunkov

Quoted 99 lines · Paolo Abeni

Quoted 58 lines · Pavel Begunkov

Quoted 48 lines · Pavel Begunkov

Hello Paolo,

Quoted 14 lines · Paolo Abeni

Hello,

Quoted 11 lines · Pavel Begunkov

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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;
};

Quoted 7 lines

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.

Quoted 3 lines

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 ;)

On the flip side the change suggested would probably not be as straighforward as I would hope for.

Quoted 1 line

ubuf_info_common/ubuf_info vs ubuf_info/ubuf_info_msgzc?

The specific names used are not much relevant.

Quoted 1 line

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.

Quoted 115 lines · Paolo Abeni

Quoted 114 lines · Pavel Begunkov

Quoted 99 lines · Paolo Abeni

Quoted 58 lines · Pavel Begunkov

Quoted 48 lines · Pavel Begunkov

Hello Paolo,

Quoted 14 lines · Paolo Abeni

Hello,

Quoted 11 lines · Pavel Begunkov

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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;
};

Quoted 7 lines

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.

Quoted 7 lines

Quoted 3 lines

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

Quoted 5 lines

On the flip side the change suggested would probably not be as straighforward as I would hope for.

Quoted 1 line

ubuf_info_common/ubuf_info vs ubuf_info/ubuf_info_msgzc?

Quoted 7 lines

The specific names used are not much relevant.

Quoted 1 line

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

Quoted 157 lines · Pavel Begunkov

Quoted 115 lines · Paolo Abeni

Quoted 114 lines · Pavel Begunkov

Quoted 99 lines · Paolo Abeni

Quoted 58 lines · Pavel Begunkov

Quoted 48 lines · Pavel Begunkov

Hello Paolo,

Quoted 14 lines · Paolo Abeni

Hello,

Quoted 11 lines · Pavel Begunkov

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.

Quoted 6 lines

Quoted 2 lines

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.

Quoted 6 lines

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.

Quoted 8 lines

Quoted 5 lines

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.

Quoted 6 lines

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;
};

Quoted 7 lines

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.

Quoted 7 lines

Quoted 3 lines

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

Quoted 5 lines

On the flip side the change suggested would probably not be as straighforward as I would hope for.

Quoted 1 line

ubuf_info_common/ubuf_info vs ubuf_info/ubuf_info_msgzc?

Quoted 7 lines

The specific names used are not much relevant.

Quoted 1 line

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:

Quoted 2 lines

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 <[email protected]>