On 2024/12/27 10:29, Jason Wang wrote:
On Thu, Dec 26, 2024 at 7:54 PM Michael S. Tsirkin <mst@xxxxxxxxxx
<mailto:mst@xxxxxxxxxx>> wrote:
On Mon, Nov 11, 2024 at 09:27:45AM +0800, Jason Wang wrote:
> On Wed, Nov 6, 2024 at 4:54 PM Michael S. Tsirkin <mst@xxxxxxxxxx
<mailto:mst@xxxxxxxxxx>> wrote:
> >
> > On Sun, Sep 15, 2024 at 10:35:53AM +0900, Akihiko Odaki wrote:
> > > The specification says the device MUST set num_buffers to 1 if
> > > VIRTIO_NET_F_MRG_RXBUF has not been negotiated.
> > >
> > > Fixes: 41e3e42108bc ("vhost/net: enable virtio 1.0")
> > > Signed-off-by: Akihiko Odaki <akihiko.odaki@xxxxxxxxxx
<mailto:akihiko.odaki@xxxxxxxxxx>>
> >
> > True, this is out of spec. But, qemu is also out of spec :(
> >
> > Given how many years this was out there, I wonder whether
> > we should just fix the spec, instead of changing now.
> >
> > Jason, what's your take?
>
> Fixing the spec (if you mean release the requirement) seems to be
less risky.
>
> Thanks
I looked at the latest spec patch.
Issue is, if we relax the requirement in the spec,
it just might break some drivers.
Something I did not realize at the time.
Also, vhost just leaves it uninitialized so there really is no chance
some driver using vhost looks at it and assumes 0.
> >
So it also has no chance to assume it for anything specific value.
Theoretically, there could be a driver written according to the
specification and tested with other device implementations that set
num_buffers to one.
Practically, I will be surprised if there is such a driver in reality.
But I also see few reasons to relax the device requirement now; if we
used to say it should be set to one and there is no better alternative
value, why don't stick to one?
I sent v2 for the virtio-spec change that retains the device requirement
so please tell me what you think about it:
https://lore.kernel.org/virtio-comment/20241227-reserved-v2-1-de9f9b0a808d@xxxxxxxxxx/T/#u
There is another thing out of spec with vhost at the moment:
it is actually leaving this field in the buffer
uninitialized. Which is out of spec, length supplied by device
must be initialized by device.
What do you mean by "length" here?
We generally just ask everyone to follow spec.
Spec can't cover all the behaviour, so there would be some leftovers.
So now I'm inclined to fix
it, and make a corresponding qemu change.
Now, about how to fix it - besides a risk to non-VM workloads, I dislike
doing an extra copy to user into buffer. So maybe we should add an ioctl
to teach tun to set num bufs to 1.
This way userspace has control.
I'm not sure I will get here. TUN has no knowledge of the mergeable
buffers if I understand it correctly.
I rather want QEMU and other vhost_net users automatically fixed instead
of opting-in the fix.
The extra copy overhead can be almost eliminated if we initialize the
field in TUN/TAP; they already writes other part of the header so we can
simply add two bytes there. But I wonder if it's worthwhile.
Regards,
Akihiko Odaki
