| Message ID | 20231017040932.62997-12-akihiko.odaki@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | virtio-net RSS/hash report fixes and improvements | expand |
On Tue, Oct 17, 2023 at 12:10 PM Akihiko Odaki <akihiko.odaki@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. > > 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.odaki@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)); Guest trigger-rable assertion should be avoided as much as possible. Thanks
On 2023/10/27 16:07, Jason Wang wrote: > On Tue, Oct 17, 2023 at 12:10 PM Akihiko Odaki <akihiko.odaki@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. >> >> 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.odaki@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)); > > Guest trigger-rable assertion should be avoided as much as possible. These assertions should never be triggered by the guest as the feature availability is checked at the device realization. Are these assertions still problematic? Regards, Akihiko Odaki
On Tue, Oct 17, 2023 at 7:10 AM Akihiko Odaki <akihiko.odaki@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.odaki@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 > >
在 2023/10/27 15:54, Akihiko Odaki 写道: > On 2023/10/27 16:07, Jason Wang wrote: >> On Tue, Oct 17, 2023 at 12:10 PM Akihiko Odaki >> <akihiko.odaki@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. >>> >>> 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.odaki@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)); >> >> Guest trigger-rable assertion should be avoided as much as possible. > > These assertions should never be triggered by the guest as the feature > availability is checked at the device realization. Are these > assertions still problematic? I still think it's better to avoid assertions as code could be changed by various people so it may end up with such possibility in the future. Thanks > > Regards, > Akihiko Odaki >
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)
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. 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.odaki@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(-)