| 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
| ||
|
Message-ID: <CAP-5=fVYXRzQjRzcDX0aJv5yg3bwDO+PWHfP-Laig0s3cnzcaQ@mail.gmail.com>
Date: Mon, 12 May 2025 12:38:23 -0700
From: Ian Rogers <irogers@...gle.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: Gautam Menghani <gautam@...ux.ibm.com>, namhyung@...nel.org, peterz@...radead.org,
mingo@...hat.com, mark.rutland@....com, alexander.shishkin@...ux.intel.com,
jolsa@...nel.org, adrian.hunter@...el.com, kan.liang@...ux.intel.com,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org,
maddy@...ux.ibm.com
Subject: Re: [PATCH v2 4/4] perf python: Add counting.py as example for
counting perf events
On Mon, May 12, 2025 at 10:49 AM Arnaldo Carvalho de Melo
<acme@...nel.org> wrote:
>
> On Mon, May 12, 2025 at 10:23:39AM -0700, Ian Rogers wrote:
> > On Sun, May 11, 2025 at 10:58 PM Gautam Menghani <gautam@...ux.ibm.com> wrote:
> > > Add counting.py - a python version of counting.c to demonstrate
> > > measuring and reading of counts for given perf events.
>
> > > Signed-off-by: Gautam Menghani <gautam@...ux.ibm.com>
> > > ---
> > > v1 -> v2:
> > > 1. Use existing iteration support instead of next
> > > 2. Read the counters on all cpus
> > > 3. Use existing helper functions
> > >
> > > tools/perf/python/counting.py | 34 ++++++++++++++++++++++++++++++++++
> > > 1 file changed, 34 insertions(+)
> > > create mode 100755 tools/perf/python/counting.py
>
> > > diff --git a/tools/perf/python/counting.py b/tools/perf/python/counting.py
> > > new file mode 100755
> > > index 000000000000..e535e3ae8bdf
> > > --- /dev/null
> > > +++ b/tools/perf/python/counting.py
> > > @@ -0,0 +1,34 @@
> > > +#!/usr/bin/env python3
> > > +# SPDX-License-Identifier: GPL-2.0
> > > +# -*- python -*-
> > > +# -*- coding: utf-8 -*-
> > > +
> > > +import perf
> > > +
> > > +def main():
> > > + cpus = perf.cpu_map()
> > > + thread_map = perf.thread_map(-1)
> > > + evlist = perf.parse_events("cpu-clock,task-clock", cpus, thread_map)
>
> > Thanks Gautam! I think this is really good. Perhaps the events could
> > be a command line option, but I can see why you want to keep this
> > similar to counting.c.
>
> > > +
> > > + for ev in evlist:
> > > + ev.read_format = perf.FORMAT_TOTAL_TIME_ENABLED | perf.FORMAT_TOTAL_TIME_RUNNING
> > > +
> > > + evlist.open()
> > > + evlist.enable()
> > > +
> > > + count = 100000
> > > + while count > 0:
> > > + count -= 1
> > > +
> > > + evlist.disable()
> > > +
> > > + for evsel in evlist:
> > > + for cpu in cpus:
> > > + for thread in range(len(thread_map)):
>
> > I kind of wish, for the reason of being intention revealing, this could just be:
>
> > for thread in thread_map:
>
> > I can see the problem though, the counts lack the thread_map and the
> > thread_map is needed to turn a thread back into an index. Perhaps when
> > the python counts is created we hold onto the evsel so that this is
> > possible. I also suspect that in the code:
>
> > for cpu in cpus:
>
> > The CPU number is being used rather than its index, which is a similar
> > story/problem.
>
> Lemme see the rest of this code...
>
> +static PyObject *pyrf_evsel__read(struct pyrf_evsel *pevsel,
> + PyObject *args, PyObject *kwargs)
> +{
> + struct evsel *evsel = &pevsel->evsel;
> + int cpu_map_idx = 0, thread = 0;
> + struct perf_counts_values counts;
> + struct pyrf_counts_values *count_values = PyObject_New(struct pyrf_counts_values,
> + &pyrf_counts_values__type);
> +
> + if (!PyArg_ParseTuple(args, "ii", &cpu_map_idx, &thread))
> + return NULL;
> +
> + perf_evsel__read(&(evsel->core), cpu_map_idx, thread, &counts);
> + count_values->values = counts;
> + return (PyObject *)count_values;
> +}
>
> Yeah, it is expecting the cpu_map_idx but the cpu number is being used,
> that is a bug.
>
> The way perf_evsel__read() is implemented:
>
> int perf_evsel__read(struct perf_evsel *evsel, int cpu_map_idx, int thread,
> struct perf_counts_values *count)
>
> It expects a cpu_map index, not a cpu and then a thread that in its
> prototype seems to imply its not an index? But it is an index as it ends
> up being the 'y' for:
>
> xyarray__entry(_evsel->mmap, _cpu_map_idx, _thread))
>
> :-/
Yeah. In the C code we've pretty much committed to notions of cpu map
index and CPU. We're more ambiguous with threads, but generally thread
is actually thread index into the thread map. As you say it is for the
xyarray so that we can densely pack things by index rather than having
huge gaps, say between PIDs. For the python we don't have to have a
1:1 mapping with the C code, so I was wondering if we could just
remove the notions of index and have them be implementation details?
This would lead to an unfortunate O(log n) translation between
thread/CPU and index (perf_cpu_map__idx) at the API boundary in
python.c.
> So probably its best to do it using indexes and when needing to know the
> pid or cpu number then use some helper to get the entry at the given
> entry? At least for the perf_evsel__read() API that seems to be the
> case, right?
>
> > Arnaldo, could you give some input on what to do wrt indices, threads
> > and CPUs at the API level? Perhaps we need a refactor and objects for
> > perf CPU and perf thread, similar to the use of struct perf_cpu in the
> > C code. The original API all pre-dates that change. The issue is that
> > changing the API could break existing scripts and we can only fix
> > those that ship with perf.
>
> So the original user of the perf python binding was:
>
> https://git.kernel.org/pub/scm/utils/tuna/tuna.git/tree/tuna/gui/procview.py
>
> That does basically what the above example does:
>
> def perf_init(self):
> self.cpu_map = perf.cpu_map()
> self.thread_map = perf.thread_map()
> self.evsel_cycles = perf.evsel(task=1, comm=1, wakeup_events=1, \
> watermark=1, sample_type=perf.SAMPLE_CPU | perf.SAMPLE_TID)
> self.evsel_cycles.open(cpus=self.cpu_map, threads=self.thread_map)
> self.evlist = perf.evlist(self.cpu_map, self.thread_map)
> self.evlist.add(self.evsel_cycles)
> self.evlist.mmap()
> self.pollfd = self.evlist.get_pollfd()
> for f in self.pollfd:
> GObject.io_add_watch(f, GObject.IO_IN, self.perf_process_events)
> self.perf_counter = {}
>
> Then:
>
> def perf_process_events(self, source, condition):
> had_events = True
> while had_events:
> had_events = False
> for cpu in self.cpu_map:
> event = self.evlist.read_on_cpu(cpu)
> if event:
> had_events = True
> if event.type == perf.RECORD_FORK:
> if event.pid == event.tid:
> try:
> self.ps.processes[event.pid] = procfs.process(event.pid)
> except: # short lived thread
> pass
> else:
> if event.pid in self.ps.processes:
> try:
> self.ps.processes[event.pid].threads.processes[event.tid] = procfs.process(event.tid)
> except (AttributeError, KeyError):
> try:
> self.ps.processes[event.pid].threads = procfs.pidstats("/proc/%d/task/" % event.pid)
> except:
> pass
> elif event.type == perf.RECORD_EXIT:
> del self.ps[int(event.tid)]
> elif event.type == perf.RECORD_SAMPLE:
> tid = event.sample_tid
> if tid in self.perf_counter:
> self.perf_counter[tid] += event.sample_period
> else:
> self.perf_counter[tid] = event.sample_period
>
> self.evlist_added = True # Mark that event arrived, so next periodic show() will refresh GUI
> return True
>
>
> So it was more for catching new/dead threads without having to process /proc.
Yeah. I think the sampling API is okay. The nice thing with Gautum's
patches is adding support for counters for use-cases like perf stat.
Thanks,
Ian
Powered by blists - more mailing lists