On Wed, Aug 07, 2024 at 09:37:00AM +0200, Philippe Mathieu-Daudé wrote:
> On 6/8/24 17:20, Daniel P. Berrangé wrote:
> > On Tue, Aug 06, 2024 at 05:11:55PM +0200, Philippe Mathieu-Daudé wrote:
> > > On 6/8/24 16:56, Daniel P. Berrangé wrote:
> > > > The eBPF code is currently reporting error messages through trace
> > > > events. Trace events are fine for debugging, but they are not to be
> > > > considered the primary error reporting mechanism, as their output
> > > > is inaccessible to callers.
> > > >
> > > > This adds an "Error **errp" parameter to all methods which have
> > > > important error scenarios to report to the caller.
> > > >
> > > > Signed-off-by: Daniel P. Berrangé <berra...@redhat.com>
> > > > ---
> > > > ebpf/ebpf_rss.c | 59
> > > > ++++++++++++++++++++++++++++++++++++---------
> > > > ebpf/ebpf_rss.h | 10 +++++---
> > > > hw/net/virtio-net.c | 7 +++---
> > > > 3 files changed, 59 insertions(+), 17 deletions(-)
> > > >
> > > > diff --git a/ebpf/ebpf_rss.c b/ebpf/ebpf_rss.c
> > > > index aa7170d997..59854c8b51 100644
> > > > --- a/ebpf/ebpf_rss.c
> > > > +++ b/ebpf/ebpf_rss.c
> > > > @@ -47,13 +47,14 @@ bool ebpf_rss_is_loaded(struct EBPFRSSContext *ctx)
> > > > return ctx != NULL && (ctx->obj != NULL || ctx->program_fd !=
> > > > -1);
> > > > }
> > > > -static bool ebpf_rss_mmap(struct EBPFRSSContext *ctx)
> > > > +static bool ebpf_rss_mmap(struct EBPFRSSContext *ctx, Error **errp)
> > > > {
> > > > ctx->mmap_configuration = mmap(NULL, qemu_real_host_page_size(),
> > > > PROT_READ | PROT_WRITE,
> > > > MAP_SHARED,
> > > > ctx->map_configuration, 0);
> > > > if (ctx->mmap_configuration == MAP_FAILED) {
> > > > trace_ebpf_error("eBPF RSS", "can not mmap eBPF
> > > > configuration array");
> > > > + error_setg(errp, "Unable to map eBPF configuration array");
> > > > return false;
> > > > }
> > > > ctx->mmap_toeplitz_key = mmap(NULL, qemu_real_host_page_size(),
> > > > @@ -61,6 +62,7 @@ static bool ebpf_rss_mmap(struct EBPFRSSContext *ctx)
> > > > ctx->map_toeplitz_key, 0);
> > > > if (ctx->mmap_toeplitz_key == MAP_FAILED) {
> > > > trace_ebpf_error("eBPF RSS", "can not mmap eBPF toeplitz
> > > > key");
> > > > + error_setg(errp, "Unable to map eBPF toeplitz array");
> > > > goto toeplitz_fail;
> > > > }
> > > > ctx->mmap_indirections_table = mmap(NULL,
> > > > qemu_real_host_page_size(),
> > > > @@ -68,6 +70,7 @@ static bool ebpf_rss_mmap(struct EBPFRSSContext *ctx)
> > > > ctx->map_indirections_table, 0);
> > > > if (ctx->mmap_indirections_table == MAP_FAILED) {
> > > > trace_ebpf_error("eBPF RSS", "can not mmap eBPF indirection
> > > > table");
> > > > + error_setg(errp, "Unable to map eBPF indirection array");
> > >
> > > Aren't these trace_ebpf_error() calls redundant now?
> >
> > Yes & no. Errors propagated up the call stack don't get included in
> > any trace output, and so if a caller doesn't log them anywhere they
> > can thus be invisible to someone just looking at trace output.
> >
> > I could remove them all from the eBPF code though, and put a single
> > trace event in the hw/net/virtio-net.c file instead ? Bit of a bike
> > shed colouring exercise to decide which is best though.
>
> No problem, I'm fine with this patch.
>
> Note from experience (although pre-existing in this patch), trace
> events can be very verbose, and a what makes them powerful is we
> can filter particular ones. The following pattern isn't practical
> to filter:
>
> trace_foo_error(const char *error_msg);
>
> While a bit tedious to add, having a single trace event per error
> is way more useful.
Yeah, I agree with you on that. I should look at changing it to add
distinct error probes.
With regards,
Daniel
--
|: https://berrange.com -o- https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org -o- https://fstop138.berrange.com :|
|: https://entangle-photo.org -o- https://www.instagram.com/dberrange :|
