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

["Liang, Kan" <kan.liang@linux.intel.com>]

On 2025-01-21 11:59 a.m., Lucas De Marchi wrote:
> 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
> 

I see. The behavior is not changed with the patch. It should be just the
topology_sibling_cpumask() which implies a misleading message.
Thanks for the confirmation.


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

Yes, please.

Thanks,
Kan