Hi all,
On Fri, May 17, 2024 at 5:00 PM Michal Prívozník <mprivozn@xxxxxxxxxx> wrote:
>
> On 5/12/24 21:45, Andrew Melnychenko wrote:
> > Added new capability ebpf_rss_fds for QEMU.
> > Added logic for loading the "RSS" eBPF program.
> > eBPF file descriptors passed to the QEMU.
> >
> > Signed-off-by: Andrew Melnychenko <andrew@xxxxxxxxxx>
> > ---
> > src/qemu/qemu_capabilities.c | 4 +++
> > src/qemu/qemu_capabilities.h | 3 ++
> > src/qemu/qemu_command.c | 61 ++++++++++++++++++++++++++++++++++++
> > src/qemu/qemu_domain.c | 4 +++
> > src/qemu/qemu_domain.h | 3 ++
> > 5 files changed, 75 insertions(+)
> >
> > diff --git a/src/qemu/qemu_capabilities.c b/src/qemu/qemu_capabilities.c
> > index 09bb6ca36e..bb6a587725 100644
> > --- a/src/qemu/qemu_capabilities.c
> > +++ b/src/qemu/qemu_capabilities.c
> > @@ -708,6 +708,9 @@ VIR_ENUM_IMPL(virQEMUCaps,
> > "usb-mtp", /* QEMU_CAPS_DEVICE_USB_MTP */
> > "machine.virt.ras", /* QEMU_CAPS_MACHINE_VIRT_RAS */
> > "virtio-sound", /* QEMU_CAPS_DEVICE_VIRTIO_SOUND */
> > +
> > + /* 460 */
> > + "virtio-net.ebpf_rss_fds", /* QEMU_CAPS_VIRTIO_NET_EBPF_RSS_FDS */
> > );
> >
> >
> > @@ -1452,6 +1455,7 @@ static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioBlk[] = {
> > static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsVirtioNet[] = {
> > { "acpi-index", QEMU_CAPS_ACPI_INDEX, NULL },
> > { "rss", QEMU_CAPS_VIRTIO_NET_RSS, NULL },
> > + { "ebpf-rss-fds", QEMU_CAPS_VIRTIO_NET_EBPF_RSS_FDS, NULL },
> > };
> >
> > static struct virQEMUCapsDevicePropsFlags virQEMUCapsDevicePropsPCIeRootPort[] = {
> > diff --git a/src/qemu/qemu_capabilities.h b/src/qemu/qemu_capabilities.h
> > index 371ea19bd0..e4bb137b21 100644
> > --- a/src/qemu/qemu_capabilities.h
> > +++ b/src/qemu/qemu_capabilities.h
> > @@ -688,6 +688,9 @@ typedef enum { /* virQEMUCapsFlags grouping marker for syntax-check */
> > QEMU_CAPS_MACHINE_VIRT_RAS, /* -machine virt,ras= */
> > QEMU_CAPS_DEVICE_VIRTIO_SOUND, /* -device virtio-sound-* */
> >
> > + /* 460 */
> > + QEMU_CAPS_VIRTIO_NET_EBPF_RSS_FDS, /* virtio-net ebpf_rss_fds feature */
> > +
> > QEMU_CAPS_LAST /* this must always be the last item */
> > } virQEMUCapsFlags;
> >
> > diff --git a/src/qemu/qemu_command.c b/src/qemu/qemu_command.c
> > index 9859ea67a4..77715cf6fe 100644
> > --- a/src/qemu/qemu_command.c
> > +++ b/src/qemu/qemu_command.c
> > @@ -3805,6 +3805,25 @@ qemuBuildLegacyNicStr(virDomainNetDef *net)
> > }
> >
> >
> > +static virJSONValue *
> > +qemuBuildEbpfRssArg(virDomainNetDef *net)
> > +{
> > + qemuDomainNetworkPrivate *netpriv = QEMU_DOMAIN_NETWORK_PRIVATE(net);
> > + g_autoptr(virJSONValue) props = NULL;
> > +
> > + GSList *n;
> > +
> > + if (netpriv->ebpfrssfds)
> > + props = virJSONValueNewArray();
>
> nitpick: A bit more convoluted than needed be.
>
> if (!netpriv->ebpfrssfds)
> return NULL;
>
> props = virJSONValueNewArray();
>
I'll refactor it in the next patch.
> > +
> > + for (n = netpriv->ebpfrssfds; n; n = n->next) {
> > + virJSONValueArrayAppendString(props, qemuFDPassDirectGetPath(n->data));
> > + }
> > +
> > + return g_steal_pointer(&props);
> > +}
> > +
> > +
> > virJSONValue *
> > qemuBuildNicDevProps(virDomainDef *def,
> > virDomainNetDef *net,
> > @@ -3813,6 +3832,7 @@ qemuBuildNicDevProps(virDomainDef *def,
> > g_autoptr(virJSONValue) props = NULL;
> > char macaddr[VIR_MAC_STRING_BUFLEN];
> > g_autofree char *netdev = g_strdup_printf("host%s", net->info.alias);
> > + g_autoptr(virJSONValue) ebpffds = NULL;
>
> We like to declare variables in their smallest possible scope. IOW, this
> should be declared ..
>
> >
> > if (virDomainNetIsVirtioModel(net)) {
> > const char *tx = NULL;
>
> .. in this block.
Ok.
> > @@ -3863,6 +3883,8 @@ qemuBuildNicDevProps(virDomainDef *def,
> > if (!(props = qemuBuildVirtioDevProps(VIR_DOMAIN_DEVICE_NET, net, qemuCaps)))
> > return NULL;
> >
> > + ebpffds = qemuBuildEbpfRssArg(net);
> > +
> > if (virJSONValueObjectAdd(&props,
> > "S:tx", tx,
> > "T:ioeventfd", net->driver.virtio.ioeventfd,
> > @@ -3887,6 +3909,7 @@ qemuBuildNicDevProps(virDomainDef *def,
> > "T:hash", net->driver.virtio.rss_hash_report,
> > "p:host_mtu", net->mtu,
> > "T:failover", failover,
> > + "A:ebpf-rss-fds", &ebpffds,
> > NULL) < 0)
> > return NULL;
> > } else {
> > @@ -4170,6 +4193,39 @@ qemuBuildWatchdogDevProps(const virDomainDef *def,
> > }
> >
> >
> > +static void
> > +qemuOpenEbpfRssFds(virDomainNetDef *net, virQEMUCaps *qemuCaps)
> > +{
> > + const char *ebpfRSSObject = NULL;
> > + int fds[16];
>
> So there can be 16 FDs top? Isn't the actual value related to number of
> virtio queues?
The 16 is just a "big enough number". Overall, I think it is possible to
dynamically figure out or allocate filedescriptors - I'll check it in
the next patches.
>
> > + int nfds = 0;
> > + size_t i = 0;
> > + qemuDomainNetworkPrivate *netpriv = QEMU_DOMAIN_NETWORK_PRIVATE(net);
> > +
> > + netpriv->libbpfRSSObject = NULL;
> > + netpriv->ebpfrssfds = NULL;
>
> This looks suspicious. When allocating private data, g_new0() made sure
> this is zeroed out.
>
> > +
> > + /* Add ebpf values */
> > + if (net->driver.virtio.rss == VIR_TRISTATE_SWITCH_ON
> > + && net->driver.virtio.rss_hash_report != VIR_TRISTATE_SWITCH_ON
> > + && virQEMUCapsGet(qemuCaps, QEMU_CAPS_VIRTIO_NET_EBPF_RSS_FDS)) {
> > + ebpfRSSObject = virQEMUCapsGetEbpf(qemuCaps, "rss");
> > + nfds = qemuInterfaceLoadEbpf(ebpfRSSObject, &netpriv->libbpfRSSObject, fds, 16);
>
> s/G_N_ELEMENTS(fds)/16/
>
> Also, at this point if nfds == -1 we're not sure whether we're lacking
> libbpf, or libpf failed to load eBPF program. While the former should be
> skipped over, the latter is a fatal error, isn't it?
I'll check it.
>
> > +
> > + if (nfds > 0) {
>
> To increase readability of this code (by decrasing level of nesting),
> I'd write this as:
>
> if (nfds <= 0)
> return;
>
> > + for (i = 0; i < nfds; ++i) {
> > + g_autofree char *name = g_strdup_printf("ebpfrssfd-%s-%zu", net->info.alias, i);
> > +
> > + netpriv->ebpfrssfds =
> > + g_slist_prepend(netpriv->ebpfrssfds, qemuFDPassDirectNew(name, fds + i));
> > + }
> > + }
> > +
> > + netpriv->ebpfrssfds = g_slist_reverse(netpriv->ebpfrssfds);
> > + }
> > +}
> > +
> > +
>
> Michal
>
