[DTrace-devel] [PATCH] Add dt_bpf_map_lookup() function to retrieve values from BPF maps

[Kris Van Hees]

On Thu, Sep 03, 2020 at 11:01:52PM -0700, Eugene Loh wrote:
> Some context here would be good.  E.g., why is this being introduced?  
> Maybe that's problematic to answer since the patch that will use this 
> will depend on a lot of leg work that must first be set up, but a 
> unified patch set would be nice.  I won't insist on it, though.  So, not 
> ideal, but so be it.

There is no need to justify the patch, especially since the purpose is quite
obvious (we're adding dt_bpf_map_lookup() as a function) and it gets used in
the very next patch.

> But maybe at least say why the libbpf/bpf.c function 
> bpf_map_lookup_elem() is not being used?  Could it be?  Are you trying 
> to wean us off of libbpf?  But then that should be its own patch?

The decision to move away from using libbpf was made a long time ago already
so there isn't really any need to mention that in this patch.  That will be
obvious when a patch does remove it.

> But at the very least (looking ahead to the next patch), the movement of 
> dt_bpf_map_[lookup|update] in "Implement a 'state' BPF map to 
> communicate tracing session state" should be moved into this patch 
> rather than introducing the new function here in the "wrong" place only 
> to fix its location in the next patch.  Just get the placement right the 
> first time.  As it is, the function being introduced here is backed out 
> in the next patch.  Just put dt_bpf_map_lookup in the right place here, 
> moving dt_bpf_map_update while you're at it.  Those gymnastics do not 
> need to be in the next patch.

Wow - that's quite a paragraph to essentially just say "shouldn't those
functions be placed in the right place here instead of having the next patch
move them"?  To which I would answer: yes - I made a mistake generating the
patch (it should have included that move here).

> On 08/27/2020 11:54 AM, Kris Van Hees wrote:
> > Signed-off-by: Kris Van Hees <kris.van.hees at oracle.com>
> > ---
> >   libdtrace/dt_bpf.c | 15 +++++++++++++++
> >   libdtrace/dt_bpf.h |  1 +
> >   2 files changed, 16 insertions(+)
> >
> > diff --git a/libdtrace/dt_bpf.c b/libdtrace/dt_bpf.c
> > index d6d05dc8..0531a37e 100644
> > --- a/libdtrace/dt_bpf.c
> > +++ b/libdtrace/dt_bpf.c
> > @@ -185,6 +185,21 @@ dt_bpf_gmap_create(dtrace_hdl_t *dtp)
> >   	return 0;
> >   }
> >   
> > +/*
> > + * Load the value for the given key in the map referenced by the given fd.
> > + */
> > +int dt_bpf_map_lookup(int fd, const void *key, void *val)
> > +{
> > +	union bpf_attr attr;
> > +
> > +	memset(&attr, 0, sizeof(attr));
> > +	attr.map_fd = fd;
> > +	attr.key = (uint64_t)(unsigned long)key;
> > +	attr.value = (uint64_t)(unsigned long)val;
> > +
> > +	return bpf(BPF_MAP_LOOKUP_ELEM, &attr);
> > +}
> > +
> >   /*
> >    * Store the (key, value) pair in the map referenced by the given fd.
> >    */
> > diff --git a/libdtrace/dt_bpf.h b/libdtrace/dt_bpf.h
> > index c4a4a99c..e87d4653 100644
> > --- a/libdtrace/dt_bpf.h
> > +++ b/libdtrace/dt_bpf.h
> > @@ -23,6 +23,7 @@ extern int perf_event_open(struct perf_event_attr *attr, pid_t pid, int cpu,
> >   extern int bpf(enum bpf_cmd cmd, union bpf_attr *attr);
> >   
> >   extern int dt_bpf_gmap_create(dtrace_hdl_t *);
> > +extern int dt_bpf_map_lookup(int fd, const void *key, void *val);
> >   extern int dt_bpf_map_update(int fd, const void *key, const void *val);
> >   extern int dt_bpf_load_progs(dtrace_hdl_t *, uint_t);
> >   
> 
> 
> _______________________________________________
> DTrace-devel mailing list
> DTrace-devel at oss.oracle.com
> https://oss.oracle.com/mailman/listinfo/dtrace-devel