Message-ID: <CAP-5=fW4JQYJ2NCRsRVePidCcZ9+JcQbfY=xQ00xZG-bSn96ew@mail.gmail.com>
Date: Tue, 16 Sep 2025 13:07:43 -0700
From: Ian Rogers <irogers@...gle.com>
To: Arnaldo Carvalho de Melo <acme@...nel.org>
Cc: Gautam Menghani <gautam@...ux.ibm.com>, peterz@...radead.org, mingo@...hat.com, 
	namhyung@...nel.org, mark.rutland@....com, alexander.shishkin@...ux.intel.com, 
	jolsa@...nel.org, adrian.hunter@...el.com, kan.liang@...ux.intel.com, 
	maddy@...ux.ibm.com, linux-perf-users@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 0/2] perf python: Add an example for sampling

On Tue, Sep 16, 2025 at 12:25 PM Arnaldo Carvalho de Melo
<acme@...nel.org> wrote:
>
> On Tue, Sep 16, 2025 at 07:00:48PM +0530, Gautam Menghani wrote:
> > Hi Ian/Arnaldo,
> >
> > Can you please review this series and let me know if any changes are
> > needed?
>
> Looking at it now, sry for the delay,

I think the patches look good. I'm a little concerned that the python
APIs are a chance to do something better than the C APIs that have
evolved. For example, we removed UID out of target recently [1] as the
BPF alternative was better. Had this patch come earlier then it seems
likely we'd have had target with UIDs. I wonder rather than having a
kwlist of:

+ static char *kwlist[] = { "target", "inherit_stat", "no_buffering",
"no_inherit",
+ "no_inherit_set", "no_samples", "raw_samples",
+ "sample_address", "sample_phys_addr", "sample_data_page_size",
+ "sample_code_page_size", "sample_weight", "sample_time",
+ "sample_time_set", "sample_cpu", "sample_identifier",
+ "sample_data_src", "period", "period_set", "running_time",
+ "full_auxtrace", "auxtrace_snapshot_mode",
+ "auxtrace_snapshot_on_exit", "auxtrace_sample_mode",
+ "record_namespaces", "record_cgroup", "record_switch_events",
+ "record_switch_events_set", "all_kernel", "all_user",
+ "kernel_callchains", "user_callchains", "tail_synthesize",
+ "overwrite", "ignore_missing_thread", "strict_freq", "sample_id",
+ "no_bpf_event", "kcore", "text_poke", "build_id", "freq",
+ "mmap_pages", "auxtrace_mmap_pages", "user_freq", "branch_stack",
+ "sample_intr_regs", "sample_user_regs", "default_interval",
+ "user_interval", "auxtrace_snapshot_size", "auxtrace_snapshot_opts",
+ "auxtrace_sample_opts", "sample_transaction", "use_clockid",
+ "clockid", "clockid_res_ns", "nr_cblocks", "affinity", "mmap_flush",
+ "comp_level", "nr_threads_synthesize", "ctl_fd", "ctl_fd_ack",
+ "ctl_fd_close", "synth", "threads_spec", "threads_user_spec",
+ "off_cpu_thresh_ns",  NULL };

but then just using this subset:

+    opts = perf.record_opts(freq=1000, target=tgt, sample_time=True,
+                            sample_cpu=True, no_buffering=True,
no_inherit=True)

The kwlist should be kept to just those necessary values for the
example to work? I kind of see this as Arnaldo's baby, so he may just
want everything, so this needn't be a blocker.

Bigger picture I wonder about migrating the `perf script` code to just
being regular python programs like the example here. I sent out
deprecating the libperl code to this ends (looking for reviews):
https://lore.kernel.org/linux-perf-users/20250908181918.3533480-1-irogers@google.com/
The issue is that `perf script` being the main thread inhibits things
like textual running until trace_end. This means we can't do things
like incremental loading support. We may want to make the perf events
support something like an asyncio interface for that.

Refactoring that support will likely raise backward compatibility
concerns. It'd be a really nice thing to do as the API has some fairly
major overheads like turning everything in a sample into a Dict
whether needed or not:
https://web.git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/tools/perf/util/scripting-engines/trace-event-python.c#n838
I mention this just to say why I'd like to minimize the API when possible.

Thanks,
Ian

[1] https://lore.kernel.org/r/20250604174545.2853620-10-irogers@google.com

Powered by blists - more mailing lists