Philippe Mathieu-Daudé <phi...@linaro.org> writes:
> On 25/8/25 14:19, Markus Armbruster wrote:
>> Daniel P. Berrangé <berra...@redhat.com> writes:
>>
>>> On Thu, Aug 07, 2025 at 03:14:56PM +0200, Markus Armbruster wrote:
>>>> Three functions in ebpf_rss.h take an Error ** argument and return bool.
>>>> Good.
>>>>
>>>> They can all fail without setting an error. Not good.
>>>>
>>>> The failures without error are:
>>>>
>>>> * All three stubs in ebpf_rss-stub.c always. Oversight?
>>>
>>> Opps, yes, we really should have added error_setg() calls for diagnosis
>>> if someone tries to use eBPF when QEMU build has it disabled.
>>
>> Some stubs exist only to mollify the linker. They are not meant to be
>> called. They should abort(), optionally with lipstick.
>
> When a host feature availability is known a compile time.
>
> These should be guarded with a if (feature_enabled()) to allow the
> compiler to elide the call, thus removing the need for stubs.
With if (...) guards, compilers need not elide the call (although a
sufficiently smart optimizing compiler will). Only #if guards remove
the need for stubs with certainty.
Guards can also be implicit. For instance, when creation of some
(sub-)object always fails, then all its methods are unreachable. This
can serve as guard for calls of stubs in the methods.
Of course, not compiling and linking unreachable code in the first place
is desirable, but sometimes the #if are just too bothersome.
>> Other stubs are called and should fail nicely.
>>
>> Can you tell me offhand which kind these are?
>
> When a host feature availability is known a runtime.
>
>>
>>>> * Non-stub ebpf_rss_load() when ebpf_rss_is_loaded(). Are these
>>>> reachable?
Scratch ebpf_rss_is_loaded(), it doesn't take @errp.
> meson calls:
>
> config_host_data.set('CONFIG_EBPF', libbpf.found())
>
> (even QAPI uses CONFIG_EBPF, see qapi/ebpf.json).
Yes.
C code doesn't use CONFIG_EBPF. Instead, compilation of certain C files
is conditional on the libbpf Meson dependency.
> The user API is via the 'ebpf-rss-fds' property,
> evaluated in virtio_net_load_ebpf_fds() without returning
> any error when 1/ ebpf_rss_load_fds() fails (due to real
> error or no CONFIG_EBPF -- the stub).
If the user requested EBPF by setting the property, we call
ebpf_rss_load_fds() via virtio_net_load_ebpf_fds().
If CONFIG_EBPF, this can fail. It sets an error then.
Else, it will fail, and it won't set an error then. Feels wrong.
Impact unclear. To find out I'd have to figure out how to configure
virtio-net to make it attempt to use EBPF.
> IMO if the normal implementation function sets some errp,
> then the stub must also set it ("feature not available").
> Otherwise such function shouldn't take an errp at all.
Convention: a function taking an @errp must set its @errp exactly when
it fails.
Does not apply when it abort()s.
I like unreachable stubs to abort().
ebpf_rss_load() and ebpf_rss_load_fds() are only called from
.get_features method virtio_net_get_features(). Reachability is not
obvious. If we assume they are reachable, we need to make the two stubs
set an error.
ebpf_rss_set_all() is only called from virtio_net_commit_rss_config()
via virtio_net_attach_ebpf_rss(). virtio_net_attach_ebpf_rss() passes
null @errp, i.e. the errors set by ebpf_rss_set_all() are all lost.
virtio_net_attach_ebpf_rss() emits a generic warning on failure. This
is crap. Better: virtio_net_attach_ebpf_rss() passes on the error, so
that virtio_net_commit_rss_config() can include cause in the warning.
If we do that, we also need to fix the stub to set an error.
If we don't do it, we can just as well drop ebpf_rss_set_all()'s @errp
parameter.
> Reasoning valid for:
> - ebpf_rss_load
> - ebpf_rss_load_fds
> - ebpf_rss_set_all
>
> As the name suggest, ebpf_rss_is_loaded() shouldn't be called
> when eBPF not available, because ebpf_rss_load() would return
> an error. Not reachable.
>
> Unfortunately ebpf_rss_init() doesn't return anything. "Feature
> available" and "Initialization successful" are different cases,
> so having it return a boolean isn't really helpful. I'd have the
> stub assert if reached, and check the feature availability upfront.
I'm not sure I follow.
> Declaring ebpf_available() in "ebpf/ebpf_rss.h" as:
>
> static inline bool ebpf_available(void)
> {
> #ifdef CONFIG_EBPF
> return true;
> #else
> return false;
> #endif
> }
>
> along with the prototypes, would allow the compiler to elide the callees
> when not available, removing the need for various stubs.
Only if we're willing to assume all compilers elide the calls even when
optimization is off.
>>> This scenario should never happen, and we should add a call like
>>>
>>> error_setg(errp, "eBPF program is already loaded");
>>>
>>> to report it correctly.
>>
>> Is it a programming error when it happens?
