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

[Jason Wang]

在 2021/3/26 下午5:09, Yuri Benditovich 写道:
> On Fri, Mar 26, 2021 at 10:51 AM Jason Wang <jasowang@redhat.com> wrote:
> > 在 2021/3/25 下午5:00, Yuri Benditovich 写道:
> > > Hi Jason,
> > >
> > > This was discussed earlier on the previous series of patches.
> > > https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg01829.html
> > > There were strong objections from both Daniel and Michael and I feel
> > > that the series was rejected.
> > > There was Michael's claim:
> > > "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."
> >
> > So for cpu feautres, it works since the management have other tool to
> > the cpuid. Then management will make sure the migration happens amongs
> > the hosts that is compatibile with the same cpuid sets.
> >
> > For vhost, we don't have such capabilities, that's why I think we need
> > to have fallback.
> Hi Jason,
> What, from your POV was the result of v1 discussion?


It looks to me we don't have an agreement on that, sorry.


> IMO, there was one critical comment that the patch does not address
> 'forcevhost' properly (indeed).
> IMO, there are many comments from Daniel and Michael that in the sum
> say that this change is not what they would like.
> If I'm mistaken please let me know.


I think I will open a new thread and summarize the different approaches 
and then we can come a conclusion.


> I have no problem to send v3 = v1 + handling of ''forcevhost'
> If this is what you want, please let me know also.
>
> > > https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03187.html
> > > And it was Michael's question:
> > > "Can we limit the change to when a VM is migrated in?"
> > > https://lists.gnu.org/archive/html/qemu-devel/2021-02/msg03163.html
> > > So I'm trying to suggest another approach:
> > > - In case of conflicting features (for example RSS and vhost) we in
> > > qemu we do not have enough information to prefer one or another.
> > > - If we drop to userspace in the first set_features we say: "vhost is
> > > less important than other requested features"
> > > - This series keeps backward compatibility, i.e. if you start with
> > > vhost and some features are not available - they are silently cleared.
> > > - But in case the features are available on source machine - they are used
> > > - In case of migration this series says: "We prefer successful
> > > migration even if for that we need to drop to userspace"
> > > - On the migration back to the 1st system we again work with all the
> > > features and with vhost as all the features are available.
> >
> > One issue for this approach is that. Consider we had two drivers:
> >
> > 1) Driver A that supports split only
> > 2) Driver B that supports packed
> >
> > Consider src support packed but dest doesn't
> >
> > So switching driver A to driver B works without migration. But if we
> > switch driver from A to B after migration it won't work?
> I assume that  both src and dest started with vhost=on.
>
> As driver B supports both packed and split, you can switch from driver
> A to driver B after migration
> and driver B will work with split. Exactly as it does today.
>
> The key question is what is more important - vhost or features that
> vhost does not support?
> current code says: vhost is more important always
> v1 patch says: features are more important always.
> v2 patch says: vhost is more important at init time, features are more
> important at migration time.
> Because we are able to drop vhost but we can't drop features when we
> have a running driver.
> Do you agree?


I think what came from cli is the most important. So if I understand 
correclty:

- vhost=on means "turn on vhost when possible" it implies that fallback 
is allowed (we had already had fallback codes)
- vhostforce=on means "turn on vhost unconditonally" it implies that we 
can't do fallback

So my understanding is that:

- "vhost=on, packed=on", we can fallback to userspace but must keep 
packed virtqueue works
- "vhost=on,vhostforce=on,packed=on", we can't fallback and must keep 
both vhost and packed virtqueue work, if we can't we need to fail

Thanks


> > Thanks
> >
> >
> > > Thanks,
> > > Yuri
> > >
> > >
> > >
> > > On Thu, Mar 25, 2021 at 8:59 AM Jason Wang <jasowang@redhat.com> wrote:
> > > > 在 2021/3/22 下午8:24, Yuri Benditovich 写道:
> > > > > Allow fallback to userspace only upon migration, only for specific features
> > > > > and only if 'vhostforce' is not requested.
> > > > >
> > > > > Changes from v1:
> > > > > Patch 1 dropeed (will be submitted in another series)
> > > > > Added device callback in case the migration should fail due to missing features
> > > > Hi Yuri:
> > > >
> > > > Have a quick glance at the series. A questions is why we need to do the
> > > > fallback only during load?
> > > >
> > > > I think we should do it in the device initializating. E.g when the vhost
> > > > features can not satisfy, we should disable vhost since there.
> > > >
> > > > Thanks
> > > >
> > > >
> > > > > Yuri Benditovich (3):
> > > > >      net: add ability to hide (disable) vhost_net
> > > > >      virtio: introduce 'missing_features_migrated' device callback
> > > > >      virtio-net: implement missing_features_migrated callback
> > > > >
> > > > >     hw/net/vhost_net.c         |  4 ++-
> > > > >     hw/net/virtio-net.c        | 51 ++++++++++++++++++++++++++++++++++++++
> > > > >     hw/virtio/virtio.c         |  8 ++++++
> > > > >     include/hw/virtio/virtio.h |  8 ++++++
> > > > >     include/net/net.h          |  1 +
> > > > >     5 files changed, 71 insertions(+), 1 deletion(-)