[PATCH v2 00/11] Python improvements for a real use of parse_events

[PATCH v2 00/11] Python improvements for a real use of parse_events

[Ian Rogers]

While parse_events access in python was added, it wasn't used by any
python script. In enabling this for the tracepoint.py script a number
of latent bugs and necessary improvements were discovered.

v2: Fix a whitespace issue on the evlist.config patch spotted by
    Howard and add his reviewed-by tags.

Ian Rogers (11):
  perf debug: Avoid stack overflow in recursive error message
  perf evlist: Add success path to evlist__create_syswide_maps
  perf evsel: tp_format accessing improvements
  perf python: Add evlist enable and disable methods
  perf python: Add member access to a number of evsel variables
  perf python: Add optional cpus and threads arguments to parse_events
  perf python: Update ungrouped evsel leader in clone
  perf python: Avoid duplicated code in get_tracepoint_field
  perf python: Add evlist all_cpus accessor
  perf python: Add evlist.config to set up record options
  perf python tracepoint: Switch to using parse_events

 tools/perf/python/tracepoint.py |  23 +++---
 tools/perf/util/debug.c         |   2 +-
 tools/perf/util/evlist.c        |  13 ++--
 tools/perf/util/evsel.c         |  16 +++-
 tools/perf/util/python.c        | 127 ++++++++++++++++++++++++++++----
 5 files changed, 145 insertions(+), 36 deletions(-)

-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 01/11] perf debug: Avoid stack overflow in recursive error message

[Ian Rogers]

In debug_file, pr_warning_once is called on error. As that function
calls debug_file the function will yield a stack overflow. Switch the
location of the call so the recursion is avoided.

Reviewed-by: Howard Chu <howardchu95@gmail.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/debug.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/debug.c b/tools/perf/util/debug.c
index 995f6bb05b5f..f9ef7d045c92 100644
--- a/tools/perf/util/debug.c
+++ b/tools/perf/util/debug.c
@@ -46,8 +46,8 @@ int debug_type_profile;
 FILE *debug_file(void)
 {
 	if (!_debug_file) {
-		pr_warning_once("debug_file not set");
 		debug_set_file(stderr);
+		pr_warning_once("debug_file not set");
 	}
 	return _debug_file;
 }
-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 02/11] perf evlist: Add success path to evlist__create_syswide_maps

[Ian Rogers]

Over various refactorings evlist__create_syswide_maps has been made to
only ever return with -ENOMEM. Fix this so that when
perf_evlist__set_maps is successfully called, 0 is returned.

Reviewed-by: Howard Chu <howardchu95@gmail.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evlist.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
index f0dd174e2deb..633df7d9204c 100644
--- a/tools/perf/util/evlist.c
+++ b/tools/perf/util/evlist.c
@@ -1373,19 +1373,18 @@ static int evlist__create_syswide_maps(struct evlist *evlist)
 	 */
 	cpus = perf_cpu_map__new_online_cpus();
 	if (!cpus)
-		goto out;
+		return -ENOMEM;
 
 	threads = perf_thread_map__new_dummy();
-	if (!threads)
-		goto out_put;
+	if (!threads) {
+		perf_cpu_map__put(cpus);
+		return -ENOMEM;
+	}
 
 	perf_evlist__set_maps(&evlist->core, cpus, threads);
-
 	perf_thread_map__put(threads);
-out_put:
 	perf_cpu_map__put(cpus);
-out:
-	return -ENOMEM;
+	return 0;
 }
 
 int evlist__open(struct evlist *evlist)
-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 03/11] perf evsel: tp_format accessing improvements

[Ian Rogers]

Ensure evsel__clone copies the tp_sys and tp_name variables.
In evsel__tp_format, if tp_sys isn't set, use the config value to find
the tp_format. This succeeds in python code where pyrf__tracepoint has
already found the format.

Reviewed-by: Howard Chu <howardchu95@gmail.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/evsel.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 4a0ef095db92..1974395492d7 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -521,6 +521,16 @@ struct evsel *evsel__clone(struct evsel *dest, struct evsel *orig)
 	}
 	evsel->cgrp = cgroup__get(orig->cgrp);
 #ifdef HAVE_LIBTRACEEVENT
+	if (orig->tp_sys) {
+		evsel->tp_sys = strdup(orig->tp_sys);
+		if (evsel->tp_sys == NULL)
+			goto out_err;
+	}
+	if (orig->tp_name) {
+		evsel->tp_name = strdup(orig->tp_name);
+		if (evsel->tp_name == NULL)
+			goto out_err;
+	}
 	evsel->tp_format = orig->tp_format;
 #endif
 	evsel->handler = orig->handler;
@@ -644,7 +654,11 @@ struct tep_event *evsel__tp_format(struct evsel *evsel)
 	if (evsel->core.attr.type != PERF_TYPE_TRACEPOINT)
 		return NULL;
 
-	tp_format = trace_event__tp_format(evsel->tp_sys, evsel->tp_name);
+	if (!evsel->tp_sys)
+		tp_format = trace_event__tp_format_id(evsel->core.attr.config);
+	else
+		tp_format = trace_event__tp_format(evsel->tp_sys, evsel->tp_name);
+
 	if (IS_ERR(tp_format)) {
 		int err = -PTR_ERR(evsel->tp_format);
 
-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 04/11] perf python: Add evlist enable and disable methods

[Ian Rogers]

By default the evsels from parse_events will be disabled. Add access
to the evlist functions so they can be enabled/disabled.

Reviewed-by: Howard Chu <howardchu95@gmail.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 26 ++++++++++++++++++++++++++
 1 file changed, 26 insertions(+)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index e2b9032c1311..0cf81cfcfafb 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -1028,6 +1028,20 @@ static PyObject *pyrf_evlist__open(struct pyrf_evlist *pevlist,
 	return Py_None;
 }
 
+static PyObject *pyrf_evlist__disable(struct pyrf_evlist *pevlist)
+{
+	evlist__disable(&pevlist->evlist);
+	Py_INCREF(Py_None);
+	return Py_None;
+}
+
+static PyObject *pyrf_evlist__enable(struct pyrf_evlist *pevlist)
+{
+	evlist__enable(&pevlist->evlist);
+	Py_INCREF(Py_None);
+	return Py_None;
+}
+
 static PyMethodDef pyrf_evlist__methods[] = {
 	{
 		.ml_name  = "mmap",
@@ -1065,6 +1079,18 @@ static PyMethodDef pyrf_evlist__methods[] = {
 		.ml_flags = METH_VARARGS | METH_KEYWORDS,
 		.ml_doc	  = PyDoc_STR("reads an event.")
 	},
+	{
+		.ml_name  = "disable",
+		.ml_meth  = (PyCFunction)pyrf_evlist__disable,
+		.ml_flags = METH_NOARGS,
+		.ml_doc	  = PyDoc_STR("Disable the evsels in the evlist.")
+	},
+	{
+		.ml_name  = "enable",
+		.ml_meth  = (PyCFunction)pyrf_evlist__enable,
+		.ml_flags = METH_NOARGS,
+		.ml_doc	  = PyDoc_STR("Enable the evsels in the evlist.")
+	},
 	{ .ml_name = NULL, }
 };
 
-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 05/11] perf python: Add member access to a number of evsel variables

[Ian Rogers]

Most variables are part of the perf_event_attr, so that they may be
queried and modified.

Reviewed-by: Howard Chu <howardchu95@gmail.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 0cf81cfcfafb..b600b6379b4e 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -811,6 +811,28 @@ static PyMethodDef pyrf_evsel__methods[] = {
 	{ .ml_name = NULL, }
 };
 
+#define evsel_member_def(member, ptype, help) \
+	{ #member, ptype, \
+	  offsetof(struct pyrf_evsel, evsel.member), \
+	  0, help }
+
+#define evsel_attr_member_def(member, ptype, help) \
+	{ #member, ptype, \
+	  offsetof(struct pyrf_evsel, evsel.core.attr.member), \
+	  0, help }
+
+static PyMemberDef pyrf_evsel__members[] = {
+	evsel_member_def(tracking, T_BOOL, "tracking event."),
+	evsel_attr_member_def(type, T_UINT, "attribute type."),
+	evsel_attr_member_def(size, T_UINT, "attribute size."),
+	evsel_attr_member_def(config, T_ULONGLONG, "attribute config."),
+	evsel_attr_member_def(sample_period, T_ULONGLONG, "attribute sample_period."),
+	evsel_attr_member_def(sample_type, T_ULONGLONG, "attribute sample_type."),
+	evsel_attr_member_def(read_format, T_ULONGLONG, "attribute read_format."),
+	evsel_attr_member_def(wakeup_events, T_UINT, "attribute wakeup_events."),
+	{ .name = NULL, },
+};
+
 static const char pyrf_evsel__doc[] = PyDoc_STR("perf event selector list object.");
 
 static PyTypeObject pyrf_evsel__type = {
@@ -820,6 +842,7 @@ static PyTypeObject pyrf_evsel__type = {
 	.tp_dealloc	= (destructor)pyrf_evsel__delete,
 	.tp_flags	= Py_TPFLAGS_DEFAULT|Py_TPFLAGS_BASETYPE,
 	.tp_doc		= pyrf_evsel__doc,
+	.tp_members	= pyrf_evsel__members,
 	.tp_methods	= pyrf_evsel__methods,
 	.tp_init	= (initproc)pyrf_evsel__init,
 	.tp_str         = pyrf_evsel__str,
-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 06/11] perf python: Add optional cpus and threads arguments to parse_events

[Ian Rogers]

Used for the evlist initialization.

Reviewed-by: Howard Chu <howardchu95@gmail.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index b600b6379b4e..4a3015e7dc83 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -1339,12 +1339,18 @@ static PyObject *pyrf__parse_events(PyObject *self, PyObject *args)
 	struct evlist evlist = {};
 	struct parse_events_error err;
 	PyObject *result;
+	PyObject *pcpus = NULL, *pthreads = NULL;
+	struct perf_cpu_map *cpus;
+	struct perf_thread_map *threads;
 
-	if (!PyArg_ParseTuple(args, "s", &input))
+	if (!PyArg_ParseTuple(args, "s|OO", &input, &pcpus, &pthreads))
 		return NULL;
 
+	threads = pthreads ? ((struct pyrf_thread_map *)pthreads)->threads : NULL;
+	cpus = pcpus ? ((struct pyrf_cpu_map *)pcpus)->cpus : NULL;
+
 	parse_events_error__init(&err);
-	evlist__init(&evlist, NULL, NULL);
+	evlist__init(&evlist, cpus, threads);
 	if (parse_events(&evlist, input, &err)) {
 		parse_events_error__print(&err, input);
 		PyErr_SetFromErrno(PyExc_OSError);
-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 07/11] perf python: Update ungrouped evsel leader in clone

[Ian Rogers]

evsels are cloned in the python code as they form part of the Python
object pyrf_evsel. The cloning doesn't update the evsel's leader, do
this for the case of an evsel being ungrouped.

Reviewed-by: Howard Chu <howardchu95@gmail.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 4a3015e7dc83..e244cc74f16d 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -1312,6 +1312,8 @@ static PyObject *pyrf_evsel__from_evsel(struct evsel *evsel)
 	evsel__init(&pevsel->evsel, &evsel->core.attr, evsel->core.idx);
 
 	evsel__clone(&pevsel->evsel, evsel);
+	if (evsel__is_group_leader(evsel))
+		evsel__set_leader(&pevsel->evsel, &pevsel->evsel);
 	return (PyObject *)pevsel;
 }
 
-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 08/11] perf python: Avoid duplicated code in get_tracepoint_field

[Ian Rogers]

The code replicates computations done in evsel__tp_format, reuse
evsel__tp_format to simplify the python C code.

Reviewed-by: Howard Chu <howardchu95@gmail.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 17 ++++-------------
 1 file changed, 4 insertions(+), 13 deletions(-)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index e244cc74f16d..7f2513ffe866 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -342,23 +342,14 @@ get_tracepoint_field(struct pyrf_event *pevent, PyObject *attr_name)
 {
 	const char *str = _PyUnicode_AsString(PyObject_Str(attr_name));
 	struct evsel *evsel = pevent->evsel;
+	struct tep_event *tp_format = evsel__tp_format(evsel);
 	struct tep_format_field *field;
 
-	if (!evsel->tp_format) {
-		struct tep_event *tp_format;
-
-		tp_format = trace_event__tp_format_id(evsel->core.attr.config);
-		if (IS_ERR_OR_NULL(tp_format))
-			return NULL;
-
-		evsel->tp_format = tp_format;
-	}
-
-	field = tep_find_any_field(evsel->tp_format, str);
-	if (!field)
+	if (IS_ERR_OR_NULL(tp_format))
 		return NULL;
 
-	return tracepoint_field(pevent, field);
+	field = tep_find_any_field(tp_format, str);
+	return field ? tracepoint_field(pevent, field) : NULL;
 }
 #endif /* HAVE_LIBTRACEEVENT */
 
-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 09/11] perf python: Add evlist all_cpus accessor

[Ian Rogers]

Add a means to get the reference counted all_cpus CPU map from an
evlist in its python form.

Reviewed-by: Howard Chu <howardchu95@gmail.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 7f2513ffe866..c55c8392bc07 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -874,6 +874,16 @@ static void pyrf_evlist__delete(struct pyrf_evlist *pevlist)
 	Py_TYPE(pevlist)->tp_free((PyObject*)pevlist);
 }
 
+static PyObject *pyrf_evlist__all_cpus(struct pyrf_evlist *pevlist)
+{
+	struct pyrf_cpu_map *pcpu_map = PyObject_New(struct pyrf_cpu_map, &pyrf_cpu_map__type);
+
+	if (pcpu_map)
+		pcpu_map->cpus = perf_cpu_map__get(pevlist->evlist.core.all_cpus);
+
+	return (PyObject *)pcpu_map;
+}
+
 static PyObject *pyrf_evlist__mmap(struct pyrf_evlist *pevlist,
 				   PyObject *args, PyObject *kwargs)
 {
@@ -1057,6 +1067,12 @@ static PyObject *pyrf_evlist__enable(struct pyrf_evlist *pevlist)
 }
 
 static PyMethodDef pyrf_evlist__methods[] = {
+	{
+		.ml_name  = "all_cpus",
+		.ml_meth  = (PyCFunction)pyrf_evlist__all_cpus,
+		.ml_flags = METH_NOARGS,
+		.ml_doc	  = PyDoc_STR("CPU map union of all evsel CPU maps.")
+	},
 	{
 		.ml_name  = "mmap",
 		.ml_meth  = (PyCFunction)pyrf_evlist__mmap,
-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 10/11] perf python: Add evlist.config to set up record options

[Ian Rogers]

Add access to evlist__config that is used to configure an evlist with
record options.

Reviewed-by: Howard Chu <howardchu95@gmail.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/util/python.c | 33 +++++++++++++++++++++++++++++++++
 1 file changed, 33 insertions(+)

diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index c55c8392bc07..69ec2ad60d98 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -9,10 +9,12 @@
 #include <event-parse.h>
 #endif
 #include <perf/mmap.h>
+#include "callchain.h"
 #include "evlist.h"
 #include "evsel.h"
 #include "event.h"
 #include "print_binary.h"
+#include "record.h"
 #include "strbuf.h"
 #include "thread_map.h"
 #include "trace-event.h"
@@ -1052,6 +1054,31 @@ static PyObject *pyrf_evlist__open(struct pyrf_evlist *pevlist,
 	return Py_None;
 }
 
+static PyObject *pyrf_evlist__config(struct pyrf_evlist *pevlist)
+{
+	struct record_opts opts = {
+		.sample_time	     = true,
+		.mmap_pages	     = UINT_MAX,
+		.user_freq	     = UINT_MAX,
+		.user_interval	     = ULLONG_MAX,
+		.freq		     = 4000,
+		.target		     = {
+			.uses_mmap   = true,
+			.default_per_cpu = true,
+		},
+		.nr_threads_synthesize = 1,
+		.ctl_fd              = -1,
+		.ctl_fd_ack          = -1,
+		.no_buffering        = true,
+		.no_inherit          = true,
+	};
+	struct evlist *evlist = &pevlist->evlist;
+
+	evlist__config(evlist, &opts, &callchain_param);
+	Py_INCREF(Py_None);
+	return Py_None;
+}
+
 static PyObject *pyrf_evlist__disable(struct pyrf_evlist *pevlist)
 {
 	evlist__disable(&pevlist->evlist);
@@ -1109,6 +1136,12 @@ static PyMethodDef pyrf_evlist__methods[] = {
 		.ml_flags = METH_VARARGS | METH_KEYWORDS,
 		.ml_doc	  = PyDoc_STR("reads an event.")
 	},
+	{
+		.ml_name  = "config",
+		.ml_meth  = (PyCFunction)pyrf_evlist__config,
+		.ml_flags = METH_NOARGS,
+		.ml_doc	  = PyDoc_STR("Apply default record options to the evlist.")
+	},
 	{
 		.ml_name  = "disable",
 		.ml_meth  = (PyCFunction)pyrf_evlist__disable,
-- 
2.48.1.711.g2feabab25a-goog

[PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Ian Rogers]

Rather than manually configuring an evsel, switch to using
parse_events for greater commonality with the rest of the perf code.

Reviewed-by: Howard Chu <howardchu95@gmail.com>
Signed-off-by: Ian Rogers <irogers@google.com>
---
 tools/perf/python/tracepoint.py | 23 +++++++++++------------
 1 file changed, 11 insertions(+), 12 deletions(-)

diff --git a/tools/perf/python/tracepoint.py b/tools/perf/python/tracepoint.py
index bba68a6d4515..38b2b6d11f64 100755
--- a/tools/perf/python/tracepoint.py
+++ b/tools/perf/python/tracepoint.py
@@ -5,24 +5,23 @@
 
 import perf
 
-class tracepoint(perf.evsel):
-    def __init__(self, sys, name):
-        config = perf.tracepoint(sys, name)
-        perf.evsel.__init__(self,
-                            type   = perf.TYPE_TRACEPOINT,
-                            config = config,
-                            freq = 0, sample_period = 1, wakeup_events = 1,
-                            sample_type = perf.SAMPLE_PERIOD | perf.SAMPLE_TID | perf.SAMPLE_CPU | perf.SAMPLE_RAW | perf.SAMPLE_TIME)
-
 def main():
-    tp      = tracepoint("sched", "sched_switch")
     cpus    = perf.cpu_map()
     threads = perf.thread_map(-1)
+    evlist = perf.parse_events("sched:sched_switch", cpus, threads)
+    # Disable tracking of mmaps and similar that are unnecessary.
+    for ev in evlist:
+        ev.tracking = False
+    # Configure evsels with default record options.
+    evlist.config()
+    # Simplify the sample_type and read_format of evsels
+    for ev in evlist:
+        ev.sample_type = ev.sample_type & ~perf.SAMPLE_IP
+        ev.read_format = 0
 
-    evlist = perf.evlist(cpus, threads)
-    evlist.add(tp)
     evlist.open()
     evlist.mmap()
+    evlist.enable();
 
     while True:
         evlist.poll(timeout = -1)
-- 
2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 00/11] Python improvements for a real use of parse_events

[Howard Chu]

Hello Ian,

On Fri, Feb 28, 2025 at 2:23 PM Ian Rogers <irogers@google.com> wrote:
>
> While parse_events access in python was added, it wasn't used by any
> python script. In enabling this for the tracepoint.py script a number
> of latent bugs and necessary improvements were discovered.
>
> v2: Fix a whitespace issue on the evlist.config patch spotted by
>     Howard and add his reviewed-by tags.
>
> Ian Rogers (11):
>   perf debug: Avoid stack overflow in recursive error message
>   perf evlist: Add success path to evlist__create_syswide_maps
>   perf evsel: tp_format accessing improvements
>   perf python: Add evlist enable and disable methods
>   perf python: Add member access to a number of evsel variables
>   perf python: Add optional cpus and threads arguments to parse_events
>   perf python: Update ungrouped evsel leader in clone
>   perf python: Avoid duplicated code in get_tracepoint_field
>   perf python: Add evlist all_cpus accessor
>   perf python: Add evlist.config to set up record options
>   perf python tracepoint: Switch to using parse_events

Reviewed-by: Howard Chu <howardchu95@gmail.com>

>
>  tools/perf/python/tracepoint.py |  23 +++---
>  tools/perf/util/debug.c         |   2 +-
>  tools/perf/util/evlist.c        |  13 ++--
>  tools/perf/util/evsel.c         |  16 +++-
>  tools/perf/util/python.c        | 127 ++++++++++++++++++++++++++++----
>  5 files changed, 145 insertions(+), 36 deletions(-)
>
> --
> 2.48.1.711.g2feabab25a-goog
>

Re: [PATCH v2 01/11] perf debug: Avoid stack overflow in recursive error message

[Arnaldo Carvalho de Melo]

On Fri, Feb 28, 2025 at 02:22:58PM -0800, Ian Rogers wrote:
> In debug_file, pr_warning_once is called on error. As that function
> calls debug_file the function will yield a stack overflow. Switch the
> location of the call so the recursion is avoided.
> 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
> Signed-off-by: Ian Rogers <irogers@google.com>

Good to add this so that stable picks it:

Fixes: ec49230cf6dda704 ("perf debug: Expose debug file")

- Arnaldo

> ---
>  tools/perf/util/debug.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/debug.c b/tools/perf/util/debug.c
> index 995f6bb05b5f..f9ef7d045c92 100644
> --- a/tools/perf/util/debug.c
> +++ b/tools/perf/util/debug.c
> @@ -46,8 +46,8 @@ int debug_type_profile;
>  FILE *debug_file(void)
>  {
>  	if (!_debug_file) {
> -		pr_warning_once("debug_file not set");
>  		debug_set_file(stderr);
> +		pr_warning_once("debug_file not set");
>  	}
>  	return _debug_file;
>  }
> -- 
> 2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 01/11] perf debug: Avoid stack overflow in recursive error message

[Arnaldo Carvalho de Melo]

On Mon, Mar 10, 2025 at 05:22:16PM -0300, Arnaldo Carvalho de Melo wrote:
> On Fri, Feb 28, 2025 at 02:22:58PM -0800, Ian Rogers wrote:
> > In debug_file, pr_warning_once is called on error. As that function
> > calls debug_file the function will yield a stack overflow. Switch the
> > location of the call so the recursion is avoided.
> > 
> > Reviewed-by: Howard Chu <howardchu95@gmail.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Good to add this so that stable picks it:
> 
> Fixes: ec49230cf6dda704 ("perf debug: Expose debug file")

Forgot to add:

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>
 
- Arnaldo
> 
> > ---
> >  tools/perf/util/debug.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/tools/perf/util/debug.c b/tools/perf/util/debug.c
> > index 995f6bb05b5f..f9ef7d045c92 100644
> > --- a/tools/perf/util/debug.c
> > +++ b/tools/perf/util/debug.c
> > @@ -46,8 +46,8 @@ int debug_type_profile;
> >  FILE *debug_file(void)
> >  {
> >  	if (!_debug_file) {
> > -		pr_warning_once("debug_file not set");
> >  		debug_set_file(stderr);
> > +		pr_warning_once("debug_file not set");
> >  	}
> >  	return _debug_file;
> >  }
> > -- 
> > 2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 02/11] perf evlist: Add success path to evlist__create_syswide_maps

[Arnaldo Carvalho de Melo]

On Fri, Feb 28, 2025 at 02:22:59PM -0800, Ian Rogers wrote:
> Over various refactorings evlist__create_syswide_maps has been made to
> only ever return with -ENOMEM. Fix this so that when
> perf_evlist__set_maps is successfully called, 0 is returned.

I think this was when the problem was introduced:

Fixes: 8c0498b6891d7ca5 ("perf evlist: Fix create_syswide_maps() not propagating maps")

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo
 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evlist.c | 13 ++++++-------
>  1 file changed, 6 insertions(+), 7 deletions(-)
> 
> diff --git a/tools/perf/util/evlist.c b/tools/perf/util/evlist.c
> index f0dd174e2deb..633df7d9204c 100644
> --- a/tools/perf/util/evlist.c
> +++ b/tools/perf/util/evlist.c
> @@ -1373,19 +1373,18 @@ static int evlist__create_syswide_maps(struct evlist *evlist)
>  	 */
>  	cpus = perf_cpu_map__new_online_cpus();
>  	if (!cpus)
> -		goto out;
> +		return -ENOMEM;
>  
>  	threads = perf_thread_map__new_dummy();
> -	if (!threads)
> -		goto out_put;
> +	if (!threads) {
> +		perf_cpu_map__put(cpus);
> +		return -ENOMEM;
> +	}
>  
>  	perf_evlist__set_maps(&evlist->core, cpus, threads);
> -
>  	perf_thread_map__put(threads);
> -out_put:
>  	perf_cpu_map__put(cpus);
> -out:
> -	return -ENOMEM;
> +	return 0;
>  }
>  
>  int evlist__open(struct evlist *evlist)
> -- 
> 2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 03/11] perf evsel: tp_format accessing improvements

[Arnaldo Carvalho de Melo]

On Fri, Feb 28, 2025 at 02:23:00PM -0800, Ian Rogers wrote:
> Ensure evsel__clone copies the tp_sys and tp_name variables.
> In evsel__tp_format, if tp_sys isn't set, use the config value to find
> the tp_format. This succeeds in python code where pyrf__tracepoint has
> already found the format.

Here those two fields were introduced but evsel__clone() wasn't update
to clone those:

Fixes: 6c8310e8380d472c ("perf evsel: Allow evsel__newtp without libtraceevent")

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo
 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/evsel.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 4a0ef095db92..1974395492d7 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -521,6 +521,16 @@ struct evsel *evsel__clone(struct evsel *dest, struct evsel *orig)
>  	}
>  	evsel->cgrp = cgroup__get(orig->cgrp);
>  #ifdef HAVE_LIBTRACEEVENT
> +	if (orig->tp_sys) {
> +		evsel->tp_sys = strdup(orig->tp_sys);
> +		if (evsel->tp_sys == NULL)
> +			goto out_err;
> +	}
> +	if (orig->tp_name) {
> +		evsel->tp_name = strdup(orig->tp_name);
> +		if (evsel->tp_name == NULL)
> +			goto out_err;
> +	}
>  	evsel->tp_format = orig->tp_format;
>  #endif
>  	evsel->handler = orig->handler;
> @@ -644,7 +654,11 @@ struct tep_event *evsel__tp_format(struct evsel *evsel)
>  	if (evsel->core.attr.type != PERF_TYPE_TRACEPOINT)
>  		return NULL;
>  
> -	tp_format = trace_event__tp_format(evsel->tp_sys, evsel->tp_name);
> +	if (!evsel->tp_sys)
> +		tp_format = trace_event__tp_format_id(evsel->core.attr.config);
> +	else
> +		tp_format = trace_event__tp_format(evsel->tp_sys, evsel->tp_name);
> +
>  	if (IS_ERR(tp_format)) {
>  		int err = -PTR_ERR(evsel->tp_format);
>  
> -- 
> 2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 04/11] perf python: Add evlist enable and disable methods

[Arnaldo Carvalho de Melo]

On Fri, Feb 28, 2025 at 02:23:01PM -0800, Ian Rogers wrote:
> By default the evsels from parse_events will be disabled. Add access
> to the evlist functions so they can be enabled/disabled.

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo
 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/python.c | 26 ++++++++++++++++++++++++++
>  1 file changed, 26 insertions(+)
> 
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index e2b9032c1311..0cf81cfcfafb 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -1028,6 +1028,20 @@ static PyObject *pyrf_evlist__open(struct pyrf_evlist *pevlist,
>  	return Py_None;
>  }
>  
> +static PyObject *pyrf_evlist__disable(struct pyrf_evlist *pevlist)
> +{
> +	evlist__disable(&pevlist->evlist);
> +	Py_INCREF(Py_None);
> +	return Py_None;
> +}
> +
> +static PyObject *pyrf_evlist__enable(struct pyrf_evlist *pevlist)
> +{
> +	evlist__enable(&pevlist->evlist);
> +	Py_INCREF(Py_None);
> +	return Py_None;
> +}
> +
>  static PyMethodDef pyrf_evlist__methods[] = {
>  	{
>  		.ml_name  = "mmap",
> @@ -1065,6 +1079,18 @@ static PyMethodDef pyrf_evlist__methods[] = {
>  		.ml_flags = METH_VARARGS | METH_KEYWORDS,
>  		.ml_doc	  = PyDoc_STR("reads an event.")
>  	},
> +	{
> +		.ml_name  = "disable",
> +		.ml_meth  = (PyCFunction)pyrf_evlist__disable,
> +		.ml_flags = METH_NOARGS,
> +		.ml_doc	  = PyDoc_STR("Disable the evsels in the evlist.")
> +	},
> +	{
> +		.ml_name  = "enable",
> +		.ml_meth  = (PyCFunction)pyrf_evlist__enable,
> +		.ml_flags = METH_NOARGS,
> +		.ml_doc	  = PyDoc_STR("Enable the evsels in the evlist.")
> +	},
>  	{ .ml_name = NULL, }
>  };
>  
> -- 
> 2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 05/11] perf python: Add member access to a number of evsel variables

[Arnaldo Carvalho de Melo]

On Fri, Feb 28, 2025 at 02:23:02PM -0800, Ian Rogers wrote:
> Most variables are part of the perf_event_attr, so that they may be
> queried and modified.
> 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo

> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/python.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 0cf81cfcfafb..b600b6379b4e 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -811,6 +811,28 @@ static PyMethodDef pyrf_evsel__methods[] = {
>  	{ .ml_name = NULL, }
>  };
>  
> +#define evsel_member_def(member, ptype, help) \
> +	{ #member, ptype, \
> +	  offsetof(struct pyrf_evsel, evsel.member), \
> +	  0, help }
> +
> +#define evsel_attr_member_def(member, ptype, help) \
> +	{ #member, ptype, \
> +	  offsetof(struct pyrf_evsel, evsel.core.attr.member), \
> +	  0, help }
> +
> +static PyMemberDef pyrf_evsel__members[] = {
> +	evsel_member_def(tracking, T_BOOL, "tracking event."),
> +	evsel_attr_member_def(type, T_UINT, "attribute type."),
> +	evsel_attr_member_def(size, T_UINT, "attribute size."),
> +	evsel_attr_member_def(config, T_ULONGLONG, "attribute config."),
> +	evsel_attr_member_def(sample_period, T_ULONGLONG, "attribute sample_period."),
> +	evsel_attr_member_def(sample_type, T_ULONGLONG, "attribute sample_type."),
> +	evsel_attr_member_def(read_format, T_ULONGLONG, "attribute read_format."),
> +	evsel_attr_member_def(wakeup_events, T_UINT, "attribute wakeup_events."),
> +	{ .name = NULL, },
> +};
> +
>  static const char pyrf_evsel__doc[] = PyDoc_STR("perf event selector list object.");
>  
>  static PyTypeObject pyrf_evsel__type = {
> @@ -820,6 +842,7 @@ static PyTypeObject pyrf_evsel__type = {
>  	.tp_dealloc	= (destructor)pyrf_evsel__delete,
>  	.tp_flags	= Py_TPFLAGS_DEFAULT|Py_TPFLAGS_BASETYPE,
>  	.tp_doc		= pyrf_evsel__doc,
> +	.tp_members	= pyrf_evsel__members,
>  	.tp_methods	= pyrf_evsel__methods,
>  	.tp_init	= (initproc)pyrf_evsel__init,
>  	.tp_str         = pyrf_evsel__str,
> -- 
> 2.48.1.711.g2feabab25a-goog
> 

Re: [PATCH v2 06/11] perf python: Add optional cpus and threads arguments to parse_events

[Arnaldo Carvalho de Melo]

On Fri, Feb 28, 2025 at 02:23:03PM -0800, Ian Rogers wrote:
> Used for the evlist initialization.

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo
 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/python.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index b600b6379b4e..4a3015e7dc83 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -1339,12 +1339,18 @@ static PyObject *pyrf__parse_events(PyObject *self, PyObject *args)
>  	struct evlist evlist = {};
>  	struct parse_events_error err;
>  	PyObject *result;
> +	PyObject *pcpus = NULL, *pthreads = NULL;
> +	struct perf_cpu_map *cpus;
> +	struct perf_thread_map *threads;
>  
> -	if (!PyArg_ParseTuple(args, "s", &input))
> +	if (!PyArg_ParseTuple(args, "s|OO", &input, &pcpus, &pthreads))
>  		return NULL;
>  
> +	threads = pthreads ? ((struct pyrf_thread_map *)pthreads)->threads : NULL;
> +	cpus = pcpus ? ((struct pyrf_cpu_map *)pcpus)->cpus : NULL;
> +
>  	parse_events_error__init(&err);
> -	evlist__init(&evlist, NULL, NULL);
> +	evlist__init(&evlist, cpus, threads);
>  	if (parse_events(&evlist, input, &err)) {
>  		parse_events_error__print(&err, input);
>  		PyErr_SetFromErrno(PyExc_OSError);
> -- 
> 2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 07/11] perf python: Update ungrouped evsel leader in clone

[Arnaldo Carvalho de Melo]

On Fri, Feb 28, 2025 at 02:23:04PM -0800, Ian Rogers wrote:
> evsels are cloned in the python code as they form part of the Python
> object pyrf_evsel. The cloning doesn't update the evsel's leader, do
> this for the case of an evsel being ungrouped.

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo
 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/python.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 4a3015e7dc83..e244cc74f16d 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -1312,6 +1312,8 @@ static PyObject *pyrf_evsel__from_evsel(struct evsel *evsel)
>  	evsel__init(&pevsel->evsel, &evsel->core.attr, evsel->core.idx);
>  
>  	evsel__clone(&pevsel->evsel, evsel);
> +	if (evsel__is_group_leader(evsel))
> +		evsel__set_leader(&pevsel->evsel, &pevsel->evsel);
>  	return (PyObject *)pevsel;
>  }
>  
> -- 
> 2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 08/11] perf python: Avoid duplicated code in get_tracepoint_field

[Arnaldo Carvalho de Melo]

On Fri, Feb 28, 2025 at 02:23:05PM -0800, Ian Rogers wrote:
> The code replicates computations done in evsel__tp_format, reuse
> evsel__tp_format to simplify the python C code.

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo
 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/python.c | 17 ++++-------------
>  1 file changed, 4 insertions(+), 13 deletions(-)
> 
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index e244cc74f16d..7f2513ffe866 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -342,23 +342,14 @@ get_tracepoint_field(struct pyrf_event *pevent, PyObject *attr_name)
>  {
>  	const char *str = _PyUnicode_AsString(PyObject_Str(attr_name));
>  	struct evsel *evsel = pevent->evsel;
> +	struct tep_event *tp_format = evsel__tp_format(evsel);
>  	struct tep_format_field *field;
>  
> -	if (!evsel->tp_format) {
> -		struct tep_event *tp_format;
> -
> -		tp_format = trace_event__tp_format_id(evsel->core.attr.config);
> -		if (IS_ERR_OR_NULL(tp_format))
> -			return NULL;
> -
> -		evsel->tp_format = tp_format;
> -	}
> -
> -	field = tep_find_any_field(evsel->tp_format, str);
> -	if (!field)
> +	if (IS_ERR_OR_NULL(tp_format))
>  		return NULL;
>  
> -	return tracepoint_field(pevent, field);
> +	field = tep_find_any_field(tp_format, str);
> +	return field ? tracepoint_field(pevent, field) : NULL;
>  }
>  #endif /* HAVE_LIBTRACEEVENT */
>  
> -- 
> 2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 09/11] perf python: Add evlist all_cpus accessor

[Arnaldo Carvalho de Melo]

On Fri, Feb 28, 2025 at 02:23:06PM -0800, Ian Rogers wrote:
> Add a means to get the reference counted all_cpus CPU map from an
> evlist in its python form.

Reviewed-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo
 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/python.c | 16 ++++++++++++++++
>  1 file changed, 16 insertions(+)
> 
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 7f2513ffe866..c55c8392bc07 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -874,6 +874,16 @@ static void pyrf_evlist__delete(struct pyrf_evlist *pevlist)
>  	Py_TYPE(pevlist)->tp_free((PyObject*)pevlist);
>  }
>  
> +static PyObject *pyrf_evlist__all_cpus(struct pyrf_evlist *pevlist)
> +{
> +	struct pyrf_cpu_map *pcpu_map = PyObject_New(struct pyrf_cpu_map, &pyrf_cpu_map__type);
> +
> +	if (pcpu_map)
> +		pcpu_map->cpus = perf_cpu_map__get(pevlist->evlist.core.all_cpus);
> +
> +	return (PyObject *)pcpu_map;
> +}
> +
>  static PyObject *pyrf_evlist__mmap(struct pyrf_evlist *pevlist,
>  				   PyObject *args, PyObject *kwargs)
>  {
> @@ -1057,6 +1067,12 @@ static PyObject *pyrf_evlist__enable(struct pyrf_evlist *pevlist)
>  }
>  
>  static PyMethodDef pyrf_evlist__methods[] = {
> +	{
> +		.ml_name  = "all_cpus",
> +		.ml_meth  = (PyCFunction)pyrf_evlist__all_cpus,
> +		.ml_flags = METH_NOARGS,
> +		.ml_doc	  = PyDoc_STR("CPU map union of all evsel CPU maps.")
> +	},
>  	{
>  		.ml_name  = "mmap",
>  		.ml_meth  = (PyCFunction)pyrf_evlist__mmap,
> -- 
> 2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 10/11] perf python: Add evlist.config to set up record options

[Arnaldo Carvalho de Melo]

On Fri, Feb 28, 2025 at 02:23:07PM -0800, Ian Rogers wrote:
> Add access to evlist__config that is used to configure an evlist with
> record options.

I guess nothing precludes adding support later for passing fields as an
optional dictionary, so I think its ok to have it with the name "config"
but being more of a "default_config".

Acked-by: Arnaldo Carvalho de Melo <acme@redhat.com>

- Arnaldo
 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/python.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
> 
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index c55c8392bc07..69ec2ad60d98 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -9,10 +9,12 @@
>  #include <event-parse.h>
>  #endif
>  #include <perf/mmap.h>
> +#include "callchain.h"
>  #include "evlist.h"
>  #include "evsel.h"
>  #include "event.h"
>  #include "print_binary.h"
> +#include "record.h"
>  #include "strbuf.h"
>  #include "thread_map.h"
>  #include "trace-event.h"
> @@ -1052,6 +1054,31 @@ static PyObject *pyrf_evlist__open(struct pyrf_evlist *pevlist,
>  	return Py_None;
>  }
>  
> +static PyObject *pyrf_evlist__config(struct pyrf_evlist *pevlist)
> +{
> +	struct record_opts opts = {
> +		.sample_time	     = true,
> +		.mmap_pages	     = UINT_MAX,
> +		.user_freq	     = UINT_MAX,
> +		.user_interval	     = ULLONG_MAX,
> +		.freq		     = 4000,
> +		.target		     = {
> +			.uses_mmap   = true,
> +			.default_per_cpu = true,
> +		},
> +		.nr_threads_synthesize = 1,
> +		.ctl_fd              = -1,
> +		.ctl_fd_ack          = -1,
> +		.no_buffering        = true,
> +		.no_inherit          = true,
> +	};
> +	struct evlist *evlist = &pevlist->evlist;

> +	evlist__config(evlist, &opts, &callchain_param);
> +	Py_INCREF(Py_None);
> +	return Py_None;
> +}
> +
>  static PyObject *pyrf_evlist__disable(struct pyrf_evlist *pevlist)
>  {
>  	evlist__disable(&pevlist->evlist);
> @@ -1109,6 +1136,12 @@ static PyMethodDef pyrf_evlist__methods[] = {
>  		.ml_flags = METH_VARARGS | METH_KEYWORDS,
>  		.ml_doc	  = PyDoc_STR("reads an event.")
>  	},
> +	{
> +		.ml_name  = "config",
> +		.ml_meth  = (PyCFunction)pyrf_evlist__config,
> +		.ml_flags = METH_NOARGS,
> +		.ml_doc	  = PyDoc_STR("Apply default record options to the evlist.")
> +	},
>  	{
>  		.ml_name  = "disable",
>  		.ml_meth  = (PyCFunction)pyrf_evlist__disable,
> -- 
> 2.48.1.711.g2feabab25a-goog

Re: [PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Arnaldo Carvalho de Melo]

On Fri, Feb 28, 2025 at 02:23:08PM -0800, Ian Rogers wrote:
> Rather than manually configuring an evsel, switch to using
> parse_events for greater commonality with the rest of the perf code.
> 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
> Signed-off-by: Ian Rogers <irogers@google.com>

Now will all in place I'm trying to test it and I am getting some
strange results:

root@number:/home/acme/git/perf-tools-next# tools/perf/python/tracepoint.py
<SNIP lots of seemingly ok lines>
time 78318710956557 prev_comm=sudo prev_pid=3133818 prev_prio=120 prev_state=0x1 ==> next_comm=swapper/14 next_pid=0 next_prio=120
time 78318720082300 prev_comm=swapper/16 prev_pid=0 prev_prio=120 prev_state=0x0 ==> next_comm=kworker/u112:8 next_pid=1752774 next_prio=120
time 78318706232435 prev_comm=kworker/u112:17 prev_pid=1551246 prev_prio=120 prev_state=0x80 ==> next_comm=swapper/21 next_pid=0 next_prio=120
time 78318708202121 prev_comm=sudo prev_pid=3133818 prev_prio=120 prev_state=0x2 ==> next_comm=swapper/25 next_pid=0 next_prio=120
time 78318748346989 prev_comm=swapper/26 prev_pid=0 prev_prio=120 prev_state=0x0 ==> next_comm=gnome-terminal- next_pid=3551 next_prio=120
Traceback (most recent call last):
  File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 47, in <module>
    main()
  File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 42, in main
    event.next_comm,
    ^^^^^^^^^^^^^^^
AttributeError: 'perf.sample_event' object has no attribute 'next_comm'
root@number:/home/acme/git/perf-tools-next# 

But it shouldn't get there as there is this check:

            if not isinstance(event, perf.sample_event):
                continue


:-\

Trying to debug that...

- Arnaldo

Re: [PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Arnaldo Carvalho de Melo]

On Mon, Mar 10, 2025 at 06:15:42PM -0300, Arnaldo Carvalho de Melo wrote:
> On Fri, Feb 28, 2025 at 02:23:08PM -0800, Ian Rogers wrote:
> > Rather than manually configuring an evsel, switch to using
> > parse_events for greater commonality with the rest of the perf code.
> > 
> > Reviewed-by: Howard Chu <howardchu95@gmail.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> 
> Now will all in place I'm trying to test it and I am getting some
> strange results:
> 
> root@number:/home/acme/git/perf-tools-next# tools/perf/python/tracepoint.py
> <SNIP lots of seemingly ok lines>
> time 78318710956557 prev_comm=sudo prev_pid=3133818 prev_prio=120 prev_state=0x1 ==> next_comm=swapper/14 next_pid=0 next_prio=120
> time 78318720082300 prev_comm=swapper/16 prev_pid=0 prev_prio=120 prev_state=0x0 ==> next_comm=kworker/u112:8 next_pid=1752774 next_prio=120
> time 78318706232435 prev_comm=kworker/u112:17 prev_pid=1551246 prev_prio=120 prev_state=0x80 ==> next_comm=swapper/21 next_pid=0 next_prio=120
> time 78318708202121 prev_comm=sudo prev_pid=3133818 prev_prio=120 prev_state=0x2 ==> next_comm=swapper/25 next_pid=0 next_prio=120
> time 78318748346989 prev_comm=swapper/26 prev_pid=0 prev_prio=120 prev_state=0x0 ==> next_comm=gnome-terminal- next_pid=3551 next_prio=120
> Traceback (most recent call last):
>   File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 47, in <module>
>     main()
>   File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 42, in main
>     event.next_comm,
>     ^^^^^^^^^^^^^^^
> AttributeError: 'perf.sample_event' object has no attribute 'next_comm'
> root@number:/home/acme/git/perf-tools-next# 
> 
> But it shouldn't get there as there is this check:
> 
>             if not isinstance(event, perf.sample_event):
>                 continue
> 
> 
> :-\
> 
> Trying to debug that...

And it doesn't seem related to your series, I checked and v6.13 has the
same problem, I nuked the build dir, etc.

- Arnaldo

Re: [PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Ian Rogers]

On Mon, Mar 10, 2025 at 2:16 PM Arnaldo Carvalho de Melo
<acme@kernel.org> wrote:
>
> On Mon, Mar 10, 2025 at 06:15:42PM -0300, Arnaldo Carvalho de Melo wrote:
> > On Fri, Feb 28, 2025 at 02:23:08PM -0800, Ian Rogers wrote:
> > > Rather than manually configuring an evsel, switch to using
> > > parse_events for greater commonality with the rest of the perf code.
> > >
> > > Reviewed-by: Howard Chu <howardchu95@gmail.com>
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> >
> > Now will all in place I'm trying to test it and I am getting some
> > strange results:
> >
> > root@number:/home/acme/git/perf-tools-next# tools/perf/python/tracepoint.py
> > <SNIP lots of seemingly ok lines>
> > time 78318710956557 prev_comm=sudo prev_pid=3133818 prev_prio=120 prev_state=0x1 ==> next_comm=swapper/14 next_pid=0 next_prio=120
> > time 78318720082300 prev_comm=swapper/16 prev_pid=0 prev_prio=120 prev_state=0x0 ==> next_comm=kworker/u112:8 next_pid=1752774 next_prio=120
> > time 78318706232435 prev_comm=kworker/u112:17 prev_pid=1551246 prev_prio=120 prev_state=0x80 ==> next_comm=swapper/21 next_pid=0 next_prio=120
> > time 78318708202121 prev_comm=sudo prev_pid=3133818 prev_prio=120 prev_state=0x2 ==> next_comm=swapper/25 next_pid=0 next_prio=120
> > time 78318748346989 prev_comm=swapper/26 prev_pid=0 prev_prio=120 prev_state=0x0 ==> next_comm=gnome-terminal- next_pid=3551 next_prio=120
> > Traceback (most recent call last):
> >   File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 47, in <module>
> >     main()
> >   File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 42, in main
> >     event.next_comm,
> >     ^^^^^^^^^^^^^^^
> > AttributeError: 'perf.sample_event' object has no attribute 'next_comm'
> > root@number:/home/acme/git/perf-tools-next#
> >
> > But it shouldn't get there as there is this check:
> >
> >             if not isinstance(event, perf.sample_event):
> >                 continue
> >
> >
> > :-\
> >
> > Trying to debug that...
>
> And it doesn't seem related to your series, I checked and v6.13 has the
> same problem, I nuked the build dir, etc.

Right. I'd seen the same issue.

Thanks,
Ian

Re: [PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Arnaldo Carvalho de Melo]

On Mon, Mar 10, 2025 at 02:17:59PM -0700, Ian Rogers wrote:
> On Mon, Mar 10, 2025 at 2:16 PM Arnaldo Carvalho de Melo
> <acme@kernel.org> wrote:
> >
> > On Mon, Mar 10, 2025 at 06:15:42PM -0300, Arnaldo Carvalho de Melo wrote:
> > > On Fri, Feb 28, 2025 at 02:23:08PM -0800, Ian Rogers wrote:
> > > > Rather than manually configuring an evsel, switch to using
> > > > parse_events for greater commonality with the rest of the perf code.
> > > >
> > > > Reviewed-by: Howard Chu <howardchu95@gmail.com>
> > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > >
> > > Now will all in place I'm trying to test it and I am getting some
> > > strange results:
> > >
> > > root@number:/home/acme/git/perf-tools-next# tools/perf/python/tracepoint.py
> > > <SNIP lots of seemingly ok lines>
> > > time 78318710956557 prev_comm=sudo prev_pid=3133818 prev_prio=120 prev_state=0x1 ==> next_comm=swapper/14 next_pid=0 next_prio=120
> > > time 78318720082300 prev_comm=swapper/16 prev_pid=0 prev_prio=120 prev_state=0x0 ==> next_comm=kworker/u112:8 next_pid=1752774 next_prio=120
> > > time 78318706232435 prev_comm=kworker/u112:17 prev_pid=1551246 prev_prio=120 prev_state=0x80 ==> next_comm=swapper/21 next_pid=0 next_prio=120
> > > time 78318708202121 prev_comm=sudo prev_pid=3133818 prev_prio=120 prev_state=0x2 ==> next_comm=swapper/25 next_pid=0 next_prio=120
> > > time 78318748346989 prev_comm=swapper/26 prev_pid=0 prev_prio=120 prev_state=0x0 ==> next_comm=gnome-terminal- next_pid=3551 next_prio=120
> > > Traceback (most recent call last):
> > >   File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 47, in <module>
> > >     main()
> > >   File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 42, in main
> > >     event.next_comm,
> > >     ^^^^^^^^^^^^^^^
> > > AttributeError: 'perf.sample_event' object has no attribute 'next_comm'
> > > root@number:/home/acme/git/perf-tools-next#
> > >
> > > But it shouldn't get there as there is this check:
> > >
> > >             if not isinstance(event, perf.sample_event):
> > >                 continue
> > >
> > >
> > > :-\
> > >
> > > Trying to debug that...
> >
> > And it doesn't seem related to your series, I checked and v6.13 has the
> > same problem, I nuked the build dir, etc.
> 
> Right. I'd seen the same issue.

time 79411977132102 prev_comm=sudo prev_pid=3133818 prev_prio=120 prev_state=0x2 ==> next_comm=swapper/12 next_pid=0 next_prio=120
{ type: sample }
time 79411977200343 prev_comm=kworker/u112:17 prev_pid=1551246 prev_prio=120 prev_state=0x80 ==> next_comm=swapper/16 next_pid=0 next_prio=120
{ type: sample }
time 79411964535268 prev_comm=kworker/u112:14 prev_pid=810939 prev_prio=120 prev_state=0x80 ==> next_comm=swapper/17 next_pid=0 next_prio=120
{ type: sample }
time 79411964746511 prev_comm=swapper/18 prev_pid=0 prev_prio=120 prev_state=0x0 ==> next_comm=kworker/u112:12 next_pid=2109251 next_prio=120
{ type: sample }
Traceback (most recent call last):
  File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 48, in <module>
    main()
  File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 43, in main
    event.next_comm,
    ^^^^^^^^^^^^^^^
AttributeError: 'perf.sample_event' object has no attribute 'next_comm'
root@number:/home/acme/git/perf-tools-next#

And it says it is a sample... 


Well, ran out of time, will try later or early tomorrow, will also try
to review the syscalltbl series and Howard's off cpu profiling.

- Arnaldo

Re: [PATCH v2 06/11] perf python: Add optional cpus and threads arguments to parse_events

[Namhyung Kim]

On Fri, Feb 28, 2025 at 02:23:03PM -0800, Ian Rogers wrote:
> Used for the evlist initialization.
> 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/python.c | 10 ++++++++--
>  1 file changed, 8 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index b600b6379b4e..4a3015e7dc83 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -1339,12 +1339,18 @@ static PyObject *pyrf__parse_events(PyObject *self, PyObject *args)
>  	struct evlist evlist = {};
>  	struct parse_events_error err;
>  	PyObject *result;
> +	PyObject *pcpus = NULL, *pthreads = NULL;
> +	struct perf_cpu_map *cpus;
> +	struct perf_thread_map *threads;
>  
> -	if (!PyArg_ParseTuple(args, "s", &input))
> +	if (!PyArg_ParseTuple(args, "s|OO", &input, &pcpus, &pthreads))
>  		return NULL;
>  
> +	threads = pthreads ? ((struct pyrf_thread_map *)pthreads)->threads : NULL;
> +	cpus = pcpus ? ((struct pyrf_cpu_map *)pcpus)->cpus : NULL;

I wonder if it needs any type checks before accessing them.

Thanks,
Namhyung

> +
>  	parse_events_error__init(&err);
> -	evlist__init(&evlist, NULL, NULL);
> +	evlist__init(&evlist, cpus, threads);
>  	if (parse_events(&evlist, input, &err)) {
>  		parse_events_error__print(&err, input);
>  		PyErr_SetFromErrno(PyExc_OSError);
> -- 
> 2.48.1.711.g2feabab25a-goog
> 

Re: [PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Namhyung Kim]

On Fri, Feb 28, 2025 at 02:23:08PM -0800, Ian Rogers wrote:
> Rather than manually configuring an evsel, switch to using
> parse_events for greater commonality with the rest of the perf code.
> 
> Reviewed-by: Howard Chu <howardchu95@gmail.com>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/python/tracepoint.py | 23 +++++++++++------------
>  1 file changed, 11 insertions(+), 12 deletions(-)
> 
> diff --git a/tools/perf/python/tracepoint.py b/tools/perf/python/tracepoint.py
> index bba68a6d4515..38b2b6d11f64 100755
> --- a/tools/perf/python/tracepoint.py
> +++ b/tools/perf/python/tracepoint.py
> @@ -5,24 +5,23 @@
>  
>  import perf
>  
> -class tracepoint(perf.evsel):
> -    def __init__(self, sys, name):
> -        config = perf.tracepoint(sys, name)
> -        perf.evsel.__init__(self,
> -                            type   = perf.TYPE_TRACEPOINT,
> -                            config = config,
> -                            freq = 0, sample_period = 1, wakeup_events = 1,
> -                            sample_type = perf.SAMPLE_PERIOD | perf.SAMPLE_TID | perf.SAMPLE_CPU | perf.SAMPLE_RAW | perf.SAMPLE_TIME)
> -
>  def main():
> -    tp      = tracepoint("sched", "sched_switch")
>      cpus    = perf.cpu_map()
>      threads = perf.thread_map(-1)
> +    evlist = perf.parse_events("sched:sched_switch", cpus, threads)
> +    # Disable tracking of mmaps and similar that are unnecessary.
> +    for ev in evlist:
> +        ev.tracking = False
> +    # Configure evsels with default record options.
> +    evlist.config()

I think the default option uses frequency of 4000 but tracepoints want
to use period of 1.  Also I'm not sure if it sets the proper sample type
bits namely PERF_SAMPLE_RAW.

Thanks,
Namhyung


> +    # Simplify the sample_type and read_format of evsels
> +    for ev in evlist:
> +        ev.sample_type = ev.sample_type & ~perf.SAMPLE_IP
> +        ev.read_format = 0
>  
> -    evlist = perf.evlist(cpus, threads)
> -    evlist.add(tp)
>      evlist.open()
>      evlist.mmap()
> +    evlist.enable();
>  
>      while True:
>          evlist.poll(timeout = -1)
> -- 
> 2.48.1.711.g2feabab25a-goog
> 

Re: [PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Ian Rogers]

On Mon, Mar 10, 2025 at 3:17 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Fri, Feb 28, 2025 at 02:23:08PM -0800, Ian Rogers wrote:
> > Rather than manually configuring an evsel, switch to using
> > parse_events for greater commonality with the rest of the perf code.
> >
> > Reviewed-by: Howard Chu <howardchu95@gmail.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/python/tracepoint.py | 23 +++++++++++------------
> >  1 file changed, 11 insertions(+), 12 deletions(-)
> >
> > diff --git a/tools/perf/python/tracepoint.py b/tools/perf/python/tracepoint.py
> > index bba68a6d4515..38b2b6d11f64 100755
> > --- a/tools/perf/python/tracepoint.py
> > +++ b/tools/perf/python/tracepoint.py
> > @@ -5,24 +5,23 @@
> >
> >  import perf
> >
> > -class tracepoint(perf.evsel):
> > -    def __init__(self, sys, name):
> > -        config = perf.tracepoint(sys, name)
> > -        perf.evsel.__init__(self,
> > -                            type   = perf.TYPE_TRACEPOINT,
> > -                            config = config,
> > -                            freq = 0, sample_period = 1, wakeup_events = 1,
> > -                            sample_type = perf.SAMPLE_PERIOD | perf.SAMPLE_TID | perf.SAMPLE_CPU | perf.SAMPLE_RAW | perf.SAMPLE_TIME)
> > -
> >  def main():
> > -    tp      = tracepoint("sched", "sched_switch")
> >      cpus    = perf.cpu_map()
> >      threads = perf.thread_map(-1)
> > +    evlist = perf.parse_events("sched:sched_switch", cpus, threads)
> > +    # Disable tracking of mmaps and similar that are unnecessary.
> > +    for ev in evlist:
> > +        ev.tracking = False
> > +    # Configure evsels with default record options.
> > +    evlist.config()
>
> I think the default option uses frequency of 4000 but tracepoints want
> to use period of 1.  Also I'm not sure if it sets the proper sample type
> bits namely PERF_SAMPLE_RAW.

I used trace to ensure they matched. Fwiw, the sample_period for a
tracepoint is set to 1 in evsel__newtp_idx:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/evsel.c?h=perf-tools-next#n621
and the evsel__config won't overwrite an already set sample_period:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/evsel.c?h=perf-tools-next#n1341

Thanks, Ian

> Thanks,
> Namhyung
>
>
> > +    # Simplify the sample_type and read_format of evsels
> > +    for ev in evlist:
> > +        ev.sample_type = ev.sample_type & ~perf.SAMPLE_IP
> > +        ev.read_format = 0
> >
> > -    evlist = perf.evlist(cpus, threads)
> > -    evlist.add(tp)
> >      evlist.open()
> >      evlist.mmap()
> > +    evlist.enable();
> >
> >      while True:
> >          evlist.poll(timeout = -1)
> > --
> > 2.48.1.711.g2feabab25a-goog
> >

Re: [PATCH v2 06/11] perf python: Add optional cpus and threads arguments to parse_events

[Ian Rogers]

On Mon, Mar 10, 2025 at 3:12 PM Namhyung Kim <namhyung@kernel.org> wrote:
>
> On Fri, Feb 28, 2025 at 02:23:03PM -0800, Ian Rogers wrote:
> > Used for the evlist initialization.
> >
> > Reviewed-by: Howard Chu <howardchu95@gmail.com>
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/python.c | 10 ++++++++--
> >  1 file changed, 8 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> > index b600b6379b4e..4a3015e7dc83 100644
> > --- a/tools/perf/util/python.c
> > +++ b/tools/perf/util/python.c
> > @@ -1339,12 +1339,18 @@ static PyObject *pyrf__parse_events(PyObject *self, PyObject *args)
> >       struct evlist evlist = {};
> >       struct parse_events_error err;
> >       PyObject *result;
> > +     PyObject *pcpus = NULL, *pthreads = NULL;
> > +     struct perf_cpu_map *cpus;
> > +     struct perf_thread_map *threads;
> >
> > -     if (!PyArg_ParseTuple(args, "s", &input))
> > +     if (!PyArg_ParseTuple(args, "s|OO", &input, &pcpus, &pthreads))
> >               return NULL;
> >
> > +     threads = pthreads ? ((struct pyrf_thread_map *)pthreads)->threads : NULL;
> > +     cpus = pcpus ? ((struct pyrf_cpu_map *)pcpus)->cpus : NULL;
>
> I wonder if it needs any type checks before accessing them.

Agreed. We don't do it yet elsewhere:
https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/python.c?h=perf-tools-next#n769
although there keywords potentially avoid ambiguity. Given this I
think improving the typing is follow up work.

Thanks,
Ian

> Thanks,
> Namhyung
>
> > +
> >       parse_events_error__init(&err);
> > -     evlist__init(&evlist, NULL, NULL);
> > +     evlist__init(&evlist, cpus, threads);
> >       if (parse_events(&evlist, input, &err)) {
> >               parse_events_error__print(&err, input);
> >               PyErr_SetFromErrno(PyExc_OSError);
> > --
> > 2.48.1.711.g2feabab25a-goog
> >

Re: [PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Arnaldo Carvalho de Melo]

On Mon, Mar 10, 2025 at 06:28:01PM -0300, Arnaldo Carvalho de Melo wrote:
> On Mon, Mar 10, 2025 at 02:17:59PM -0700, Ian Rogers wrote:
> > On Mon, Mar 10, 2025 at 2:16 PM Arnaldo Carvalho de Melo
> > <acme@kernel.org> wrote:
> > >
> > > On Mon, Mar 10, 2025 at 06:15:42PM -0300, Arnaldo Carvalho de Melo wrote:
> > > > On Fri, Feb 28, 2025 at 02:23:08PM -0800, Ian Rogers wrote:
> > > > > Rather than manually configuring an evsel, switch to using
> > > > > parse_events for greater commonality with the rest of the perf code.
> > > > >
> > > > > Reviewed-by: Howard Chu <howardchu95@gmail.com>
> > > > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > >
> > > > Now will all in place I'm trying to test it and I am getting some
> > > > strange results:
> > > >
> > > > root@number:/home/acme/git/perf-tools-next# tools/perf/python/tracepoint.py
> > > > <SNIP lots of seemingly ok lines>
> > > > time 78318710956557 prev_comm=sudo prev_pid=3133818 prev_prio=120 prev_state=0x1 ==> next_comm=swapper/14 next_pid=0 next_prio=120
> > > > time 78318720082300 prev_comm=swapper/16 prev_pid=0 prev_prio=120 prev_state=0x0 ==> next_comm=kworker/u112:8 next_pid=1752774 next_prio=120
> > > > time 78318706232435 prev_comm=kworker/u112:17 prev_pid=1551246 prev_prio=120 prev_state=0x80 ==> next_comm=swapper/21 next_pid=0 next_prio=120
> > > > time 78318708202121 prev_comm=sudo prev_pid=3133818 prev_prio=120 prev_state=0x2 ==> next_comm=swapper/25 next_pid=0 next_prio=120
> > > > time 78318748346989 prev_comm=swapper/26 prev_pid=0 prev_prio=120 prev_state=0x0 ==> next_comm=gnome-terminal- next_pid=3551 next_prio=120
> > > > Traceback (most recent call last):
> > > >   File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 47, in <module>
> > > >     main()
> > > >   File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 42, in main
> > > >     event.next_comm,
> > > >     ^^^^^^^^^^^^^^^
> > > > AttributeError: 'perf.sample_event' object has no attribute 'next_comm'
> > > > root@number:/home/acme/git/perf-tools-next#
> > > >
> > > > But it shouldn't get there as there is this check:
> > > >
> > > >             if not isinstance(event, perf.sample_event):
> > > >                 continue
> > > >
> > > >
> > > > :-\
> > > >
> > > > Trying to debug that...
> > >
> > > And it doesn't seem related to your series, I checked and v6.13 has the
> > > same problem, I nuked the build dir, etc.
> > 
> > Right. I'd seen the same issue.
> 
> time 79411977132102 prev_comm=sudo prev_pid=3133818 prev_prio=120 prev_state=0x2 ==> next_comm=swapper/12 next_pid=0 next_prio=120
> { type: sample }
> time 79411977200343 prev_comm=kworker/u112:17 prev_pid=1551246 prev_prio=120 prev_state=0x80 ==> next_comm=swapper/16 next_pid=0 next_prio=120
> { type: sample }
> time 79411964535268 prev_comm=kworker/u112:14 prev_pid=810939 prev_prio=120 prev_state=0x80 ==> next_comm=swapper/17 next_pid=0 next_prio=120
> { type: sample }
> time 79411964746511 prev_comm=swapper/18 prev_pid=0 prev_prio=120 prev_state=0x0 ==> next_comm=kworker/u112:12 next_pid=2109251 next_prio=120
> { type: sample }
> Traceback (most recent call last):
>   File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 48, in <module>
>     main()
>   File "/home/acme/git/perf-tools-next/tools/perf/python/tracepoint.py", line 43, in main
>     event.next_comm,
>     ^^^^^^^^^^^^^^^
> AttributeError: 'perf.sample_event' object has no attribute 'next_comm'
> root@number:/home/acme/git/perf-tools-next#
> 
> And it says it is a sample... 
> 
> Well, ran out of time, will try later or early tomorrow, will also try
> to review the syscalltbl series and Howard's off cpu profiling.

Its some sort of corruption, I added printing the sample_* fields and
then up to the field before next_comm:

time 2462907120158 prev_comm=swapper/8 prev_pid=0 prev_prio=120 prev_state=0x0 ==>
next_comm=gnome-terminal- next_pid=3646 next_prio=120
ip 0 pid=0 tid=0 cpu=12
time 2462916970223 prev_comm=swapper/12 prev_pid=0 prev_prio=120 prev_state=0x0 ==>
next_comm=gnome-terminal- next_pid=3646 next_prio=120
ip 0 pid=0 tid=0 cpu=14
time 2462902120635 prev_comm= prev_pid=1919907691 prev_prio=796026219 prev_state=0x323a32313175 ==>
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'
root@number:/home/acme/git/perf-tools-next# 

See the empty prev_comm, bogus pref_pid, ditto for prev_state and
pref_prio and then it simple doesn't have that next_comm...

Also I noticed that in tracepoint_field() (tools/perf/util/python.c) we
can get the string tracepoint fields as a bytearray or as a string,
ahere:

                if (field->flags & TEP_FIELD_IS_STRING &&
                    is_printable_array(data + offset, len)) {
                        ret = PyUnicode_FromString((char *)data + offset);
                } else {
                        ret = PyByteArray_FromStringAndSize((const char *) data + offset, len);
                        field->flags &= ~TEP_FIELD_IS_STRING;
                }


For some reason in sessions where bytearrays are returned, and all comes
as bytearrays, the problem is not noticed.

But when it comes as a string it breaks after a short time, /me
scratches head...

- Arnaldo

Re: [PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Arnaldo Carvalho de Melo]

On Tue, Mar 11, 2025 at 12:32:05PM -0300, Arnaldo Carvalho de Melo wrote:
>                 if (field->flags & TEP_FIELD_IS_STRING &&
>                     is_printable_array(data + offset, len)) {
>                         ret = PyUnicode_FromString((char *)data + offset);
>                 } else {
>                         ret = PyByteArray_FromStringAndSize((const char *) data + offset, len);
>                         field->flags &= ~TEP_FIELD_IS_STRING;
>                 }
> 
> 
> For some reason in sessions where bytearrays are returned, and all comes
> as bytearrays, the problem is not noticed.
> 
> But when it comes as a string it breaks after a short time, /me
> scratches head...

I think I'm getting closer, with some debugging sprinkled in the python
binding:

ip 0 pid=74131 tid=74131 cpu=3
 ( field 'prev_comm' ret=0x7f0a66a1b970 )  ( field 'prev_pid' ret=0x7f0a66b1bed0 )  ( field 'prev_prio' ret=0x7f0a74f60d08 )  ( field 'prev_state' ret=0x7f0a74f60e08 ) time 4503271651784 prev_comm=kworker/u112:14 prev_pid=74131 prev_prio=120 prev_state=0x80 ==>
 ( field 'next_comm' ret=0x7f0a66a1b970 )  ( field 'next_pid' ret=0x7f0a74f5fe08 )  ( field 'next_prio' ret=0x7f0a74f60d08 ) next_comm=swapper/3 next_pid=0 next_prio=120
ip 0 pid=80209 tid=80209 cpu=4
 ( field 'prev_comm' ret=0x7f0a66a1b970 )  ( field 'prev_pid' ret=0x7f0a66b1bed0 )  ( field 'prev_prio' ret=0x7f0a74f60d08 )  ( field 'prev_state' ret=0x7f0a74f60e08 ) time 4503280531143 prev_comm=kworker/u112:3 prev_pid=80209 prev_prio=120 prev_state=0x80 ==>
 ( field 'next_comm' ret=0x7f0a66a1b970 )  ( field 'next_pid' ret=0x7f0a74f5fe08 )  ( field 'next_prio' ret=0x7f0a74f60d08 ) next_comm=swapper/4 next_pid=0 next_prio=120
ip 0 pid=74131 tid=74131 cpu=5
 ( XXX '�!' len=16)  ( field 'prev_comm' ret=(nil) ) 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 40, in main
    event.prev_comm,
    ^^^^^^^^^^^^^^^
AttributeError: 'perf.sample_event' object has no attribute 'prev_comm'


So the size of the field, in the tracepoint format file is 16, its a
comm:

root@number:/home/acme/git/perf-tools-next# grep '_comm\[' /sys/kernel/tracing/events/sched/sched_switch/format 
	field:char prev_comm[16];	offset:8;	size:16;	signed:0;
	field:char next_comm[16];	offset:40;	size:16;	signed:0;
root@number:/home/acme/git/perf-tools-next#

But:

root@number:/home/acme/git/perf-tools-next# cat /proc/74131/comm
kworker/u112:14-events_unbound
root@number:/home/acme/git/perf-tools-next#

root@number:/home/acme/git/perf-tools-next# echo -n "kworker/u112:14-events_unbound" | wc -c
30
root@number:/home/acme/git/perf-tools-next#

Which should explain that:

( XXX '�!' len=16)  ( field 'prev_comm' ret=(nil) )

That is printed by:

+++ b/tools/perf/util/python.c
@@ -318,16 +318,17 @@ tracepoint_field(const struct pyrf_event *pe, struct tep_format_field *field)
                        if (tep_field_is_relative(field->flags))
                                offset += field->offset + field->size;
                }
-               if (field->flags & TEP_FIELD_IS_STRING &&
-                   is_printable_array(data + offset, len)) {
+               if (field->flags & TEP_FIELD_IS_STRING) {
                        ret = PyUnicode_FromString((char *)data + offset);
+                       if (ret == NULL) {
+                               printf(" ( XXX '%.*s' len=%d) ", len, (char *)data + offset, len); fflush(stdout);
+                       }
                } else {
                        ret = PyByteArray_FromStringAndSize((const char *) data + offset, len);
-                       field->flags &= ~TEP_FIELD_IS_STRING;
                }

So now trying to figure out why when the comm lenght is more than 16 the
raw_data appears to contain trash...

- Arnaldo

Re: [PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Arnaldo Carvalho de Melo]

On Tue, Mar 11, 2025 at 02:06:45PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, Mar 11, 2025 at 12:32:05PM -0300, Arnaldo Carvalho de Melo wrote:
> >                 if (field->flags & TEP_FIELD_IS_STRING &&
> >                     is_printable_array(data + offset, len)) {
> >                         ret = PyUnicode_FromString((char *)data + offset);
> >                 } else {
> >                         ret = PyByteArray_FromStringAndSize((const char *) data + offset, len);
> >                         field->flags &= ~TEP_FIELD_IS_STRING;
> >                 }
> > 
> > 
> > For some reason in sessions where bytearrays are returned, and all comes
> > as bytearrays, the problem is not noticed.
> > 
> > But when it comes as a string it breaks after a short time, /me
> > scratches head...
> 
> I think I'm getting closer, with some debugging sprinkled in the python
> binding:
> 
> ip 0 pid=74131 tid=74131 cpu=3
>  ( field 'prev_comm' ret=0x7f0a66a1b970 )  ( field 'prev_pid' ret=0x7f0a66b1bed0 )  ( field 'prev_prio' ret=0x7f0a74f60d08 )  ( field 'prev_state' ret=0x7f0a74f60e08 ) time 4503271651784 prev_comm=kworker/u112:14 prev_pid=74131 prev_prio=120 prev_state=0x80 ==>
>  ( field 'next_comm' ret=0x7f0a66a1b970 )  ( field 'next_pid' ret=0x7f0a74f5fe08 )  ( field 'next_prio' ret=0x7f0a74f60d08 ) next_comm=swapper/3 next_pid=0 next_prio=120
> ip 0 pid=80209 tid=80209 cpu=4
>  ( field 'prev_comm' ret=0x7f0a66a1b970 )  ( field 'prev_pid' ret=0x7f0a66b1bed0 )  ( field 'prev_prio' ret=0x7f0a74f60d08 )  ( field 'prev_state' ret=0x7f0a74f60e08 ) time 4503280531143 prev_comm=kworker/u112:3 prev_pid=80209 prev_prio=120 prev_state=0x80 ==>
>  ( field 'next_comm' ret=0x7f0a66a1b970 )  ( field 'next_pid' ret=0x7f0a74f5fe08 )  ( field 'next_prio' ret=0x7f0a74f60d08 ) next_comm=swapper/4 next_pid=0 next_prio=120
> ip 0 pid=74131 tid=74131 cpu=5
>  ( XXX '�!' len=16)  ( field 'prev_comm' ret=(nil) ) 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 40, in main
>     event.prev_comm,
>     ^^^^^^^^^^^^^^^
> AttributeError: 'perf.sample_event' object has no attribute 'prev_comm'
> 
> 
> So the size of the field, in the tracepoint format file is 16, its a
> comm:
> 
> root@number:/home/acme/git/perf-tools-next# grep '_comm\[' /sys/kernel/tracing/events/sched/sched_switch/format 
> 	field:char prev_comm[16];	offset:8;	size:16;	signed:0;
> 	field:char next_comm[16];	offset:40;	size:16;	signed:0;
> root@number:/home/acme/git/perf-tools-next#
> 
> But:
> 
> root@number:/home/acme/git/perf-tools-next# cat /proc/74131/comm
> kworker/u112:14-events_unbound
> root@number:/home/acme/git/perf-tools-next#
> 
> root@number:/home/acme/git/perf-tools-next# echo -n "kworker/u112:14-events_unbound" | wc -c
> 30
> root@number:/home/acme/git/perf-tools-next#
> 
> Which should explain that:
> 
> ( XXX '�!' len=16)  ( field 'prev_comm' ret=(nil) )
> 
> That is printed by:
> 
> +++ b/tools/perf/util/python.c
> @@ -318,16 +318,17 @@ tracepoint_field(const struct pyrf_event *pe, struct tep_format_field *field)
>                         if (tep_field_is_relative(field->flags))
>                                 offset += field->offset + field->size;
>                 }
> -               if (field->flags & TEP_FIELD_IS_STRING &&
> -                   is_printable_array(data + offset, len)) {
> +               if (field->flags & TEP_FIELD_IS_STRING) {
>                         ret = PyUnicode_FromString((char *)data + offset);
> +                       if (ret == NULL) {
> +                               printf(" ( XXX '%.*s' len=%d) ", len, (char *)data + offset, len); fflush(stdout);
> +                       }
>                 } else {
>                         ret = PyByteArray_FromStringAndSize((const char *) data + offset, len);
> -                       field->flags &= ~TEP_FIELD_IS_STRING;
>                 }
> 
> So now trying to figure out why when the comm lenght is more than 16 the
> raw_data appears to contain trash...

So it seems to be something just in the python binding, as perf trace
seems to handle it well:

 ( 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'
root@number:/home/acme/git/perf-tools-next# cat /proc/125355/comm
kworker/u112:0-i915
root@number:/home/acme/git/perf-tools-next# 
root@number:/home/acme/git/perf-tools-next# 
root@number:/home/acme/git/perf-tools-next# perf trace -e sched:sched_switch -p 125355
     0.000 sched:sched_switch(prev_comm: "kworker/u112:0", prev_pid: 125355 (kworker/u112:0-), prev_prio: 120, prev_state: 128, next_comm: "swapper/6", next_prio: 120)
^Croot@number:/home/acme/git/perf-tools-next#

I.e. it chops up the prev_comm size to what is specified in the
tracepoint format.

And that sample->raw_size is the same accross the sched:sched_switch
raw_datas (seemingly suboptimal, most are less than 16 bytes, but
probably its not guaranteed that the \0 will be there, so copy all the
16 bytes).

Now to try to figure out why simply using PyUnicode_FromStringAndSize
doesn't work...

- Arnaldo

Re: [PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Arnaldo Carvalho de Melo]

On Tue, Mar 11, 2025 at 03:49:37PM -0300, Arnaldo Carvalho de Melo wrote:
> So it seems to be something just in the python binding, as perf trace
> seems to handle it well:
> 
>  ( 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'
> root@number:/home/acme/git/perf-tools-next# cat /proc/125355/comm
> kworker/u112:0-i915
> root@number:/home/acme/git/perf-tools-next# 
> root@number:/home/acme/git/perf-tools-next# 
> root@number:/home/acme/git/perf-tools-next# perf trace -e sched:sched_switch -p 125355
>      0.000 sched:sched_switch(prev_comm: "kworker/u112:0", prev_pid: 125355 (kworker/u112:0-), prev_prio: 120, prev_state: 128, next_comm: "swapper/6", next_prio: 120)
> ^Croot@number:/home/acme/git/perf-tools-next#
> 
> I.e. it chops up the prev_comm size to what is specified in the
> tracepoint format.
> 
> And that sample->raw_size is the same accross the sched:sched_switch
> raw_datas (seemingly suboptimal, most are less than 16 bytes, but
> probably its not guaranteed that the \0 will be there, so copy all the
> 16 bytes).
> 
> Now to try to figure out why simply using PyUnicode_FromStringAndSize
> doesn't work...

Didn't manage to make progress on this, I spent more time than I
expected as I think this could be some sort of canary on some coal mine,
but with the patch below, that gives up and just avoids touching the
COMM fields and don't switch from string to bytearray in the binding, it
runs forever, this is just a data point in case somebody wants to
pursue.

That flipping from string to not string based on just one entry not
being acceptable is questionable, and I think it should go away, but why
when COMM fields are bigger what is alloted to them in the tracepoint
ends up tripping up just the python binding is something I couldn't
grasp in today's session.

Namhyung, this is something open, but not caused by Ian's patchset, for
which I give my:

Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>

In addition to the tags I provided patch by patch.

- Arnaldo

diff --git a/tools/perf/python/tracepoint.py b/tools/perf/python/tracepoint.py
index 38b2b6d11f64566a..965b50afbdafeeb2 100755
--- a/tools/perf/python/tracepoint.py
+++ b/tools/perf/python/tracepoint.py
@@ -33,15 +33,12 @@ def main():
             if not isinstance(event, perf.sample_event):
                 continue
 
-            print("time %u prev_comm=%s prev_pid=%d prev_prio=%d prev_state=0x%x ==> next_comm=%s next_pid=%d next_prio=%d" % (
-                   event.sample_time,
-                   event.prev_comm,
-                   event.prev_pid,
-                   event.prev_prio,
-                   event.prev_state,
-                   event.next_comm,
-                   event.next_pid,
-                   event.next_prio))
+            try:
+                print("time %u prev_comm=%s prev_pid=%d prev_prio=%d prev_state=0x%x ==> next_comm=%s next_pid=%d next_prio=%d" % (
+                       event.sample_time, event.prev_comm, event.prev_pid, event.prev_prio, event.prev_state, event.next_comm, event.next_pid, event.next_prio))
+            except:
+                print("time %u prev_pid=%d prev_prio=%d prev_state=0x%x ==> next_pid=%d next_prio=%d" % (
+                       event.sample_time, event.prev_pid, event.prev_prio, event.prev_state, event.next_pid, event.next_prio))
 
 if __name__ == '__main__':
     main()
diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
index 6c5bb5e8893998ae..3eb77bd270077cb3 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -318,13 +318,10 @@ tracepoint_field(const struct pyrf_event *pe, struct tep_format_field *field)
 			if (tep_field_is_relative(field->flags))
 				offset += field->offset + field->size;
 		}
-		if (field->flags & TEP_FIELD_IS_STRING &&
-		    is_printable_array(data + offset, len)) {
+		if (field->flags & TEP_FIELD_IS_STRING)
 			ret = PyUnicode_FromString((char *)data + offset);
-		} else {
+		else
 			ret = PyByteArray_FromStringAndSize((const char *) data + offset, len);
-			field->flags &= ~TEP_FIELD_IS_STRING;
-		}
 	} else {
 		val = tep_read_number(pevent, data + field->offset,
 				      field->size);

Re: [PATCH v2 06/11] perf python: Add optional cpus and threads arguments to parse_events

[Namhyung Kim]

Hi Ian,

On Mon, Mar 10, 2025 at 05:28:48PM -0700, Ian Rogers wrote:
> On Mon, Mar 10, 2025 at 3:12 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Fri, Feb 28, 2025 at 02:23:03PM -0800, Ian Rogers wrote:
> > > Used for the evlist initialization.
> > >
> > > Reviewed-by: Howard Chu <howardchu95@gmail.com>
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/util/python.c | 10 ++++++++--
> > >  1 file changed, 8 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> > > index b600b6379b4e..4a3015e7dc83 100644
> > > --- a/tools/perf/util/python.c
> > > +++ b/tools/perf/util/python.c
> > > @@ -1339,12 +1339,18 @@ static PyObject *pyrf__parse_events(PyObject *self, PyObject *args)
> > >       struct evlist evlist = {};
> > >       struct parse_events_error err;
> > >       PyObject *result;
> > > +     PyObject *pcpus = NULL, *pthreads = NULL;
> > > +     struct perf_cpu_map *cpus;
> > > +     struct perf_thread_map *threads;
> > >
> > > -     if (!PyArg_ParseTuple(args, "s", &input))
> > > +     if (!PyArg_ParseTuple(args, "s|OO", &input, &pcpus, &pthreads))
> > >               return NULL;
> > >
> > > +     threads = pthreads ? ((struct pyrf_thread_map *)pthreads)->threads : NULL;
> > > +     cpus = pcpus ? ((struct pyrf_cpu_map *)pcpus)->cpus : NULL;
> >
> > I wonder if it needs any type checks before accessing them.
> 
> Agreed. We don't do it yet elsewhere:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/python.c?h=perf-tools-next#n769
> although there keywords potentially avoid ambiguity. Given this I
> think improving the typing is follow up work.

Sounds good.

Thanks,
Namhyung

> >
> > > +
> > >       parse_events_error__init(&err);
> > > -     evlist__init(&evlist, NULL, NULL);
> > > +     evlist__init(&evlist, cpus, threads);
> > >       if (parse_events(&evlist, input, &err)) {
> > >               parse_events_error__print(&err, input);
> > >               PyErr_SetFromErrno(PyExc_OSError);
> > > --
> > > 2.48.1.711.g2feabab25a-goog
> > >

Re: [PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Namhyung Kim]

On Mon, Mar 10, 2025 at 04:40:36PM -0700, Ian Rogers wrote:
> On Mon, Mar 10, 2025 at 3:17 PM Namhyung Kim <namhyung@kernel.org> wrote:
> >
> > On Fri, Feb 28, 2025 at 02:23:08PM -0800, Ian Rogers wrote:
> > > Rather than manually configuring an evsel, switch to using
> > > parse_events for greater commonality with the rest of the perf code.
> > >
> > > Reviewed-by: Howard Chu <howardchu95@gmail.com>
> > > Signed-off-by: Ian Rogers <irogers@google.com>
> > > ---
> > >  tools/perf/python/tracepoint.py | 23 +++++++++++------------
> > >  1 file changed, 11 insertions(+), 12 deletions(-)
> > >
> > > diff --git a/tools/perf/python/tracepoint.py b/tools/perf/python/tracepoint.py
> > > index bba68a6d4515..38b2b6d11f64 100755
> > > --- a/tools/perf/python/tracepoint.py
> > > +++ b/tools/perf/python/tracepoint.py
> > > @@ -5,24 +5,23 @@
> > >
> > >  import perf
> > >
> > > -class tracepoint(perf.evsel):
> > > -    def __init__(self, sys, name):
> > > -        config = perf.tracepoint(sys, name)
> > > -        perf.evsel.__init__(self,
> > > -                            type   = perf.TYPE_TRACEPOINT,
> > > -                            config = config,
> > > -                            freq = 0, sample_period = 1, wakeup_events = 1,
> > > -                            sample_type = perf.SAMPLE_PERIOD | perf.SAMPLE_TID | perf.SAMPLE_CPU | perf.SAMPLE_RAW | perf.SAMPLE_TIME)
> > > -
> > >  def main():
> > > -    tp      = tracepoint("sched", "sched_switch")
> > >      cpus    = perf.cpu_map()
> > >      threads = perf.thread_map(-1)
> > > +    evlist = perf.parse_events("sched:sched_switch", cpus, threads)
> > > +    # Disable tracking of mmaps and similar that are unnecessary.
> > > +    for ev in evlist:
> > > +        ev.tracking = False
> > > +    # Configure evsels with default record options.
> > > +    evlist.config()
> >
> > I think the default option uses frequency of 4000 but tracepoints want
> > to use period of 1.  Also I'm not sure if it sets the proper sample type
> > bits namely PERF_SAMPLE_RAW.
> 
> I used trace to ensure they matched. Fwiw, the sample_period for a
> tracepoint is set to 1 in evsel__newtp_idx:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/evsel.c?h=perf-tools-next#n621
> and the evsel__config won't overwrite an already set sample_period:
> https://web.git.kernel.org/pub/scm/linux/kernel/git/perf/perf-tools-next.git/tree/tools/perf/util/evsel.c?h=perf-tools-next#n1341

Ok, it only sets the default value and it will use the attr from
evsel__newtp_idx().  Thanks for checking this.

Thanks,
Namhyung

> >
> >
> > > +    # Simplify the sample_type and read_format of evsels
> > > +    for ev in evlist:
> > > +        ev.sample_type = ev.sample_type & ~perf.SAMPLE_IP
> > > +        ev.read_format = 0
> > >
> > > -    evlist = perf.evlist(cpus, threads)
> > > -    evlist.add(tp)
> > >      evlist.open()
> > >      evlist.mmap()
> > > +    evlist.enable();
> > >
> > >      while True:
> > >          evlist.poll(timeout = -1)
> > > --
> > > 2.48.1.711.g2feabab25a-goog
> > >

Re: [PATCH v2 11/11] perf python tracepoint: Switch to using parse_events

[Namhyung Kim]

On Tue, Mar 11, 2025 at 06:52:45PM -0300, Arnaldo Carvalho de Melo wrote:
> On Tue, Mar 11, 2025 at 03:49:37PM -0300, Arnaldo Carvalho de Melo wrote:
> > So it seems to be something just in the python binding, as perf trace
> > seems to handle it well:
> > 
> >  ( 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'
> > root@number:/home/acme/git/perf-tools-next# cat /proc/125355/comm
> > kworker/u112:0-i915
> > root@number:/home/acme/git/perf-tools-next# 
> > root@number:/home/acme/git/perf-tools-next# 
> > root@number:/home/acme/git/perf-tools-next# perf trace -e sched:sched_switch -p 125355
> >      0.000 sched:sched_switch(prev_comm: "kworker/u112:0", prev_pid: 125355 (kworker/u112:0-), prev_prio: 120, prev_state: 128, next_comm: "swapper/6", next_prio: 120)
> > ^Croot@number:/home/acme/git/perf-tools-next#
> > 
> > I.e. it chops up the prev_comm size to what is specified in the
> > tracepoint format.
> > 
> > And that sample->raw_size is the same accross the sched:sched_switch
> > raw_datas (seemingly suboptimal, most are less than 16 bytes, but
> > probably its not guaranteed that the \0 will be there, so copy all the
> > 16 bytes).
> > 
> > Now to try to figure out why simply using PyUnicode_FromStringAndSize
> > doesn't work...
> 
> Didn't manage to make progress on this, I spent more time than I
> expected as I think this could be some sort of canary on some coal mine,
> but with the patch below, that gives up and just avoids touching the
> COMM fields and don't switch from string to bytearray in the binding, it
> runs forever, this is just a data point in case somebody wants to
> pursue.
> 
> That flipping from string to not string based on just one entry not
> being acceptable is questionable, and I think it should go away, but why
> when COMM fields are bigger what is alloted to them in the tracepoint
> ends up tripping up just the python binding is something I couldn't
> grasp in today's session.
> 
> Namhyung, this is something open, but not caused by Ian's patchset, for
> which I give my:
> 
> Tested-by: Arnaldo Carvalho de Melo <acme@redhat.com>

Thanks for the analysis!

> 
> In addition to the tags I provided patch by patch.

Ian, can you check if it works well for you?

Thanks,
Namhyung

 
> diff --git a/tools/perf/python/tracepoint.py b/tools/perf/python/tracepoint.py
> index 38b2b6d11f64566a..965b50afbdafeeb2 100755
> --- a/tools/perf/python/tracepoint.py
> +++ b/tools/perf/python/tracepoint.py
> @@ -33,15 +33,12 @@ def main():
>              if not isinstance(event, perf.sample_event):
>                  continue
>  
> -            print("time %u prev_comm=%s prev_pid=%d prev_prio=%d prev_state=0x%x ==> next_comm=%s next_pid=%d next_prio=%d" % (
> -                   event.sample_time,
> -                   event.prev_comm,
> -                   event.prev_pid,
> -                   event.prev_prio,
> -                   event.prev_state,
> -                   event.next_comm,
> -                   event.next_pid,
> -                   event.next_prio))
> +            try:
> +                print("time %u prev_comm=%s prev_pid=%d prev_prio=%d prev_state=0x%x ==> next_comm=%s next_pid=%d next_prio=%d" % (
> +                       event.sample_time, event.prev_comm, event.prev_pid, event.prev_prio, event.prev_state, event.next_comm, event.next_pid, event.next_prio))
> +            except:
> +                print("time %u prev_pid=%d prev_prio=%d prev_state=0x%x ==> next_pid=%d next_prio=%d" % (
> +                       event.sample_time, event.prev_pid, event.prev_prio, event.prev_state, event.next_pid, event.next_prio))
>  
>  if __name__ == '__main__':
>      main()
> diff --git a/tools/perf/util/python.c b/tools/perf/util/python.c
> index 6c5bb5e8893998ae..3eb77bd270077cb3 100644
> --- a/tools/perf/util/python.c
> +++ b/tools/perf/util/python.c
> @@ -318,13 +318,10 @@ tracepoint_field(const struct pyrf_event *pe, struct tep_format_field *field)
>  			if (tep_field_is_relative(field->flags))
>  				offset += field->offset + field->size;
>  		}
> -		if (field->flags & TEP_FIELD_IS_STRING &&
> -		    is_printable_array(data + offset, len)) {
> +		if (field->flags & TEP_FIELD_IS_STRING)
>  			ret = PyUnicode_FromString((char *)data + offset);
> -		} else {
> +		else
>  			ret = PyByteArray_FromStringAndSize((const char *) data + offset, len);
> -			field->flags &= ~TEP_FIELD_IS_STRING;
> -		}
>  	} else {
>  		val = tep_read_number(pevent, data + field->offset,
>  				      field->size);

Re: [PATCH v2 00/11] Python improvements for a real use of parse_events

[Namhyung Kim]

On Fri, 28 Feb 2025 14:22:57 -0800, Ian Rogers wrote:
> While parse_events access in python was added, it wasn't used by any
> python script. In enabling this for the tracepoint.py script a number
> of latent bugs and necessary improvements were discovered.
> 
> v2: Fix a whitespace issue on the evlist.config patch spotted by
>     Howard and add his reviewed-by tags.
> 
> [...]
Applied to perf-tools-next, thanks!

Best regards,
Namhyung