| Message ID | 20250131231304.4151998-2-lucas.demarchi@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] drm/i915/pmu: Drop custom hotplug code | expand |
On 2025-01-31 6:11 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. > > The i915 counters are system-wide, even though the migration code > is using the wrong topology mask: > > target = cpumask_any_but(topology_sibling_cpumask(cpu), cpu); > > So one could think the counter has core scope rather than system scope, > which is not the case. That was never caught in tests since they would > disable just one cpu at a time. After the removal of hotpluggable CPU0 > in commit 5475abbde77f ("x86/smpboot: Remove the CPU0 hotplug kludge") > and commit e59e74dc48a3 ("x86/topology: Remove CPU0 hotplug option") > this became essentially non-testable for our systems. > > Using the generic hotplug code, the same cpu0 is still reported in > cpumask - only if it was possible to unplug it, it would migrate to > another cpu, so there isn't much a change in behavior. > > The one thing that changes is the return code for perf_event_open() if > an invalid cpu is used: previously i915 would return -EINVAL, but > generic perf code returns -ENODEV. That should be ok for all the users > and not cause breakages. > > Cc: Kan Liang <kan.liang@linux.intel.com> > Cc: Peter Zijlstra (Intel) <peterz@infradead.org> > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> Reviewed-by: Kan Liang <kan.liang@linux.intel.com> Thanks, Kan > --- > > v2: Expand commit message explanation to clarify what was discussed with > Kan, Tvrtko and Umesh in > https://patchwork.freedesktop.org/patch/msgid/20250116222426.77757-1-lucas.demarchi@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 2f88970cc0a93..5862754c662c4 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 2eecd42f61ba1..69a109d02116e 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 > @@ -935,23 +928,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), \ > @@ -1168,92 +1144,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); > - > - /* 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; > @@ -1261,7 +1157,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)) { > @@ -1290,6 +1185,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; > @@ -1301,16 +1197,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: > @@ -1334,8 +1224,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 0ec78c2b4f203..5826cc81858c4 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) {}
On 31/01/2025 23:11, 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. > > The i915 counters are system-wide, even though the migration code > is using the wrong topology mask: > > target = cpumask_any_but(topology_sibling_cpumask(cpu), cpu); > > So one could think the counter has core scope rather than system scope, > which is not the case. That was never caught in tests since they would > disable just one cpu at a time. After the removal of hotpluggable CPU0 > in commit 5475abbde77f ("x86/smpboot: Remove the CPU0 hotplug kludge") > and commit e59e74dc48a3 ("x86/topology: Remove CPU0 hotplug option") > this became essentially non-testable for our systems. > > Using the generic hotplug code, the same cpu0 is still reported in > cpumask - only if it was possible to unplug it, it would migrate to > another cpu, so there isn't much a change in behavior. > > The one thing that changes is the return code for perf_event_open() if > an invalid cpu is used: previously i915 would return -EINVAL, but > generic perf code returns -ENODEV. That should be ok for all the users > and not cause breakages. > > Cc: Kan Liang <kan.liang@linux.intel.com> > Cc: Peter Zijlstra (Intel) <peterz@infradead.org> > Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> > Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> > Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> > --- > > v2: Expand commit message explanation to clarify what was discussed with > Kan, Tvrtko and Umesh in > https://patchwork.freedesktop.org/patch/msgid/20250116222426.77757-1-lucas.demarchi@intel.com I *think* the comment in perf_event_setup_cpumask clarifies my question of whether "evil" userspace could setup a counter group with mixed cpus - that it couldn't. Even though I cannot actually untangle where perf_online_sys_mask gets set. In any case I don't think it is concern for this patch. Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com> Regards, Tvrtko > 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 2f88970cc0a93..5862754c662c4 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 2eecd42f61ba1..69a109d02116e 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 > @@ -935,23 +928,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), \ > @@ -1168,92 +1144,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); > - > - /* 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; > @@ -1261,7 +1157,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)) { > @@ -1290,6 +1185,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; > @@ -1301,16 +1197,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: > @@ -1334,8 +1224,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 0ec78c2b4f203..5826cc81858c4 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) {}
diff --git a/drivers/gpu/drm/i915/i915_module.c b/drivers/gpu/drm/i915/i915_module.c index 2f88970cc0a93..5862754c662c4 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 2eecd42f61ba1..69a109d02116e 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 @@ -935,23 +928,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), \ @@ -1168,92 +1144,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); - - /* 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; @@ -1261,7 +1157,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)) { @@ -1290,6 +1185,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; @@ -1301,16 +1197,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: @@ -1334,8 +1224,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 0ec78c2b4f203..5826cc81858c4 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) {}
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. The i915 counters are system-wide, even though the migration code is using the wrong topology mask: target = cpumask_any_but(topology_sibling_cpumask(cpu), cpu); So one could think the counter has core scope rather than system scope, which is not the case. That was never caught in tests since they would disable just one cpu at a time. After the removal of hotpluggable CPU0 in commit 5475abbde77f ("x86/smpboot: Remove the CPU0 hotplug kludge") and commit e59e74dc48a3 ("x86/topology: Remove CPU0 hotplug option") this became essentially non-testable for our systems. Using the generic hotplug code, the same cpu0 is still reported in cpumask - only if it was possible to unplug it, it would migrate to another cpu, so there isn't much a change in behavior. The one thing that changes is the return code for perf_event_open() if an invalid cpu is used: previously i915 would return -EINVAL, but generic perf code returns -ENODEV. That should be ok for all the users and not cause breakages. Cc: Kan Liang <kan.liang@linux.intel.com> Cc: Peter Zijlstra (Intel) <peterz@infradead.org> Cc: Vinay Belgaumkar <vinay.belgaumkar@intel.com> Reviewed-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com> Signed-off-by: Lucas De Marchi <lucas.demarchi@intel.com> --- v2: Expand commit message explanation to clarify what was discussed with Kan, Tvrtko and Umesh in https://patchwork.freedesktop.org/patch/msgid/20250116222426.77757-1-lucas.demarchi@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(-)