Re: [PATCH 0/3] virtio-net: graceful drop of vhost for TAP

[Daniel P . Berrangé]

On Thu, Feb 18, 2021 at 09:55:25PM +0200, Yuri Benditovich wrote:
> On Thu, Feb 18, 2021 at 11:35 AM Daniel P. Berrangé <berrange@redhat.com>
> wrote:
> 
> > On Wed, Feb 10, 2021 at 02:19:59PM +0800, Jason Wang wrote:
> > >
> > > On 2021/2/9 下午11:04, Michael S. Tsirkin wrote:
> > > > On Tue, Feb 09, 2021 at 02:51:05PM +0000, Daniel P. Berrangé wrote:
> > > > > On Tue, Feb 09, 2021 at 09:34:20AM -0500, Michael S. Tsirkin wrote:
> > > > > > On Thu, Feb 04, 2021 at 10:29:12PM +0200, Yuri Benditovich wrote:
> > > > > > > This set of patches introduces graceful switch from tap-vhost to
> > > > > > > tap-no-vhost depending on guest features. Before that the
> > features
> > > > > > > that vhost does not support were silently cleared in
> > get_features.
> > > > > > > This creates potential problem of migration from the machine
> > where
> > > > > > > some of virtio-net features are supported by the vhost kernel to
> > the
> > > > > > > machine where they are not supported (packed ring as an example).
> > > > > > I still worry that adding new features will silently disable vhost
> > for people.
> > > > > > Can we limit the change to when a VM is migrated in?
> > > > > Some management applications expect bi-directional live migration to
> > > > > work, so taking specific actions on incoming migration only feels
> > > > > dangerous.
> > > > Could you be more specific?
> > > >
> > > > Bi-directional migration is currently broken
> > > > when migrating new kernel->old kernel.
> > > >
> > > > This seems to be the motivation for this patch, though I wish
> > > > it was spelled out more explicitly.
> > > >
> > > > People don't complain much, but I'm fine with fixing that
> > > > with a userspace fallback.
> > > >
> > > >
> > > > I'd rather not force the fallback on others though: vhost is generally
> > > > specified explicitly by user while features are generally set
> > > > automatically, so this patch will make us override what user specified,
> > > > not nice.
> > > >
> > > >
> > > > > IMHO if the features we're adding cannot be expected to exist in
> > > > > host kernels in general, then the feature should defualt to off
> > > > > and require explicit user config to enable.
> > > > > Downstream distros which can guarantee newer kernels can flip the
> > > > > default in their custom machine types if they desire.
> > > > >
> > > > > Regards,
> > > > > Daniel
> > > > Unfortunately that will basically mean we are stuck with no new
> > features
> > > > for years. We did what this patch is trying to change for years now, in
> > > > particular KVM also seems to happily disable CPU features not supported
> > > > by kernel so I wonder why we can't keep doing it, with tweaks for some
> > > > corner cases.
> > >
> > >
> > > It's probably not the corner case.
> > >
> > > So my understanding is when a feature is turned on via command line, it
> > > should not be cleared silently otherwise we may break migration for sure.
> > >
> > > E.g when packed=on is specified, we should disable vhost instead of
> > clear it
> > > from the device.
> >
> > If something is explicitly turned on by the user, they expect that feature
> > to be honoured, or an error to be raised.
> >
> > If something is not explicitly turned on by the user, the behaviour wrt the
> > default should be stable for any given machine type version.
> >
> > IOW, if you disable vhost by default when packed=on is set, then you can't
> > later switch to letting vhost be enabled with packed=on, unless you tie
> > that change to a new machine type.
> >
> > If the user has explicitly said  packed=on *and* vhost=on, then should
> > must honour that, or raise an error if the combination is unsupportable.
> > Silently disabling vhost, then vhost=on is not ok.
> >
> 
> If I'm not mistaken:
> Inside qemu there is no possibility to determine whether the user
> explicitly turned vhost on.
> For qemu the vhost is off by default but libvirt creates a new profile with
> vhost on.

Yes, libvirt will always attempt to enable vhost if it is present on te
host with /dev/vhost-net, except where the user explicitly told us not
to.


Regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|