On Fri, Dec 27, 2024 at 01:34:10PM +0900, Akihiko Odaki wrote: > 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. qemu can be automatic. kernel I am not sure. > 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. Try? > Regards, > Akihiko Odaki
