Re: [PATCH v2 4/4] virtio-net: Add support for USO features

[Peter Xu]

On Mon, Jul 29, 2024 at 01:45:12PM +0900, Akihiko Odaki wrote:
> On 2024/07/29 12:50, Jason Wang wrote:
> > On Sun, Jul 28, 2024 at 11:19 PM Akihiko Odaki <akihiko.odaki@daynix.com> 
> > wrote:
> > > 
> > > On 2024/07/27 5:47, Peter Xu wrote:
> > > > On Fri, Jul 26, 2024 at 04:17:12PM +0100, Daniel P. Berrangé wrote:
> > > > > On Fri, Jul 26, 2024 at 10:43:42AM -0400, Peter Xu wrote:
> > > > > > On Fri, Jul 26, 2024 at 09:48:02AM +0100, Daniel P. Berrangé wrote:
> > > > > > > On Fri, Jul 26, 2024 at 09:03:24AM +0200, Thomas Huth wrote:
> > > > > > > > On 26/07/2024 08.08, Michael S. Tsirkin wrote:
> > > > > > > > > On Thu, Jul 25, 2024 at 06:18:20PM -0400, Peter Xu wrote:
> > > > > > > > > > On Tue, Aug 01, 2023 at 01:31:48AM +0300, Yuri Benditovich 
> > > > > > > > > > wrote:
> > > > > > > > > > > USO features of virtio-net device depend on kernel ability
> > > > > > > > > > > to support them, for backward compatibility by default the
> > > > > > > > > > > features are disabled on 8.0 and earlier.
> > > > > > > > > > > 
> > > > > > > > > > > Signed-off-by: Yuri Benditovich 
> > > > > > > > > > > <yuri.benditovich@daynix.com>
> > > > > > > > > > > Signed-off-by: Andrew Melnychecnko <andrew@daynix.com>
> > > > > > > > > > 
> > > > > > > > > > Looks like this patch broke migration when the VM starts on 
> > > > > > > > > > a host that has
> > > > > > > > > > USO supported, to another host that doesn't..
> > > > > > > > > 
> > > > > > > > > This was always the case with all offloads. The answer at the 
> > > > > > > > > moment is,
> > > > > > > > > don't do this.
> > > > > > > > 
> > > > > > > > May I ask for my understanding:
> > > > > > > > "don't do this" = don't automatically enable/disable virtio 
> > > > > > > > features in QEMU
> > > > > > > > depending on host kernel features, or "don't do this" = don't 
> > > > > > > > try to migrate
> > > > > > > > between machines that have different host kernel features?
> > > > > > > > 
> > > > > > > > > Long term, we need to start exposing management APIs
> > > > > > > > > to discover this, and management has to disable unsupported 
> > > > > > > > > features.
> > > > > > > > 
> > > > > > > > Ack, this likely needs some treatments from the libvirt side, 
> > > > > > > > too.
> > > > > > > 
> > > > > > > When QEMU automatically toggles machine type featuers based on 
> > > > > > > host
> > > > > > > kernel, relying on libvirt to then disable them again is 
> > > > > > > impractical,
> > > > > > > as we cannot assume that the libvirt people are using knows about
> > > > > > > newly introduced features. Even if libvirt is updated to know 
> > > > > > > about
> > > > > > > it, people can easily be using a previous libvirt release.
> > > > > > > 
> > > > > > > QEMU itself needs to make the machine types do that they are there
> > > > > > > todo, which is to define a stable machine ABI.
> > > > > > > 
> > > > > > > What QEMU is missing here is a "platform ABI" concept, to encode
> > > > > > > sets of features which are tied to specific platform generations.
> > > > > > > As long as we don't have that we'll keep having these broken
> > > > > > > migration problems from machine types dynamically changing instead
> > > > > > > of providing a stable guest ABI.
> > > > > > 
> > > > > > Any more elaboration on this idea?  Would it be easily feasible in
> > > > > > implementation?
> > > > > 
> > > > > In terms of launching QEMU I'd imagine:
> > > > > 
> > > > >     $QEMU -machine pc-q35-9.1 -platform linux-6.9 ...args...
> > > > > 
> > > > > Any virtual machine HW features which are tied to host kernel features
> > > > > would have their defaults set based on the requested -platform. The
> > > > > -machine will be fully invariant wrt the host kernel.
> > > > > 
> > > > > You would have -platform hlep to list available platforms, and
> > > > > corresonding QMP "query-platforms" command to list what platforms
> > > > > are supported on a given host OS.
> > > > > 
> > > > > Downstream distros can provide their own platforms definitions
> > > > > (eg "linux-rhel-9.5") if they have kernels whose feature set
> > > > > diverges from upstream due to backports.
> > > > > 
> > > > > Mgmt apps won't need to be taught about every single little QEMU
> > > > > setting whose default is derived from the kernel. Individual
> > > > > defaults are opaque and controlled by the requested platform.
> > > > > 
> > > > > Live migration has clearly defined semantics, and mgmt app can
> > > > > use query-platforms to validate two hosts are compatible.
> > > > > 
> > > > > Omitting -platform should pick the very latest platform that is
> > > > > cmpatible with the current host (not neccessarily the latest
> > > > > platform built-in to QEMU).
> > > > 
> > > > This seems to add one more layer to maintain, and so far I don't know
> > > > whether it's a must.
> > > > 
> > > > To put it simple, can we simply rely on qemu cmdline as "the guest 
> > > > ABI"?  I
> > > > thought it was mostly the case already, except some extremely rare
> > > > outliers.
> > > > 
> > > > When we have one host that boots up a VM using:
> > > > 
> > > >     $QEMU1 $cmdline
> > > > 
> > > > Then another host boots up:
> > > > 
> > > >     $QEMU2 $cmdline -incoming XXX
> > > > 
> > > > Then migration should succeed if $cmdline is exactly the same, and the 
> > > > VM
> > > > can boot up all fine without errors on both sides.
> > > > 
> > > > AFAICT this has nothing to do with what kernel is underneath, even not
> > > > Linux?  I think either QEMU1 / QEMU2 has the option to fail.  But if it
> > > > didn't, I thought the ABI should be guaranteed.
> > > > 
> > > > That's why I think this is a migration violation, as 99.99% of other 
> > > > device
> > > > properties should be following this rule.  The issue here is, we have 
> > > > the
> > > > same virtio-net-pci cmdline on both sides in this case, but the ABI got
> > > > break.
> > > > 
> > > > That's also why I was suggesting if the property contributes to the 
> > > > guest
> > > > ABI, then AFAIU QEMU needs to:
> > > > 
> > > >     - Firstly, never quietly flipping any bit that affects the ABI...
> > > > 
> > > >     - Have a default value of off, then QEMU will always allow the VM 
> > > > to boot
> > > >       by default, while advanced users can opt-in on new features.  We 
> > > > can't
> > > >       make this ON by default otherwise some VMs can already fail to 
> > > > boot,
> > > 
> > > It may not be necessary the case that old features are supported by
> > > every systems. In an extreme case, a user may migrate a VM from Linux to
> > > Windows, which probably doesn't support any offloading at all. A more
> > > convincing scenario is RSS offloading with eBPF; using eBPF requires a
> > > privilege so we cannot assume it is always available even on the latest
> > > version of Linux.
> > 
> > I don't get why eBPF matters here. It is something that is not noticed
> > by the guest and we have a fallback anyhow.
> > 
> > > 
> > > > 
> > > >     - If the host doesn't support the feature while the cmdline enabled 
> > > > it,
> > > >       it needs to fail QEMU boot rather than flipping, so that it says 
> > > > "hey,
> > > >       this host does not support running such VM specified, due to XXX
> > > >       feature missing".
> > > 
> > > This is handled in:
> > > 
> > > "virtio-net: Convert feature properties to OnOffAuto"
> > > 20240714-auto-v3-0-e27401aabab3@daynix.com/">https://patchew.org/QEMU/20240714-auto-v3-0-e27401aabab3@daynix.com/
> > 
> > I may miss something but I think "Auto" doesn't make sense to libvirt.
> 
> The point is libvirt can explicitly set "on" to avoid the "auto" behavior.
> libvirt does not have to use the "auto" value.
> 
> libvirt can still use "auto" if desired. virDomainNetDefParseXMLDriver() in
> libvirt actually parses tristate values (libvirt uses "default" instead of
> "auto" as the mnemonic) for these features though "default" is currently
> disabled by the schema (src/conf/schemas/domaincommon.rng). Allowing user to
> specify "default" is only a matter of editing the schema. Of course
> specifying "default" will make the VM unsafe for migration.

Isn't keeping the default AUTO the same as before when it used to be ON?  I
mean, AUTO in a qemu cmdline doesn't guarantee guest API either.

Indeed it looks like it's a step forward to make ON having the clear
semantics of "fail when unsupported".  It's just that I am not sure how
useful is AUTO here, because anyway we'll need to break ON semantics even
with AUTO, so that an old QEMU script with USO=ON used to boot on old
kernels but not it won't.

What I was trying to say is whether we should make the default parameter to
be migratable.  IOW, it looks to me AUTO should deserve a migration
blocker when chosen.

After all, Libvirt hopefully shouldn't use AUTO at all but only ON/OFF,
while any user when not caring much on these perf details should always use
OFF on any kernel dependent features that may affect the guest ABI.

Thanks,

-- 
Peter Xu