[PATCH v2 0/6] perf python binding fixes

[PATCH v2 0/6] perf python binding fixes

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Hi Namhyung,

        So this simplifies it greatly, it almost ends up as a one-liner,
but there is an improvement as well to mark the event as consumed to
then parse its sample, because the copy of the whole event was done all
along.

        This is brittle, as the header size can be bigger, than the
space we use and in that case we fail to parse the event by bounds
checking it.

	Supporting larger event payloads can be done on top of this,
possibly by deferring consuming the event in the ring buffer by parsing
it all instead of having pre-allocated space, measurements need to be
made to see whats best. I'd say leave this for when it proves necessary.

        With this series I managed to run it for a long time without
crashes and 'top' says it doesn't seem to be leaking anything, as its
memory usage stays the same for as long as I looked.

        Please consider applying to perf-tools-next,

Best regards

P.S.: In other news, the syscalltbl series from Ian built on all my
containers, I'm now trying to go over it patch by patch.

Arnaldo Carvalho de Melo (6):
  perf python: Fixup description of sample.id event member
  perf python: Remove some unused macros (_PyUnicode_FromString(arg), etc)
  perf python tracepoint.py: Change the COMM using setproctitle if available
  perf python: Decrement the refcount of just created event on failure
  perf python: Don't keep a raw_data pointer to consumed ring buffer space
  perf python: Check if there is space to copy all the event

 tools/perf/python/tracepoint.py |  8 ++++++++
 tools/perf/util/python.c        | 24 ++++++++++++------------
 2 files changed, 20 insertions(+), 12 deletions(-)

-- 
2.48.1

[PATCH 1/6] perf python: Fixup description of sample.id event member

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Some old cut'n'paste error, its "ip", so the description should be
"event ip", not "event type".

Fixes: 877108e42b1b9ba6 ("perf tools: Initial python binding")
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/python.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 69ec2ad60d98ba38..6c5bb5e8893998ae 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -49,7 +49,7 @@ struct pyrf_event {
 };
 
 #define sample_members \
-	sample_member_def(sample_ip, ip, T_ULONGLONG, "event type"),			 \
+	sample_member_def(sample_ip, ip, T_ULONGLONG, "event ip"),			 \
 	sample_member_def(sample_pid, pid, T_INT, "event pid"),			 \
 	sample_member_def(sample_tid, tid, T_INT, "event tid"),			 \
 	sample_member_def(sample_time, time, T_ULONGLONG, "event timestamp"),		 \
-- 
2.48.1

[PATCH 2/6] perf python: Remove some unused macros (_PyUnicode_FromString(arg), etc)

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

When python2 support was removed in e7e9943c87d857da ("perf python:
Remove python 2 scripting support"), all use of the
_PyUnicode_FromString(arg), _PyUnicode_FromFormat(...), and
_PyLong_FromLong(arg) macros was removed as well, so remove it.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/python.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 6c5bb5e8893998ae..f9491b6699764fbc 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -22,13 +22,6 @@
 #include "util/sample.h"
 #include <internal/lib.h>
 
-#define _PyUnicode_FromString(arg) \
-  PyUnicode_FromString(arg)
-#define _PyUnicode_FromFormat(...) \
-  PyUnicode_FromFormat(__VA_ARGS__)
-#define _PyLong_FromLong(arg) \
-  PyLong_FromLong(arg)
-
 PyMODINIT_FUNC PyInit_perf(void);
 
 #define member_def(type, member, ptype, help) \
-- 
2.48.1

[PATCH 3/6] perf python tracepoint.py: Change the COMM using setproctitle if available

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

Otherwise when debugging we see just "python" in perf, top, etc.

Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/python/tracepoint.py | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/python/tracepoint.py b/tools/perf/python/tracepoint.py
index 38b2b6d11f64566a..15b0c82689966f23 100755
--- a/tools/perf/python/tracepoint.py
+++ b/tools/perf/python/tracepoint.py
@@ -5,7 +5,15 @@
 
 import perf
 
+def change_proctitle():
+    try:
+        import setproctitle
+        setproctitle.setproctitle("tracepoint.py")
+    except:
+        print("Install the setproctitle python package to help with top and friends")
+
 def main():
+    change_proctitle()
     cpus    = perf.cpu_map()
     threads = perf.thread_map(-1)
     evlist = perf.parse_events("sched:sched_switch", cpus, threads)
-- 
2.48.1

[PATCH 4/6] perf python: Decrement the refcount of just created event on failure

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

To avoid a leak if we have the python object but then something happens
and we need to return the operation, decrement the offset of the newly
created object.

Fixes: 377f698db12150a1 ("perf python: Add struct evsel into struct pyrf_event")
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/python.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index f9491b6699764fbc..31a877a8eb8fbf09 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -1012,6 +1012,7 @@ static PyObject *pyrf_evlist__read_on_cpu(struct pyrf_evlist *pevlist,
 
 		evsel = evlist__event2evsel(evlist, event);
 		if (!evsel) {
+			Py_DECREF(pyevent);
 			Py_INCREF(Py_None);
 			return Py_None;
 		}
@@ -1023,9 +1024,12 @@ static PyObject *pyrf_evlist__read_on_cpu(struct pyrf_evlist *pevlist,
 		/* Consume the even only after we parsed it out. */
 		perf_mmap__consume(&md->core);
 
-		if (err)
+		if (err) {
+			Py_DECREF(pyevent);
 			return PyErr_Format(PyExc_OSError,
 					    "perf: can't parse sample, err=%d", err);
+		}
+
 		return pyevent;
 	}
 end:
-- 
2.48.1

[PATCH 5/6] perf python: Don't keep a raw_data pointer to consumed ring buffer space

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

When processing tracepoints the perf python binding was parsing the
event before calling perf_mmap__consume(&md->core) in
pyrf_evlist__read_on_cpu().

But part of this event parsing was to set the perf_sample->raw_data
pointer to the payload of the event, which then could be overwritten by
other event before tracepoint fields were asked for via event.prev_comm
in a python program, for instance.

This also happened with other fields, but strings were were problems
were surfacing, as there is UTF-8 validation for the potentially garbled
data.

This ended up showing up as (with some added debugging messages):

  ( field 'prev_comm' ret=0x7f7c31f65110, raw_size=68 )  ( field 'prev_pid' ret=0x7f7c23b1bed0, raw_size=68 )  ( field 'prev_prio' ret=0x7f7c239c0030, raw_size=68 )  ( field 'prev_state' ret=0x7f7c239c0250, raw_size=68 ) time 14771421785867 prev_comm= prev_pid=1919907691 prev_prio=796026219 prev_state=0x303a32313175 ==>
  ( XXX '��' len=16, raw_size=68)  ( field 'next_comm' ret=(nil), raw_size=68 ) Traceback (most recent call last):
   File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 51, in <module>
     main()
   File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 46, in main
     event.next_comm,
     ^^^^^^^^^^^^^^^
  AttributeError: 'perf.sample_event' object has no attribute 'next_comm'

When event.next_comm was asked for, the PyUnicode_FromString() python
API would fail and that tracepoint field wouldn't be available, stopping
the tools/perf/python/tracepoint.py test tool.

But, since we already do a copy of the whole event in pyrf_event__new,
just use it and while at it remove what was done in in e8968e654191390a
("perf python: Fix pyrf_evlist__read_on_cpu event consuming") because we
don't really need to wait for parsing the sample before declaring the
event as consumed.

This copy is questionable as is now, as it limits the maximum event +
sample_type and tracepoint payload to sizeof(union perf_event), this all
has been "working" because 'struct perf_event_mmap2', the largest entry
in 'union perf_event' is:

  $ pahole -C perf_event ~/bin/perf | grep mmap2
	struct perf_record_mmap2   mmap2;              /*     0  4168 */
  $

Fixes: bae57e3825a3dded ("perf python: Add support to resolve tracepoint fields")
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/python.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 31a877a8eb8fbf09..6a03341e17881337 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -1019,11 +1019,9 @@ static PyObject *pyrf_evlist__read_on_cpu(struct pyrf_evlist *pevlist,
 
 		pevent->evsel = evsel;
 
-		err = evsel__parse_sample(evsel, event, &pevent->sample);
-
-		/* Consume the even only after we parsed it out. */
 		perf_mmap__consume(&md->core);
 
+		err = evsel__parse_sample(evsel, &pevent->event, &pevent->sample);
 		if (err) {
 			Py_DECREF(pyevent);
 			return PyErr_Format(PyExc_OSError,
-- 
2.48.1

[PATCH 6/6] perf python: Check if there is space to copy all the event

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo <acme@redhat.com>

The pyrf_event__new() method copies the event obtained from the perf
ring buffer to a structure that will then be turned into a python object
for further consumption, so it copies perf_event.header.size bytes to
its 'event' member:

  $ pahole -C pyrf_event /tmp/build/perf-tools-next/python/perf.cpython-312-x86_64-linux-gnu.so
  struct pyrf_event {
  	PyObject                   ob_base;              /*     0    16 */
  	struct evsel *             evsel;                /*    16     8 */
  	struct perf_sample         sample;               /*    24   312 */

  	/* XXX last struct has 7 bytes of padding, 2 holes */

  	/* --- cacheline 5 boundary (320 bytes) was 16 bytes ago --- */
  	union perf_event           event;                /*   336  4168 */

  	/* size: 4504, cachelines: 71, members: 4 */
  	/* member types with holes: 1, total: 2 */
  	/* paddings: 1, sum paddings: 7 */
  	/* last cacheline: 24 bytes */
  };

  $

It was doing so without checking if the event just obtained has more
than that space, fix it.

This isn't a proper, final solution, as we need to support larger
events, but for the time being we at least bounds check and document it.

Fixes: 877108e42b1b9ba6 ("perf tools: Initial python binding")
Cc: Adrian Hunter <adrian.hunter@intel.com>
Cc: Ian Rogers <irogers@google.com>
Cc: James Clark <james.clark@linaro.org>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: Kan Liang <kan.liang@linux.intel.com>
Cc: Namhyung Kim <namhyung@kernel.org>
Signed-off-by: Arnaldo Carvalho de Melo <acme@redhat.com>
---
 tools/perf/util/python.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 6a03341e17881337..f3c05da25b4af8c0 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -476,6 +476,11 @@ static PyObject *pyrf_event__new(const union perf_event *event)
 	      event->header.type == PERF_RECORD_SWITCH_CPU_WIDE))
 		return NULL;
 
+	// FIXME this better be dynamic or we need to parse everything
+	// before calling perf_mmap__consume(), including tracepoint fields.
+	if (sizeof(pevent->event) < event->header.size)
+		return NULL;
+
 	ptype = pyrf_event__type[event->header.type];
 	pevent = PyObject_New(struct pyrf_event, ptype);
 	if (pevent != NULL)
-- 
2.48.1

Re: [PATCH v2 0/6] perf python binding fixes

[Ian Rogers]

On Wed, Mar 12, 2025 at 1:31 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
>
> Hi Namhyung,
>
>         So this simplifies it greatly, it almost ends up as a one-liner,
> but there is an improvement as well to mark the event as consumed to
> then parse its sample, because the copy of the whole event was done all
> along.
>
>         This is brittle, as the header size can be bigger, than the
> space we use and in that case we fail to parse the event by bounds
> checking it.
>
>         Supporting larger event payloads can be done on top of this,
> possibly by deferring consuming the event in the ring buffer by parsing
> it all instead of having pre-allocated space, measurements need to be
> made to see whats best. I'd say leave this for when it proves necessary.
>
>         With this series I managed to run it for a long time without
> crashes and 'top' says it doesn't seem to be leaking anything, as its
> memory usage stays the same for as long as I looked.
>
>         Please consider applying to perf-tools-next,
>
> Best regards
>
> P.S.: In other news, the syscalltbl series from Ian built on all my
> containers, I'm now trying to go over it patch by patch.
>
> Arnaldo Carvalho de Melo (6):
>   perf python: Fixup description of sample.id event member
>   perf python: Remove some unused macros (_PyUnicode_FromString(arg), etc)
>   perf python tracepoint.py: Change the COMM using setproctitle if available
>   perf python: Decrement the refcount of just created event on failure
>   perf python: Don't keep a raw_data pointer to consumed ring buffer space
>   perf python: Check if there is space to copy all the event

Reviewed-by: Ian Rogers <irogers@google.com>

Thanks,
Ian

Re: [PATCH v2 0/6] perf python binding fixes

[Arnaldo Carvalho de Melo]

On Mon, Mar 17, 2025 at 10:41:28AM -0700, Ian Rogers wrote:
> On Wed, Mar 12, 2025 at 1:31 PM Arnaldo Carvalho de Melo
> > P.S.: In other news, the syscalltbl series from Ian built on all my
> > containers, I'm now trying to go over it patch by patch.

Doing some more tests on it, the -e case is fixed with Namhyung's fix
for re-reading the sc variable after the realloc.

> > Arnaldo Carvalho de Melo (6):
> >   perf python: Fixup description of sample.id event member
> >   perf python: Remove some unused macros (_PyUnicode_FromString(arg), etc)
> >   perf python tracepoint.py: Change the COMM using setproctitle if available
> >   perf python: Decrement the refcount of just created event on failure
> >   perf python: Don't keep a raw_data pointer to consumed ring buffer space
> >   perf python: Check if there is space to copy all the event
 
> Reviewed-by: Ian Rogers <irogers@google.com>

Thanks!

- Arnaldo

Re: [PATCH v2 0/6] perf python binding fixes

[Namhyung Kim]

On Wed, 12 Mar 2025 17:31:35 -0300, Arnaldo Carvalho de Melo wrote:
> From: Arnaldo Carvalho de Melo <acme@redhat.com>
> 
> Hi Namhyung,
> 
>         So this simplifies it greatly, it almost ends up as a one-liner,
> but there is an improvement as well to mark the event as consumed to
> then parse its sample, because the copy of the whole event was done all
> along.
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung