On Wed, Oct 16, 2024 at 08:01:21AM -0700, Ian Rogers wrote:
On Wed, Oct 16, 2024 at 1:29 AM James Clark <james.clark@xxxxxxxxxx> wrote:
On 15/10/2024 4:14 pm, Ian Rogers wrote:
On Tue, Oct 15, 2024 at 7:54 AM James Clark <james.clark@xxxxxxxxxx> wrote:
Since the linked fixes: commit, specifying a CPU on hybrid platforms
results in an error because Perf tries to open an extended type event
on "any" CPU which isn't valid. Extended type events can only be opened
on CPUs that match the type.
Before (working):
$ perf record --cpu 1 -- true
[ perf record: Woken up 1 times to write data ]
[ perf record: Captured and wrote 2.385 MB perf.data (7 samples) ]
After (not working):
$ perf record -C 1 -- true
WARNING: A requested CPU in '1' is not supported by PMU 'cpu_atom' (CPUs 16-27) for event 'cycles:P'
Error:
The sys_perf_event_open() syscall returned with 22 (Invalid argument) for event (cpu_atom/cycles:P/).
/bin/dmesg | grep -i perf may provide additional information.
(Ignore the warning message, that's expected and not particularly
relevant to this issue).
This is because perf_cpu_map__intersect() of the user specified CPU (1)
and one of the PMU's CPUs (16-27) correctly results in an empty (NULL)
CPU map. However for the purposes of opening an event, libperf converts
empty CPU maps into an any CPU (-1) which the kernel rejects.
Ugh. The cpumap API tries its best to confuse NULL == empty but empty
can give you dummy, dummy is also known as 'any' or -1, 'any' sounds a
lot like 'all' but sometimes 'all' is only online CPUs. I tried to
tidy up the naming a while ago, but there is still a mess.
I don't know if you think this is a good opportunity for me to have a go
at finishing separating those? Or is it a dead end?
So cpumap (and threadmap) underpin a lot of things, we also used to
routinely confuse CPU numbers with cpumap indices that are used to
densely index xyarrays with file descriptors, etc. My thought was that
we may end up doing a proper Rust libperf that can be under a more
permissive license like libbpf - currently libperf is a source of GPL
infection. The rewrite would be a good time to clear these things up.
I believe someone at RedHat has looked at doing a Rust libperf.
I really want to rewrite CPU/thread map related code but didn't have
time to work on it. :(
It'd be great if we can rewrite it in Rust! But current libperf API is
pretty bad and it's not clearly separated from the tools code. For
example, accessing internals like evsel->core.xxx should be changed
first.
Fix it by deleting evsels with empty CPU maps in the specific case where
user requested CPU maps are evaluated.
If we delete evsels than the indices can be broken for certain things.
I'm guessing asan testing is clean but the large number of side data
structures that are indexed by things in another data structure makes
the whole code base brittle and I am nervous around this change.
Fixes: 251aa040244a ("perf parse-events: Wildcard most "numeric" events")
Signed-off-by: James Clark <james.clark@xxxxxxxxxx>
Reviewed-by: Ian Rogers <irogers@xxxxxxxxxx>
Thanks,
Ian
Ok if we're not completely opposed to doing it this way I will dig a bit
more and double check everything is working.
I think it is okay to do it this way (hence the reviewed-by tag) as
propagate maps should happen before the xyarrays are set up, it'd be
nice if these things were checked at runtime, or by the compiler...
Right, evsel index is used some places probably we need to update it
too.
Thanks,
Namhyung