[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <20241227084256-mutt-send-email-mst@kernel.org>
Date: Fri, 27 Dec 2024 08:44:13 -0500
From: "Michael S. Tsirkin" <mst@...hat.com>
To: Akihiko Odaki <akihiko.odaki@...nix.com>
Cc: Jason Wang <jasowang@...hat.com>,
Eugenio Pérez <eperezma@...hat.com>,
kvm@...r.kernel.org, virtualization@...ts.linux.dev,
netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] vhost/net: Set num_buffers for virtio 1.0
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@...hat.com
> > <mailto:mst@...hat.com>> 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@...hat.com
> > <mailto:mst@...hat.com>> 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@...nix.com
> > <mailto:akihiko.odaki@...nix.com>>
> > > >
> > > > 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@daynix.com/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
Powered by blists - more mailing lists