| Message ID | 20210204202915.15925-4-yuri.benditovich@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | [1/3] vhost-net: add VIRTIO_NET_F_HASH_REPORT to the list of kernel features | expand |
On Thu, Feb 04, 2021 at 10:29:15PM +0200, Yuri Benditovich wrote: > Currently virtio-net silently clears features if they are > not supported by respective vhost. This may create migration > problems in future if vhost features on the source and destination > are different. Implement graceful fallback to no-vhost mode > when some acked features contradict with vhost. The decision is > taken on set_features call and the vhost will be disabled > till next reset (or migration). > Such fallback is currently enabled only for TAP netdev. > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> Sounds good, but I don't think we should do this if vhostforce=on is set. Also, let's document this behaviour with the vhost option so people are not suprized. > --- > hw/net/virtio-net.c | 58 ++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 50 insertions(+), 8 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 5150f295e8..b353060e63 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -515,6 +515,15 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc) > return info; > } > > +static void virtio_net_allow_vhost(VirtIONet *n, bool allow) > +{ > + int i; > + for (i = 0; i < n->max_queues; i++) { > + NetClientState *nc = qemu_get_subqueue(n->nic, i)->peer; > + nc->vhost_net_disabled = !allow; > + } > +} > + > static void virtio_net_reset(VirtIODevice *vdev) > { > VirtIONet *n = VIRTIO_NET(vdev); > @@ -552,6 +561,7 @@ static void virtio_net_reset(VirtIODevice *vdev) > assert(!virtio_net_get_subqueue(nc)->async_tx.elem); > } > } > + virtio_net_allow_vhost(n, true); > } > > static void peer_test_vnet_hdr(VirtIONet *n) > @@ -689,6 +699,15 @@ static void virtio_net_set_queues(VirtIONet *n) > } > } > > +static bool can_disable_vhost(VirtIONet *n) > +{ > + NetClientState *peer = qemu_get_queue(n->nic)->peer; > + if (!get_vhost_net(peer)) { > + return false; > + } > + return !peer || peer->info->type == NET_CLIENT_DRIVER_TAP; > +} > + > static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue); > > static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, > @@ -725,14 +744,14 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, > return features; > } > > - virtio_clear_feature(&features, VIRTIO_NET_F_RSS); > - virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT); > - features = vhost_net_get_features(get_vhost_net(nc->peer), features); > - vdev->backend_features = features; > + vdev->backend_features = vhost_net_get_features(get_vhost_net(nc->peer), features); > > - if (n->mtu_bypass_backend && > - (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) { > - features |= (1ULL << VIRTIO_NET_F_MTU); > + if (!can_disable_vhost(n)) { > + features = vdev->backend_features; > + if (n->mtu_bypass_backend && > + (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) { > + features |= (1ULL << VIRTIO_NET_F_MTU); > + } > } > > return features; > @@ -872,10 +891,25 @@ static void failover_add_primary(VirtIONet *n, Error **errp) > error_propagate(errp, err); > } > > +static bool check_vhost_features(VirtIONet *n, uint64_t features) > +{ > + NetClientState *nc = qemu_get_queue(n->nic); > + uint64_t filtered; > + if (n->rss_data.redirect) { > + return false; > + } > + filtered = vhost_net_get_features(get_vhost_net(nc->peer), features); > + if (filtered != features) { > + return false; > + } > + return true; > +} > + > static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) > { > VirtIONet *n = VIRTIO_NET(vdev); > Error *err = NULL; > + bool disable_vhost = false; > int i; > > if (n->mtu_bypass_backend && > @@ -894,13 +928,21 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) > VIRTIO_F_VERSION_1), > virtio_has_feature(features, > VIRTIO_NET_F_HASH_REPORT)); > - > n->rsc4_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) && > virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO4); > n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) && > virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6); > n->rss_data.redirect = virtio_has_feature(features, VIRTIO_NET_F_RSS); > > + if (can_disable_vhost(n)) { > + disable_vhost = !check_vhost_features(n, features); > + } > + if (disable_vhost) { > + warn_report("Some of requested features aren't supported by vhost, " > + "vhost is turned off till next reset"); > + virtio_net_allow_vhost(n, false); > + } > + > if (n->has_vnet_hdr) { > n->curr_guest_offloads = > virtio_net_guest_offloads_by_features(features); > -- > 2.17.1
On Fri, Feb 05, 2021 at 08:38:49AM -0500, Michael S. Tsirkin wrote: > On Thu, Feb 04, 2021 at 10:29:15PM +0200, Yuri Benditovich wrote: > > Currently virtio-net silently clears features if they are > > not supported by respective vhost. This may create migration > > problems in future if vhost features on the source and destination > > are different. Implement graceful fallback to no-vhost mode > > when some acked features contradict with vhost. The decision is > > taken on set_features call and the vhost will be disabled > > till next reset (or migration). > > Such fallback is currently enabled only for TAP netdev. > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> > > > Sounds good, but I don't think we should do this if > vhostforce=on is set. > > Also, let's document this behaviour with the vhost option so people > are not suprized. Here's another thing that bothers me. At the moment we easily add new features, enabled by default, as long as kernels are consistent on source and destination everything works fine. With this patch first time we add a new feature that kernel does not support, vhost gets disabled. Given lots of people update their kernels less frequently than userspace, lots of users will start running with vhost off all of a sudden. Don't have good suggestions yet.
On 2021/2/5 下午9:38, Michael S. Tsirkin wrote: > On Thu, Feb 04, 2021 at 10:29:15PM +0200, Yuri Benditovich wrote: >> Currently virtio-net silently clears features if they are >> not supported by respective vhost. This may create migration >> problems in future if vhost features on the source and destination >> are different. Implement graceful fallback to no-vhost mode >> when some acked features contradict with vhost. The decision is >> taken on set_features call and the vhost will be disabled >> till next reset (or migration). >> Such fallback is currently enabled only for TAP netdev. >> >> Signed-off-by: Yuri Benditovich<yuri.benditovich@daynix.com> > Sounds good, but I don't think we should do this if > vhostforce=on is set. If we do this, does it mean we won't maintain migration compatibility when vhostforce is on? Thanks > > Also, let's document this behaviour with the vhost option so people > are not suprized. >
On 2021/2/5 上午4:29, Yuri Benditovich wrote: > Currently virtio-net silently clears features if they are > not supported by respective vhost. This may create migration > problems in future if vhost features on the source and destination > are different. Implement graceful fallback to no-vhost mode > when some acked features contradict with vhost. The decision is > taken on set_features call and the vhost will be disabled > till next reset (or migration). > Such fallback is currently enabled only for TAP netdev. > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> > --- > hw/net/virtio-net.c | 58 ++++++++++++++++++++++++++++++++++++++------- > 1 file changed, 50 insertions(+), 8 deletions(-) > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 5150f295e8..b353060e63 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -515,6 +515,15 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc) > return info; > } > > +static void virtio_net_allow_vhost(VirtIONet *n, bool allow) > +{ > + int i; > + for (i = 0; i < n->max_queues; i++) { > + NetClientState *nc = qemu_get_subqueue(n->nic, i)->peer; > + nc->vhost_net_disabled = !allow; > + } > +} > + > static void virtio_net_reset(VirtIODevice *vdev) > { > VirtIONet *n = VIRTIO_NET(vdev); > @@ -552,6 +561,7 @@ static void virtio_net_reset(VirtIODevice *vdev) > assert(!virtio_net_get_subqueue(nc)->async_tx.elem); > } > } > + virtio_net_allow_vhost(n, true); > } > > static void peer_test_vnet_hdr(VirtIONet *n) > @@ -689,6 +699,15 @@ static void virtio_net_set_queues(VirtIONet *n) > } > } > > +static bool can_disable_vhost(VirtIONet *n) > +{ > + NetClientState *peer = qemu_get_queue(n->nic)->peer; > + if (!get_vhost_net(peer)) { > + return false; > + } > + return !peer || peer->info->type == NET_CLIENT_DRIVER_TAP; > +} > + > static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue); > > static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, > @@ -725,14 +744,14 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, > return features; > } > > - virtio_clear_feature(&features, VIRTIO_NET_F_RSS); > - virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT); > - features = vhost_net_get_features(get_vhost_net(nc->peer), features); > - vdev->backend_features = features; > + vdev->backend_features = vhost_net_get_features(get_vhost_net(nc->peer), features); > > - if (n->mtu_bypass_backend && > - (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) { > - features |= (1ULL << VIRTIO_NET_F_MTU); > + if (!can_disable_vhost(n)) { > + features = vdev->backend_features; > + if (n->mtu_bypass_backend && > + (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) { > + features |= (1ULL << VIRTIO_NET_F_MTU); > + } > } > > return features; > @@ -872,10 +891,25 @@ static void failover_add_primary(VirtIONet *n, Error **errp) > error_propagate(errp, err); > } > > +static bool check_vhost_features(VirtIONet *n, uint64_t features) > +{ > + NetClientState *nc = qemu_get_queue(n->nic); > + uint64_t filtered; > + if (n->rss_data.redirect) { > + return false; > + } > + filtered = vhost_net_get_features(get_vhost_net(nc->peer), features); > + if (filtered != features) { > + return false; > + } > + return true; > +} > + > static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) > { > VirtIONet *n = VIRTIO_NET(vdev); > Error *err = NULL; > + bool disable_vhost = false; > int i; > > if (n->mtu_bypass_backend && > @@ -894,13 +928,21 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) > VIRTIO_F_VERSION_1), > virtio_has_feature(features, > VIRTIO_NET_F_HASH_REPORT)); > - > n->rsc4_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) && > virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO4); > n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) && > virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6); > n->rss_data.redirect = virtio_has_feature(features, VIRTIO_NET_F_RSS); > > + if (can_disable_vhost(n)) { > + disable_vhost = !check_vhost_features(n, features); > + } > + if (disable_vhost) { > + warn_report("Some of requested features aren't supported by vhost, " > + "vhost is turned off till next reset"); > + virtio_net_allow_vhost(n, false); > + } This looks more complicated than I expected. See virtio_net_vhost_status() we had a fallback there: r = vhost_net_start(vdev, n->nic->ncs, queues); if (r < 0) { error_report("unable to start vhost net: %d: " "falling back on userspace virtio", -r); n->vhost_started = 0; } I wonder if we can simply depends on this (which is proved to be work for the past years) by not clearing dev->acked_features like: if (!can_disable_vhost(n)) { features = vhost_net_get_features(get_vhost_net(nc->peer), features); } else { vdev->backend_features = features; } Then we probably don't need other tricks. Thanks > + > if (n->has_vnet_hdr) { > n->curr_guest_offloads = > virtio_net_guest_offloads_by_features(features);
On Mon, Feb 8, 2021 at 5:15 AM Jason Wang <jasowang@redhat.com> wrote: > > > On 2021/2/5 下午9:38, Michael S. Tsirkin wrote: > > On Thu, Feb 04, 2021 at 10:29:15PM +0200, Yuri Benditovich wrote: > >> Currently virtio-net silently clears features if they are > >> not supported by respective vhost. This may create migration > >> problems in future if vhost features on the source and destination > >> are different. Implement graceful fallback to no-vhost mode > >> when some acked features contradict with vhost. The decision is > >> taken on set_features call and the vhost will be disabled > >> till next reset (or migration). > >> Such fallback is currently enabled only for TAP netdev. > >> > >> Signed-off-by: Yuri Benditovich<yuri.benditovich@daynix.com> > > Sounds good, but I don't think we should do this if > > vhostforce=on is set. > > > If we do this, does it mean we won't maintain migration compatibility > when vhostforce is on? AFAIU, the 'vhostforce=on' should mean the vhost can't be disabled (if I'm not mistaken this is typically used for vhost-user). So we can view this case as similar to vhost-vdpa and vhost-user. > > Thanks > > > > > > Also, let's document this behaviour with the vhost option so people > > are not suprized. > > >
On Mon, Feb 8, 2021 at 6:11 AM Jason Wang <jasowang@redhat.com> wrote: > > > On 2021/2/5 上午4:29, Yuri Benditovich wrote: > > Currently virtio-net silently clears features if they are > > not supported by respective vhost. This may create migration > > problems in future if vhost features on the source and destination > > are different. Implement graceful fallback to no-vhost mode > > when some acked features contradict with vhost. The decision is > > taken on set_features call and the vhost will be disabled > > till next reset (or migration). > > Such fallback is currently enabled only for TAP netdev. > > > > Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> > > --- > > hw/net/virtio-net.c | 58 ++++++++++++++++++++++++++++++++++++++------- > > 1 file changed, 50 insertions(+), 8 deletions(-) > > > > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > > index 5150f295e8..b353060e63 100644 > > --- a/hw/net/virtio-net.c > > +++ b/hw/net/virtio-net.c > > @@ -515,6 +515,15 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc) > > return info; > > } > > > > +static void virtio_net_allow_vhost(VirtIONet *n, bool allow) > > +{ > > + int i; > > + for (i = 0; i < n->max_queues; i++) { > > + NetClientState *nc = qemu_get_subqueue(n->nic, i)->peer; > > + nc->vhost_net_disabled = !allow; > > + } > > +} > > + > > static void virtio_net_reset(VirtIODevice *vdev) > > { > > VirtIONet *n = VIRTIO_NET(vdev); > > @@ -552,6 +561,7 @@ static void virtio_net_reset(VirtIODevice *vdev) > > assert(!virtio_net_get_subqueue(nc)->async_tx.elem); > > } > > } > > + virtio_net_allow_vhost(n, true); > > } > > > > static void peer_test_vnet_hdr(VirtIONet *n) > > @@ -689,6 +699,15 @@ static void virtio_net_set_queues(VirtIONet *n) > > } > > } > > > > +static bool can_disable_vhost(VirtIONet *n) > > +{ > > + NetClientState *peer = qemu_get_queue(n->nic)->peer; > > + if (!get_vhost_net(peer)) { > > + return false; > > + } > > + return !peer || peer->info->type == NET_CLIENT_DRIVER_TAP; > > +} > > + > > static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue); > > > > static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, > > @@ -725,14 +744,14 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, > > return features; > > } > > > > - virtio_clear_feature(&features, VIRTIO_NET_F_RSS); > > - virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT); > > - features = vhost_net_get_features(get_vhost_net(nc->peer), features); > > - vdev->backend_features = features; > > + vdev->backend_features = vhost_net_get_features(get_vhost_net(nc->peer), features); > > > > - if (n->mtu_bypass_backend && > > - (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) { > > - features |= (1ULL << VIRTIO_NET_F_MTU); > > + if (!can_disable_vhost(n)) { > > + features = vdev->backend_features; > > + if (n->mtu_bypass_backend && > > + (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) { > > + features |= (1ULL << VIRTIO_NET_F_MTU); > > + } > > } > > > > return features; > > @@ -872,10 +891,25 @@ static void failover_add_primary(VirtIONet *n, Error **errp) > > error_propagate(errp, err); > > } > > > > +static bool check_vhost_features(VirtIONet *n, uint64_t features) > > +{ > > + NetClientState *nc = qemu_get_queue(n->nic); > > + uint64_t filtered; > > + if (n->rss_data.redirect) { > > + return false; > > + } > > + filtered = vhost_net_get_features(get_vhost_net(nc->peer), features); > > + if (filtered != features) { > > + return false; > > + } > > + return true; > > +} > > + > > static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) > > { > > VirtIONet *n = VIRTIO_NET(vdev); > > Error *err = NULL; > > + bool disable_vhost = false; > > int i; > > > > if (n->mtu_bypass_backend && > > @@ -894,13 +928,21 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) > > VIRTIO_F_VERSION_1), > > virtio_has_feature(features, > > VIRTIO_NET_F_HASH_REPORT)); > > - > > n->rsc4_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) && > > virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO4); > > n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) && > > virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6); > > n->rss_data.redirect = virtio_has_feature(features, VIRTIO_NET_F_RSS); > > > > + if (can_disable_vhost(n)) { > > + disable_vhost = !check_vhost_features(n, features); > > + } > > + if (disable_vhost) { > > + warn_report("Some of requested features aren't supported by vhost, " > > + "vhost is turned off till next reset"); > > + virtio_net_allow_vhost(n, false); > > + } > > > This looks more complicated than I expected. See > virtio_net_vhost_status() we had a fallback there: > > r = vhost_net_start(vdev, n->nic->ncs, queues); > if (r < 0) { > error_report("unable to start vhost net: %d: " > "falling back on userspace virtio", -r); > n->vhost_started = 0; > } > > I wonder if we can simply depends on this (which is proved to be work > for the past years) by not clearing dev->acked_features like: > > if (!can_disable_vhost(n)) { > features = vhost_net_get_features(get_vhost_net(nc->peer), features); > } else { > vdev->backend_features = features; > } > > Then we probably don't need other tricks. Of course we can. But I would prefer to make things more clear, i.e. make get_vhost_net() always return a meaningful result, even (as an example) in procedures called from set_feature() after the vhost_disabled is set. Otherwise people can rely on get_vhost_net() and call its methods which potentially can do something that we do not expect. In some places in the code (also in future) we'll need to check not only get_vhost_net() but also is_vhost_disabled. IMO, not too elegant, but possible. Of course, being a decision maker, you can request to do it simpler and do not maintain a consistent result of get_vhost_net(). But then please tell it explicitly. > > Thanks > > > > + > > if (n->has_vnet_hdr) { > > n->curr_guest_offloads = > > virtio_net_guest_offloads_by_features(features); >
On 2021/2/9 上午3:46, Yuri Benditovich wrote: > On Mon, Feb 8, 2021 at 5:15 AM Jason Wang <jasowang@redhat.com> wrote: >> >> On 2021/2/5 下午9:38, Michael S. Tsirkin wrote: >>> On Thu, Feb 04, 2021 at 10:29:15PM +0200, Yuri Benditovich wrote: >>>> Currently virtio-net silently clears features if they are >>>> not supported by respective vhost. This may create migration >>>> problems in future if vhost features on the source and destination >>>> are different. Implement graceful fallback to no-vhost mode >>>> when some acked features contradict with vhost. The decision is >>>> taken on set_features call and the vhost will be disabled >>>> till next reset (or migration). >>>> Such fallback is currently enabled only for TAP netdev. >>>> >>>> Signed-off-by: Yuri Benditovich<yuri.benditovich@daynix.com> >>> Sounds good, but I don't think we should do this if >>> vhostforce=on is set. >> >> If we do this, does it mean we won't maintain migration compatibility >> when vhostforce is on? > AFAIU, the 'vhostforce=on' should mean the vhost can't be disabled (if > I'm not mistaken this is typically used for vhost-user). > So we can view this case as similar to vhost-vdpa and vhost-user. Right, but since it was used by libivrt. Then it turns out to be a compatibility breaker. Thanks > >> Thanks >> >> >>> Also, let's document this behaviour with the vhost option so people >>> are not suprized. >>>
On 2021/2/9 上午3:59, Yuri Benditovich wrote: > On Mon, Feb 8, 2021 at 6:11 AM Jason Wang <jasowang@redhat.com> wrote: >> >> On 2021/2/5 上午4:29, Yuri Benditovich wrote: >>> Currently virtio-net silently clears features if they are >>> not supported by respective vhost. This may create migration >>> problems in future if vhost features on the source and destination >>> are different. Implement graceful fallback to no-vhost mode >>> when some acked features contradict with vhost. The decision is >>> taken on set_features call and the vhost will be disabled >>> till next reset (or migration). >>> Such fallback is currently enabled only for TAP netdev. >>> >>> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> >>> --- >>> hw/net/virtio-net.c | 58 ++++++++++++++++++++++++++++++++++++++------- >>> 1 file changed, 50 insertions(+), 8 deletions(-) >>> >>> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c >>> index 5150f295e8..b353060e63 100644 >>> --- a/hw/net/virtio-net.c >>> +++ b/hw/net/virtio-net.c >>> @@ -515,6 +515,15 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc) >>> return info; >>> } >>> >>> +static void virtio_net_allow_vhost(VirtIONet *n, bool allow) >>> +{ >>> + int i; >>> + for (i = 0; i < n->max_queues; i++) { >>> + NetClientState *nc = qemu_get_subqueue(n->nic, i)->peer; >>> + nc->vhost_net_disabled = !allow; >>> + } >>> +} >>> + >>> static void virtio_net_reset(VirtIODevice *vdev) >>> { >>> VirtIONet *n = VIRTIO_NET(vdev); >>> @@ -552,6 +561,7 @@ static void virtio_net_reset(VirtIODevice *vdev) >>> assert(!virtio_net_get_subqueue(nc)->async_tx.elem); >>> } >>> } >>> + virtio_net_allow_vhost(n, true); >>> } >>> >>> static void peer_test_vnet_hdr(VirtIONet *n) >>> @@ -689,6 +699,15 @@ static void virtio_net_set_queues(VirtIONet *n) >>> } >>> } >>> >>> +static bool can_disable_vhost(VirtIONet *n) >>> +{ >>> + NetClientState *peer = qemu_get_queue(n->nic)->peer; >>> + if (!get_vhost_net(peer)) { >>> + return false; >>> + } >>> + return !peer || peer->info->type == NET_CLIENT_DRIVER_TAP; >>> +} >>> + >>> static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue); >>> >>> static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, >>> @@ -725,14 +744,14 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, >>> return features; >>> } >>> >>> - virtio_clear_feature(&features, VIRTIO_NET_F_RSS); >>> - virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT); >>> - features = vhost_net_get_features(get_vhost_net(nc->peer), features); >>> - vdev->backend_features = features; >>> + vdev->backend_features = vhost_net_get_features(get_vhost_net(nc->peer), features); >>> >>> - if (n->mtu_bypass_backend && >>> - (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) { >>> - features |= (1ULL << VIRTIO_NET_F_MTU); >>> + if (!can_disable_vhost(n)) { >>> + features = vdev->backend_features; >>> + if (n->mtu_bypass_backend && >>> + (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) { >>> + features |= (1ULL << VIRTIO_NET_F_MTU); >>> + } >>> } >>> >>> return features; >>> @@ -872,10 +891,25 @@ static void failover_add_primary(VirtIONet *n, Error **errp) >>> error_propagate(errp, err); >>> } >>> >>> +static bool check_vhost_features(VirtIONet *n, uint64_t features) >>> +{ >>> + NetClientState *nc = qemu_get_queue(n->nic); >>> + uint64_t filtered; >>> + if (n->rss_data.redirect) { >>> + return false; >>> + } >>> + filtered = vhost_net_get_features(get_vhost_net(nc->peer), features); >>> + if (filtered != features) { >>> + return false; >>> + } >>> + return true; >>> +} >>> + >>> static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) >>> { >>> VirtIONet *n = VIRTIO_NET(vdev); >>> Error *err = NULL; >>> + bool disable_vhost = false; >>> int i; >>> >>> if (n->mtu_bypass_backend && >>> @@ -894,13 +928,21 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) >>> VIRTIO_F_VERSION_1), >>> virtio_has_feature(features, >>> VIRTIO_NET_F_HASH_REPORT)); >>> - >>> n->rsc4_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) && >>> virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO4); >>> n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) && >>> virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6); >>> n->rss_data.redirect = virtio_has_feature(features, VIRTIO_NET_F_RSS); >>> >>> + if (can_disable_vhost(n)) { >>> + disable_vhost = !check_vhost_features(n, features); >>> + } >>> + if (disable_vhost) { >>> + warn_report("Some of requested features aren't supported by vhost, " >>> + "vhost is turned off till next reset"); >>> + virtio_net_allow_vhost(n, false); >>> + } >> >> This looks more complicated than I expected. See >> virtio_net_vhost_status() we had a fallback there: >> >> r = vhost_net_start(vdev, n->nic->ncs, queues); >> if (r < 0) { >> error_report("unable to start vhost net: %d: " >> "falling back on userspace virtio", -r); >> n->vhost_started = 0; >> } >> >> I wonder if we can simply depends on this (which is proved to be work >> for the past years) by not clearing dev->acked_features like: >> >> if (!can_disable_vhost(n)) { >> features = vhost_net_get_features(get_vhost_net(nc->peer), features); >> } else { >> vdev->backend_features = features; >> } >> >> Then we probably don't need other tricks. > Of course we can. > But I would prefer to make things more clear, i.e. make > get_vhost_net() always return a meaningful result, even (as an > example) in procedures called from set_feature() after the > vhost_disabled is set. > Otherwise people can rely on get_vhost_net() and call its methods > which potentially can do something that we do not expect. > In some places in the code (also in future) we'll need to check not > only get_vhost_net() but also is_vhost_disabled. IMO, not too elegant, > but possible. I see. > Of course, being a decision maker, you can request to do it simpler > and do not maintain a consistent result of get_vhost_net(). > But then please tell it explicitly. So the reason that I prefer the above simple checks is that we've already had two falling backs points in virtio_net_vhost_status(): 1) for vnet_hdr_swap: if (n->needs_vnet_hdr_swap) { error_report("backend does not support %s vnet headers; " "falling back on userspace virtio", virtio_is_big_endian(vdev) ? "BE" : "LE"); return; } 2) for all of the other cases: n->vhost_started = 1; r = vhost_net_start(vdev, n->nic->ncs, queues); if (r < 0) { error_report("unable to start vhost net: %d: " "falling back on userspace virtio", -r); n->vhost_started = 0; } So it's better to have a consistent way: 1) Introduce the variable disable_vhost as you suggest or 2) falling back by checking the return status of vhost_net_start() In this case, 2) looks much more easier than convert all the falling backs to 1). Thanks > >> Thanks >> >> >>> + >>> if (n->has_vnet_hdr) { >>> n->curr_guest_offloads = >>> virtio_net_guest_offloads_by_features(features);
diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c index 5150f295e8..b353060e63 100644 --- a/hw/net/virtio-net.c +++ b/hw/net/virtio-net.c @@ -515,6 +515,15 @@ static RxFilterInfo *virtio_net_query_rxfilter(NetClientState *nc) return info; } +static void virtio_net_allow_vhost(VirtIONet *n, bool allow) +{ + int i; + for (i = 0; i < n->max_queues; i++) { + NetClientState *nc = qemu_get_subqueue(n->nic, i)->peer; + nc->vhost_net_disabled = !allow; + } +} + static void virtio_net_reset(VirtIODevice *vdev) { VirtIONet *n = VIRTIO_NET(vdev); @@ -552,6 +561,7 @@ static void virtio_net_reset(VirtIODevice *vdev) assert(!virtio_net_get_subqueue(nc)->async_tx.elem); } } + virtio_net_allow_vhost(n, true); } static void peer_test_vnet_hdr(VirtIONet *n) @@ -689,6 +699,15 @@ static void virtio_net_set_queues(VirtIONet *n) } } +static bool can_disable_vhost(VirtIONet *n) +{ + NetClientState *peer = qemu_get_queue(n->nic)->peer; + if (!get_vhost_net(peer)) { + return false; + } + return !peer || peer->info->type == NET_CLIENT_DRIVER_TAP; +} + static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue); static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, @@ -725,14 +744,14 @@ static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features, return features; } - virtio_clear_feature(&features, VIRTIO_NET_F_RSS); - virtio_clear_feature(&features, VIRTIO_NET_F_HASH_REPORT); - features = vhost_net_get_features(get_vhost_net(nc->peer), features); - vdev->backend_features = features; + vdev->backend_features = vhost_net_get_features(get_vhost_net(nc->peer), features); - if (n->mtu_bypass_backend && - (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) { - features |= (1ULL << VIRTIO_NET_F_MTU); + if (!can_disable_vhost(n)) { + features = vdev->backend_features; + if (n->mtu_bypass_backend && + (n->host_features & 1ULL << VIRTIO_NET_F_MTU)) { + features |= (1ULL << VIRTIO_NET_F_MTU); + } } return features; @@ -872,10 +891,25 @@ static void failover_add_primary(VirtIONet *n, Error **errp) error_propagate(errp, err); } +static bool check_vhost_features(VirtIONet *n, uint64_t features) +{ + NetClientState *nc = qemu_get_queue(n->nic); + uint64_t filtered; + if (n->rss_data.redirect) { + return false; + } + filtered = vhost_net_get_features(get_vhost_net(nc->peer), features); + if (filtered != features) { + return false; + } + return true; +} + static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) { VirtIONet *n = VIRTIO_NET(vdev); Error *err = NULL; + bool disable_vhost = false; int i; if (n->mtu_bypass_backend && @@ -894,13 +928,21 @@ static void virtio_net_set_features(VirtIODevice *vdev, uint64_t features) VIRTIO_F_VERSION_1), virtio_has_feature(features, VIRTIO_NET_F_HASH_REPORT)); - n->rsc4_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) && virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO4); n->rsc6_enabled = virtio_has_feature(features, VIRTIO_NET_F_RSC_EXT) && virtio_has_feature(features, VIRTIO_NET_F_GUEST_TSO6); n->rss_data.redirect = virtio_has_feature(features, VIRTIO_NET_F_RSS); + if (can_disable_vhost(n)) { + disable_vhost = !check_vhost_features(n, features); + } + if (disable_vhost) { + warn_report("Some of requested features aren't supported by vhost, " + "vhost is turned off till next reset"); + virtio_net_allow_vhost(n, false); + } + if (n->has_vnet_hdr) { n->curr_guest_offloads = virtio_net_guest_offloads_by_features(features);
Currently virtio-net silently clears features if they are not supported by respective vhost. This may create migration problems in future if vhost features on the source and destination are different. Implement graceful fallback to no-vhost mode when some acked features contradict with vhost. The decision is taken on set_features call and the vhost will be disabled till next reset (or migration). Such fallback is currently enabled only for TAP netdev. Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com> --- hw/net/virtio-net.c | 58 ++++++++++++++++++++++++++++++++++++++------- 1 file changed, 50 insertions(+), 8 deletions(-)