[PATCH] drm/i915/pmu: Drop custom hotplug code

[Lucas De Marchi]

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
>
>