On Tue, Oct 17, 2023 at 7:10 AM Akihiko Odaki <akihiko.od...@daynix.com> wrote:
> vhost requires eBPF for RSS. Even when eBPF is not available, virtio-net > reported RSS availability, and raised a warning only after the > guest requested RSS, and the guest could not know that RSS is not > available. > > The existing code suggests the RSS feature for vhost case only when the ebpf is loaded. https://github.com/qemu/qemu/blob/master/hw/net/virtio-net.c#L828 Am I wrong? > Check RSS availability during device realization and return an error > if RSS is requested but not available. Assert RSS availability when > the guest actually requests the feature. > > Signed-off-by: Akihiko Odaki <akihiko.od...@daynix.com> > --- > ebpf/ebpf_rss.h | 2 +- > ebpf/ebpf_rss-stub.c | 4 +- > ebpf/ebpf_rss.c | 68 +++++++++----------------- > hw/net/virtio-net.c | 114 +++++++++++++++++++++---------------------- > 4 files changed, 82 insertions(+), 106 deletions(-) > > diff --git a/ebpf/ebpf_rss.h b/ebpf/ebpf_rss.h > index bf3f2572c7..1128173572 100644 > --- a/ebpf/ebpf_rss.h > +++ b/ebpf/ebpf_rss.h > @@ -36,7 +36,7 @@ bool ebpf_rss_is_loaded(struct EBPFRSSContext *ctx); > > bool ebpf_rss_load(struct EBPFRSSContext *ctx); > > -bool ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig > *config, > +void ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig > *config, > uint16_t *indirections_table, uint8_t > *toeplitz_key); > > void ebpf_rss_unload(struct EBPFRSSContext *ctx); > diff --git a/ebpf/ebpf_rss-stub.c b/ebpf/ebpf_rss-stub.c > index e71e229190..525b358597 100644 > --- a/ebpf/ebpf_rss-stub.c > +++ b/ebpf/ebpf_rss-stub.c > @@ -28,10 +28,10 @@ bool ebpf_rss_load(struct EBPFRSSContext *ctx) > return false; > } > > -bool ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig > *config, > +void ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig > *config, > uint16_t *indirections_table, uint8_t *toeplitz_key) > { > - return false; > + g_assert_not_reached(); > } > > void ebpf_rss_unload(struct EBPFRSSContext *ctx) > diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c > index cee658c158..6cdf82d059 100644 > --- a/ebpf/ebpf_rss.c > +++ b/ebpf/ebpf_rss.c > @@ -74,42 +74,32 @@ error: > return false; > } > > -static bool ebpf_rss_set_config(struct EBPFRSSContext *ctx, > +static void ebpf_rss_set_config(struct EBPFRSSContext *ctx, > struct EBPFRSSConfig *config) > { > uint32_t map_key = 0; > > - if (!ebpf_rss_is_loaded(ctx)) { > - return false; > - } > - if (bpf_map_update_elem(ctx->map_configuration, > - &map_key, config, 0) < 0) { > - return false; > - } > - return true; > + assert(ebpf_rss_is_loaded(ctx)); > + assert(!bpf_map_update_elem(ctx->map_configuration, &map_key, config, > 0)); > } > > -static bool ebpf_rss_set_indirections_table(struct EBPFRSSContext *ctx, > +static void ebpf_rss_set_indirections_table(struct EBPFRSSContext *ctx, > uint16_t *indirections_table, > size_t len) > { > uint32_t i = 0; > > - if (!ebpf_rss_is_loaded(ctx) || indirections_table == NULL || > - len > VIRTIO_NET_RSS_MAX_TABLE_LEN) { > - return false; > - } > + assert(ebpf_rss_is_loaded(ctx)); > + assert(indirections_table); > + assert(len <= VIRTIO_NET_RSS_MAX_TABLE_LEN); > > for (; i < len; ++i) { > - if (bpf_map_update_elem(ctx->map_indirections_table, &i, > - indirections_table + i, 0) < 0) { > - return false; > - } > + assert(!bpf_map_update_elem(ctx->map_indirections_table, &i, > + indirections_table + i, 0)); > } > - return true; > } > > -static bool ebpf_rss_set_toepliz_key(struct EBPFRSSContext *ctx, > +static void ebpf_rss_set_toepliz_key(struct EBPFRSSContext *ctx, > uint8_t *toeplitz_key) > { > uint32_t map_key = 0; > @@ -117,41 +107,29 @@ static bool ebpf_rss_set_toepliz_key(struct > EBPFRSSContext *ctx, > /* prepare toeplitz key */ > uint8_t toe[VIRTIO_NET_RSS_MAX_KEY_SIZE] = {}; > > - if (!ebpf_rss_is_loaded(ctx) || toeplitz_key == NULL) { > - return false; > - } > + assert(ebpf_rss_is_loaded(ctx)); > + assert(toeplitz_key); > + > memcpy(toe, toeplitz_key, VIRTIO_NET_RSS_MAX_KEY_SIZE); > *(uint32_t *)toe = ntohl(*(uint32_t *)toe); > > - if (bpf_map_update_elem(ctx->map_toeplitz_key, &map_key, toe, > - 0) < 0) { > - return false; > - } > - return true; > + assert(!bpf_map_update_elem(ctx->map_toeplitz_key, &map_key, toe, 0)); > } > > -bool ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig > *config, > +void ebpf_rss_set_all(struct EBPFRSSContext *ctx, struct EBPFRSSConfig > *config, > uint16_t *indirections_table, uint8_t *toeplitz_key) > { > - if (!ebpf_rss_is_loaded(ctx) || config == NULL || > - indirections_table == NULL || toeplitz_key == NULL) { > - return false; > - } > - > - if (!ebpf_rss_set_config(ctx, config)) { > - return false; > - } > + assert(ebpf_rss_is_loaded(ctx)); > + assert(config); > + assert(indirections_table); > + assert(toeplitz_key); > > - if (!ebpf_rss_set_indirections_table(ctx, indirections_table, > - config->indirections_len)) { > - return false; > - } > + ebpf_rss_set_config(ctx, config); > > - if (!ebpf_rss_set_toepliz_key(ctx, toeplitz_key)) { > - return false; > - } > + ebpf_rss_set_indirections_table(ctx, indirections_table, > + config->indirections_len); > > - return true; > + ebpf_rss_set_toepliz_key(ctx, toeplitz_key); > } > > void ebpf_rss_unload(struct EBPFRSSContext *ctx) > diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c > index 25fc06bd93..20feb20bb1 100644 > --- a/hw/net/virtio-net.c > +++ b/hw/net/virtio-net.c > @@ -1242,14 +1242,10 @@ static bool virtio_net_attach_epbf_rss(VirtIONet > *n) > > rss_data_to_rss_config(&n->rss_data, &config); > > - if (!ebpf_rss_set_all(&n->ebpf_rss, &config, > - n->rss_data.indirections_table, > n->rss_data.key)) { > - return false; > - } > + ebpf_rss_set_all(&n->ebpf_rss, &config, > + n->rss_data.indirections_table, n->rss_data.key); > > - if (!virtio_net_attach_ebpf_to_backend(n->nic, > n->ebpf_rss.program_fd)) { > - return false; > - } > + assert(virtio_net_attach_ebpf_to_backend(n->nic, > n->ebpf_rss.program_fd)); > > return true; > } > @@ -1266,12 +1262,7 @@ static void virtio_net_commit_rss_config(VirtIONet > *n) > if (n->rss_data.populate_hash) { > virtio_net_detach_epbf_rss(n); > } else if (!virtio_net_attach_epbf_rss(n)) { > - if (get_vhost_net(qemu_get_queue(n->nic)->peer)) { > - warn_report("Can't use eBPF RSS for vhost"); > - } else { > - warn_report("Can't use eBPF RSS - fallback to software > RSS"); > - n->rss_data.enabled_software_rss = true; > - } > + n->rss_data.enabled_software_rss = true; > } > > trace_virtio_net_rss_enable(n->rss_data.hash_types, > @@ -3514,6 +3505,50 @@ static bool > failover_hide_primary_device(DeviceListener *listener, > return qatomic_read(&n->failover_primary_hidden); > } > > +static void virtio_net_device_unrealize(DeviceState *dev) > +{ > + VirtIODevice *vdev = VIRTIO_DEVICE(dev); > + VirtIONet *n = VIRTIO_NET(dev); > + int i, max_queue_pairs; > + > + if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS)) { > + virtio_net_unload_ebpf(n); > + } > + > + /* This will stop vhost backend if appropriate. */ > + virtio_net_set_status(vdev, 0); > + > + g_free(n->netclient_name); > + n->netclient_name = NULL; > + g_free(n->netclient_type); > + n->netclient_type = NULL; > + > + g_free(n->mac_table.macs); > + g_free(n->vlans); > + > + if (n->failover) { > + qobject_unref(n->primary_opts); > + device_listener_unregister(&n->primary_listener); > + remove_migration_state_change_notifier(&n->migration_state); > + } else { > + assert(n->primary_opts == NULL); > + } > + > + max_queue_pairs = n->multiqueue ? n->max_queue_pairs : 1; > + for (i = 0; i < max_queue_pairs; i++) { > + virtio_net_del_queue(n, i); > + } > + /* delete also control vq */ > + virtio_del_queue(vdev, max_queue_pairs * 2); > + qemu_announce_timer_del(&n->announce_timer, false); > + g_free(n->vqs); > + qemu_del_nic(n->nic); > + virtio_net_rsc_cleanup(n); > + g_free(n->rss_data.indirections_table); > + net_rx_pkt_uninit(n->rx_pkt); > + virtio_cleanup(vdev); > +} > + > static void virtio_net_device_realize(DeviceState *dev, Error **errp) > { > VirtIODevice *vdev = VIRTIO_DEVICE(dev); > @@ -3685,53 +3720,16 @@ static void virtio_net_device_realize(DeviceState > *dev, Error **errp) > > net_rx_pkt_init(&n->rx_pkt); > > - if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS)) { > - virtio_net_load_ebpf(n); > - } > -} > - > -static void virtio_net_device_unrealize(DeviceState *dev) > -{ > - VirtIODevice *vdev = VIRTIO_DEVICE(dev); > - VirtIONet *n = VIRTIO_NET(dev); > - int i, max_queue_pairs; > - > - if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS)) { > - virtio_net_unload_ebpf(n); > - } > - > - /* This will stop vhost backend if appropriate. */ > - virtio_net_set_status(vdev, 0); > - > - g_free(n->netclient_name); > - n->netclient_name = NULL; > - g_free(n->netclient_type); > - n->netclient_type = NULL; > - > - g_free(n->mac_table.macs); > - g_free(n->vlans); > - > - if (n->failover) { > - qobject_unref(n->primary_opts); > - device_listener_unregister(&n->primary_listener); > - remove_migration_state_change_notifier(&n->migration_state); > - } else { > - assert(n->primary_opts == NULL); > - } > + if (virtio_has_feature(n->host_features, VIRTIO_NET_F_RSS) && > + !virtio_net_load_ebpf(n)) { > + if (get_vhost_net(nc->peer)) { > + error_setg(errp, "Can't load eBPF RSS for vhost"); > + virtio_net_device_unrealize(dev); > + return; > + } > > - max_queue_pairs = n->multiqueue ? n->max_queue_pairs : 1; > - for (i = 0; i < max_queue_pairs; i++) { > - virtio_net_del_queue(n, i); > + warn_report_once("Can't load eBPF RSS - fallback to software > RSS"); > } > - /* delete also control vq */ > - virtio_del_queue(vdev, max_queue_pairs * 2); > - qemu_announce_timer_del(&n->announce_timer, false); > - g_free(n->vqs); > - qemu_del_nic(n->nic); > - virtio_net_rsc_cleanup(n); > - g_free(n->rss_data.indirections_table); > - net_rx_pkt_uninit(n->rx_pkt); > - virtio_cleanup(vdev); > } > > static void virtio_net_reset(VirtIODevice *vdev) > -- > 2.42.0 > >
