| Message ID | 20241018095745.57057-3-yangyicong@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Refactor the common parts to the HiSilicon Uncore PMU core and cleanups | expand |
On Fri, 18 Oct 2024 17:57:39 +0800 Yicong Yang <yangyicong@huawei.com> wrote: > From: Yicong Yang <yangyicong@hisilicon.com> > > Currently we detect the associated CPUs in the cpuhp online callback. Should really avoid we in patch descriptions. Currently associated CPUs are detected in the cphup online callback. etc > If the CPU's sccl_id or the ccl_id matches the PMU's, they're > associated. There's an exception that some PMUs locate on the SICL > and will match no CPUs. The events of these PMUs can be opened on > any online CPUs. To handle this we just check whether the PMU's > sccl_id is -1, if so we know it locates on SICL and make any CPU > associated to it. > > This can be tweaked so in this patch just do the below changes: > - If the PMU doesn't match any CPU then associated it to online CPUs > - Choose the target CPU according to the NUMA affinity for opening > events > > The function is implemented by hisi_pmu_init_associated_cpus() and > invoked in hisi_pmu_init(). > > Also we maintained the associated_cpus with all the online CPUs. This > is redundant since we'll always schedule the events on the online CPUs. > Get rid of this and make associated_cpus contain offline CPUs as well. > > Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> One trivial comment inline otherwise LGTM Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > /* > * The Super CPU Cluster (SCCL) and CPU Cluster (CCL) IDs can be > @@ -446,10 +467,6 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu) > { > int sccl_id, ccl_id; > > - /* If SCCL_ID is -1, the PMU is in a SICL and has no CPU affinity */ > - if (hisi_pmu->sccl_id == -1) > - return true; > - > if (hisi_pmu->ccl_id == -1) { > /* If CCL_ID is -1, the PMU only shares the same SCCL */ > hisi_read_sccl_and_ccl_id(&sccl_id, NULL); > @@ -467,13 +484,29 @@ int hisi_uncore_pmu_online_cpu(unsigned int cpu, struct hlist_node *node) > struct hisi_pmu *hisi_pmu = hlist_entry_safe(node, struct hisi_pmu, > node); > > - if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu)) > - return 0; > + /* > + * If the CPU is not in the associated_cpus, it maybe a new CPU we didn't > + * access. Test whether it's associated or not. it maybe a new CPU. Test whether... I'm not sure what the "didn't access" adds. > + */ > + if (!cpumask_test_cpu(cpu, &hisi_pmu->associated_cpus)) { > + if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu)) > + return 0; > + > + /* > + * We found an associated CPU so we don't need to use the dummy > + * associated CPUs. Update it. > + */ > + if (hisi_pmu->dummy_associated_cpus) { > + cpumask_clear(&hisi_pmu->associated_cpus); > + hisi_pmu->dummy_associated_cpus = false; > + } > > - cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus); > + cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus); > + }
On 2024/10/18 20:48, Jonathan Cameron wrote: > On Fri, 18 Oct 2024 17:57:39 +0800 > Yicong Yang <yangyicong@huawei.com> wrote: > >> From: Yicong Yang <yangyicong@hisilicon.com> >> >> Currently we detect the associated CPUs in the cpuhp online callback. > Should really avoid we in patch descriptions. > > Currently associated CPUs are detected in the cphup online callback. > etc > >> If the CPU's sccl_id or the ccl_id matches the PMU's, they're >> associated. There's an exception that some PMUs locate on the SICL >> and will match no CPUs. The events of these PMUs can be opened on >> any online CPUs. To handle this we just check whether the PMU's >> sccl_id is -1, if so we know it locates on SICL and make any CPU >> associated to it. >> >> This can be tweaked so in this patch just do the below changes: >> - If the PMU doesn't match any CPU then associated it to online CPUs >> - Choose the target CPU according to the NUMA affinity for opening >> events >> >> The function is implemented by hisi_pmu_init_associated_cpus() and >> invoked in hisi_pmu_init(). >> >> Also we maintained the associated_cpus with all the online CPUs. This >> is redundant since we'll always schedule the events on the online CPUs. >> Get rid of this and make associated_cpus contain offline CPUs as well. >> >> Signed-off-by: Yicong Yang <yangyicong@hisilicon.com> > > One trivial comment inline otherwise LGTM > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@huawei.com> > >> /* >> * The Super CPU Cluster (SCCL) and CPU Cluster (CCL) IDs can be >> @@ -446,10 +467,6 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu) >> { >> int sccl_id, ccl_id; >> >> - /* If SCCL_ID is -1, the PMU is in a SICL and has no CPU affinity */ >> - if (hisi_pmu->sccl_id == -1) >> - return true; >> - >> if (hisi_pmu->ccl_id == -1) { >> /* If CCL_ID is -1, the PMU only shares the same SCCL */ >> hisi_read_sccl_and_ccl_id(&sccl_id, NULL); >> @@ -467,13 +484,29 @@ int hisi_uncore_pmu_online_cpu(unsigned int cpu, struct hlist_node *node) >> struct hisi_pmu *hisi_pmu = hlist_entry_safe(node, struct hisi_pmu, >> node); >> >> - if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu)) >> - return 0; >> + /* >> + * If the CPU is not in the associated_cpus, it maybe a new CPU we didn't >> + * access. Test whether it's associated or not. > it maybe a new CPU. Test whether... > > I'm not sure what the "didn't access" adds. I mean the newly onlined CPU hasn't been "tested" by us. Will refine the comments. Thanks. > >> + */ >> + if (!cpumask_test_cpu(cpu, &hisi_pmu->associated_cpus)) { >> + if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu)) >> + return 0; >> + >> + /* >> + * We found an associated CPU so we don't need to use the dummy >> + * associated CPUs. Update it. >> + */ >> + if (hisi_pmu->dummy_associated_cpus) { >> + cpumask_clear(&hisi_pmu->associated_cpus); >> + hisi_pmu->dummy_associated_cpus = false; >> + } >> >> - cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus); >> + cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus); >> + } > . >
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c index 416f72a813fc..5b9373d25f9d 100644 --- a/drivers/perf/hisilicon/hisi_uncore_pmu.c +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c @@ -399,6 +399,27 @@ void hisi_uncore_pmu_disable(struct pmu *pmu) } EXPORT_SYMBOL_NS_GPL(hisi_uncore_pmu_disable, HISI_PMU); +static void hisi_pmu_init_associated_cpus(struct hisi_pmu *hisi_pmu) +{ + /* + * If the associated_cpus has already been initialized, for example + * determined by comparing the sccl_id and ccl_id with the CPU's + * mpidr_el1, then do nothing here. Otherwise the PMU has no affinity + * and could be opened on any online CPU. + */ + if (!cpumask_empty(&hisi_pmu->associated_cpus)) + return; + + cpumask_copy(&hisi_pmu->associated_cpus, cpu_online_mask); + hisi_pmu->dummy_associated_cpus = true; + + /* + * Otherwise the associated CPUs haven't been online yet. Pick a + * nearest CPU according to the PMU's numa node instead. + */ + hisi_pmu->on_cpu = cpumask_local_spread(0, dev_to_node(hisi_pmu->dev)); + WARN_ON(irq_set_affinity(hisi_pmu->irq, cpumask_of(hisi_pmu->on_cpu))); +} /* * The Super CPU Cluster (SCCL) and CPU Cluster (CCL) IDs can be @@ -446,10 +467,6 @@ static bool hisi_pmu_cpu_is_associated_pmu(struct hisi_pmu *hisi_pmu) { int sccl_id, ccl_id; - /* If SCCL_ID is -1, the PMU is in a SICL and has no CPU affinity */ - if (hisi_pmu->sccl_id == -1) - return true; - if (hisi_pmu->ccl_id == -1) { /* If CCL_ID is -1, the PMU only shares the same SCCL */ hisi_read_sccl_and_ccl_id(&sccl_id, NULL); @@ -467,13 +484,29 @@ int hisi_uncore_pmu_online_cpu(unsigned int cpu, struct hlist_node *node) struct hisi_pmu *hisi_pmu = hlist_entry_safe(node, struct hisi_pmu, node); - if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu)) - return 0; + /* + * If the CPU is not in the associated_cpus, it maybe a new CPU we didn't + * access. Test whether it's associated or not. + */ + if (!cpumask_test_cpu(cpu, &hisi_pmu->associated_cpus)) { + if (!hisi_pmu_cpu_is_associated_pmu(hisi_pmu)) + return 0; + + /* + * We found an associated CPU so we don't need to use the dummy + * associated CPUs. Update it. + */ + if (hisi_pmu->dummy_associated_cpus) { + cpumask_clear(&hisi_pmu->associated_cpus); + hisi_pmu->dummy_associated_cpus = false; + } - cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus); + cpumask_set_cpu(cpu, &hisi_pmu->associated_cpus); + } - /* If another CPU is already managing this PMU, simply return. */ - if (hisi_pmu->on_cpu != -1) + /* If another associated CPU is already managing this PMU, simply return. */ + if (hisi_pmu->on_cpu != -1 && + cpumask_test_cpu(hisi_pmu->on_cpu, &hisi_pmu->associated_cpus)) return 0; /* Use this CPU in cpumask for event counting */ @@ -492,9 +525,6 @@ int hisi_uncore_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node) node); unsigned int target; - if (!cpumask_test_and_clear_cpu(cpu, &hisi_pmu->associated_cpus)) - return 0; - /* Nothing to do if this CPU doesn't own the PMU */ if (hisi_pmu->on_cpu != cpu) return 0; @@ -521,6 +551,8 @@ void hisi_pmu_init(struct hisi_pmu *hisi_pmu, struct module *module) { struct pmu *pmu = &hisi_pmu->pmu; + hisi_pmu_init_associated_cpus(hisi_pmu); + pmu->module = module; pmu->parent = hisi_pmu->dev; pmu->task_ctx_nr = perf_invalid_context; diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.h b/drivers/perf/hisilicon/hisi_uncore_pmu.h index 008f18bbef7e..30f824173bcf 100644 --- a/drivers/perf/hisilicon/hisi_uncore_pmu.h +++ b/drivers/perf/hisilicon/hisi_uncore_pmu.h @@ -91,6 +91,11 @@ struct hisi_pmu { struct hisi_pmu_hwevents pmu_events; /* associated_cpus: All CPUs associated with the PMU */ cpumask_t associated_cpus; + /* + * If the real associated CPUs not onlined by the time initializing, + * we'll initialize with online CPUs and indicate it. + */ + bool dummy_associated_cpus; /* CPU used for counting */ int on_cpu; int irq;