Re: [RFC PATCH v2 4/5] virtio-net: Added eBPF RSS to virtio-net.

[Jason Wang <jasowang@redhat.com>]

On 2020/11/19 下午7:13, Andrew Melnychenko wrote:
> From: Andrew <andrew@daynix.com>
>
> When RSS is enabled the device tries to load the eBPF program
> to select RX virtqueue in the TUN. If eBPF can be loaded
> the RSS will function also with vhost (works with kernel 5.8 and later).
> Software RSS is used as a fallback with vhost=off when eBPF can't be loaded
> or when hash population requested by the guest.
>
> Signed-off-by: Yuri Benditovich <yuri.benditovich@daynix.com>
> Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> ---
>   hw/net/vhost_net.c             |   2 +
>   hw/net/virtio-net.c            | 120 +++++++++++++++++++++++++++++++--
>   include/hw/virtio/virtio-net.h |   4 ++
>   net/vhost-vdpa.c               |   2 +
>   4 files changed, 124 insertions(+), 4 deletions(-)
>
> diff --git a/hw/net/vhost_net.c b/hw/net/vhost_net.c
> index 24d555e764..16124f99c3 100644
> --- a/hw/net/vhost_net.c
> +++ b/hw/net/vhost_net.c
> @@ -71,6 +71,8 @@ static const int user_feature_bits[] = {
>       VIRTIO_NET_F_MTU,
>       VIRTIO_F_IOMMU_PLATFORM,
>       VIRTIO_F_RING_PACKED,
> +    VIRTIO_NET_F_RSS,
> +    VIRTIO_NET_F_HASH_REPORT,
>   
>       /* This bit implies RARP isn't sent by QEMU out of band */
>       VIRTIO_NET_F_GUEST_ANNOUNCE,
> diff --git a/hw/net/virtio-net.c b/hw/net/virtio-net.c
> index 277289d56e..afcc3032ec 100644
> --- a/hw/net/virtio-net.c
> +++ b/hw/net/virtio-net.c
> @@ -698,6 +698,19 @@ static void virtio_net_set_queues(VirtIONet *n)
>   
>   static void virtio_net_set_multiqueue(VirtIONet *n, int multiqueue);
>   
> +static uint64_t fix_ebpf_vhost_features(uint64_t features)
> +{
> +    /* If vhost=on & CONFIG_EBPF doesn't set - disable RSS feature */
> +    uint64_t ret = features;
> +#ifndef CONFIG_EBPF
> +    virtio_clear_feature(&ret, VIRTIO_NET_F_RSS);
> +#endif
> +    /* for now, there is no solution for populating the hash from eBPF */
> +    virtio_clear_feature(&ret, VIRTIO_NET_F_HASH_REPORT);


I think there's still some misunderstanding here.

When "rss" is enabled via command line, qemu can't not turn it off 
silently, otherwise it may break migration. Instead, qemu should disable 
vhost-net if eBPF can't be loaded.

When "hash_report" is enabled via command line, qemu should disable 
vhost-net unconditionally.


> +
> +    return ret;
> +}
> +
>   static uint64_t virtio_net_get_features(VirtIODevice *vdev, uint64_t features,
>                                           Error **errp)
>   {
> @@ -732,9 +745,9 @@ 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);
> +    features = fix_ebpf_vhost_features(
> +            vhost_net_get_features(get_vhost_net(nc->peer), features));
> +
>       vdev->backend_features = features;
>   
>       if (n->mtu_bypass_backend &&
> @@ -1169,12 +1182,75 @@ static int virtio_net_handle_announce(VirtIONet *n, uint8_t cmd,
>       }
>   }
>   
> +static void virtio_net_unload_epbf_rss(VirtIONet *n);
> +
>   static void virtio_net_disable_rss(VirtIONet *n)
>   {
>       if (n->rss_data.enabled) {
>           trace_virtio_net_rss_disable();
>       }
>       n->rss_data.enabled = false;
> +
> +    if (!n->rss_data.enabled_software_rss && ebpf_rss_is_loaded(&n->ebpf_rss)) {
> +        virtio_net_unload_epbf_rss(n);
> +    }
> +}
> +
> +static bool virtio_net_attach_steering_ebpf(NICState *nic, int prog_fd)
> +{
> +    NetClientState *nc = qemu_get_peer(qemu_get_queue(nic), 0);
> +    if (nc == NULL || nc->info->set_steering_ebpf == NULL) {
> +        return false;
> +    }
> +
> +    return nc->info->set_steering_ebpf(nc, prog_fd);
> +}
> +
> +static void rss_data_to_rss_config(struct VirtioNetRssData *data,
> +                                   struct EBPFRSSConfig *config)
> +{
> +    config->redirect = data->redirect;
> +    config->populate_hash = data->populate_hash;
> +    config->hash_types = data->hash_types;
> +    config->indirections_len = data->indirections_len;
> +    config->default_queue = data->default_queue;
> +}
> +
> +static bool virtio_net_load_epbf_rss(VirtIONet *n)
> +{
> +    struct EBPFRSSConfig config = {};
> +
> +    if (!n->rss_data.enabled) {
> +        if (ebpf_rss_is_loaded(&n->ebpf_rss)) {
> +            ebpf_rss_unload(&n->ebpf_rss);
> +        }
> +        return true;
> +    }
> +
> +    if (!ebpf_rss_is_loaded(&n->ebpf_rss) && !ebpf_rss_load(&n->ebpf_rss)) {
> +        return false;
> +    }
> +
> +    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)) {
> +        ebpf_rss_unload(&n->ebpf_rss);
> +        return false;
> +    }
> +
> +    if (!virtio_net_attach_steering_ebpf(n->nic, n->ebpf_rss.program_fd)) {
> +        ebpf_rss_unload(&n->ebpf_rss);
> +        return false;
> +    }
> +
> +    return true;
> +}
> +
> +static void virtio_net_unload_epbf_rss(VirtIONet *n)
> +{
> +    virtio_net_attach_steering_ebpf(n->nic, -1);
> +    ebpf_rss_unload(&n->ebpf_rss);
>   }
>   
>   static uint16_t virtio_net_handle_rss(VirtIONet *n,
> @@ -1208,6 +1284,7 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n,
>           err_value = (uint32_t)s;
>           goto error;
>       }
> +    n->rss_data.enabled_software_rss = false;
>       n->rss_data.hash_types = virtio_ldl_p(vdev, &cfg.hash_types);
>       n->rss_data.indirections_len =
>           virtio_lduw_p(vdev, &cfg.indirection_table_mask);
> @@ -1289,9 +1366,30 @@ static uint16_t virtio_net_handle_rss(VirtIONet *n,
>           goto error;
>       }
>       n->rss_data.enabled = true;
> +
> +    if (!n->rss_data.populate_hash) {
> +        /* load EBPF RSS */


The code explains itself, so the comment is no necessary.


> +        if (!virtio_net_load_epbf_rss(n)) {


Any reason that we load eBPF RSS here? I thought it would be easier to 
do it during set_features (e.g when RSS is negotiated but not 
HASH_REPORT) and if we do that we don't need extra care about migration.


> +            /* EBPF mast be loaded for vhost */


Typo.


> +            if (get_vhost_net(qemu_get_queue(n->nic)->peer)) {
> +                warn_report("Can't load eBPF RSS for vhost");
> +                goto error;
> +            }
> +            /* fallback to software RSS */
> +            warn_report("Can't load eBPF RSS - fallback to software RSS");
> +            n->rss_data.enabled_software_rss = true;
> +        }
> +    } else {
> +        /* use software RSS for hash populating */
> +        /* and unload eBPF if was loaded before */
> +        virtio_net_unload_epbf_rss(n);
> +        n->rss_data.enabled_software_rss = true;
> +    }
> +
>       trace_virtio_net_rss_enable(n->rss_data.hash_types,
>                                   n->rss_data.indirections_len,
>                                   temp.b);
> +


Unnecessary changes.


>       return queues;
>   error:
>       trace_virtio_net_rss_error(err_msg, err_value);
> @@ -1674,7 +1772,7 @@ static ssize_t virtio_net_receive_rcu(NetClientState *nc, const uint8_t *buf,
>           return -1;
>       }
>   
> -    if (!no_rss && n->rss_data.enabled) {
> +    if (!no_rss && n->rss_data.enabled && n->rss_data.enabled_software_rss) {
>           int index = virtio_net_process_rss(nc, buf, size);
>           if (index >= 0) {
>               NetClientState *nc2 = qemu_get_subqueue(n->nic, index);
> @@ -2780,6 +2878,18 @@ static int virtio_net_post_load_device(void *opaque, int version_id)
>       }
>   
>       if (n->rss_data.enabled) {
> +        n->rss_data.enabled_software_rss = n->rss_data.populate_hash;
> +        if (!n->rss_data.populate_hash) {
> +            if (!virtio_net_load_epbf_rss(n)) {
> +                if (get_vhost_net(qemu_get_queue(n->nic)->peer)) {
> +                    error_report("Can't post-load eBPF RSS for vhost");
> +                } else {
> +                    warn_report("Can't post-load eBPF RSS - fallback to software RSS");
> +                    n->rss_data.enabled_software_rss = true;
> +                }
> +            }
> +        }


btw, I don't see the save and restore of bpf maps, or is it unnecessary?

Thanks


> +
>           trace_virtio_net_rss_enable(n->rss_data.hash_types,
>                                       n->rss_data.indirections_len,
>                                       sizeof(n->rss_data.key));
> @@ -3453,6 +3563,8 @@ static void virtio_net_instance_init(Object *obj)
>       device_add_bootindex_property(obj, &n->nic_conf.bootindex,
>                                     "bootindex", "/ethernet-phy@0",
>                                     DEVICE(n));
> +
> +    ebpf_rss_init(&n->ebpf_rss);
>   }
>   
>   static int virtio_net_pre_save(void *opaque)
> diff --git a/include/hw/virtio/virtio-net.h b/include/hw/virtio/virtio-net.h
> index f4852ac27b..4d29a577eb 100644
> --- a/include/hw/virtio/virtio-net.h
> +++ b/include/hw/virtio/virtio-net.h
> @@ -21,6 +21,8 @@
>   #include "qemu/option_int.h"
>   #include "qom/object.h"
>   
> +#include "ebpf/ebpf_rss.h"
> +
>   #define TYPE_VIRTIO_NET "virtio-net-device"
>   OBJECT_DECLARE_SIMPLE_TYPE(VirtIONet, VIRTIO_NET)
>   
> @@ -130,6 +132,7 @@ typedef struct VirtioNetRscChain {
>   
>   typedef struct VirtioNetRssData {
>       bool    enabled;
> +    bool    enabled_software_rss;
>       bool    redirect;
>       bool    populate_hash;
>       uint32_t hash_types;
> @@ -214,6 +217,7 @@ struct VirtIONet {
>       Notifier migration_state;
>       VirtioNetRssData rss_data;
>       struct NetRxPkt *rx_pkt;
> +    struct EBPFRSSContext ebpf_rss;
>   };
>   
>   void virtio_net_set_netclient_name(VirtIONet *n, const char *name,
> diff --git a/net/vhost-vdpa.c b/net/vhost-vdpa.c
> index 99c476db8c..feb5fa8624 100644
> --- a/net/vhost-vdpa.c
> +++ b/net/vhost-vdpa.c
> @@ -54,6 +54,8 @@ const int vdpa_feature_bits[] = {
>       VIRTIO_NET_F_MTU,
>       VIRTIO_F_IOMMU_PLATFORM,
>       VIRTIO_F_RING_PACKED,
> +    VIRTIO_NET_F_RSS,
> +    VIRTIO_NET_F_HASH_REPORT,
>       VIRTIO_NET_F_GUEST_ANNOUNCE,
>       VIRTIO_NET_F_STATUS,
>       VHOST_INVALID_FEATURE_BIT