Re: [PATCH] libperf: Add perf_evsel__id() function

[Charlie Jenkins]

On Mon, Sep 09, 2024 at 03:26:08PM -0700, Namhyung Kim wrote:
> On Thu, Sep 05, 2024 at 11:59:37AM -0700, Charlie Jenkins wrote:
> > On Wed, Sep 04, 2024 at 09:22:33PM -0700, Namhyung Kim wrote:
> > > Hello,
> > > 
> > > On Fri, Aug 23, 2024 at 11:58:38AM -0700, Charlie Jenkins wrote:
> > > > Introduce perf_evsel__id() to collect the id of an evsel. It is not
> > > > currently possible when using libperf to determine the id of an evsel.
> > > > This will allow applications to link the id returned by PERF_SAMPLE_ID
> > > > to the event being sampled.
> > > 
> > > I'm not sure what's your use case.  We have evlist__id2evsel() to
> > > convert sample-ID to evsel already.  It'd read the IDs from perf data
> > > file.
> > 
> > That function is unfortunately not exposed to be used by applications.
> > This patch is for libperf, not for the util internals of userspace perf.
> > Since I am using libperf (and not the python wrapper) I am collecting
> > data directly from the mmap buffer and not from the perf data file. The
> > mmap buffer only contains the id of the sample, and no way of
> > associating this id with the evsel that is backing the id using only
> > libperf API functions. A libperf function that provides a pointer to the
> > associated evsel could be another alternative to this. However an API
> > that returns a pointer to the evsel is not very useful as libperf
> > applications cannot read members of perf structs, as the layout of perf
> > structs are not part of the API.
> 
> Right, thanks for sharing your use case.  The libperf lacks the API to
> map event ID to evsel.  And perf tools access the internal data struct
> in the libperf which is not good.  But making it a proper library will
> take a lot more time.
> 
> > 
> > > 
> > > > 
> > > > Signed-off-by: Charlie Jenkins <charlie@xxxxxxxxxxxx>
> > > > ---
> > > >  tools/lib/perf/Documentation/libperf.txt |  2 ++
> > > >  tools/lib/perf/evsel.c                   | 10 ++++++++++
> > > >  tools/lib/perf/include/perf/evsel.h      |  1 +
> > > >  3 files changed, 13 insertions(+)
> > > > 
> > > > diff --git a/tools/lib/perf/Documentation/libperf.txt b/tools/lib/perf/Documentation/libperf.txt
> > > > index fcfb9499ef9c..69c1d7efb659 100644
> > > > --- a/tools/lib/perf/Documentation/libperf.txt
> > > > +++ b/tools/lib/perf/Documentation/libperf.txt
> > > > @@ -94,6 +94,8 @@ SYNOPSIS
> > > >    void perf_evlist__enable(struct perf_evlist *evlist);
> > > >    void perf_evlist__disable(struct perf_evlist *evlist);
> > > >  
> > > > +  void perf_evsel__id(struct perf_evsel *evsel, u64 *id);
> > > > +
> > > >    #define perf_evlist__for_each_evsel(evlist, pos)
> > > >  
> > > >    void perf_evlist__set_maps(struct perf_evlist *evlist,
> > > > diff --git a/tools/lib/perf/evsel.c b/tools/lib/perf/evsel.c
> > > > index c07160953224..765b17045342 100644
> > > > --- a/tools/lib/perf/evsel.c
> > > > +++ b/tools/lib/perf/evsel.c
> > > > @@ -484,6 +484,16 @@ int perf_evsel__disable(struct perf_evsel *evsel)
> > > >  	return err;
> > > >  }
> > > >  
> > > > +int perf_evsel__id(struct perf_evsel *evsel, __u64 *id)
> > > > +{
> > > > +	int i;
> > > > +	int err = 0;
> > > > +
> > > > +	for (i = 0; i < xyarray__max_x(evsel->fd) && !err; i++)
> > > > +		err = perf_evsel__run_ioctl(evsel, PERF_EVENT_IOC_ID, (unsigned long)id, i);
> > > 
> > > IIUC this would update the place pointed by 'id' for every fd then you
> > > will get the last ID only.
> > 
> > Yes I have a misunderstanding with how this is supposed to be written.
> > Can there be multiple ids associated with a given evsel? When parsing
> > the data in the mmap buffer, I want to be able to associate a sample id
> > with the event encoding. I suppose an API to get the evsel from an id,
> > and then require libperf applications to search some data structure to
> > get the associated event with the evsel is reasonable.
> 
> Yes an evsel can have multiple IDs as it can have multiple FDs.  The
> evsel is an abstraction to keep related events together and they are
> actually opened for each CPU and thread, hence the xyarray.  Each opened
> event (for the evsel) has its own ID.

I see, that makes sense.

> 
> You will need an array of a same size of file descriptors to retrieve
> the ID info and also a hash table to map ID to evsel.

I will extend this to be able to support returning an id for every open
FD.

Right now in my application I have a hash table to map the sample ID
back to the evsel.

- Charlie

> 
> Thanks,
> Namhyung
> 
> > > 
> > > > +	return err;
> > > > +}
> > > > +
> > > >  int perf_evsel__apply_filter(struct perf_evsel *evsel, const char *filter)
> > > >  {
> > > >  	int err = 0, i;
> > > > diff --git a/tools/lib/perf/include/perf/evsel.h b/tools/lib/perf/include/perf/evsel.h
> > > > index 6f92204075c2..1457e5a46b28 100644
> > > > --- a/tools/lib/perf/include/perf/evsel.h
> > > > +++ b/tools/lib/perf/include/perf/evsel.h
> > > > @@ -41,6 +41,7 @@ LIBPERF_API int perf_evsel__enable_cpu(struct perf_evsel *evsel, int cpu_map_idx
> > > >  LIBPERF_API int perf_evsel__enable_thread(struct perf_evsel *evsel, int thread);
> > > >  LIBPERF_API int perf_evsel__disable(struct perf_evsel *evsel);
> > > >  LIBPERF_API int perf_evsel__disable_cpu(struct perf_evsel *evsel, int cpu_map_idx);
> > > > +LIBPERF_API int perf_evsel__id(struct perf_evsel *evsel, __u64 *id);
> > > >  LIBPERF_API struct perf_cpu_map *perf_evsel__cpus(struct perf_evsel *evsel);
> > > >  LIBPERF_API struct perf_thread_map *perf_evsel__threads(struct perf_evsel *evsel);
> > > >  LIBPERF_API struct perf_event_attr *perf_evsel__attr(struct perf_evsel *evsel);
> > > > 
> > > > ---
> > > > base-commit: 47ac09b91befbb6a235ab620c32af719f8208399
> > > > change-id: 20240822-perf_evsel_get_id-f7e11f15504b
> > > > -- 
> > > > - Charlie
> > > >