On Thu, Feb 18, 2021 at 11:35 AM Daniel P. Berrangé <berra...@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. > > 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 :| > >
