On Tue, Jan 21, 2025 at 10:53:31AM -0500, Liang, Kan wrote:
On 2025-01-21 9:29 a.m., Lucas De Marchi wrote:
On Mon, Jan 20, 2025 at 08:42:41PM -0500, Liang, Kan wrote:
-static int i915_pmu_cpu_offline(unsigned int cpu, struct hlist_node
*node)
-{
- struct i915_pmu *pmu = hlist_entry_safe(node, typeof(*pmu),
cpuhp.node);
- unsigned int target = i915_pmu_target_cpu;
-
- /*
- * Unregistering an instance generates a CPU offline event which
we must
- * ignore to avoid incorrectly modifying the shared
i915_pmu_cpumask.
- */
- if (!pmu->registered)
- return 0;
-
- if (cpumask_test_and_clear_cpu(cpu, &i915_pmu_cpumask)) {
- target = cpumask_any_but(topology_sibling_cpumask(cpu), cpu);
-
I'm not familar with the i915 PMU, but it seems suggest a core scope
PMU, not a system-wide scope.
counter is in a complete separate device - it doesn't depend on core or
die or pkg - not sure why it cared about topology_sibling_cpumask here.
OK. But it's still a behavior change. Please make it clear in the
description that the patch also changes/fixes the scope from core scope
to system-wide.
sure... do you have a suggestion how to test the hotplug? For testing
purposes, can I force the perf cpu assigned to be something other than
the cpu0?
Yes, it's a bit tricky to verify the hotplug if the assigned CPU is
CPU0. I don't know a way to force another CPU without changing the code.
You may have to instrument the code for the test.
Another test you may want to do is the perf system-wide test, e.g., perf
stat -a -e i915/actual-frequency/ sleep 1.
The existing code assumes the counter is core scope. So the result
should be huge, since perf will read the counter on each core and add
them up.
that is not allowed and it simply fails to init the counter:
static int i915_pmu_event_init(struct perf_event *event)
...
if (event->cpu < 0)
return -EINVAL;
if (!cpumask_test_cpu(event->cpu, &i915_pmu_cpumask))
return -EINVAL;
...
}
event only succeeds the initialization in the assigned cpu. I see no
differences in results (using i915/interrupts/ since freq is harder to
compare):
$ sudo perf stat -e i915/interrupts/ sleep 1
Performance counter stats for 'system wide':
253 i915/interrupts/
1.002215175 seconds time elapsed
$ sudo perf stat -a -e i915/interrupts/ sleep 1
Performance counter stats for 'system wide':
251 i915/interrupts/
1.000900818 seconds time elapsed
Note that our cpumask attr already returns just the assigned cpu and
perf-stat only tries to open on that cpu:
$ strace --follow -s 1024 -e perf_event_open -- perf stat -a -e i915/interrupts/ sleep 1
[pid 55777] perf_event_open({type=0x24 /* PERF_TYPE_??? */, size=0x88 /* PERF_ATTR_SIZE_??? */, config=0x100002, sample_period=0, sample_type=PERF_SAMPLE_IDENTIFIER, read_format=PERF_FORMAT_TOTAL_TIME_ENABLED|PERF_FORMAT_TOTAL_TIME_RUNNING, disabled=1, inherit=1, precise_ip=0 /* arbitrary skid */, exclude_guest=1, ...}, -1, 0, -1, PERF_FLAG_FD_CLOEXEC) = 3
Lucas De Marchi
But this patch claims that the counter is system-wide. With the patch,
the same perf command should only read the counter on the assigned CPU.
Please also post the test results in the changelog. That's the reason
why the scope has to be changed.
it seems that migration code is simply wrong, not that we are changing
the scope here - it was already considered system-wide. I can add a
paragraph in the commit message explaining it.
thanks
Lucas De Marchi
Thanks,
Kan
