Re: [RFC] libperf: Add support for user space counter access

[Jiri Olsa]

On Tue, Aug 11, 2020 at 10:49:30AM -0600, Rob Herring wrote:
> On Tue, Aug 11, 2020 at 4:50 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> >
> > On Mon, Aug 10, 2020 at 12:11:23PM -0600, Rob Herring wrote:
> > > On Sat, Aug 8, 2020 at 4:22 AM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> > > >
> > > > On Fri, Aug 07, 2020 at 05:05:17PM -0600, Rob Herring wrote:
> > > > > x86 and arm64 can both support direct access of event counters in
> > > > > userspace. The access sequence is less than trivial and currently exists
> > > > > in perf test code (tools/perf/arch/x86/tests/rdpmc.c) with copies in
> > > > > projects such as PAPI and libpfm4.
> > > > >
> > > > > Patches to add arm64 userspace support are pending[1].
> > > > >
> > > > > For this RFC, looking for a yes, seems like a good idea, or no, go away we
> > > > > don't want this in libperf.
> > > >
> > > > hi,
> > > > looks great!
> > > >
> > > > I wanted to add this for very long time.. so yes, we want this ;-)
> > >
> > > Thanks for the quick feedback. Would this be better implemented as a
> > > fast path for perf_evsel__read()? If so, how to get the mmap data
> >
> > if it works for all events, which I'm not sure of
> >
> > > which is associated with a evlist rather than a evsel?
> >
> > not sure what you mean, you can mmap evsel, not evlist
> 
> While yes the mmap is created from an evsel fd, they are ultimately
> associated with the evlist struct and are per thread or cpu. If
> there's more than 1 evsel, then the additional ones are set to the 1st
> mmap with PERF_EVENT_IOC_SET_OUTPUT. Which I now realize means this
> RFC only works for the first evsel. So I guess the API needs to work
> something like this:
> 
> threads = perf_thread_map__new_dummy();
> perf_thread_map__set_pid(threads, 0, 0);
> 
> evsel = perf_evsel__new(&attr);
> perf_evsel__open(evsel, NULL, threads);
> 

hum, I wonder we should remove maps from perf_evsel__open
args and factor out some perf_evsel__set_map function..

> perf_evsel__mmap(evsel);    <--- *new*

.. because you'll need those maps in here, right?

> 
> perf_evsel__read(evsel, 0, 0, &counts);  // If we have an mmap, then
> try a direct read
> 
> 
> Perhaps some refactoring of the mmap code in evlist.c will be needed,
> but the usage seems pretty orthogonal. I'd propose that mmapping via
> perf_evlist__mmap() behavior remain unchanged and direct access is not
> supported in that case.

seems ok to me, perf_evlist__mmap and perf_evsel__mmap would
mean to mutually exclusive usages

thanks,
jirka