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

Gautam Menghani posted 4 patches 9 months, 1 week ago
There is a newer version of this series
[PATCH 3/4] perf python: Add evlist close and next methods
Posted by Gautam Menghani 9 months, 1 week ago
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@linux.ibm.com>
---
 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;
+}
+
 static PyMethodDef pyrf_evlist__methods[] = {
 	{
 		.ml_name  = "all_cpus",
@@ -1221,6 +1256,12 @@ static PyMethodDef pyrf_evlist__methods[] = {
 		.ml_flags = METH_VARARGS | METH_KEYWORDS,
 		.ml_doc	  = PyDoc_STR("open the file descriptors.")
 	},
+	{
+		.ml_name  = "close",
+		.ml_meth  = (PyCFunction)pyrf_evlist__close,
+		.ml_flags = METH_NOARGS,
+		.ml_doc	  = PyDoc_STR("close the file descriptors.")
+	},
 	{
 		.ml_name  = "poll",
 		.ml_meth  = (PyCFunction)pyrf_evlist__poll,
@@ -1263,6 +1304,12 @@ static PyMethodDef pyrf_evlist__methods[] = {
 		.ml_flags = METH_NOARGS,
 		.ml_doc	  = PyDoc_STR("Enable the evsels in the evlist.")
 	},
+	{
+		.ml_name  = "next",
+		.ml_meth  = (PyCFunction)pyrf_evlist__next,
+		.ml_flags = METH_VARARGS | METH_KEYWORDS,
+		.ml_doc	  = PyDoc_STR("Return next evsel")
+	},
 	{ .ml_name = NULL, }
 };
 
-- 
2.49.0
Re: [PATCH 3/4] perf python: Add evlist close and next methods
Posted by Ian Rogers 9 months, 1 week ago
On Thu, May 1, 2025 at 2:37 AM Gautam Menghani <gautam@linux.ibm.com> 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@linux.ibm.com>
> ---
>  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.

Thanks,
Ian


>  static PyMethodDef pyrf_evlist__methods[] = {
>         {
>                 .ml_name  = "all_cpus",
> @@ -1221,6 +1256,12 @@ static PyMethodDef pyrf_evlist__methods[] = {
>                 .ml_flags = METH_VARARGS | METH_KEYWORDS,
>                 .ml_doc   = PyDoc_STR("open the file descriptors.")
>         },
> +       {
> +               .ml_name  = "close",
> +               .ml_meth  = (PyCFunction)pyrf_evlist__close,
> +               .ml_flags = METH_NOARGS,
> +               .ml_doc   = PyDoc_STR("close the file descriptors.")
> +       },
>         {
>                 .ml_name  = "poll",
>                 .ml_meth  = (PyCFunction)pyrf_evlist__poll,
> @@ -1263,6 +1304,12 @@ static PyMethodDef pyrf_evlist__methods[] = {
>                 .ml_flags = METH_NOARGS,
>                 .ml_doc   = PyDoc_STR("Enable the evsels in the evlist.")
>         },
> +       {
> +               .ml_name  = "next",
> +               .ml_meth  = (PyCFunction)pyrf_evlist__next,
> +               .ml_flags = METH_VARARGS | METH_KEYWORDS,
> +               .ml_doc   = PyDoc_STR("Return next evsel")
> +       },
>         { .ml_name = NULL, }
>  };
>
> --
> 2.49.0
>
Re: [PATCH 3/4] perf python: Add evlist close and next methods
Posted by Gautam Menghani 9 months, 1 week ago
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@linux.ibm.com> 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@linux.ibm.com>
> > ---
> >  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
Re: [PATCH 3/4] perf python: Add evlist close and next methods
Posted by Ian Rogers 9 months, 1 week ago
On Mon, May 5, 2025 at 4:49 AM Gautam Menghani <gautam@linux.ibm.com> wrote:
>
> 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@linux.ibm.com> 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@linux.ibm.com>
> > > ---
> > >  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?

So the perf code is following the kernel style and using invasive data
structures a lot. The kernel does this as memory allocation is a pain
and can fail causing complicated error paths. It gets kind of crazy,
if you look at perf_event in the kernel it has like 11 invasive data
structures:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/include/linux/perf_event.h?h=perf-tools-next#n706
```
struct perf_event {
struct list_head event_entry;
struct list_head sibling_list;
struct list_head active_list;
struct rb_node group_node;
struct list_head migrate_entry;
struct hlist_node hlist_entry;
struct list_head active_entry;
struct list_head child_list;
struct list_head owner_entry;
struct list_head rb_entry;
struct list_head sb_list;
};
```
Managed languages like Python don't tend to use invasive data
structures and I'm not sure exposing next in this way makes sense. We
may want to use an array for evlist in the future, which we've done in
the past to make reference count accounting easier, for example:
https://lore.kernel.org/r/20240210031746.4057262-2-irogers@google.com
But if next is exposed then we'd need to support that for scripts that
may be using it. I think it is easier to avoid the problem by just not
adding the function.

Thanks,
Ian

> Thanks,
> Gautam