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