On 6/26/23 11:37, David Marchand wrote: > On Fri, Jun 23, 2023 at 11:52 PM Ilya Maximets <[email protected]> wrote: >>>> But we can't really do that from the OVS side, can we? >>>> DPDK will clear the bit even if we enable it... >>> >>> Hum, I did not test it yet, but I was thinking of calling >>> rte_vhost_driver_enable_features. >>> Now that I look at the code, I think it works, since the clearing >>> happens in rte_vhost_driver_register(). >> >> Yeah, it should work. >> I think, we should just always disable every F_HOST_* and F_CSUM right >> after rte_vhost_driver_register(), and then enable what we need. Might >> be easier to follow the code this way. It's hard to always think in >> negatives. > > I prefer too, let's see how it goes when implementing. > > > > > >>> >>> >>> Another thought.. so far, CSUM was serving as a gate for HOST_UFO, >>> HOST_TSO etc... >>> >>> Leaving ECN exposed without TSO is dirty, but the specification states >>> that ECN depends on TSO features, so OVS can "hide" behind this >>> assumption and expect no application sends ECN offloading requests. >>> >>> On the other hand, if OVS leaves UFO exposed and CSUM is now the >>> default, an application may (rightfully?) expect UFO works... >>> And I may have to support a two stage retry for my "recovery patch" >>> (not sure I am explaining clearly enough..). >> >> Yeah, we can't really enable checksum offload if UFO is negotiated. >> The checksum is not negotiated in these old VMs, but IIUC, if the >> guest driver will be re-initialized, it may pick up new features. >> >> Currently possible configurations: >> >> 1. CSUM + TSO + ___ + ___ OVS 3.1 with userspace-tso=true >> 2. ____ + TSO + ECN + ___ OVS 3.1 with userspace-tso=false >> 3. ____ + ___ + ECN + UFO OVS 2.11 >> >> The "___" means not negotiated feature. >> Only in case 1 we can actually receive packets with offloading >> configured, because it's the only case where a base prerequisite >> CSUM is negotiated. >> >> Is that correct or am I missing some case? > > I don't think TSO is exposed without CSUM when tso is disabled in OVS, > neither OVS nor the vhost library seem to reenable this feature.
OK. > > > In practice, I caught (with gdb) calls to vhost_user_get_features so > that I only look at what we expose to the guest (regardless of what > the guest later negotiates): > > ____ + ____ + ECN + UFO : 2.11 > ____ + ____ + ECN + ___ : [2.13 .. 3.1] - tso > CSUM + ____ + ECN + ___ : origin/master - tso > CSUM + TSO* + ___ + ___ : [2.13 .. origin/master] + tso > > So I think we have two issues: > > - when upgrading from 2.11 to 2.13 with running VMs. > Which I confirmed with a RHEL8 guest, looking at the qemu endless logs: > vhost lacks feature mask 16384 for backend > 2023-06-26T08:53:53.542798Z qemu-kvm: failed to init vhost_net for queue 0 > > $ ./features.sh 16384 > VIRTIO_NET_F_HOST_UFO > > I am surprised nobody complained about the issue, I reproduce it with > a default configuration. > Maybe people stop/restart their guests if network is not working.. ? > > And I wonder whether we need a backportable fix for this issue. Yeah, something we can backport down to 2.17 should be useful, I guess. > > > - if the UFO feature is "restored" in the master branch, OVS can't > expose CSUM if the guest negotiated UFO. Yep. IIUC, that will require un-doing/re-wrking some of the changes Mike did in order to restore ability to disable advertising of checksum offload. > > >> >> Configurations that we can allow (we can allow any subset of them): >> >> a. CSUM + TSO + ___ + ___ >> b. CSUM + ___ + ECN + ___ > > Ack. > >> c. ____ + TSO + ECN + UFO > > I suppose you meant above that if CSUM is not negociated, we can allow > any subset of TSO, ECN, UFO. Yes. > > >> >> So, we can only safely upgrade/migrate: >> >> 1 --> a >> 2 --> c >> 3 --> c >> >> "Safe" configurations: >> >> a. CSUM + TSO + ___ + ___ >> c. ____ + TSO + ECN + UFO >> >> Configurations we actually want: >> >> x. CSUM + TSO + ___ + ___ >> y. CSUM + ___ + ___ + ___ >> >> >> Logic can be: >> >> 1. Try 'a/x' if userspace-tso=true >> 2. Try 'y' if userspace-tso=false >> 3. Try 'c' if above failed >> >> As far as I understand we can advertise new features, but we can't remove >> already acked ones. So, the configuration 'c' is a catch-all for broken >> cases. 'y' will only be possible for new/restarted VMs. >> >> Does that make sense? > > I think you covered all cases. > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
