On 12/05/22 21:53, Namhyung Kim wrote:
On Thu, May 12, 2022 at 3:35 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
On 12/05/22 08:27, Namhyung Kim wrote:
On Fri, May 6, 2022 at 5:27 AM Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
Currently, user_requested_cpus supplants system-wide CPUs when the evlist
has_user_cpus. Change that so that system-wide events retain their own
CPUs and they are added to all_cpus.
Acked-by: Ian Rogers <irogers@xxxxxxxxxx>
Signed-off-by: Adrian Hunter <adrian.hunter@xxxxxxxxx>
---
tools/lib/perf/evlist.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/tools/lib/perf/evlist.c b/tools/lib/perf/evlist.c
index 1c801f8da44f..9a6801b53274 100644
--- a/tools/lib/perf/evlist.c
+++ b/tools/lib/perf/evlist.c
@@ -40,12 +40,11 @@ static void __perf_evlist__propagate_maps(struct perf_evlist *evlist,
* We already have cpus for evsel (via PMU sysfs) so
* keep it, if there's no target cpu list defined.
*/
- if (!evsel->own_cpus || evlist->has_user_cpus) {
- perf_cpu_map__put(evsel->cpus);
- evsel->cpus = perf_cpu_map__get(evlist->user_requested_cpus);
- } else if (!evsel->system_wide &&
- !evsel->requires_cpu &&
- perf_cpu_map__empty(evlist->user_requested_cpus)) {
+ if (!evsel->own_cpus ||
+ (!evsel->system_wide && evlist->has_user_cpus) ||
+ (!evsel->system_wide &&
+ !evsel->requires_cpu &&
+ perf_cpu_map__empty(evlist->user_requested_cpus))) {
This is getting hard to understand. IIUC this propagation basically
sets user requested cpus to evsel unless it has its own cpus, right?
I put the conditional logic altogether because that is kernel style but
it does make it practically unreadable.
If we start with the original logic:
if (!evsel->own_cpus || evlist->has_user_cpus) {
perf_cpu_map__put(evsel->cpus);
evsel->cpus = perf_cpu_map__get(evlist->user_requested_cpus);
} else if (!evsel->system_wide && perf_cpu_map__empty(evlist->user_requested_cpus)) {
perf_cpu_map__put(evsel->cpus);
evsel->cpus = perf_cpu_map__get(evlist->user_requested_cpus);
} else if (evsel->cpus != evsel->own_cpus) {
perf_cpu_map__put(evsel->cpus);
evsel->cpus = perf_cpu_map__get(evsel->own_cpus);
}
Then make it more readable, i.e. same functionality
struct perf_cpu_map *cpus;
if (!evsel->own_cpus || evlist->has_user_cpus)
cpus = evlist->user_requested_cpus;
else if (!evsel->system_wide && perf_cpu_map__empty(evlist->user_requested_cpus))
cpus = evlist->user_requested_cpus;
else
cpus = evsel->own_cpus;
if (evsel->cpus != cpus) {
perf_cpu_map__put(evsel->cpus);
evsel->cpus = perf_cpu_map__get(cpus);
}
Then separate out the conditions, i.e. still same functionality
if (!evsel->own_cpus)
cpus = evlist->user_requested_cpus;
else if (evlist->has_user_cpus)
cpus = evlist->user_requested_cpus;
else if (evsel->system_wide)
cpus = evsel->own_cpus;
else if (perf_cpu_map__empty(evlist->user_requested_cpus)) /* per-thread */
cpus = evlist->user_requested_cpus;
else
cpus = evsel->own_cpus;
Then add the new requires_cpu flag:
if (!evsel->own_cpus)
cpus = evlist->user_requested_cpus;
else if (evlist->has_user_cpus)
cpus = evlist->user_requested_cpus;
else if (evsel->system_wide)
cpus = evsel->own_cpus;
- else if (perf_cpu_map__empty(evlist->user_requested_cpus)) /* per-thread */
+ else if (!evsel->requres_cpu && perf_cpu_map__empty(evlist->user_requested_cpus)) /* per-thread */
cpus = evlist->user_requested_cpus;
else
cpus = evsel->own_cpus;
Then make system_wide keep own_cpus even if has_user_cpus:
if (!evsel->own_cpus)
cpus = evlist->user_requested_cpus;
+ else if (evsel->system_wide)
+ cpus = evsel->own_cpus;
else if (evlist->has_user_cpus)
cpus = evlist->user_requested_cpus;
- else if (evsel->system_wide)
- cpus = evsel->own_cpus;
else if (!evsel->requres_cpu && perf_cpu_map__empty(evlist->user_requested_cpus)) /* per-thread */
cpus = evlist->user_requested_cpus;
else
cpus = evsel->own_cpus;
Which leaves:
if (!evsel->own_cpus)
cpus = evlist->user_requested_cpus;
else if (evsel->system_wide)
cpus = evsel->own_cpus;
else if (evlist->has_user_cpus)
cpus = evlist->user_requested_cpus;
else if (!evsel->requres_cpu && perf_cpu_map__empty(evlist->user_requested_cpus)) /* per-thread */
cpus = evlist->user_requested_cpus;
else
cpus = evsel->own_cpus;
And putting it back together:
if (!evsel->own_cpus ||
(!evsel->system_wide && evlist->has_user_cpus) ||
(!evsel->system_wide &&
!evsel->requires_cpu &&
perf_cpu_map__empty(evlist->user_requested_cpus))) {
cpus = evlist->user_requested_cpus;
else
cpus = evsel->own_cpus;
Perhaps I shouldn't put it together?
Cool, thanks a lot for explaining it in detail.
I do not oppose your change but little worried about the
complexity. And I think we have some issues with uncore
events already.
Yes it is a bit complicated because we are handling
many different use cases.
So do you have any idea where evsel->own_cpus
doesn't propagate to evsel->cpus?
We let the user's list of CPUs override it i.e. the
evlist->has_user_cpus case. Essentially we are expecting
the user to know what they are doing.
I think evsel->system_wide and evsel->requires_cpu
can be replaced to check evsel->own_cpus instead.
Not at the moment because we let the user override
own_cpus.
Actually evlist->has_user_cpus is checked first so
uncore events' own_cpus might not be used.
Yes
In my laptop, perf stat -a -A -e imc/data_reads/
will use cpu 0 as it's listed in the pmu cpumask.
But when I use -C1,2 it'll use the both cpus and
returns the similar values each (so the sum is 2x).
We expect the user to understand the uncore PMU they
are using. AFAICT an uncore PMU cpu mask with only
CPU 0 typically means a single PMU that counts events
that could be indrectly caused by any CPU. When the
cpu mask has more than one CPU, it means a PMU for
each of a group of CPU's (e.g. a core or socket)
So in the example you gave above, there is only 1 PMU
and reading from any CPU will give it's value.
A user providing a list of CPUs for uncore events
is useful only in certain cases. For example when
each core has an uncore PMU and you only want to get
values from one core.
I'm not sure if it's intended. I expect it runs on
cpu 0 or one of the given cpus. Or it runs on both
cpus and returns value in half so that the sum is
the same as the original value (from a cpu).
I don't know if there is anything wrong with the way
we are handling uncore PMUs, except that I don't know
if it is documented anywhere.
But the hybrid pmus make this complex. Maybe we can move the
logic in evlist__fix_hybrid_cpus() here and simplify it like below
if (evsel->own_cpus) {
if (evsel->pmu->is_hybrid)
evsel->cpus = fixup_hybrid_cpus(evsel>own_cpus,
evlist->user_requested_cpus); //?
else
evsel->cpus = evlist->own_cpus; // put + get
} else {
evsel->cpus = evlist->user_requested_cpus; // put + get
}
Then we need to make sure evsel->pmu is set properly.
What do you think?
Hybrid handling looks complicated. I would have to spend time
better understanding it.
So, in the context of this patch set, I don't want to look at
issues with hybrid CPUs, except that there should be no change
to how they are handled.