[PATCH] drm/i915/pmu: Drop custom hotplug code
Tvrtko Ursulin
tursulin at ursulin.net
Thu Jan 23 18:06:29 UTC 2025
On 23/01/2025 16:27, Lucas De Marchi wrote:
> On Thu, Jan 23, 2025 at 09:43:35AM +0000, Tvrtko Ursulin wrote:
>>
>> On 20/01/2025 22:57, Lucas De Marchi wrote:
>>> On Mon, Jan 20, 2025 at 10:08:39AM -0500, Liang, Kan wrote:
>>>>
>>>>
>>>> On 2025-01-16 5:24 p.m., Lucas De Marchi wrote:
>>>>> Since commit 4ba4f1afb6a9 ("perf: Generic hotplug support for a PMU
>>>>> with
>>>>> a scope"), there's generic support for system-wide counters and
>>>>> integration with cpu hotplug. Set our scope to PERF_PMU_SCOPE_SYS_WIDE
>>>>> instead of all the boilerplate code for handling hotplug.
>>>>>
>>>>> Cc: Kan Liang <kan.liang at linux.intel.com>
>>>>> Cc: Peter Zijlstra (Intel) <peterz at infradead.org>
>>>>> Cc: Vinay Belgaumkar <vinay.belgaumkar at intel.com>
>>>>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>>>>> ---
>>>>> drivers/gpu/drm/i915/i915_module.c | 2 -
>>>>> drivers/gpu/drm/i915/i915_pmu.c | 114
>>>>> +----------------------------
>>>>> drivers/gpu/drm/i915/i915_pmu.h | 11 ---
>>>>> 3 files changed, 1 insertion(+), 126 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/i915/i915_module.c
>>>>> b/drivers/gpu/drm/i915/i915_module.c
>>>>> index 7ed6d70389af9..7affe07f84f45 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_module.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_module.c
>>>>> @@ -71,8 +71,6 @@ static const struct {
>>>>> { .init = i915_vma_resource_module_init,
>>>>> .exit = i915_vma_resource_module_exit },
>>>>> { .init = i915_mock_selftests },
>>>>> - { .init = i915_pmu_init,
>>>>> - .exit = i915_pmu_exit },
>>>>> { .init = i915_pci_register_driver,
>>>>> .exit = i915_pci_unregister_driver },
>>>>> { .init = i915_perf_sysctl_register,
>>>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.c
>>>>> b/drivers/gpu/drm/i915/i915_pmu.c
>>>>> index e55db036be1bb..652964ef0643c 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>>>> @@ -28,9 +28,6 @@
>>>>> BIT(I915_SAMPLE_WAIT) | \
>>>>> BIT(I915_SAMPLE_SEMA))
>>>>>
>>>>> -static cpumask_t i915_pmu_cpumask;
>>>>> -static unsigned int i915_pmu_target_cpu = -1;
>>>>> -
>>>>> static struct i915_pmu *event_to_pmu(struct perf_event *event)
>>>>> {
>>>>> return container_of(event->pmu, struct i915_pmu, base);
>>>>> @@ -642,10 +639,6 @@ static int i915_pmu_event_init(struct
>>>>> perf_event *event)
>>>>> if (event->cpu < 0)
>>>>> return -EINVAL;
>>>>>
>>>>> - /* only allow running on one cpu at a time */
>>>>> - if (!cpumask_test_cpu(event->cpu, &i915_pmu_cpumask))
>>>>> - return -EINVAL;
>>>>> -
>>>>> if (is_engine_event(event))
>>>>> ret = engine_event_init(event);
>>>>> else
>>>>> @@ -940,23 +933,6 @@ static ssize_t i915_pmu_event_show(struct
>>>>> device *dev,
>>>>> return sprintf(buf, "config=0x%lx\n", eattr->val);
>>>>> }
>>>>>
>>>>> -static ssize_t cpumask_show(struct device *dev,
>>>>> - struct device_attribute *attr, char *buf)
>>>>> -{
>>>>> - return cpumap_print_to_pagebuf(true, buf, &i915_pmu_cpumask);
>>>>> -}
>>>>> -
>>>>> -static DEVICE_ATTR_RO(cpumask);
>>>>> -
>>>>> -static struct attribute *i915_cpumask_attrs[] = {
>>>>> - &dev_attr_cpumask.attr,
>>>>> - NULL,
>>>>> -};
>>>>> -
>>>>> -static const struct attribute_group i915_pmu_cpumask_attr_group = {
>>>>> - .attrs = i915_cpumask_attrs,
>>>>> -};
>>>>> -
>>>>> #define __event(__counter, __name, __unit) \
>>>>> { \
>>>>> .counter = (__counter), \
>>>>> @@ -1173,92 +1149,12 @@ static void free_event_attributes(struct
>>>>> i915_pmu *pmu)
>>>>> pmu->pmu_attr = NULL;
>>>>> }
>>>>>
>>>>> -static int i915_pmu_cpu_online(unsigned int cpu, struct hlist_node
>>>>> *node)
>>>>> -{
>>>>> - struct i915_pmu *pmu = hlist_entry_safe(node, typeof(*pmu),
>>>>> cpuhp.node);
>>>>> -
>>>>> - /* Select the first online CPU as a designated reader. */
>>>>> - if (cpumask_empty(&i915_pmu_cpumask))
>>>>> - cpumask_set_cpu(cpu, &i915_pmu_cpumask);
>>>>> -
>>>>> - return 0;
>>>>> -}
>>>>> -
>>>>> -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.
>>>
>>> Also, in my tests it always chose cpu0 that is the boot cpu and can't be
>>> offlined. Looking at our CI it seems this entire code is not tested at
>>> all: the only test that in theory would exercise this just skips since
>>> cpu0 can't go offline -
>>> https://intel-gfx-ci.01.org/tree/drm-tip/shards-all.html?testfilter=hotplug
>>
>> s/not tested at all/not currently tested/
>>
>> commit e59e74dc48a309cb848ffc3d76a0d61aa6803c05
>> Author: Thomas Gleixner <tglx at linutronix.de>
>> Date: Fri May 12 23:07:04 2023 +0200
>>
>> x86/topology: Remove CPU0 hotplug option
>>
>> So test worked for ~6 years and then transitioned to skip. :shrug:
>
> right, indeed looking at the CI repo I see it there:
>
> $ git grep CONFIG_BOOTPARAM_HOTPLUG_CPU0
> kconfig/debug:CONFIG_BOOTPARAM_HOTPLUG_CPU0=y
> kconfig/debug-kasan:CONFIG_BOOTPARAM_HOTPLUG_CPU0=y
>
> $ git log --oneline -G CONFIG_BOOTPARAM_HOTPLUG_CPU0 kconfig/debug
> 3553bf4 kconfig: Dump current kconfigs used by CI
>
>
> I sent this to igt yesterday:
> https://patchwork.freedesktop.org/patch/633357/?series=143874&rev=1
>
> I will reword that commit message to note that it was actually tested
> before that kernel commit. The test worked because it only tried to
> toggle the
> cpu0 and thus a migration succeeded to a sibling cpu, right?
No, it was off-lining all CPUs one by one.
If topology_sibling_cpumask is not right then maybe that did not show
because of the one by one nature? I mean if the test was trying to
offline multiple CPUs ie. leave multiple off lines, maybe then it would
break?
> For the kernel commit message I can add a comment about the counter
> being indeed a system one and the use of topology_sibling_cpumask()
> being wrong here. With that, can you review this patch?
I don't know about this new system wide vs something else stuff. It will
not interfere with the fact there can be multiple instances of the same
PMU driver, one per device?
I can review but cannot promise it will be quick.
Regards,
Tvrtko
>>
>> Regards,
>>
>> Tvrtko
>>
>>>>
>>>>> - /* Migrate events if there is a valid target */
>>>>> - if (target < nr_cpu_ids) {
>>>>> - cpumask_set_cpu(target, &i915_pmu_cpumask);
>>>>> - i915_pmu_target_cpu = target;
>>>>> - }
>>>>> - }
>>>>> -
>>>>> - if (target < nr_cpu_ids && target != pmu->cpuhp.cpu) {
>>>>> - perf_pmu_migrate_context(&pmu->base, cpu, target);
>>>>> - pmu->cpuhp.cpu = target;
>>>>> - }
>>>>> -
>>>>> - return 0;
>>>>> -}
>>>>> -
>>>>> -static enum cpuhp_state cpuhp_state = CPUHP_INVALID;
>>>>> -
>>>>> -int i915_pmu_init(void)
>>>>> -{
>>>>> - int ret;
>>>>> -
>>>>> - ret = cpuhp_setup_state_multi(CPUHP_AP_ONLINE_DYN,
>>>>> - "perf/x86/intel/i915:online",
>>>>> - i915_pmu_cpu_online,
>>>>> - i915_pmu_cpu_offline);
>>>>> - if (ret < 0)
>>>>> - pr_notice("Failed to setup cpuhp state for i915 PMU! (%d)\n",
>>>>> - ret);
>>>>> - else
>>>>> - cpuhp_state = ret;
>>>>> -
>>>>> - return 0;
>>>>> -}
>>>>> -
>>>>> -void i915_pmu_exit(void)
>>>>> -{
>>>>> - if (cpuhp_state != CPUHP_INVALID)
>>>>> - cpuhp_remove_multi_state(cpuhp_state);
>>>>> -}
>>>>> -
>>>>> -static int i915_pmu_register_cpuhp_state(struct i915_pmu *pmu)
>>>>> -{
>>>>> - if (cpuhp_state == CPUHP_INVALID)
>>>>> - return -EINVAL;
>>>>> -
>>>>> - return cpuhp_state_add_instance(cpuhp_state, &pmu->cpuhp.node);
>>>>> -}
>>>>> -
>>>>> -static void i915_pmu_unregister_cpuhp_state(struct i915_pmu *pmu)
>>>>> -{
>>>>> - cpuhp_state_remove_instance(cpuhp_state, &pmu->cpuhp.node);
>>>>> -}
>>>>> -
>>>>> void i915_pmu_register(struct drm_i915_private *i915)
>>>>> {
>>>>> struct i915_pmu *pmu = &i915->pmu;
>>>>> const struct attribute_group *attr_groups[] = {
>>>>> &i915_pmu_format_attr_group,
>>>>> &pmu->events_attr_group,
>>>>> - &i915_pmu_cpumask_attr_group,
>>>>> NULL
>>>>> };
>>>>> int ret = -ENOMEM;
>>>>> @@ -1266,7 +1162,6 @@ void i915_pmu_register(struct
>>>>> drm_i915_private *i915)
>>>>> spin_lock_init(&pmu->lock);
>>>>> hrtimer_init(&pmu->timer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
>>>>> pmu->timer.function = i915_sample;
>>>>> - pmu->cpuhp.cpu = -1;
>>>>> init_rc6(pmu);
>>>>>
>>>>> if (IS_DGFX(i915)) {
>>>>> @@ -1295,6 +1190,7 @@ void i915_pmu_register(struct
>>>>> drm_i915_private *i915)
>>>>>
>>>>> pmu->base.module = THIS_MODULE;
>>>>> pmu->base.task_ctx_nr = perf_invalid_context;
>>>>> + pmu->base.scope = PERF_PMU_SCOPE_SYS_WIDE;
>>>>> pmu->base.event_init = i915_pmu_event_init;
>>>>> pmu->base.add = i915_pmu_event_add;
>>>>> pmu->base.del = i915_pmu_event_del;
>>>>> @@ -1307,16 +1203,10 @@ void i915_pmu_register(struct
>>>>> drm_i915_private *i915)
>>>>> if (ret)
>>>>> goto err_groups;
>>>>>
>>>>> - ret = i915_pmu_register_cpuhp_state(pmu);
>>>>> - if (ret)
>>>>> - goto err_unreg;
>>>>> -
>>>>> pmu->registered = true;
>>>>>
>>>>> return;
>>>>>
>>>>> -err_unreg:
>>>>> - perf_pmu_unregister(&pmu->base);
>>>>> err_groups:
>>>>> kfree(pmu->base.attr_groups);
>>>>> err_attr:
>>>>> @@ -1340,8 +1230,6 @@ void i915_pmu_unregister(struct
>>>>> drm_i915_private *i915)
>>>>>
>>>>> hrtimer_cancel(&pmu->timer);
>>>>>
>>>>> - i915_pmu_unregister_cpuhp_state(pmu);
>>>>> -
>>>>> perf_pmu_unregister(&pmu->base);
>>>>> kfree(pmu->base.attr_groups);
>>>>> if (IS_DGFX(i915))
>>>>> diff --git a/drivers/gpu/drm/i915/i915_pmu.h
>>>>> b/drivers/gpu/drm/i915/i915_pmu.h
>>>>> index 8e66d63d0c9f9..53bce3d8bfbaf 100644
>>>>> --- a/drivers/gpu/drm/i915/i915_pmu.h
>>>>> +++ b/drivers/gpu/drm/i915/i915_pmu.h
>>>>> @@ -56,13 +56,6 @@ struct i915_pmu_sample {
>>>>> };
>>>>>
>>>>> struct i915_pmu {
>>>>> - /**
>>>>> - * @cpuhp: Struct used for CPU hotplug handling.
>>>>> - */
>>>>> - struct {
>>>>> - struct hlist_node node;
>>>>> - unsigned int cpu;
>>>>> - } cpuhp;
>>>>> /**
>>>>> * @base: PMU base.
>>>>> */
>>>>> @@ -155,15 +148,11 @@ struct i915_pmu {
>>>>> };
>>>>>
>>>>> #ifdef CONFIG_PERF_EVENTS
>>>>> -int i915_pmu_init(void);
>>>>> -void i915_pmu_exit(void);
>>>>> void i915_pmu_register(struct drm_i915_private *i915);
>>>>> void i915_pmu_unregister(struct drm_i915_private *i915);
>>>>> void i915_pmu_gt_parked(struct intel_gt *gt);
>>>>> void i915_pmu_gt_unparked(struct intel_gt *gt);
>>>>> #else
>>>>> -static inline int i915_pmu_init(void) { return 0; }
>>>>> -static inline void i915_pmu_exit(void) {}
>>>>> static inline void i915_pmu_register(struct drm_i915_private
>>>>> *i915) {}
>>>>> static inline void i915_pmu_unregister(struct drm_i915_private
>>>>> *i915) {}
>>>>> static inline void i915_pmu_gt_parked(struct intel_gt *gt) {}
>>>>
More information about the Intel-gfx
mailing list