From: Yuri Benditovich <yuri.benditovich@daynix.com>
To: Akihiko Odaki <akihiko.odaki@daynix.com>
Cc: qemu-devel@nongnu.org, Andrew Melnychenko <andrew@daynix.com>,
"Michael S . Tsirkin" <mst@redhat.com>,
Jason Wang <jasowang@redhat.com>
Subject: Re: [PATCH v5 11/21] virtio-net: Return an error when vhost cannot enable RSS
Date: Mon, 30 Oct 2023 00:15:56 +0200 [thread overview]
Message-ID: <CAOEp5OfG4nY9ymuryyARuCeOC3Y2uyFzeuAZOTR5qDkJcTL_BQ@mail.gmail.com> (raw)
In-Reply-To: <20231017040932.62997-12-akihiko.odaki@daynix.com>
[-- Attachment #1: Type: text/plain, Size: 11703 bytes --]
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
>
>
[-- Attachment #2: Type: text/html, Size: 14095 bytes --]
next prev parent reply other threads:[~2023-10-29 22:17 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-10-17 4:09 [PATCH v5 00/21] virtio-net RSS/hash report fixes and improvements Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 01/21] tap: Remove tap_probe_vnet_hdr_len() Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 02/21] tap: Remove qemu_using_vnet_hdr() Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 03/21] net: Move virtio-net header length assertion Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 04/21] net: Remove receive_raw() Akihiko Odaki
2023-10-27 6:49 ` Jason Wang
2023-10-27 7:52 ` Akihiko Odaki
2023-10-30 3:06 ` Jason Wang
2023-10-30 4:03 ` Akihiko Odaki
2023-10-30 4:08 ` Jason Wang
2023-10-30 4:16 ` Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 05/21] tap: Remove tap_receive() Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 06/21] net: Remove flag propagation Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 07/21] tap: Shrink zeroed virtio-net header Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 08/21] virtio-net: Copy header only when necessary Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 09/21] virtio-net: Disable RSS on reset Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 10/21] virtio-net: Unify the logic to update NIC state for RSS Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 11/21] virtio-net: Return an error when vhost cannot enable RSS Akihiko Odaki
2023-10-27 7:07 ` Jason Wang
2023-10-27 7:54 ` Akihiko Odaki
2023-10-30 3:44 ` Jason Wang
2023-10-29 22:15 ` Yuri Benditovich [this message]
2023-10-17 4:09 ` [PATCH v5 12/21] virtio-net: Always set populate_hash Akihiko Odaki
2023-10-27 7:08 ` Jason Wang
2023-10-27 7:57 ` Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 13/21] virtio-net: Do not clear VIRTIO_NET_F_RSS Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 14/21] virtio-net: Do not write hashes to peer buffer Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 15/21] virtio-net: Do not clear VIRTIO_NET_F_HASH_REPORT Akihiko Odaki
2023-10-27 7:14 ` Jason Wang
2023-10-27 8:07 ` Akihiko Odaki
2023-10-29 21:56 ` Yuri Benditovich
2023-10-30 3:59 ` Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 16/21] ebpf: Fix RSS error handling Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 17/21] ebpf: Use standard section name Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 18/21] ebpf: Simplify error handling Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 19/21] ebpf: Return 0 when configuration fails Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 20/21] ebpf: Refactor tun_rss_steering_prog() Akihiko Odaki
2023-10-17 4:09 ` [PATCH v5 21/21] ebpf: Add a separate target for skeleton Akihiko Odaki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAOEp5OfG4nY9ymuryyARuCeOC3Y2uyFzeuAZOTR5qDkJcTL_BQ@mail.gmail.com \
--to=yuri.benditovich@daynix.com \
--cc=akihiko.odaki@daynix.com \
--cc=andrew@daynix.com \
--cc=jasowang@redhat.com \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).