| Message ID | 20201119111305.485202-5-andrew@daynix.com |
|---|---|
| State | New |
| Headers | show |
| Series | eBPF RSS support for virtio-net | expand |
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
On Tue, Nov 24, 2020 at 10:49 AM Jason Wang <jasowang@redhat.com> wrote: > > 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. > > I agree in general with this requirement and I'm preparing an implementation of such fallback. The problem is that qemu already uses the mechanism of turning off host features silently if they are not supported by the current vhost in kernel: https://github.com/qemu/qemu/blob/b0f8c22d6d4d07f3bd2307bcc62e1660ef965472/hw/virtio/vhost.c#L1526 Can you please comment on it and let me know how it should be modified in future? I've planned to use it in next work (implementing hash report in kernel) > > > + > > + 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 > >
On 2020/12/1 下午3:40, Yuri Benditovich wrote: > > > On Tue, Nov 24, 2020 at 10:49 AM Jason Wang <jasowang@redhat.com > <mailto:jasowang@redhat.com>> wrote: > > > On 2020/11/19 下午7:13, Andrew Melnychenko wrote: > > From: Andrew <andrew@daynix.com <mailto: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 > <mailto:yuri.benditovich@daynix.com>> > > Signed-off-by: Andrew Melnychenko <andrew@daynix.com > <mailto: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. > > > I agree in general with this requirement and I'm preparing an > implementation of such fallback. > > The problem is that qemu already uses the mechanism of turning off > host features > silently if they are not supported by the current vhost in kernel: > https://github.com/qemu/qemu/blob/b0f8c22d6d4d07f3bd2307bcc62e1660ef965472/hw/virtio/vhost.c#L1526 > > Can you please comment on it and let me know how it should be modified > in future? > I've planned to use it in next work (implementing hash report in kernel) This looks like a bug that needs to be solved. Otherwise we break migration from rss=on, vhost=off to rss=on,vhost=on. I think you can keep the current code as is and I will try to seek a way to solve the issue. Thanks
On Wed, Dec 2, 2020 at 6:06 AM Jason Wang <jasowang@redhat.com> wrote: > > On 2020/12/1 下午3:40, Yuri Benditovich wrote: > > > > > > On Tue, Nov 24, 2020 at 10:49 AM Jason Wang <jasowang@redhat.com > > <mailto:jasowang@redhat.com>> wrote: > > > > > > On 2020/11/19 下午7:13, Andrew Melnychenko wrote: > > > From: Andrew <andrew@daynix.com <mailto: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 > > <mailto:yuri.benditovich@daynix.com>> > > > Signed-off-by: Andrew Melnychenko <andrew@daynix.com > > <mailto: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. > > > > > > I agree in general with this requirement and I'm preparing an > > implementation of such fallback. > > > > The problem is that qemu already uses the mechanism of turning off > > host features > > silently if they are not supported by the current vhost in kernel: > > > https://github.com/qemu/qemu/blob/b0f8c22d6d4d07f3bd2307bcc62e1660ef965472/hw/virtio/vhost.c#L1526 > > > > Can you please comment on it and let me know how it should be modified > > in future? > > I've planned to use it in next work (implementing hash report in kernel) > > > This looks like a bug that needs to be solved. Otherwise we break > migration from rss=on, vhost=off to rss=on,vhost=on. > > I think I need to fill the gap in my understanding of migration's prerequisites. According to https://github.com/qemu/qemu/blob/b0f8c22d6d4d07f3bd2307bcc62e1660ef965472/docs/devel/migration.rst "... QEMU has to be launched with the same arguments the two times ..." and we test the migration during development according to this statement. What are the real requirements and prerequisites of the migration? > I think you can keep the current code as is and I will try to seek a way > to solve the issue. > > Thanks > >
On 2020/12/2 下午3:16, Yuri Benditovich wrote: > > > On Wed, Dec 2, 2020 at 6:06 AM Jason Wang <jasowang@redhat.com > <mailto:jasowang@redhat.com>> wrote: > > > On 2020/12/1 下午3:40, Yuri Benditovich wrote: > > > > > > On Tue, Nov 24, 2020 at 10:49 AM Jason Wang <jasowang@redhat.com > <mailto:jasowang@redhat.com> > > <mailto:jasowang@redhat.com <mailto:jasowang@redhat.com>>> wrote: > > > > > > On 2020/11/19 下午7:13, Andrew Melnychenko wrote: > > > From: Andrew <andrew@daynix.com <mailto:andrew@daynix.com> > <mailto:andrew@daynix.com <mailto: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 <mailto:yuri.benditovich@daynix.com> > > <mailto:yuri.benditovich@daynix.com > <mailto:yuri.benditovich@daynix.com>>> > > > Signed-off-by: Andrew Melnychenko <andrew@daynix.com > <mailto:andrew@daynix.com> > > <mailto:andrew@daynix.com <mailto: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. > > > > > > I agree in general with this requirement and I'm preparing an > > implementation of such fallback. > > > > The problem is that qemu already uses the mechanism of turning off > > host features > > silently if they are not supported by the current vhost in kernel: > > > https://github.com/qemu/qemu/blob/b0f8c22d6d4d07f3bd2307bcc62e1660ef965472/hw/virtio/vhost.c#L1526 > > > > Can you please comment on it and let me know how it should be > modified > > in future? > > I've planned to use it in next work (implementing hash report in > kernel) > > > This looks like a bug that needs to be solved. Otherwise we break > migration from rss=on, vhost=off to rss=on,vhost=on. > > I think I need to fill the gap in my understanding of migration's > prerequisites. > According to > https://github.com/qemu/qemu/blob/b0f8c22d6d4d07f3bd2307bcc62e1660ef965472/docs/devel/migration.rst > "... QEMU has to be launched with the same arguments the two times > ..." and we test the migration during development > according to this statement. Yes, that's the overall requirement. And it shows the issue of disabling feature silently. If we had src whose vhost support feature A and dst vhost doesn't support. Even if we launch the QEMU with the same arguments, it can still fail. So actually two issues: 1) whether or not to disable features silently 2) whether or not to support migration between vhost=on to vhost=off For 1), I think we'd better don't do that, and if we can do 1), it would be possible to achieve 2). > What are the real requirements and prerequisites of the migration? For virtio, I think we allow some kind of extra flexibility. We try to make migration work between vhost=off and vhost=on. What we need is just to keep everything that visible to guest after migration the same as before migration. Obviously, device features is one of the thing. Thanks > > I think you can keep the current code as is and I will try to seek > a way > to solve the issue. > > Thanks >
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); + + 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 */ + if (!virtio_net_load_epbf_rss(n)) { + /* EBPF mast be loaded for vhost */ + 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); + 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; + } + } + } + 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