Re: [PATCH 3/4] perf python: Add evlist close and next methods

[Gautam Menghani]

On Thu, May 01, 2025 at 08:49:08AM -0700, Ian Rogers wrote:
> On Thu, May 1, 2025 at 2:37 AM Gautam Menghani <gautam@xxxxxxxxxxxxx> wrote:
> >
> > Add support for the evlist close and next methods. The next method
> > enables iterating over the evsels in an evlist.
> >
> > Signed-off-by: Gautam Menghani <gautam@xxxxxxxxxxxxx>
> > ---
> >  tools/perf/util/python.c | 47 ++++++++++++++++++++++++++++++++++++++++
> >  1 file changed, 47 insertions(+)
> >
> > diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> > index 5a4d2c9aaabd..599cb37600f1 100644
> > --- a/tools/perf/util/python.c
> > +++ b/tools/perf/util/python.c
> > @@ -1163,6 +1163,16 @@ static PyObject *pyrf_evlist__open(struct pyrf_evlist *pevlist,
> >         return Py_None;
> >  }
> >
> > +static PyObject *pyrf_evlist__close(struct pyrf_evlist *pevlist)
> > +{
> > +       struct evlist *evlist = &pevlist->evlist;
> > +
> > +       evlist__close(evlist);
> > +
> > +       Py_INCREF(Py_None);
> > +       return Py_None;
> > +}
> > +
> >  static PyObject *pyrf_evlist__config(struct pyrf_evlist *pevlist)
> >  {
> >         struct record_opts opts = {
> > @@ -1202,6 +1212,31 @@ static PyObject *pyrf_evlist__enable(struct pyrf_evlist *pevlist)
> >         return Py_None;
> >  }
> >
> > +static PyObject *pyrf_evlist__next(struct pyrf_evlist *pevlist,
> > +                                  PyObject *args, PyObject *kwargs)
> > +{
> > +       struct evlist *evlist = &pevlist->evlist;
> > +       PyObject *py_evsel;
> > +       struct perf_evsel *pevsel;
> > +       struct evsel *evsel;
> > +       struct pyrf_evsel *next_evsel = PyObject_New(struct pyrf_evsel, &pyrf_evsel__type);
> > +       static char *kwlist[] = { "evsel", NULL };
> > +
> > +       if (!PyArg_ParseTupleAndKeywords(args, kwargs, "O", kwlist,
> > +                                        &py_evsel))
> > +               return NULL;
> > +
> > +       pevsel = (py_evsel == Py_None) ? NULL : &(((struct pyrf_evsel *)py_evsel)->evsel.core);
> > +       pevsel = perf_evlist__next(&(evlist->core), pevsel);
> > +       if (pevsel != NULL) {
> > +               evsel = container_of(pevsel, struct evsel, core);
> > +               next_evsel = container_of(evsel, struct pyrf_evsel, evsel);
> > +               return (PyObject *) next_evsel;
> > +       }
> > +
> > +       return Py_None;
> > +}
> > +
> 
> Thanks for this! Have you looked at the existing iteration support?
> There's an example here:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/python/tracepoint.py?h=perf-tools-next#n26
> ```
>     for ev in evlist:
>         ev.sample_type = ev.sample_type & ~perf.SAMPLE_IP
>         ev.read_format = 0
> ```
> In the next patch you have:
> ```
>         evsel = evlist.next(None)
>         while evsel != None:
>             counts = evsel.read(0, 0)
>             print(counts.val, counts.ena, counts.run)
>             evsel = evlist.next(evsel)
> ```
> I believe the former looks better. It also isn't clear to me if next
> belongs on evlist or evsel.

Yes, the existing support would be the right way, I missed that. Will fix in
v2.

and regarding the next() function, I think we should keep it for evlist
because for the C code it's defined in the context of evlist, so would
avoid confusion. But since it is not needed for the iteration, should
I keep it in v2?

Thanks,
Gautam