* [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack
@ 2024-04-02 10:56 Dawei Li
2024-04-02 10:56 ` [PATCH 1/9] perf/alibaba_uncore_drw: " Dawei Li
` (9 more replies)
0 siblings, 10 replies; 19+ messages in thread
From: Dawei Li @ 2024-04-02 10:56 UTC (permalink / raw
To: will, mark.rutland
Cc: xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm,
Dawei Li
Hi,
This series try to eliminate direct cpumask var allocation from stack
for perf subsystem.
Direct/explicit allocation of cpumask on stack could be dangerous since
it can lead to stack overflow for systems with big NR_CPUS or
CONFIG_CPUMASK_OFFSTACK=y.
For arm64, it's more urgent since commit 3fbd56f0e7c1 ("ARM64: Dynamically
allocate cpumasks and increase supported CPUs to 512").
It's sort of a pattern that almost every cpumask var in perf subystem
occurs in teardown callback of cpuhp. In which case, if dynamic
allocation failed(which is unlikely), we choose return 0 rather than
-ENOMEM to caller cuz:
@teardown is not supposed to fail and if it does, system crashes:
static int cpuhp_issue_call(int cpu, enum cpuhp_state state, bool bringup,
struct hlist_node *node)
{
struct cpuhp_step *sp = cpuhp_get_step(state);
int ret;
/*
* If there's nothing to do, we done.
* Relies on the union for multi_instance.
*/
if (cpuhp_step_empty(bringup, sp))
return 0;
/*
* The non AP bound callbacks can fail on bringup. On teardown
* e.g. module removal we crash for now.
*/
#ifdef CONFIG_SMP
if (cpuhp_is_ap_state(state))
ret = cpuhp_invoke_ap_callback(cpu, state, bringup, node);
else
ret = cpuhp_invoke_callback(cpu, state, bringup, node,
NULL);
#else
ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
#endif
BUG_ON(ret && !bringup);
return ret;
}
Dawei Li (9):
perf/alibaba_uncore_drw: Avoid explicit cpumask var allocation from
stack
perf/arm-cmn: Avoid explicit cpumask var allocation from stack
perf/arm_cspmu: Avoid explicit cpumask var allocation from stack
perf/arm_dsu: Avoid explicit cpumask var allocation from stack
perf/dwc_pcie: Avoid explicit cpumask var allocation from stack
perf/hisi_pcie: Avoid explicit cpumask var allocation from stack
perf/hisi_uncore: Avoid explicit cpumask var allocation from stack
perf/qcom_l2: Avoid explicit cpumask var allocation from stack
perf/thunder_x2: Avoid explicit cpumask var allocation from stack
drivers/perf/alibaba_uncore_drw_pmu.c | 13 +++++++++----
drivers/perf/arm-cmn.c | 13 +++++++++----
drivers/perf/arm_cspmu/arm_cspmu.c | 13 +++++++++----
drivers/perf/arm_dsu_pmu.c | 18 +++++++++++++-----
drivers/perf/dwc_pcie_pmu.c | 17 +++++++++++------
drivers/perf/hisilicon/hisi_pcie_pmu.c | 15 ++++++++++-----
drivers/perf/hisilicon/hisi_uncore_pmu.c | 13 +++++++++----
drivers/perf/qcom_l2_pmu.c | 15 ++++++++++-----
drivers/perf/thunderx2_pmu.c | 20 ++++++++++++--------
9 files changed, 92 insertions(+), 45 deletions(-)
Thanks,
Dawei
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH 1/9] perf/alibaba_uncore_drw: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack Dawei Li
@ 2024-04-02 10:56 ` Dawei Li
2024-04-02 11:06 ` Mark Rutland
2024-04-02 10:56 ` [PATCH 2/9] perf/arm-cmn: " Dawei Li
` (8 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Dawei Li @ 2024-04-02 10:56 UTC (permalink / raw
To: will, mark.rutland
Cc: xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm,
Dawei Li
For CONFIG_CPUMASK_OFFSTACK=y kernel, explicit allocation of cpumask
variable on stack is not recommended since it can cause potential stack
overflow.
Instead, kernel code should always use *cpumask_var API(s) to allocate
cpumask var in config- neutral way, leaving allocation strategy to
CONFIG_CPUMASK_OFFSTACK.
Use *cpumask_var API(s) to address it.
Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
drivers/perf/alibaba_uncore_drw_pmu.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/perf/alibaba_uncore_drw_pmu.c b/drivers/perf/alibaba_uncore_drw_pmu.c
index a9277dcf90ce..251f0a2dee84 100644
--- a/drivers/perf/alibaba_uncore_drw_pmu.c
+++ b/drivers/perf/alibaba_uncore_drw_pmu.c
@@ -743,25 +743,28 @@ static void ali_drw_pmu_remove(struct platform_device *pdev)
static int ali_drw_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
{
+ cpumask_var_t node_online_cpus;
struct ali_drw_pmu_irq *irq;
struct ali_drw_pmu *drw_pmu;
unsigned int target;
int ret;
- cpumask_t node_online_cpus;
irq = hlist_entry_safe(node, struct ali_drw_pmu_irq, node);
if (cpu != irq->cpu)
return 0;
- ret = cpumask_and(&node_online_cpus,
+ if (!alloc_cpumask_var(&node_online_cpus, GFP_KERNEL))
+ return 0;
+
+ ret = cpumask_and(node_online_cpus,
cpumask_of_node(cpu_to_node(cpu)), cpu_online_mask);
if (ret)
- target = cpumask_any_but(&node_online_cpus, cpu);
+ target = cpumask_any_but(node_online_cpus, cpu);
else
target = cpumask_any_but(cpu_online_mask, cpu);
if (target >= nr_cpu_ids)
- return 0;
+ goto __free_cpumask;
/* We're only reading, but this isn't the place to be involving RCU */
mutex_lock(&ali_drw_pmu_irqs_lock);
@@ -772,6 +775,8 @@ static int ali_drw_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
WARN_ON(irq_set_affinity_hint(irq->irq_num, cpumask_of(target)));
irq->cpu = target;
+__free_cpumask:
+ free_cpumask_var(node_online_cpus);
return 0;
}
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/9] perf/arm-cmn: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack Dawei Li
2024-04-02 10:56 ` [PATCH 1/9] perf/alibaba_uncore_drw: " Dawei Li
@ 2024-04-02 10:56 ` Dawei Li
2024-04-05 14:30 ` Robin Murphy
2024-04-02 10:56 ` [PATCH 3/9] perf/arm_cspmu: " Dawei Li
` (7 subsequent siblings)
9 siblings, 1 reply; 19+ messages in thread
From: Dawei Li @ 2024-04-02 10:56 UTC (permalink / raw
To: will, mark.rutland
Cc: xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm,
Dawei Li
For CONFIG_CPUMASK_OFFSTACK=y kernel, explicit allocation of cpumask
variable on stack is not recommended since it can cause potential stack
overflow.
Instead, kernel code should always use *cpumask_var API(s) to allocate
cpumask var in config- neutral way, leaving allocation strategy to
CONFIG_CPUMASK_OFFSTACK.
Use *cpumask_var API(s) to address it.
Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
drivers/perf/arm-cmn.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index 7ef9c7e4836b..7278fd72d3da 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -1949,21 +1949,26 @@ static int arm_cmn_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_no
{
struct arm_cmn *cmn;
unsigned int target;
+ cpumask_var_t mask;
int node;
- cpumask_t mask;
cmn = hlist_entry_safe(cpuhp_node, struct arm_cmn, cpuhp_node);
if (cpu != cmn->cpu)
return 0;
+ if (!alloc_cpumask_var(&mask, GFP_KERNEL))
+ return 0;
+
node = dev_to_node(cmn->dev);
- if (cpumask_and(&mask, cpumask_of_node(node), cpu_online_mask) &&
- cpumask_andnot(&mask, &mask, cpumask_of(cpu)))
- target = cpumask_any(&mask);
+ if (cpumask_and(mask, cpumask_of_node(node), cpu_online_mask) &&
+ cpumask_andnot(mask, mask, cpumask_of(cpu)))
+ target = cpumask_any(mask);
else
target = cpumask_any_but(cpu_online_mask, cpu);
if (target < nr_cpu_ids)
arm_cmn_migrate(cmn, target);
+
+ free_cpumask_var(mask);
return 0;
}
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/9] perf/arm_cspmu: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack Dawei Li
2024-04-02 10:56 ` [PATCH 1/9] perf/alibaba_uncore_drw: " Dawei Li
2024-04-02 10:56 ` [PATCH 2/9] perf/arm-cmn: " Dawei Li
@ 2024-04-02 10:56 ` Dawei Li
2024-04-02 10:56 ` [PATCH 4/9] perf/arm_dsu: " Dawei Li
` (6 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Dawei Li @ 2024-04-02 10:56 UTC (permalink / raw
To: will, mark.rutland
Cc: xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm,
Dawei Li
For CONFIG_CPUMASK_OFFSTACK=y kernel, explicit allocation of cpumask
variable on stack is not recommended since it can cause potential stack
overflow.
Instead, kernel code should always use *cpumask_var API(s) to allocate
cpumask var in config- neutral way, leaving allocation strategy to
CONFIG_CPUMASK_OFFSTACK.
Use *cpumask_var API(s) to address it.
Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
drivers/perf/arm_cspmu/arm_cspmu.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/perf/arm_cspmu/arm_cspmu.c b/drivers/perf/arm_cspmu/arm_cspmu.c
index b9a252272f1e..8fa7c26aec28 100644
--- a/drivers/perf/arm_cspmu/arm_cspmu.c
+++ b/drivers/perf/arm_cspmu/arm_cspmu.c
@@ -1322,8 +1322,8 @@ static int arm_cspmu_cpu_online(unsigned int cpu, struct hlist_node *node)
static int arm_cspmu_cpu_teardown(unsigned int cpu, struct hlist_node *node)
{
+ cpumask_var_t online_supported;
int dst;
- struct cpumask online_supported;
struct arm_cspmu *cspmu =
hlist_entry_safe(node, struct arm_cspmu, cpuhp_node);
@@ -1332,17 +1332,22 @@ static int arm_cspmu_cpu_teardown(unsigned int cpu, struct hlist_node *node)
if (!cpumask_test_and_clear_cpu(cpu, &cspmu->active_cpu))
return 0;
+ if (!alloc_cpumask_var(&online_supported, GFP_KERNEL))
+ return 0;
+
/* Choose a new CPU to migrate ownership of the PMU to */
- cpumask_and(&online_supported, &cspmu->associated_cpus,
+ cpumask_and(online_supported, &cspmu->associated_cpus,
cpu_online_mask);
- dst = cpumask_any_but(&online_supported, cpu);
+ dst = cpumask_any_but(online_supported, cpu);
if (dst >= nr_cpu_ids)
- return 0;
+ goto __free_cpumask;
/* Use this CPU for event counting */
perf_pmu_migrate_context(&cspmu->pmu, cpu, dst);
arm_cspmu_set_active_cpu(dst, cspmu);
+__free_cpumask:
+ free_cpumask_var(online_supported);
return 0;
}
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 4/9] perf/arm_dsu: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack Dawei Li
` (2 preceding siblings ...)
2024-04-02 10:56 ` [PATCH 3/9] perf/arm_cspmu: " Dawei Li
@ 2024-04-02 10:56 ` Dawei Li
2024-04-02 23:58 ` kernel test robot
2024-04-03 1:43 ` kernel test robot
2024-04-02 10:56 ` [PATCH 5/9] perf/dwc_pcie: " Dawei Li
` (5 subsequent siblings)
9 siblings, 2 replies; 19+ messages in thread
From: Dawei Li @ 2024-04-02 10:56 UTC (permalink / raw
To: will, mark.rutland
Cc: xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm,
Dawei Li
For CONFIG_CPUMASK_OFFSTACK=y kernel, explicit allocation of cpumask
variable on stack is not recommended since it can cause potential stack
overflow.
Instead, kernel code should always use *cpumask_var API(s) to allocate
cpumask var in config- neutral way, leaving allocation strategy to
CONFIG_CPUMASK_OFFSTACK.
Use *cpumask_var API(s) to address it.
Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
drivers/perf/arm_dsu_pmu.c | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/drivers/perf/arm_dsu_pmu.c b/drivers/perf/arm_dsu_pmu.c
index bae3ca37f846..87efdca05807 100644
--- a/drivers/perf/arm_dsu_pmu.c
+++ b/drivers/perf/arm_dsu_pmu.c
@@ -230,13 +230,21 @@ static const struct attribute_group *dsu_pmu_attr_groups[] = {
NULL,
};
-static int dsu_pmu_get_online_cpu_any_but(struct dsu_pmu *dsu_pmu, int cpu)
+static unsigned int dsu_pmu_get_online_cpu_any_but(struct dsu_pmu *dsu_pmu, int cpu)
{
- struct cpumask online_supported;
+ cpumask_var_t online_supported;
+ unsigned int ret;
- cpumask_and(&online_supported,
- &dsu_pmu->associated_cpus, cpu_online_mask);
- return cpumask_any_but(&online_supported, cpu);
+ if (!alloc_cpumask_var(&online_supported, GFP_KERNEL))
+ return -ENOMEM;
+
+ cpumask_and(online_supported,
+ &dsu_pmu->associated_cpus, cpu_online_mask);
+ ret = cpumask_any_but(&online_supported, cpu);
+
+ free_cpumask_var(online_supported);
+
+ return ret;
}
static inline bool dsu_pmu_counter_valid(struct dsu_pmu *dsu_pmu, u32 idx)
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 5/9] perf/dwc_pcie: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack Dawei Li
` (3 preceding siblings ...)
2024-04-02 10:56 ` [PATCH 4/9] perf/arm_dsu: " Dawei Li
@ 2024-04-02 10:56 ` Dawei Li
2024-04-02 10:56 ` [PATCH 6/9] perf/hisi_pcie: " Dawei Li
` (4 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Dawei Li @ 2024-04-02 10:56 UTC (permalink / raw
To: will, mark.rutland
Cc: xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm,
Dawei Li
For CONFIG_CPUMASK_OFFSTACK=y kernel, explicit allocation of cpumask
variable on stack is not recommended since it can cause potential stack
overflow.
Instead, kernel code should always use *cpumask_var API(s) to allocate
cpumask var in config- neutral way, leaving allocation strategy to
CONFIG_CPUMASK_OFFSTACK.
Use *cpumask_var API(s) to address it.
Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
drivers/perf/dwc_pcie_pmu.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/perf/dwc_pcie_pmu.c b/drivers/perf/dwc_pcie_pmu.c
index 957058ad0099..97037b6aaa97 100644
--- a/drivers/perf/dwc_pcie_pmu.c
+++ b/drivers/perf/dwc_pcie_pmu.c
@@ -690,33 +690,38 @@ static int dwc_pcie_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_n
{
struct dwc_pcie_pmu *pcie_pmu;
struct pci_dev *pdev;
- int node;
- cpumask_t mask;
unsigned int target;
+ cpumask_var_t mask;
+ int node;
pcie_pmu = hlist_entry_safe(cpuhp_node, struct dwc_pcie_pmu, cpuhp_node);
/* Nothing to do if this CPU doesn't own the PMU */
if (cpu != pcie_pmu->on_cpu)
return 0;
+ if (!alloc_cpumask_var(&mask, GFP_KERNEL))
+ return 0;
+
pcie_pmu->on_cpu = -1;
pdev = pcie_pmu->pdev;
node = dev_to_node(&pdev->dev);
- if (cpumask_and(&mask, cpumask_of_node(node), cpu_online_mask) &&
- cpumask_andnot(&mask, &mask, cpumask_of(cpu)))
- target = cpumask_any(&mask);
+ if (cpumask_and(mask, cpumask_of_node(node), cpu_online_mask) &&
+ cpumask_andnot(mask, mask, cpumask_of(cpu)))
+ target = cpumask_any(mask);
else
target = cpumask_any_but(cpu_online_mask, cpu);
if (target >= nr_cpu_ids) {
pci_err(pdev, "There is no CPU to set\n");
- return 0;
+ goto __free_cpumask;
}
/* This PMU does NOT support interrupt, just migrate context. */
perf_pmu_migrate_context(&pcie_pmu->pmu, cpu, target);
pcie_pmu->on_cpu = target;
+__free_cpumask:
+ free_cpumask_var(mask);
return 0;
}
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 6/9] perf/hisi_pcie: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack Dawei Li
` (4 preceding siblings ...)
2024-04-02 10:56 ` [PATCH 5/9] perf/dwc_pcie: " Dawei Li
@ 2024-04-02 10:56 ` Dawei Li
2024-04-02 10:56 ` [PATCH 7/9] perf/hisi_uncore: " Dawei Li
` (3 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Dawei Li @ 2024-04-02 10:56 UTC (permalink / raw
To: will, mark.rutland
Cc: xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm,
Dawei Li
For CONFIG_CPUMASK_OFFSTACK=y kernel, explicit allocation of cpumask
variable on stack is not recommended since it can cause potential stack
overflow.
Instead, kernel code should always use *cpumask_var API(s) to allocate
cpumask var in config- neutral way, leaving allocation strategy to
CONFIG_CPUMASK_OFFSTACK.
Use *cpumask_var API(s) to address it.
Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
drivers/perf/hisilicon/hisi_pcie_pmu.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/perf/hisilicon/hisi_pcie_pmu.c b/drivers/perf/hisilicon/hisi_pcie_pmu.c
index 5d1f0e9fdb08..0183640db2de 100644
--- a/drivers/perf/hisilicon/hisi_pcie_pmu.c
+++ b/drivers/perf/hisilicon/hisi_pcie_pmu.c
@@ -673,26 +673,29 @@ static int hisi_pcie_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
{
struct hisi_pcie_pmu *pcie_pmu = hlist_entry_safe(node, struct hisi_pcie_pmu, node);
unsigned int target;
- cpumask_t mask;
+ cpumask_var_t mask;
int numa_node;
/* Nothing to do if this CPU doesn't own the PMU */
if (pcie_pmu->on_cpu != cpu)
return 0;
+ if (!alloc_cpumask_var(&mask, GFP_KERNEL))
+ return 0;
+
pcie_pmu->on_cpu = -1;
/* Choose a local CPU from all online cpus. */
numa_node = dev_to_node(&pcie_pmu->pdev->dev);
- if (cpumask_and(&mask, cpumask_of_node(numa_node), cpu_online_mask) &&
- cpumask_andnot(&mask, &mask, cpumask_of(cpu)))
- target = cpumask_any(&mask);
+ if (cpumask_and(mask, cpumask_of_node(numa_node), cpu_online_mask) &&
+ cpumask_andnot(mask, mask, cpumask_of(cpu)))
+ target = cpumask_any(mask);
else
target = cpumask_any_but(cpu_online_mask, cpu);
if (target >= nr_cpu_ids) {
pci_err(pcie_pmu->pdev, "There is no CPU to set\n");
- return 0;
+ goto __free_cpumask;
}
perf_pmu_migrate_context(&pcie_pmu->pmu, cpu, target);
@@ -700,6 +703,8 @@ static int hisi_pcie_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
pcie_pmu->on_cpu = target;
WARN_ON(irq_set_affinity(pcie_pmu->irq, cpumask_of(target)));
+__free_cpumask:
+ free_cpumask_var(mask);
return 0;
}
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 7/9] perf/hisi_uncore: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack Dawei Li
` (5 preceding siblings ...)
2024-04-02 10:56 ` [PATCH 6/9] perf/hisi_pcie: " Dawei Li
@ 2024-04-02 10:56 ` Dawei Li
2024-04-02 10:56 ` [PATCH 8/9] perf/qcom_l2: " Dawei Li
` (2 subsequent siblings)
9 siblings, 0 replies; 19+ messages in thread
From: Dawei Li @ 2024-04-02 10:56 UTC (permalink / raw
To: will, mark.rutland
Cc: xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm,
Dawei Li
For CONFIG_CPUMASK_OFFSTACK=y kernel, explicit allocation of cpumask
variable on stack is not recommended since it can cause potential stack
overflow.
Instead, kernel code should always use *cpumask_var API(s) to allocate
cpumask var in config- neutral way, leaving allocation strategy to
CONFIG_CPUMASK_OFFSTACK.
Use *cpumask_var API(s) to address it.
Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
drivers/perf/hisilicon/hisi_uncore_pmu.c | 13 +++++++++----
1 file changed, 9 insertions(+), 4 deletions(-)
diff --git a/drivers/perf/hisilicon/hisi_uncore_pmu.c b/drivers/perf/hisilicon/hisi_uncore_pmu.c
index 04031450d5fe..06891d23b21c 100644
--- a/drivers/perf/hisilicon/hisi_uncore_pmu.c
+++ b/drivers/perf/hisilicon/hisi_uncore_pmu.c
@@ -504,7 +504,7 @@ int hisi_uncore_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
{
struct hisi_pmu *hisi_pmu = hlist_entry_safe(node, struct hisi_pmu,
node);
- cpumask_t pmu_online_cpus;
+ cpumask_var_t pmu_online_cpus;
unsigned int target;
if (!cpumask_test_and_clear_cpu(cpu, &hisi_pmu->associated_cpus))
@@ -514,21 +514,26 @@ int hisi_uncore_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
if (hisi_pmu->on_cpu != cpu)
return 0;
+ if (!alloc_cpumask_var(&pmu_online_cpus, GFP_KERNEL))
+ return 0;
+
/* Give up ownership of the PMU */
hisi_pmu->on_cpu = -1;
/* Choose a new CPU to migrate ownership of the PMU to */
- cpumask_and(&pmu_online_cpus, &hisi_pmu->associated_cpus,
+ cpumask_and(pmu_online_cpus, &hisi_pmu->associated_cpus,
cpu_online_mask);
- target = cpumask_any_but(&pmu_online_cpus, cpu);
+ target = cpumask_any_but(pmu_online_cpus, cpu);
if (target >= nr_cpu_ids)
- return 0;
+ goto __free_cpumask;
perf_pmu_migrate_context(&hisi_pmu->pmu, cpu, target);
/* Use this CPU for event counting */
hisi_pmu->on_cpu = target;
WARN_ON(irq_set_affinity(hisi_pmu->irq, cpumask_of(target)));
+__free_cpumask:
+ free_cpumask_var(pmu_online_cpus);
return 0;
}
EXPORT_SYMBOL_GPL(hisi_uncore_pmu_offline_cpu);
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 8/9] perf/qcom_l2: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack Dawei Li
` (6 preceding siblings ...)
2024-04-02 10:56 ` [PATCH 7/9] perf/hisi_uncore: " Dawei Li
@ 2024-04-02 10:56 ` Dawei Li
2024-04-02 10:56 ` [PATCH 9/9] perf/thunder_x2: " Dawei Li
2024-04-02 11:12 ` [PATCH 0/9] perf: " Mark Rutland
9 siblings, 0 replies; 19+ messages in thread
From: Dawei Li @ 2024-04-02 10:56 UTC (permalink / raw
To: will, mark.rutland
Cc: xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm,
Dawei Li
For CONFIG_CPUMASK_OFFSTACK=y kernel, explicit allocation of cpumask
variable on stack is not recommended since it can cause potential stack
overflow.
Instead, kernel code should always use *cpumask_var API(s) to allocate
cpumask var in config- neutral way, leaving allocation strategy to
CONFIG_CPUMASK_OFFSTACK.
Use *cpumask_var API(s) to address it.
Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
drivers/perf/qcom_l2_pmu.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/drivers/perf/qcom_l2_pmu.c b/drivers/perf/qcom_l2_pmu.c
index 148df5ae8ef8..8fe0c7557521 100644
--- a/drivers/perf/qcom_l2_pmu.c
+++ b/drivers/perf/qcom_l2_pmu.c
@@ -801,9 +801,9 @@ static int l2cache_pmu_online_cpu(unsigned int cpu, struct hlist_node *node)
static int l2cache_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
{
- struct cluster_pmu *cluster;
+ cpumask_var_t cluster_online_cpus;
struct l2cache_pmu *l2cache_pmu;
- cpumask_t cluster_online_cpus;
+ struct cluster_pmu *cluster;
unsigned int target;
l2cache_pmu = hlist_entry_safe(node, struct l2cache_pmu, node);
@@ -815,17 +815,20 @@ static int l2cache_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
if (cluster->on_cpu != cpu)
return 0;
+ if (!alloc_cpumask_var(&cluster_online_cpus, GFP_KERNEL))
+ return 0;
+
/* Give up ownership of cluster */
cpumask_clear_cpu(cpu, &l2cache_pmu->cpumask);
cluster->on_cpu = -1;
/* Any other CPU for this cluster which is still online */
- cpumask_and(&cluster_online_cpus, &cluster->cluster_cpus,
+ cpumask_and(cluster_online_cpus, &cluster->cluster_cpus,
cpu_online_mask);
- target = cpumask_any_but(&cluster_online_cpus, cpu);
+ target = cpumask_any_but(cluster_online_cpus, cpu);
if (target >= nr_cpu_ids) {
disable_irq(cluster->irq);
- return 0;
+ goto __free_cpumask;
}
perf_pmu_migrate_context(&l2cache_pmu->pmu, cpu, target);
@@ -833,6 +836,8 @@ static int l2cache_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
cpumask_set_cpu(target, &l2cache_pmu->cpumask);
WARN_ON(irq_set_affinity(cluster->irq, cpumask_of(target)));
+__free_cpumask:
+ free_cpumask_var(cluster_online_cpus);
return 0;
}
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 9/9] perf/thunder_x2: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack Dawei Li
` (7 preceding siblings ...)
2024-04-02 10:56 ` [PATCH 8/9] perf/qcom_l2: " Dawei Li
@ 2024-04-02 10:56 ` Dawei Li
2024-04-02 11:12 ` [PATCH 0/9] perf: " Mark Rutland
9 siblings, 0 replies; 19+ messages in thread
From: Dawei Li @ 2024-04-02 10:56 UTC (permalink / raw
To: will, mark.rutland
Cc: xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm,
Dawei Li
For CONFIG_CPUMASK_OFFSTACK=y kernel, explicit allocation of cpumask
variable on stack is not recommended since it can cause potential stack
overflow.
Instead, kernel code should always use *cpumask_var API(s) to allocate
cpumask var in config- neutral way, leaving allocation strategy to
CONFIG_CPUMASK_OFFSTACK.
Use *cpumask_var API(s) to address it.
Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
---
drivers/perf/thunderx2_pmu.c | 20 ++++++++++++--------
1 file changed, 12 insertions(+), 8 deletions(-)
diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
index e16d10c763de..8a02a4533b32 100644
--- a/drivers/perf/thunderx2_pmu.c
+++ b/drivers/perf/thunderx2_pmu.c
@@ -932,9 +932,9 @@ static int tx2_uncore_pmu_online_cpu(unsigned int cpu,
static int tx2_uncore_pmu_offline_cpu(unsigned int cpu,
struct hlist_node *hpnode)
{
- int new_cpu;
+ cpumask_var_t cpu_online_mask_temp;
struct tx2_uncore_pmu *tx2_pmu;
- struct cpumask cpu_online_mask_temp;
+ int new_cpu;
tx2_pmu = hlist_entry_safe(hpnode,
struct tx2_uncore_pmu, hpnode);
@@ -945,17 +945,21 @@ static int tx2_uncore_pmu_offline_cpu(unsigned int cpu,
if (tx2_pmu->hrtimer_callback)
hrtimer_cancel(&tx2_pmu->hrtimer);
- cpumask_copy(&cpu_online_mask_temp, cpu_online_mask);
- cpumask_clear_cpu(cpu, &cpu_online_mask_temp);
- new_cpu = cpumask_any_and(
- cpumask_of_node(tx2_pmu->node),
- &cpu_online_mask_temp);
+ if (!alloc_cpumask_var(&cpu_online_mask_temp, GFP_KERNEL))
+ return 0;
+
+ cpumask_copy(cpu_online_mask_temp, cpu_online_mask);
+ cpumask_clear_cpu(cpu, cpu_online_mask_temp);
+ new_cpu = cpumask_any_and(cpumask_of_node(tx2_pmu->node),
+ cpu_online_mask_temp);
tx2_pmu->cpu = new_cpu;
if (new_cpu >= nr_cpu_ids)
- return 0;
+ goto __free_cpumask;
perf_pmu_migrate_context(&tx2_pmu->pmu, cpu, new_cpu);
+__free_cpumask:
+ free_cpumask_var(cpu_online_mask_temp);
return 0;
}
--
2.27.0
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/9] perf/alibaba_uncore_drw: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 ` [PATCH 1/9] perf/alibaba_uncore_drw: " Dawei Li
@ 2024-04-02 11:06 ` Mark Rutland
0 siblings, 0 replies; 19+ messages in thread
From: Mark Rutland @ 2024-04-02 11:06 UTC (permalink / raw
To: Dawei Li
Cc: will, xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm
On Tue, Apr 02, 2024 at 06:56:02PM +0800, Dawei Li wrote:
> For CONFIG_CPUMASK_OFFSTACK=y kernel, explicit allocation of cpumask
> variable on stack is not recommended since it can cause potential stack
> overflow.
>
> Instead, kernel code should always use *cpumask_var API(s) to allocate
> cpumask var in config- neutral way, leaving allocation strategy to
> CONFIG_CPUMASK_OFFSTACK.
>
> Use *cpumask_var API(s) to address it.
>
> Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
> ---
> drivers/perf/alibaba_uncore_drw_pmu.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/perf/alibaba_uncore_drw_pmu.c b/drivers/perf/alibaba_uncore_drw_pmu.c
> index a9277dcf90ce..251f0a2dee84 100644
> --- a/drivers/perf/alibaba_uncore_drw_pmu.c
> +++ b/drivers/perf/alibaba_uncore_drw_pmu.c
> @@ -743,25 +743,28 @@ static void ali_drw_pmu_remove(struct platform_device *pdev)
>
> static int ali_drw_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
> {
> + cpumask_var_t node_online_cpus;
> struct ali_drw_pmu_irq *irq;
> struct ali_drw_pmu *drw_pmu;
> unsigned int target;
> int ret;
> - cpumask_t node_online_cpus;
>
> irq = hlist_entry_safe(node, struct ali_drw_pmu_irq, node);
> if (cpu != irq->cpu)
> return 0;
>
> - ret = cpumask_and(&node_online_cpus,
> + if (!alloc_cpumask_var(&node_online_cpus, GFP_KERNEL))
> + return 0;
NAK. This error path leaves things in an incorrect state and this approach does
not make sense.
Please allocate the cpumasks when we allocate the PMU. Then we can reasonably
fail to probe the PMU if we don't have enough memory, and the masks will
definitely be accessible in gotplug paths.
The same comment applies to the whole series.
Mark.
> +
> + ret = cpumask_and(node_online_cpus,
> cpumask_of_node(cpu_to_node(cpu)), cpu_online_mask);
> if (ret)
> - target = cpumask_any_but(&node_online_cpus, cpu);
> + target = cpumask_any_but(node_online_cpus, cpu);
> else
> target = cpumask_any_but(cpu_online_mask, cpu);
>
> if (target >= nr_cpu_ids)
> - return 0;
> + goto __free_cpumask;
>
> /* We're only reading, but this isn't the place to be involving RCU */
> mutex_lock(&ali_drw_pmu_irqs_lock);
> @@ -772,6 +775,8 @@ static int ali_drw_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
> WARN_ON(irq_set_affinity_hint(irq->irq_num, cpumask_of(target)));
> irq->cpu = target;
>
> +__free_cpumask:
> + free_cpumask_var(node_online_cpus);
> return 0;
> }
>
> --
> 2.27.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack Dawei Li
` (8 preceding siblings ...)
2024-04-02 10:56 ` [PATCH 9/9] perf/thunder_x2: " Dawei Li
@ 2024-04-02 11:12 ` Mark Rutland
2024-04-02 13:40 ` Dawei Li
9 siblings, 1 reply; 19+ messages in thread
From: Mark Rutland @ 2024-04-02 11:12 UTC (permalink / raw
To: Dawei Li
Cc: will, xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm
On Tue, Apr 02, 2024 at 06:56:01PM +0800, Dawei Li wrote:
> Hi,
>
> This series try to eliminate direct cpumask var allocation from stack
> for perf subsystem.
>
> Direct/explicit allocation of cpumask on stack could be dangerous since
> it can lead to stack overflow for systems with big NR_CPUS or
> CONFIG_CPUMASK_OFFSTACK=y.
>
> For arm64, it's more urgent since commit 3fbd56f0e7c1 ("ARM64: Dynamically
> allocate cpumasks and increase supported CPUs to 512").
>
> It's sort of a pattern that almost every cpumask var in perf subystem
> occurs in teardown callback of cpuhp. In which case, if dynamic
> allocation failed(which is unlikely), we choose return 0 rather than
> -ENOMEM to caller cuz:
> @teardown is not supposed to fail and if it does, system crashes:
... but we've left the system in an incorrect state, so that makes no sense.
As I commented on the first patch, NAK to dynamically allocating cpumasks in
the CPUHP callbacks. Please allocate the necessry cpumasks up-front when we
probe the PMU. At that time we can handle an allocation failure by cleaning up
and failing to probe the PMU, and then the CPUHP callbacks don't need to
allocate memory to offline a CPU...
Also, for the titles it'd be better to say something like "avoid placing
cpumasks on the stack", because "explicit cpumask var allocation" sounds like
the use of alloc_cpumask_var().
Mark.
>
> static int cpuhp_issue_call(int cpu, enum cpuhp_state state, bool bringup,
> struct hlist_node *node)
> {
> struct cpuhp_step *sp = cpuhp_get_step(state);
> int ret;
>
> /*
> * If there's nothing to do, we done.
> * Relies on the union for multi_instance.
> */
> if (cpuhp_step_empty(bringup, sp))
> return 0;
> /*
> * The non AP bound callbacks can fail on bringup. On teardown
> * e.g. module removal we crash for now.
> */
> #ifdef CONFIG_SMP
> if (cpuhp_is_ap_state(state))
> ret = cpuhp_invoke_ap_callback(cpu, state, bringup, node);
> else
> ret = cpuhp_invoke_callback(cpu, state, bringup, node,
> NULL);
> #else
> ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
> #endif
> BUG_ON(ret && !bringup);
> return ret;
> }
>
> Dawei Li (9):
> perf/alibaba_uncore_drw: Avoid explicit cpumask var allocation from
> stack
> perf/arm-cmn: Avoid explicit cpumask var allocation from stack
> perf/arm_cspmu: Avoid explicit cpumask var allocation from stack
> perf/arm_dsu: Avoid explicit cpumask var allocation from stack
> perf/dwc_pcie: Avoid explicit cpumask var allocation from stack
> perf/hisi_pcie: Avoid explicit cpumask var allocation from stack
> perf/hisi_uncore: Avoid explicit cpumask var allocation from stack
> perf/qcom_l2: Avoid explicit cpumask var allocation from stack
> perf/thunder_x2: Avoid explicit cpumask var allocation from stack
>
> drivers/perf/alibaba_uncore_drw_pmu.c | 13 +++++++++----
> drivers/perf/arm-cmn.c | 13 +++++++++----
> drivers/perf/arm_cspmu/arm_cspmu.c | 13 +++++++++----
> drivers/perf/arm_dsu_pmu.c | 18 +++++++++++++-----
> drivers/perf/dwc_pcie_pmu.c | 17 +++++++++++------
> drivers/perf/hisilicon/hisi_pcie_pmu.c | 15 ++++++++++-----
> drivers/perf/hisilicon/hisi_uncore_pmu.c | 13 +++++++++----
> drivers/perf/qcom_l2_pmu.c | 15 ++++++++++-----
> drivers/perf/thunderx2_pmu.c | 20 ++++++++++++--------
> 9 files changed, 92 insertions(+), 45 deletions(-)
>
>
> Thanks,
>
> Dawei
>
> --
> 2.27.0
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack
2024-04-02 11:12 ` [PATCH 0/9] perf: " Mark Rutland
@ 2024-04-02 13:40 ` Dawei Li
2024-04-02 14:41 ` Mark Rutland
0 siblings, 1 reply; 19+ messages in thread
From: Dawei Li @ 2024-04-02 13:40 UTC (permalink / raw
To: Mark Rutland
Cc: will, xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm
Hi Mark,
Thanks for the quick review.
On Tue, Apr 02, 2024 at 12:12:50PM +0100, Mark Rutland wrote:
> On Tue, Apr 02, 2024 at 06:56:01PM +0800, Dawei Li wrote:
> > Hi,
> >
> > This series try to eliminate direct cpumask var allocation from stack
> > for perf subsystem.
> >
> > Direct/explicit allocation of cpumask on stack could be dangerous since
> > it can lead to stack overflow for systems with big NR_CPUS or
> > CONFIG_CPUMASK_OFFSTACK=y.
> >
> > For arm64, it's more urgent since commit 3fbd56f0e7c1 ("ARM64: Dynamically
> > allocate cpumasks and increase supported CPUs to 512").
> >
> > It's sort of a pattern that almost every cpumask var in perf subystem
> > occurs in teardown callback of cpuhp. In which case, if dynamic
> > allocation failed(which is unlikely), we choose return 0 rather than
> > -ENOMEM to caller cuz:
> > @teardown is not supposed to fail and if it does, system crashes:
>
> .. but we've left the system in an incorrect state, so that makes no sense.
>
> As I commented on the first patch, NAK to dynamically allocating cpumasks in
> the CPUHP callbacks. Please allocate the necessry cpumasks up-front when we
> probe the PMU. At that time we can handle an allocation failure by cleaning up
> and failing to probe the PMU, and then the CPUHP callbacks don't need to
> allocate memory to offline a CPU...
Agreed that dynamically allocation in callbacks lead to inconsistency
to system.
My (original)alternative plan is simple but ugly, just make cpumask var
_static_ and add extra static lock to protect it.
The only difference between solution above and your proposal is static/
dynamic alloction. CPUHP's teardown cb is supposed to run in targetted
cpuhp thread for most cases, and it's racy. Even the cpumask var is
wrapped in dynamically allocated struct xxx_pmu, it's still shareable
between different threads/contexts and needs proper protection.
Simple as this(_untested_):
diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
index 7ef9c7e4836b..fa89c3db4d7d 100644
--- a/drivers/perf/arm-cmn.c
+++ b/drivers/perf/arm-cmn.c
@@ -1950,18 +1950,24 @@ static int arm_cmn_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_no
struct arm_cmn *cmn;
unsigned int target;
int node;
- cpumask_t mask;
+ static cpumask_t mask;
+ static DEFINE_SPINLOCK(cpumask_lock);
cmn = hlist_entry_safe(cpuhp_node, struct arm_cmn, cpuhp_node);
if (cpu != cmn->cpu)
return 0;
+ spin_lock(&cpumask_lock);
+
node = dev_to_node(cmn->dev);
if (cpumask_and(&mask, cpumask_of_node(node), cpu_online_mask) &&
cpumask_andnot(&mask, &mask, cpumask_of(cpu)))
target = cpumask_any(&mask);
else
target = cpumask_any_but(cpu_online_mask, cpu);
+
+ spin_unlock(&cpumask_lock);
+
if (target < nr_cpu_ids)
arm_cmn_migrate(cmn, target);
return 0;
And yes, static allocation is evil :)
Thanks,
Dawei
>
> Also, for the titles it'd be better to say something like "avoid placing
> cpumasks on the stack", because "explicit cpumask var allocation" sounds like
> the use of alloc_cpumask_var().
Sound great! I will update it.
>
> Mark.
>
> >
> > static int cpuhp_issue_call(int cpu, enum cpuhp_state state, bool bringup,
> > struct hlist_node *node)
> > {
> > struct cpuhp_step *sp = cpuhp_get_step(state);
> > int ret;
> >
> > /*
> > * If there's nothing to do, we done.
> > * Relies on the union for multi_instance.
> > */
> > if (cpuhp_step_empty(bringup, sp))
> > return 0;
> > /*
> > * The non AP bound callbacks can fail on bringup. On teardown
> > * e.g. module removal we crash for now.
> > */
> > #ifdef CONFIG_SMP
> > if (cpuhp_is_ap_state(state))
> > ret = cpuhp_invoke_ap_callback(cpu, state, bringup, node);
> > else
> > ret = cpuhp_invoke_callback(cpu, state, bringup, node,
> > NULL);
> > #else
> > ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
> > #endif
> > BUG_ON(ret && !bringup);
> > return ret;
> > }
> >
> > Dawei Li (9):
> > perf/alibaba_uncore_drw: Avoid explicit cpumask var allocation from
> > stack
> > perf/arm-cmn: Avoid explicit cpumask var allocation from stack
> > perf/arm_cspmu: Avoid explicit cpumask var allocation from stack
> > perf/arm_dsu: Avoid explicit cpumask var allocation from stack
> > perf/dwc_pcie: Avoid explicit cpumask var allocation from stack
> > perf/hisi_pcie: Avoid explicit cpumask var allocation from stack
> > perf/hisi_uncore: Avoid explicit cpumask var allocation from stack
> > perf/qcom_l2: Avoid explicit cpumask var allocation from stack
> > perf/thunder_x2: Avoid explicit cpumask var allocation from stack
> >
> > drivers/perf/alibaba_uncore_drw_pmu.c | 13 +++++++++----
> > drivers/perf/arm-cmn.c | 13 +++++++++----
> > drivers/perf/arm_cspmu/arm_cspmu.c | 13 +++++++++----
> > drivers/perf/arm_dsu_pmu.c | 18 +++++++++++++-----
> > drivers/perf/dwc_pcie_pmu.c | 17 +++++++++++------
> > drivers/perf/hisilicon/hisi_pcie_pmu.c | 15 ++++++++++-----
> > drivers/perf/hisilicon/hisi_uncore_pmu.c | 13 +++++++++----
> > drivers/perf/qcom_l2_pmu.c | 15 ++++++++++-----
> > drivers/perf/thunderx2_pmu.c | 20 ++++++++++++--------
> > 9 files changed, 92 insertions(+), 45 deletions(-)
> >
> >
> > Thanks,
> >
> > Dawei
> >
> > --
> > 2.27.0
> >
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack
2024-04-02 13:40 ` Dawei Li
@ 2024-04-02 14:41 ` Mark Rutland
2024-04-03 10:41 ` Dawei Li
0 siblings, 1 reply; 19+ messages in thread
From: Mark Rutland @ 2024-04-02 14:41 UTC (permalink / raw
To: Dawei Li
Cc: will, xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm
On Tue, Apr 02, 2024 at 09:40:08PM +0800, Dawei Li wrote:
> Hi Mark,
>
> Thanks for the quick review.
>
> On Tue, Apr 02, 2024 at 12:12:50PM +0100, Mark Rutland wrote:
> > On Tue, Apr 02, 2024 at 06:56:01PM +0800, Dawei Li wrote:
> > > Hi,
> > >
> > > This series try to eliminate direct cpumask var allocation from stack
> > > for perf subsystem.
> > >
> > > Direct/explicit allocation of cpumask on stack could be dangerous since
> > > it can lead to stack overflow for systems with big NR_CPUS or
> > > CONFIG_CPUMASK_OFFSTACK=y.
> > >
> > > For arm64, it's more urgent since commit 3fbd56f0e7c1 ("ARM64: Dynamically
> > > allocate cpumasks and increase supported CPUs to 512").
> > >
> > > It's sort of a pattern that almost every cpumask var in perf subystem
> > > occurs in teardown callback of cpuhp. In which case, if dynamic
> > > allocation failed(which is unlikely), we choose return 0 rather than
> > > -ENOMEM to caller cuz:
> > > @teardown is not supposed to fail and if it does, system crashes:
> >
> > .. but we've left the system in an incorrect state, so that makes no sense.
> >
> > As I commented on the first patch, NAK to dynamically allocating cpumasks in
> > the CPUHP callbacks. Please allocate the necessry cpumasks up-front when we
> > probe the PMU. At that time we can handle an allocation failure by cleaning up
> > and failing to probe the PMU, and then the CPUHP callbacks don't need to
> > allocate memory to offline a CPU...
>
> Agreed that dynamically allocation in callbacks lead to inconsistency
> to system.
>
> My (original)alternative plan is simple but ugly, just make cpumask var
> _static_ and add extra static lock to protect it.
>
> The only difference between solution above and your proposal is static/
> dynamic alloction. CPUHP's teardown cb is supposed to run in targetted
> cpuhp thread for most cases, and it's racy. Even the cpumask var is
> wrapped in dynamically allocated struct xxx_pmu, it's still shareable
> between different threads/contexts and needs proper protection.
I was under the impression that the cpuhp callbacks were *strictly* serialised.
If that's not the case, the existing offlining callbacks are horrendously
broken.
Are you *certain* these can race?
Regardless, adding additional locking here is not ok.
> Simple as this(_untested_):
>
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 7ef9c7e4836b..fa89c3db4d7d 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -1950,18 +1950,24 @@ static int arm_cmn_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_no
> struct arm_cmn *cmn;
> unsigned int target;
> int node;
> - cpumask_t mask;
> + static cpumask_t mask;
> + static DEFINE_SPINLOCK(cpumask_lock);
>
> cmn = hlist_entry_safe(cpuhp_node, struct arm_cmn, cpuhp_node);
> if (cpu != cmn->cpu)
> return 0;
>
> + spin_lock(&cpumask_lock);
> +
> node = dev_to_node(cmn->dev);
> if (cpumask_and(&mask, cpumask_of_node(node), cpu_online_mask) &&
> cpumask_andnot(&mask, &mask, cpumask_of(cpu)))
> target = cpumask_any(&mask);
> else
> target = cpumask_any_but(cpu_online_mask, cpu);
> +
> + spin_unlock(&cpumask_lock);
> +
> if (target < nr_cpu_ids)
> arm_cmn_migrate(cmn, target);
> return 0;
Looking at this case, the only reason we need the mask is because it made the
logic a little easier to write. All we really want is to choose some CPU in the
intersection of two masks ignoring a specific CPU, and there was no helper
function to do that.
We can add a new helper to do that for us, which would avoid redundant work to
manipulate the entire mask, and it would make the existing code simpler. I had
a series a few years back to add cpumask_any_and_but():
https://lore.kernel.org/lkml/1486381132-5610-1-git-send-email-mark.rutland@arm.com/
... and that's easy to use here, e.g.
| diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
| index 7ef9c7e4836b7..c6bbd387ccf8b 100644
| --- a/drivers/perf/arm-cmn.c
| +++ b/drivers/perf/arm-cmn.c
| @@ -1950,17 +1950,15 @@ static int arm_cmn_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_no
| struct arm_cmn *cmn;
| unsigned int target;
| int node;
| - cpumask_t mask;
|
| cmn = hlist_entry_safe(cpuhp_node, struct arm_cmn, cpuhp_node);
| if (cpu != cmn->cpu)
| return 0;
|
| node = dev_to_node(cmn->dev);
| - if (cpumask_and(&mask, cpumask_of_node(node), cpu_online_mask) &&
| - cpumask_andnot(&mask, &mask, cpumask_of(cpu)))
| - target = cpumask_any(&mask);
| - else
| + target = cpumask_any_and_but(cpu_online_mask, cpumask_of_node(node),
| + cpu);
| + if (target >= nr_cpu_ids)
| target = cpumask_any_but(cpu_online_mask, cpu);
| if (target < nr_cpu_ids)
| arm_cmn_migrate(cmn, target);
It doesn't trivially rebase since the cpumask code has changed a fair amount,
but I've managed to do that locally, and I can send that out as a
seven-years-late v2 if it's useful.
From a quick scan, it looks like that'd handle all cases in this series. Are
there any patterns in this series for which that would not be sufficient?
Mark.
>
> And yes, static allocation is evil :)
>
>
> Thanks,
>
> Dawei
>
> >
> > Also, for the titles it'd be better to say something like "avoid placing
> > cpumasks on the stack", because "explicit cpumask var allocation" sounds like
> > the use of alloc_cpumask_var().
>
> Sound great! I will update it.
>
> >
> > Mark.
> >
> > >
> > > static int cpuhp_issue_call(int cpu, enum cpuhp_state state, bool bringup,
> > > struct hlist_node *node)
> > > {
> > > struct cpuhp_step *sp = cpuhp_get_step(state);
> > > int ret;
> > >
> > > /*
> > > * If there's nothing to do, we done.
> > > * Relies on the union for multi_instance.
> > > */
> > > if (cpuhp_step_empty(bringup, sp))
> > > return 0;
> > > /*
> > > * The non AP bound callbacks can fail on bringup. On teardown
> > > * e.g. module removal we crash for now.
> > > */
> > > #ifdef CONFIG_SMP
> > > if (cpuhp_is_ap_state(state))
> > > ret = cpuhp_invoke_ap_callback(cpu, state, bringup, node);
> > > else
> > > ret = cpuhp_invoke_callback(cpu, state, bringup, node,
> > > NULL);
> > > #else
> > > ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
> > > #endif
> > > BUG_ON(ret && !bringup);
> > > return ret;
> > > }
> > >
> > > Dawei Li (9):
> > > perf/alibaba_uncore_drw: Avoid explicit cpumask var allocation from
> > > stack
> > > perf/arm-cmn: Avoid explicit cpumask var allocation from stack
> > > perf/arm_cspmu: Avoid explicit cpumask var allocation from stack
> > > perf/arm_dsu: Avoid explicit cpumask var allocation from stack
> > > perf/dwc_pcie: Avoid explicit cpumask var allocation from stack
> > > perf/hisi_pcie: Avoid explicit cpumask var allocation from stack
> > > perf/hisi_uncore: Avoid explicit cpumask var allocation from stack
> > > perf/qcom_l2: Avoid explicit cpumask var allocation from stack
> > > perf/thunder_x2: Avoid explicit cpumask var allocation from stack
> > >
> > > drivers/perf/alibaba_uncore_drw_pmu.c | 13 +++++++++----
> > > drivers/perf/arm-cmn.c | 13 +++++++++----
> > > drivers/perf/arm_cspmu/arm_cspmu.c | 13 +++++++++----
> > > drivers/perf/arm_dsu_pmu.c | 18 +++++++++++++-----
> > > drivers/perf/dwc_pcie_pmu.c | 17 +++++++++++------
> > > drivers/perf/hisilicon/hisi_pcie_pmu.c | 15 ++++++++++-----
> > > drivers/perf/hisilicon/hisi_uncore_pmu.c | 13 +++++++++----
> > > drivers/perf/qcom_l2_pmu.c | 15 ++++++++++-----
> > > drivers/perf/thunderx2_pmu.c | 20 ++++++++++++--------
> > > 9 files changed, 92 insertions(+), 45 deletions(-)
> > >
> > >
> > > Thanks,
> > >
> > > Dawei
> > >
> > > --
> > > 2.27.0
> > >
> >
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/9] perf/arm_dsu: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 ` [PATCH 4/9] perf/arm_dsu: " Dawei Li
@ 2024-04-02 23:58 ` kernel test robot
2024-04-03 1:43 ` kernel test robot
1 sibling, 0 replies; 19+ messages in thread
From: kernel test robot @ 2024-04-02 23:58 UTC (permalink / raw
To: Dawei Li, will, mark.rutland
Cc: oe-kbuild-all, xueshuai, renyu.zj, yangyicong, jonathan.cameron,
andersson, konrad.dybcio, linux-arm-kernel, linux-kernel,
linux-arm-msm, Dawei Li
Hi Dawei,
kernel test robot noticed the following build errors:
[auto build test ERROR on soc/for-next]
[also build test ERROR on linus/master v6.9-rc2 next-20240402]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dawei-Li/perf-alibaba_uncore_drw-Avoid-explicit-cpumask-var-allocation-from-stack/20240402-192244
base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link: https://lore.kernel.org/r/20240402105610.1695644-5-dawei.li%40shingroup.cn
patch subject: [PATCH 4/9] perf/arm_dsu: Avoid explicit cpumask var allocation from stack
config: arm64-defconfig (https://download.01.org/0day-ci/archive/20240403/202404030702.vL5Ep98R-lkp@intel.com/config)
compiler: aarch64-linux-gcc (GCC) 13.2.0
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240403/202404030702.vL5Ep98R-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404030702.vL5Ep98R-lkp@intel.com/
All errors (new ones prefixed by >>):
drivers/perf/arm_dsu_pmu.c: In function 'dsu_pmu_get_online_cpu_any_but':
>> drivers/perf/arm_dsu_pmu.c:243:31: error: passing argument 1 of 'cpumask_any_but' from incompatible pointer type [-Werror=incompatible-pointer-types]
243 | ret = cpumask_any_but(&online_supported, cpu);
| ^~~~~~~~~~~~~~~~~
| |
| struct cpumask (*)[1]
In file included from arch/arm64/include/asm/cpufeature.h:26,
from arch/arm64/include/asm/ptrace.h:11,
from arch/arm64/include/asm/irqflags.h:10,
from include/linux/irqflags.h:18,
from include/linux/spinlock.h:59,
from include/linux/mmzone.h:8,
from include/linux/gfp.h:7,
from include/linux/slab.h:16,
from include/linux/resource_ext.h:11,
from include/linux/acpi.h:13,
from drivers/perf/arm_dsu_pmu.c:14:
include/linux/cpumask.h:379:52: note: expected 'const struct cpumask *' but argument is of type 'struct cpumask (*)[1]'
379 | unsigned int cpumask_any_but(const struct cpumask *mask, unsigned int cpu)
| ~~~~~~~~~~~~~~~~~~~~~~^~~~
cc1: some warnings being treated as errors
vim +/cpumask_any_but +243 drivers/perf/arm_dsu_pmu.c
232
233 static unsigned int dsu_pmu_get_online_cpu_any_but(struct dsu_pmu *dsu_pmu, int cpu)
234 {
235 cpumask_var_t online_supported;
236 unsigned int ret;
237
238 if (!alloc_cpumask_var(&online_supported, GFP_KERNEL))
239 return -ENOMEM;
240
241 cpumask_and(online_supported,
242 &dsu_pmu->associated_cpus, cpu_online_mask);
> 243 ret = cpumask_any_but(&online_supported, cpu);
244
245 free_cpumask_var(online_supported);
246
247 return ret;
248 }
249
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 4/9] perf/arm_dsu: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 ` [PATCH 4/9] perf/arm_dsu: " Dawei Li
2024-04-02 23:58 ` kernel test robot
@ 2024-04-03 1:43 ` kernel test robot
1 sibling, 0 replies; 19+ messages in thread
From: kernel test robot @ 2024-04-03 1:43 UTC (permalink / raw
To: Dawei Li, will, mark.rutland
Cc: llvm, oe-kbuild-all, xueshuai, renyu.zj, yangyicong,
jonathan.cameron, andersson, konrad.dybcio, linux-arm-kernel,
linux-kernel, linux-arm-msm, Dawei Li
Hi Dawei,
kernel test robot noticed the following build errors:
[auto build test ERROR on soc/for-next]
[also build test ERROR on linus/master v6.9-rc2 next-20240402]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]
url: https://github.com/intel-lab-lkp/linux/commits/Dawei-Li/perf-alibaba_uncore_drw-Avoid-explicit-cpumask-var-allocation-from-stack/20240402-192244
base: https://git.kernel.org/pub/scm/linux/kernel/git/soc/soc.git for-next
patch link: https://lore.kernel.org/r/20240402105610.1695644-5-dawei.li%40shingroup.cn
patch subject: [PATCH 4/9] perf/arm_dsu: Avoid explicit cpumask var allocation from stack
config: arm64-randconfig-001-20240403 (https://download.01.org/0day-ci/archive/20240403/202404030906.L4BhRzHw-lkp@intel.com/config)
compiler: clang version 16.0.4 (https://github.com/llvm/llvm-project.git ae42196bc493ffe877a7e3dff8be32035dea4d07)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20240403/202404030906.L4BhRzHw-lkp@intel.com/reproduce)
If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202404030906.L4BhRzHw-lkp@intel.com/
All errors (new ones prefixed by >>):
>> drivers/perf/arm_dsu_pmu.c:243:24: error: incompatible pointer types passing 'cpumask_var_t *' (aka 'struct cpumask **') to parameter of type 'const struct cpumask *'; remove & [-Werror,-Wincompatible-pointer-types]
ret = cpumask_any_but(&online_supported, cpu);
^~~~~~~~~~~~~~~~~
include/linux/cpumask.h:379:52: note: passing argument to parameter 'mask' here
unsigned int cpumask_any_but(const struct cpumask *mask, unsigned int cpu)
^
1 error generated.
vim +243 drivers/perf/arm_dsu_pmu.c
232
233 static unsigned int dsu_pmu_get_online_cpu_any_but(struct dsu_pmu *dsu_pmu, int cpu)
234 {
235 cpumask_var_t online_supported;
236 unsigned int ret;
237
238 if (!alloc_cpumask_var(&online_supported, GFP_KERNEL))
239 return -ENOMEM;
240
241 cpumask_and(online_supported,
242 &dsu_pmu->associated_cpus, cpu_online_mask);
> 243 ret = cpumask_any_but(&online_supported, cpu);
244
245 free_cpumask_var(online_supported);
246
247 return ret;
248 }
249
--
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack
2024-04-02 14:41 ` Mark Rutland
@ 2024-04-03 10:41 ` Dawei Li
2024-04-03 11:10 ` Mark Rutland
0 siblings, 1 reply; 19+ messages in thread
From: Dawei Li @ 2024-04-03 10:41 UTC (permalink / raw
To: Mark Rutland
Cc: will, xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm
On Tue, Apr 02, 2024 at 03:41:51PM +0100, Mark Rutland wrote:
> On Tue, Apr 02, 2024 at 09:40:08PM +0800, Dawei Li wrote:
> > Hi Mark,
> >
> > Thanks for the quick review.
> >
> > On Tue, Apr 02, 2024 at 12:12:50PM +0100, Mark Rutland wrote:
> > > On Tue, Apr 02, 2024 at 06:56:01PM +0800, Dawei Li wrote:
> > > > Hi,
> > > >
> > > > This series try to eliminate direct cpumask var allocation from stack
> > > > for perf subsystem.
> > > >
> > > > Direct/explicit allocation of cpumask on stack could be dangerous since
> > > > it can lead to stack overflow for systems with big NR_CPUS or
> > > > CONFIG_CPUMASK_OFFSTACK=y.
> > > >
> > > > For arm64, it's more urgent since commit 3fbd56f0e7c1 ("ARM64: Dynamically
> > > > allocate cpumasks and increase supported CPUs to 512").
> > > >
> > > > It's sort of a pattern that almost every cpumask var in perf subystem
> > > > occurs in teardown callback of cpuhp. In which case, if dynamic
> > > > allocation failed(which is unlikely), we choose return 0 rather than
> > > > -ENOMEM to caller cuz:
> > > > @teardown is not supposed to fail and if it does, system crashes:
> > >
> > > .. but we've left the system in an incorrect state, so that makes no sense.
> > >
> > > As I commented on the first patch, NAK to dynamically allocating cpumasks in
> > > the CPUHP callbacks. Please allocate the necessry cpumasks up-front when we
> > > probe the PMU. At that time we can handle an allocation failure by cleaning up
> > > and failing to probe the PMU, and then the CPUHP callbacks don't need to
> > > allocate memory to offline a CPU...
> >
> > Agreed that dynamically allocation in callbacks lead to inconsistency
> > to system.
> >
> > My (original)alternative plan is simple but ugly, just make cpumask var
> > _static_ and add extra static lock to protect it.
> >
> > The only difference between solution above and your proposal is static/
> > dynamic alloction. CPUHP's teardown cb is supposed to run in targetted
> > cpuhp thread for most cases, and it's racy. Even the cpumask var is
> > wrapped in dynamically allocated struct xxx_pmu, it's still shareable
> > between different threads/contexts and needs proper protection.
>
> I was under the impression that the cpuhp callbacks were *strictly* serialised.
> If that's not the case, the existing offlining callbacks are horrendously
> broken.
>
> Are you *certain* these can race?
>
> Regardless, adding additional locking here is not ok.
>
> > Simple as this(_untested_):
> >
> > diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> > index 7ef9c7e4836b..fa89c3db4d7d 100644
> > --- a/drivers/perf/arm-cmn.c
> > +++ b/drivers/perf/arm-cmn.c
> > @@ -1950,18 +1950,24 @@ static int arm_cmn_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_no
> > struct arm_cmn *cmn;
> > unsigned int target;
> > int node;
> > - cpumask_t mask;
> > + static cpumask_t mask;
> > + static DEFINE_SPINLOCK(cpumask_lock);
> >
> > cmn = hlist_entry_safe(cpuhp_node, struct arm_cmn, cpuhp_node);
> > if (cpu != cmn->cpu)
> > return 0;
> >
> > + spin_lock(&cpumask_lock);
> > +
> > node = dev_to_node(cmn->dev);
> > if (cpumask_and(&mask, cpumask_of_node(node), cpu_online_mask) &&
> > cpumask_andnot(&mask, &mask, cpumask_of(cpu)))
> > target = cpumask_any(&mask);
> > else
> > target = cpumask_any_but(cpu_online_mask, cpu);
> > +
> > + spin_unlock(&cpumask_lock);
> > +
> > if (target < nr_cpu_ids)
> > arm_cmn_migrate(cmn, target);
> > return 0;
>
> Looking at this case, the only reason we need the mask is because it made the
> logic a little easier to write. All we really want is to choose some CPU in the
> intersection of two masks ignoring a specific CPU, and there was no helper
> function to do that.
>
> We can add a new helper to do that for us, which would avoid redundant work to
> manipulate the entire mask, and it would make the existing code simpler. I had
> a series a few years back to add cpumask_any_and_but():
>
> https://lore.kernel.org/lkml/1486381132-5610-1-git-send-email-mark.rutland@arm.com/
Sounds a perfect idea!
Actually I am re-implementing new series on top of your seven-years-late-yet-still-helpful
patch, with minor update on it:
diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index 1c29947db848..121f3ac757ff 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -388,6 +388,29 @@ unsigned int cpumask_any_but(const struct cpumask *mask, unsigned int cpu)
return i;
}
+/**
+ * cpumask_any_and_but - pick a "random" cpu from *mask1 & *mask2, but not this one.
+ * @mask1: the first input cpumask
+ * @mask2: the second input cpumask
+ * @cpu: the cpu to ignore
+ *
+ * Returns >= nr_cpu_ids if no cpus set.
+ */
+static inline
+unsigned int cpumask_any_and_but(const struct cpumask *mask1,
+ const struct cpumask *mask2,
+ unsigned int cpu)
+{
+ unsigned int i;
+
+ cpumask_check(cpu);
+ i = cpumask_first_and(mask1, mask2);
+ if (i != cpu)
+ return i;
+
+ return cpumask_next_and(cpu, mask1, mask2);
+}
+
/**
* cpumask_nth - get the Nth cpu in a cpumask
* @srcp: the cpumask pointer
Change from your original version:
1 Moved to cpumask.h, just like other helpers.
2 Return value converted to unsigned int.
3 Remove EXPORT_SYMBOL, for obvious reason.
I will respin V2 as a whole as soon as possible.
Thanks,
Dawei
>
> .. and that's easy to use here, e.g.
>
> | diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> | index 7ef9c7e4836b7..c6bbd387ccf8b 100644
> | --- a/drivers/perf/arm-cmn.c
> | +++ b/drivers/perf/arm-cmn.c
> | @@ -1950,17 +1950,15 @@ static int arm_cmn_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_no
> | struct arm_cmn *cmn;
> | unsigned int target;
> | int node;
> | - cpumask_t mask;
> |
> | cmn = hlist_entry_safe(cpuhp_node, struct arm_cmn, cpuhp_node);
> | if (cpu != cmn->cpu)
> | return 0;
> |
> | node = dev_to_node(cmn->dev);
> | - if (cpumask_and(&mask, cpumask_of_node(node), cpu_online_mask) &&
> | - cpumask_andnot(&mask, &mask, cpumask_of(cpu)))
> | - target = cpumask_any(&mask);
> | - else
> | + target = cpumask_any_and_but(cpu_online_mask, cpumask_of_node(node),
> | + cpu);
> | + if (target >= nr_cpu_ids)
> | target = cpumask_any_but(cpu_online_mask, cpu);
> | if (target < nr_cpu_ids)
> | arm_cmn_migrate(cmn, target);
>
> It doesn't trivially rebase since the cpumask code has changed a fair amount,
> but I've managed to do that locally, and I can send that out as a
> seven-years-late v2 if it's useful.
>
> From a quick scan, it looks like that'd handle all cases in this series. Are
> there any patterns in this series for which that would not be sufficient?
>
> Mark.
>
> >
> > And yes, static allocation is evil :)
> >
> >
> > Thanks,
> >
> > Dawei
> >
> > >
> > > Also, for the titles it'd be better to say something like "avoid placing
> > > cpumasks on the stack", because "explicit cpumask var allocation" sounds like
> > > the use of alloc_cpumask_var().
> >
> > Sound great! I will update it.
> >
> > >
> > > Mark.
> > >
> > > >
> > > > static int cpuhp_issue_call(int cpu, enum cpuhp_state state, bool bringup,
> > > > struct hlist_node *node)
> > > > {
> > > > struct cpuhp_step *sp = cpuhp_get_step(state);
> > > > int ret;
> > > >
> > > > /*
> > > > * If there's nothing to do, we done.
> > > > * Relies on the union for multi_instance.
> > > > */
> > > > if (cpuhp_step_empty(bringup, sp))
> > > > return 0;
> > > > /*
> > > > * The non AP bound callbacks can fail on bringup. On teardown
> > > > * e.g. module removal we crash for now.
> > > > */
> > > > #ifdef CONFIG_SMP
> > > > if (cpuhp_is_ap_state(state))
> > > > ret = cpuhp_invoke_ap_callback(cpu, state, bringup, node);
> > > > else
> > > > ret = cpuhp_invoke_callback(cpu, state, bringup, node,
> > > > NULL);
> > > > #else
> > > > ret = cpuhp_invoke_callback(cpu, state, bringup, node, NULL);
> > > > #endif
> > > > BUG_ON(ret && !bringup);
> > > > return ret;
> > > > }
> > > >
> > > > Dawei Li (9):
> > > > perf/alibaba_uncore_drw: Avoid explicit cpumask var allocation from
> > > > stack
> > > > perf/arm-cmn: Avoid explicit cpumask var allocation from stack
> > > > perf/arm_cspmu: Avoid explicit cpumask var allocation from stack
> > > > perf/arm_dsu: Avoid explicit cpumask var allocation from stack
> > > > perf/dwc_pcie: Avoid explicit cpumask var allocation from stack
> > > > perf/hisi_pcie: Avoid explicit cpumask var allocation from stack
> > > > perf/hisi_uncore: Avoid explicit cpumask var allocation from stack
> > > > perf/qcom_l2: Avoid explicit cpumask var allocation from stack
> > > > perf/thunder_x2: Avoid explicit cpumask var allocation from stack
> > > >
> > > > drivers/perf/alibaba_uncore_drw_pmu.c | 13 +++++++++----
> > > > drivers/perf/arm-cmn.c | 13 +++++++++----
> > > > drivers/perf/arm_cspmu/arm_cspmu.c | 13 +++++++++----
> > > > drivers/perf/arm_dsu_pmu.c | 18 +++++++++++++-----
> > > > drivers/perf/dwc_pcie_pmu.c | 17 +++++++++++------
> > > > drivers/perf/hisilicon/hisi_pcie_pmu.c | 15 ++++++++++-----
> > > > drivers/perf/hisilicon/hisi_uncore_pmu.c | 13 +++++++++----
> > > > drivers/perf/qcom_l2_pmu.c | 15 ++++++++++-----
> > > > drivers/perf/thunderx2_pmu.c | 20 ++++++++++++--------
> > > > 9 files changed, 92 insertions(+), 45 deletions(-)
> > > >
> > > >
> > > > Thanks,
> > > >
> > > > Dawei
> > > >
> > > > --
> > > > 2.27.0
> > > >
> > >
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack
2024-04-03 10:41 ` Dawei Li
@ 2024-04-03 11:10 ` Mark Rutland
0 siblings, 0 replies; 19+ messages in thread
From: Mark Rutland @ 2024-04-03 11:10 UTC (permalink / raw
To: Dawei Li
Cc: will, xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm
On Wed, Apr 03, 2024 at 06:41:23PM +0800, Dawei Li wrote:
> On Tue, Apr 02, 2024 at 03:41:51PM +0100, Mark Rutland wrote:
> > Looking at this case, the only reason we need the mask is because it made the
> > logic a little easier to write. All we really want is to choose some CPU in the
> > intersection of two masks ignoring a specific CPU, and there was no helper
> > function to do that.
> >
> > We can add a new helper to do that for us, which would avoid redundant work to
> > manipulate the entire mask, and it would make the existing code simpler. I had
> > a series a few years back to add cpumask_any_and_but():
> >
> > https://lore.kernel.org/lkml/1486381132-5610-1-git-send-email-mark.rutland@arm.com/
>
> Sounds a perfect idea!
>
> Actually I am re-implementing new series on top of your seven-years-late-yet-still-helpful
> patch, with minor update on it:
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index 1c29947db848..121f3ac757ff 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -388,6 +388,29 @@ unsigned int cpumask_any_but(const struct cpumask *mask, unsigned int cpu)
> return i;
> }
>
> +/**
> + * cpumask_any_and_but - pick a "random" cpu from *mask1 & *mask2, but not this one.
> + * @mask1: the first input cpumask
> + * @mask2: the second input cpumask
> + * @cpu: the cpu to ignore
> + *
> + * Returns >= nr_cpu_ids if no cpus set.
> + */
> +static inline
> +unsigned int cpumask_any_and_but(const struct cpumask *mask1,
> + const struct cpumask *mask2,
> + unsigned int cpu)
> +{
> + unsigned int i;
> +
> + cpumask_check(cpu);
> + i = cpumask_first_and(mask1, mask2);
> + if (i != cpu)
> + return i;
> +
> + return cpumask_next_and(cpu, mask1, mask2);
> +}
> +
> /**
> * cpumask_nth - get the Nth cpu in a cpumask
> * @srcp: the cpumask pointer
>
> Change from your original version:
> 1 Moved to cpumask.h, just like other helpers.
> 2 Return value converted to unsigned int.
> 3 Remove EXPORT_SYMBOL, for obvious reason.
That's exactly how I rebased it locally, so that looks good to me!
> I will respin V2 as a whole as soon as possible.
Great!
Thanks,
Mark.
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH 2/9] perf/arm-cmn: Avoid explicit cpumask var allocation from stack
2024-04-02 10:56 ` [PATCH 2/9] perf/arm-cmn: " Dawei Li
@ 2024-04-05 14:30 ` Robin Murphy
0 siblings, 0 replies; 19+ messages in thread
From: Robin Murphy @ 2024-04-05 14:30 UTC (permalink / raw
To: Dawei Li, will, mark.rutland
Cc: xueshuai, renyu.zj, yangyicong, jonathan.cameron, andersson,
konrad.dybcio, linux-arm-kernel, linux-kernel, linux-arm-msm
On 2024-04-02 11:56 am, Dawei Li wrote:
> For CONFIG_CPUMASK_OFFSTACK=y kernel, explicit allocation of cpumask
> variable on stack is not recommended since it can cause potential stack
> overflow.
>
> Instead, kernel code should always use *cpumask_var API(s) to allocate
> cpumask var in config- neutral way, leaving allocation strategy to
> CONFIG_CPUMASK_OFFSTACK.
>
> Use *cpumask_var API(s) to address it.
I think the temporary mask may simply be redundant anyway. It seems like
I may have misunderstood, and cpumask_of_node() actually only covers
online CPUs already.
Thanks,
Robin.
> Signed-off-by: Dawei Li <dawei.li@shingroup.cn>
> ---
> drivers/perf/arm-cmn.c | 13 +++++++++----
> 1 file changed, 9 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> index 7ef9c7e4836b..7278fd72d3da 100644
> --- a/drivers/perf/arm-cmn.c
> +++ b/drivers/perf/arm-cmn.c
> @@ -1949,21 +1949,26 @@ static int arm_cmn_pmu_offline_cpu(unsigned int cpu, struct hlist_node *cpuhp_no
> {
> struct arm_cmn *cmn;
> unsigned int target;
> + cpumask_var_t mask;
> int node;
> - cpumask_t mask;
>
> cmn = hlist_entry_safe(cpuhp_node, struct arm_cmn, cpuhp_node);
> if (cpu != cmn->cpu)
> return 0;
>
> + if (!alloc_cpumask_var(&mask, GFP_KERNEL))
> + return 0;
> +
> node = dev_to_node(cmn->dev);
> - if (cpumask_and(&mask, cpumask_of_node(node), cpu_online_mask) &&
> - cpumask_andnot(&mask, &mask, cpumask_of(cpu)))
> - target = cpumask_any(&mask);
> + if (cpumask_and(mask, cpumask_of_node(node), cpu_online_mask) &&
> + cpumask_andnot(mask, mask, cpumask_of(cpu)))
> + target = cpumask_any(mask);
> else
> target = cpumask_any_but(cpu_online_mask, cpu);
> if (target < nr_cpu_ids)
> arm_cmn_migrate(cmn, target);
> +
> + free_cpumask_var(mask);
> return 0;
> }
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2024-04-05 14:31 UTC | newest]
Thread overview: 19+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-04-02 10:56 [PATCH 0/9] perf: Avoid explicit cpumask var allocation from stack Dawei Li
2024-04-02 10:56 ` [PATCH 1/9] perf/alibaba_uncore_drw: " Dawei Li
2024-04-02 11:06 ` Mark Rutland
2024-04-02 10:56 ` [PATCH 2/9] perf/arm-cmn: " Dawei Li
2024-04-05 14:30 ` Robin Murphy
2024-04-02 10:56 ` [PATCH 3/9] perf/arm_cspmu: " Dawei Li
2024-04-02 10:56 ` [PATCH 4/9] perf/arm_dsu: " Dawei Li
2024-04-02 23:58 ` kernel test robot
2024-04-03 1:43 ` kernel test robot
2024-04-02 10:56 ` [PATCH 5/9] perf/dwc_pcie: " Dawei Li
2024-04-02 10:56 ` [PATCH 6/9] perf/hisi_pcie: " Dawei Li
2024-04-02 10:56 ` [PATCH 7/9] perf/hisi_uncore: " Dawei Li
2024-04-02 10:56 ` [PATCH 8/9] perf/qcom_l2: " Dawei Li
2024-04-02 10:56 ` [PATCH 9/9] perf/thunder_x2: " Dawei Li
2024-04-02 11:12 ` [PATCH 0/9] perf: " Mark Rutland
2024-04-02 13:40 ` Dawei Li
2024-04-02 14:41 ` Mark Rutland
2024-04-03 10:41 ` Dawei Li
2024-04-03 11:10 ` Mark Rutland
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).