From: Andrew Melnichenko <andrew@daynix.com>
To: Markus Armbruster <armbru@redhat.com>
Cc: "Daniel P. Berrangé" <berrange@redhat.com>,
"Michael S. Tsirkin" <mst@redhat.com>,
"Jason Wang" <jasowang@redhat.com>,
qemu-devel@nongnu.org,
"Yuri Benditovich" <yuri.benditovich@daynix.com>,
"Yan Vugenfirer" <yan@daynix.com>,
"Eric Blake" <eblake@redhat.com>
Subject: Re: [RFC PATCH 4/5] qmp: Added qemu-ebpf-rss-path command.
Date: Wed, 16 Jun 2021 02:16:25 +0300 [thread overview]
Message-ID: <CABcq3pHsSyk7d42Qb6k5ke1QxME1TArNkWB9LLzZq8f4UYmk4A@mail.gmail.com> (raw)
In-Reply-To: <87fsxnejnw.fsf@dusky.pond.sub.org>
[-- Attachment #1: Type: text/plain, Size: 9810 bytes --]
Hi all,
> Seems like this function is duplicating what glib should already be
> able to do.
>
Yea, but it's required a Linux specific header - without it, qemu builds
but crashes.
Could we use a compile-time determination of where we were (supposed)
> to be installed, and therefore where our helper should be installed,
> rather than the dynamic /proc/self/exe munging?
>
Yes, we can define something like CONFIG_QEMU_HELPERDIR ##
"qemu-ebpf-rss-helper", for RSS helper.
But I've tried to implement generic function for possible other helpers.
Yeah I think avoiding /proc/self/exe is desirable, because I can
> imagine scenarios where this can lead to picking the wrong helper.
> Better to always use the compile time install directory.
>
The main scenario that find_helper() should solve - non installed qemu use
helper from own build.
That's why reading /proc/self/exe is implemented.
So the intent is that we can make this list larger if we write other
> helper binaries. But this code is in an overall #ifdef CONFIG_LINUX,
> which means it won't work on other platforms.
>
Yes, for now, eBPF RSS is only for virtio-net + Linux TAP.
Checking F_OK (existence) instea of X_OK is odd.
>
Libvirt launches qemu to get different properties. That qemu may not have
permission to launch the helper.
It uses /proc/self/exe to find the running executable's directory. This
> is specific to Linux[*]. You get different behavior on Linux vs. other
> systems.
>
The code guarded by CONFIG_LINUX.
* If the host isn't Linux, it returns /usr/libexec/qemu-ebpf-rss-helper.
> Not good.
>
No, "query-helper-paths" will return an empty list.
* If Alice runs bld/x86_64-softmmu/qemu-system-x86_64, it also returns
> /usr/libexec/qemu-ebpf-rss-helper. Not good.
>
No, /proc/self/exe dereferences "bld/x86_64-softmmu/qemu-system-x86_64" to
"bld/qemu-system-x86_64"
and we will get bld/qemu-ebpf-rss-helper.
The name query-helper-paths is generic, the documented purpose "Query
> specific eBPF RSS helper" is specific.
>
> qemu-ebpf-rss-helper isn't necessarily the only helper that needs to be
> in sync with QEMU.
>
Yea, I'll update the document.
If we want to ensure the right helper runs, we should use a build
> identifier compiled into the programs, like we do for modules.
>
Thanks, I'll check. Overall, current idea was to avoid the use of the helper
from CONFIG_QEMU_HELPERDIR if qemu is not installed(like in your examples).
Helpers QEMU code runs itself should be run as
> CONFIG_QEMU_HELPERDIR/HELPER, with a suitable user override. This is
> how qemu-bridge-helper works.
>
> Helpers some other program runs are that other program's problem.
> They'll probably work the same: built-in default that can be overridden
> with configuration.
>
Well, for qemu it does not really matter how TAP fd was created. It can be
the helper, Libvirt itself, or a script.
In the end, "netdev" gets its fds and for qemu there is no difference. TAP
fd is TAP fd.
And Libvirt would use the same qemu-bridge-helper(from libvirt/qemu.conf)
for every qemu "emulator".
For eBPF we need to create specific maps(and/or thair quantities) that
contain specific structures and for different
qemu it may be different.
On Sat, Jun 12, 2021 at 8:28 AM Markus Armbruster <armbru@redhat.com> wrote:
> Andrew Melnychenko <andrew@daynix.com> writes:
>
> > New qmp command to query ebpf helper.
> > It's crucial that qemu and helper are in sync and in touch.
> > Technically helper should pass eBPF fds that qemu may accept.
> > And different qemu's builds may have different eBPF programs and helpers.
> > Qemu returns helper that should "fit" to virtio-net.
> >
> > Signed-off-by: Andrew Melnychenko <andrew@daynix.com>
> > ---
> > monitor/qmp-cmds.c | 78 ++++++++++++++++++++++++++++++++++++++++++++++
> > qapi/misc.json | 29 +++++++++++++++++
> > 2 files changed, 107 insertions(+)
> >
> > diff --git a/monitor/qmp-cmds.c b/monitor/qmp-cmds.c
> > index f7d64a6457..5dd2a58ea2 100644
> > --- a/monitor/qmp-cmds.c
> > +++ b/monitor/qmp-cmds.c
> > @@ -351,3 +351,81 @@ void qmp_display_reload(DisplayReloadOptions *arg,
> Error **errp)
> > abort();
> > }
> > }
> > +
> > +#ifdef CONFIG_LINUX
> > +
> > +static const char *get_dirname(char *path)
> > +{
> > + char *sep;
> > +
> > + sep = strrchr(path, '/');
> > + if (sep == path) {
> > + return "/";
> > + } else if (sep) {
> > + *sep = 0;
> > + return path;
> > + }
> > + return ".";
> > +}
> > +
> > +static char *find_helper(const char *name)
> > +{
> > + char qemu_exec[PATH_MAX];
> > + const char *qemu_dir = NULL;
> > + char *helper = NULL;
> > +
> > + if (name == NULL) {
> > + return NULL;
> > + }
> > +
> > + if (readlink("/proc/self/exe", qemu_exec, PATH_MAX) > 0) {
> > + qemu_dir = get_dirname(qemu_exec);
> > +
> > + helper = g_strdup_printf("%s/%s", qemu_dir, name);
> > + if (access(helper, F_OK) == 0) {
> > + return helper;
> > + }
> > + g_free(helper);
> > + }
> > +
> > + helper = g_strdup_printf("%s/%s", CONFIG_QEMU_HELPERDIR, name);
> > + if (access(helper, F_OK) == 0) {
> > + return helper;
> > + }
> > + g_free(helper);
> > +
> > + return NULL;
> > +}
>
> This returns the helper in the same directory as the running executable,
> or as a fallback the helper in CONFIG_QEMU_HELPERDIR.
>
> Checking F_OK (existence) instea of X_OK is odd.
>
> It uses /proc/self/exe to find the running executable's directory. This
> is specific to Linux[*]. You get different behavior on Linux vs. other
> systems.
>
> CONFIG_QEMU_HELPERDIR is $prefix/libexec/.
>
> If $prefix is /usr, then qemu-system-FOO is normally installed in
> /usr/bin/, and the helper in /usr/libexec/. We look for the helper in
> the wrong place first, and the right one only when it isn't in the wrong
> place. Feels overcomplicated and fragile.
>
> Consider the following scenario:
>
> * The system has a binary package's /usr/bin/qemu-system-x86_64 and
> /usr/libexec/qemu-ebpf-rss-helper installed
>
> * Alice builds her own QEMU with prefix /usr (and no intention to
> install), resulting in bld/qemu-system-x86_64, bld/qemu-ebpf-rss-path,
> and a symlink bld/x86_64-softmmu/qemu-system-x86_64.
>
> Now:
>
> * If Alice runs bld/qemu-system-x86_64, and the host is Linux,
> find_helper() returns bld/qemu-ebpf-rss-path. Good.
>
> * If the host isn't Linux, it returns /usr/libexec/qemu-ebpf-rss-helper.
> Not good.
>
> * If Alice runs bld/x86_64-softmmu/qemu-system-x86_64, it also returns
> /usr/libexec/qemu-ebpf-rss-helper. Not good.
>
> > +
> > +HelperPathList *qmp_query_helper_paths(Error **errp)
> > +{
> > + HelperPathList *ret = NULL;
> > + const char *helpers_list[] = {
> > +#ifdef CONFIG_EBPF
> > + "qemu-ebpf-rss-helper",
> > +#endif
> > + NULL
> > + };
> > + const char **helper_iter = helpers_list;
> > +
> > + for (; *helper_iter != NULL; ++helper_iter) {
> > + char *path = find_helper(*helper_iter);
> > + if (path) {
> > + HelperPath *helper = g_new0(HelperPath, 1);
> > + helper->name = g_strdup(*helper_iter);
> > + helper->path = path;
> > +
> > + QAPI_LIST_PREPEND(ret, helper);
> > + }
> > + }
> > +
> > + return ret;
> > +}
> > +#else
> > +
> > +HelperPathList *qmp_query_helper_paths(Error **errp)
> > +{
> > + return NULL;
> > +}
> > +
> > +#endif
> > diff --git a/qapi/misc.json b/qapi/misc.json
> > index 156f98203e..023bd2120d 100644
> > --- a/qapi/misc.json
> > +++ b/qapi/misc.json
> > @@ -519,3 +519,32 @@
> > 'data': { '*option': 'str' },
> > 'returns': ['CommandLineOptionInfo'],
> > 'allow-preconfig': true }
> > +
> > +##
> > +# @HelperPath:
> > +#
> > +# Name of the helper and binary location.
> > +##
> > +{ 'struct': 'HelperPath',
> > + 'data': {'name': 'str', 'path': 'str'} }
> > +
> > +##
> > +# @query-helper-paths:
> > +#
> > +# Query specific eBPF RSS helper for current qemu binary.
> > +#
> > +# Returns: list of object that contains name and path for helper.
> > +#
> > +# Example:
> > +#
> > +# -> { "execute": "query-helper-paths" }
> > +# <- { "return": [
> > +# {
> > +# "name": "qemu-ebpf-rss-helper",
> > +# "path": "/usr/local/libexec/qemu-ebpf-rss-helper"
> > +# }
> > +# ]
> > +# }
> > +#
> > +##
> > +{ 'command': 'query-helper-paths', 'returns': ['HelperPath'] }
>
> The name query-helper-paths is generic, the documented purpose "Query
> specific eBPF RSS helper" is specific.
>
> qemu-ebpf-rss-helper isn't necessarily the only helper that needs to be
> in sync with QEMU.
>
> I doubt a query command is a good way to help with using the right one.
> qemu-system-FOO doesn't really know where the right one is. Only the
> person or program that put them where they are does.
>
> If we want to ensure the right helper runs, we should use a build
> identifier compiled into the programs, like we do for modules.
>
> For modules, the program loading a module checks the module's build
> identifier matches its own.
>
> For programs talking to each other, the peers together check their build
> identifiers match.
>
> For programs where that isn't practical, the management application can
> check.
>
> This should be a lot more reliable.
>
> Helpers QEMU code runs itself should be run as
> CONFIG_QEMU_HELPERDIR/HELPER, with a suitable user override. This is
> how qemu-bridge-helper works.
>
> Helpers some other program runs are that other program's problem.
> They'll probably work the same: built-in default that can be overridden
> with configuration.
>
>
> [*] For detailed advice, see
>
> https://stackoverflow.com/questions/1023306/finding-current-executables-path-without-proc-self-exe
>
>
[-- Attachment #2: Type: text/html, Size: 14910 bytes --]
next prev parent reply other threads:[~2021-06-15 23:17 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-09 10:04 [RFC PATCH 0/5] ebpf: Added ebpf helper for libvirtd Andrew Melnychenko
2021-06-09 10:04 ` [RFC PATCH 1/5] ebpf: Added eBPF initialization by fds Andrew Melnychenko
2021-06-09 10:04 ` [RFC PATCH 2/5] virtio-net: Added property to load eBPF RSS with fds Andrew Melnychenko
2021-06-09 10:04 ` [RFC PATCH 3/5] ebpf_rss_helper: Added helper for eBPF RSS Andrew Melnychenko
2021-06-09 10:04 ` [RFC PATCH 4/5] qmp: Added qemu-ebpf-rss-path command Andrew Melnychenko
2021-06-11 14:15 ` Eric Blake
2021-06-11 17:21 ` Daniel P. Berrangé
2021-06-12 5:28 ` Markus Armbruster
2021-06-15 23:16 ` Andrew Melnichenko [this message]
2021-07-05 13:50 ` Andrew Melnichenko
2021-06-09 10:04 ` [RFC PATCH 5/5] meson: libbpf dependency now exclusively for Linux Andrew Melnychenko
2021-06-10 6:41 ` [RFC PATCH 0/5] ebpf: Added ebpf helper for libvirtd Jason Wang
2021-06-10 6:55 ` Yuri Benditovich
2021-06-11 5:36 ` Jason Wang
2021-06-11 16:49 ` Andrew Melnichenko
2021-06-11 17:24 ` Daniel P. Berrangé
2021-06-15 9:13 ` Jason Wang
2021-06-15 22:18 ` Andrew Melnichenko
2021-06-18 20:03 ` Andrew Melnichenko
2021-06-21 9:20 ` Jason Wang
2021-06-22 3:29 ` Yuri Benditovich
2021-06-22 4:58 ` Jason Wang
2021-06-22 8:25 ` Toke Høiland-Jørgensen
2021-06-22 8:27 ` Daniel P. Berrangé
2021-06-22 9:09 ` Toke Høiland-Jørgensen
2021-06-22 13:01 ` Andrew Melnichenko
2021-06-22 13:17 ` Toke Høiland-Jørgensen
2021-06-23 0:47 ` Jason Wang
2021-06-28 11:18 ` Yuri Benditovich
2021-06-29 3:39 ` Jason Wang
2021-06-30 16:40 ` Andrew Melnichenko
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=CABcq3pHsSyk7d42Qb6k5ke1QxME1TArNkWB9LLzZq8f4UYmk4A@mail.gmail.com \
--to=andrew@daynix.com \
--cc=armbru@redhat.com \
--cc=berrange@redhat.com \
--cc=eblake@redhat.com \
--cc=jasowang@redhat.com \
--cc=mst@redhat.com \
--cc=qemu-devel@nongnu.org \
--cc=yan@daynix.com \
--cc=yuri.benditovich@daynix.com \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).