lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z9CwrQTEJoT_rRNA@x1>
Date: Tue, 11 Mar 2025 18:52:45 -0300
From: Arnaldo Carvalho de Melo <acme@...nel.org>
To: Namhyung Kim <namhyung@...nel.org>, Ian Rogers <irogers@...gle.com>
Cc: Peter Zijlstra <peterz@...radead.org>, Ingo Molnar <mingo@...hat.com>,
	Mark Rutland <mark.rutland@....com>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
	Jiri Olsa <jolsa@...nel.org>,
	Adrian Hunter <adrian.hunter@...el.com>,
	Kan Liang <kan.liang@...ux.intel.com>,
	Yicong Yang <yangyicong@...ilicon.com>,
	James Clark <james.clark@...aro.org>,
	"Dr. David Alan Gilbert" <linux@...blig.org>,
	Levi Yun <yeoreum.yun@....com>, Ze Gao <zegao2021@...il.com>,
	Weilin Wang <weilin.wang@...el.com>, Xu Yang <xu.yang_2@....com>,
	linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
	Howard Chu <howardchu95@...il.com>
Subject: Re: [PATCH v2 11/11] perf python tracepoint: Switch to using
 parse_events

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@...ber:/home/acme/git/perf-tools-next# cat /proc/125355/comm
> kworker/u112:0-i915
> root@...ber:/home/acme/git/perf-tools-next# 
> root@...ber:/home/acme/git/perf-tools-next# 
> root@...ber:/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@...ber:/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@...hat.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);

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ