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

[PATCH v1 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.

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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 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.

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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 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.

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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 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.

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 697428efa644..5324a8916da1 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -511,6 +511,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;
@@ -634,7 +644,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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 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.

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 b4bc57859f73..8f68320130c4 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -1019,6 +1019,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",
@@ -1056,6 +1070,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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 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.

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 8f68320130c4..6a7042fd2569 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -802,6 +802,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 = {
@@ -811,6 +833,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.47.1.613.gc27f4b7a9f-goog

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

[Ian Rogers]

Used for the evlist initialization.

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 6a7042fd2569..45ee6479e4dd 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -1330,12 +1330,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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 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.

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 45ee6479e4dd..9ffe1b633027 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -1303,6 +1303,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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 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.

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 9ffe1b633027..d1ce3b6b5bf1 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -336,23 +336,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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 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.

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 d1ce3b6b5bf1..1e8ab5291637 100644
--- a/tools/perf/util/python.c
+++ b/tools/perf/util/python.c
@@ -865,6 +865,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)
 {
@@ -1048,6 +1058,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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 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.

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 1e8ab5291637..38a7e16e09c9 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"
@@ -1043,6 +1045,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);
@@ -1100,6 +1127,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.47.1.613.gc27f4b7a9f-goog

[PATCH v1 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.

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.47.1.613.gc27f4b7a9f-goog

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

[Ian Rogers]

On Wed, Jan 8, 2025 at 11:51 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.

Ping.

Thanks,
Ian

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

[Ian Rogers]

On Tue, Feb 4, 2025 at 2:23 PM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Jan 8, 2025 at 11:51 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.
>
> Ping.

Ping.

Thanks,
Ian

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

[Namhyung Kim]

Hi Ian,

On Mon, Feb 10, 2025 at 08:06:34AM -0800, Ian Rogers wrote:
> On Tue, Feb 4, 2025 at 2:23 PM Ian Rogers <irogers@google.com> wrote:
> >
> > On Wed, Jan 8, 2025 at 11:51 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.
> >
> > Ping.
> 
> Ping.

Thanks for the reminder.  I'll review pending patches this week.

Arnaldo, can you please help reviewing this series?

Thanks,
Namhyung

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

[Ian Rogers]

On Mon, Feb 10, 2025 at 1:52 PM Arnaldo Carvalho de Melo
<arnaldo.melo@gmail.com> wrote:
>
>
>
> On Mon, Feb 10, 2025, 4:42 PM Namhyung Kim <namhyung@kernel.org> wrote:
>>
>> Hi Ian,
>>
>> On Mon, Feb 10, 2025 at 08:06:34AM -0800, Ian Rogers wrote:
>> > On Tue, Feb 4, 2025 at 2:23 PM Ian Rogers <irogers@google.com> wrote:
>> > >
>> > > On Wed, Jan 8, 2025 at 11:51 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.
>> > >
>> > > Ping.
>> >
>> > Ping.
>>
>> Thanks for the reminder.  I'll review pending patches this week.
>>
>> Arnaldo, can you please help reviewing this series?
>
>
> Sure, I'm getting back to doing that, more slowly than I'd like this time. :-/

Ping.

Thanks,
Ian

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

[Howard Chu]

Hi Ian,

On Wed, Jan 8, 2025 at 11:51 PM Ian Rogers <irogers@google.com> 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.

May I ask how it created the recursion? Too ignorant to understand sorry...

Thanks,
Howard

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

[Howard Chu]

Hello,

Thanks for fixing another bug! :) The freeing in evsel__exit() is
present as well.

Thanks,
Howard

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

[Ian Rogers]

On Wed, Feb 26, 2025 at 9:40 PM Howard Chu <howardchu95@gmail.com> wrote:
>
> Hi Ian,
>
> On Wed, Jan 8, 2025 at 11:51 PM Ian Rogers <irogers@google.com> 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.
>
> May I ask how it created the recursion? Too ignorant to understand sorry...

If _debug_file is NULL then debug_file calls pr_warning_once which
calls debug_file which calls pr_warning_once again, etc ... :-)

Thanks,
Ian

> Thanks,
> Howard

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

[Howard Chu]

Hello,

On Wed, Jan 8, 2025 at 11:51 PM Ian Rogers <irogers@google.com> wrote:
>
> Add access to evlist__config that is used to configure an evlist with
> record options.
>
> Signed-off-by: Ian Rogers <irogers@google.com>
> ---
>  tools/perf/util/python.c | 33 +++++++++++++++++++++++++++++++++
>  1 file changed, 33 insertions(+)
>

<SNIP>

> +               .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,

Are these indentations intentional?

Thanks,
Howard

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

[Howard Chu]

Hello Ian,

Sorry for the late reply—I just recovered from a nasty flu.

perf $ git log --oneline | head -n 11
0c1d18794589 perf python tracepoint: Switch to using parse_events
94dc6574ff24 perf python: Add evlist.config to set up record options
ebc33f96a37d perf python: Add evlist all_cpus accessor
c1ff82e366ee perf python: Avoid duplicated code in get_tracepoint_field
5293474151de perf python: Update ungrouped evsel leader in clone
47f66c784109 perf python: Add optional cpus and threads arguments to
parse_events
d4f592ec635e perf python: Add member access to a number of evsel variables
f4a3006a2c87 perf python: Add evlist enable and disable methods
2e040a422d0a perf evsel: tp_format accessing improvements
d79fd699f99a perf evlist: Add success path to evlist__create_syswide_maps
054b63706989 perf debug: Avoid stack overflow in recursive error message


perf $ sudo python python/tracepoint.py
libperf: mmap_per_cpu: nr cpu values 4 nr threads 1
libperf: idx 0: mmapping fd 3
libperf: idx 1: mmapping fd 4
libperf: idx 2: mmapping fd 5
libperf: idx 3: mmapping fd 6
time 35749194145330 prev_comm=python prev_pid=233668 prev_prio=120
prev_state=0x100 ==> next_comm=migration/0 next_pid=20 next_prio=0
time 35749196508397 prev_comm=python prev_pid=233668 prev_prio=120
prev_state=0x100 ==> next_comm=migration/1 next_pid=25 next_prio=0
time 35749196575872 prev_comm=python prev_pid=233668 prev_prio=120
prev_state=0x100 ==> next_comm=migration/2 next_pid=31 next_prio=0
time 35749194158623 prev_comm=migration/0 prev_pid=20 prev_prio=0
prev_state=0x1 ==> next_comm=swapper/0 next_pid=0 next_prio=120

Worked very well.

On Wed, Jan 8, 2025 at 11:51 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.
>
> 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

Not an expert in this but:

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

Thanks,
Howard
>
>  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.47.1.613.gc27f4b7a9f-goog
>

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

[Howard Chu]

Hello,

On Wed, Feb 26, 2025 at 9:45 PM Ian Rogers <irogers@google.com> wrote:
>
> On Wed, Feb 26, 2025 at 9:40 PM Howard Chu <howardchu95@gmail.com> wrote:
> >
> > Hi Ian,
> >
> > On Wed, Jan 8, 2025 at 11:51 PM Ian Rogers <irogers@google.com> 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.
> >
> > May I ask how it created the recursion? Too ignorant to understand sorry...
>
> If _debug_file is NULL then debug_file calls pr_warning_once which
> calls debug_file which calls pr_warning_once again, etc ... :-)

Got it,

pr_warning()
  eprintf()
    veprintf()
      debug_file()

ack.

Thanks,
Howard
>
> Thanks,
> Ian
>
> > Thanks,
> > Howard

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

[Ian Rogers]

On Wed, Feb 26, 2025 at 9:46 PM Howard Chu <howardchu95@gmail.com> wrote:
>
> Hello,
>
> On Wed, Jan 8, 2025 at 11:51 PM Ian Rogers <irogers@google.com> wrote:
> >
> > Add access to evlist__config that is used to configure an evlist with
> > record options.
> >
> > Signed-off-by: Ian Rogers <irogers@google.com>
> > ---
> >  tools/perf/util/python.c | 33 +++++++++++++++++++++++++++++++++
> >  1 file changed, 33 insertions(+)
> >
>
> <SNIP>
>
> > +               .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,
>
> Are these indentations intentional?

Nope, not sure what happened. I'll fix it in v2.

Thanks and thanks for the tags!
Ian

>
> Thanks,
> Howard