2024-04-20 05:17:00

by Dawei Li

Subject: [PATCH v2 1/7] sparc/srmmu: Remove on-stack cpumask var

In general it's preferable to avoid placing cpumasks on the stack, as
for large values of NR_CPUS these can consume significant amounts of
stack space and make stack overflows more likely.

Use cpumask_any_but() to avoid the need for a temporary cpumask on
the stack.

Signed-off-by: Dawei Li <[email protected]>
---
arch/sparc/mm/srmmu.c | 40 ++++++++++++----------------------------
1 file changed, 12 insertions(+), 28 deletions(-)

diff --git a/arch/sparc/mm/srmmu.c b/arch/sparc/mm/srmmu.c
index 852085ada368..86fd20c878ae 100644
--- a/arch/sparc/mm/srmmu.c
+++ b/arch/sparc/mm/srmmu.c
@@ -1653,13 +1653,15 @@ static void smp_flush_tlb_all(void)
local_ops->tlb_all();
}

+static bool cpumask_any_but_current(struct mm_struct *mm)
+{
+ return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids;
+}
+
static void smp_flush_cache_mm(struct mm_struct *mm)
{
if (mm->context != NO_CONTEXT) {
- cpumask_t cpu_mask;
- cpumask_copy(&cpu_mask, mm_cpumask(mm));
- cpumask_clear_cpu(smp_processor_id(), &cpu_mask);
- if (!cpumask_empty(&cpu_mask))
+ if (cpumask_any_but_current(mm))
xc1(local_ops->cache_mm, (unsigned long)mm);
local_ops->cache_mm(mm);
}
@@ -1668,10 +1670,7 @@ static void smp_flush_cache_mm(struct mm_struct *mm)
static void smp_flush_tlb_mm(struct mm_struct *mm)
{
if (mm->context != NO_CONTEXT) {
- cpumask_t cpu_mask;
- cpumask_copy(&cpu_mask, mm_cpumask(mm));
- cpumask_clear_cpu(smp_processor_id(), &cpu_mask);
- if (!cpumask_empty(&cpu_mask)) {
+ if (cpumask_any_but_current(mm)) {
xc1(local_ops->tlb_mm, (unsigned long)mm);
if (atomic_read(&mm->mm_users) == 1 && current->active_mm == mm)
cpumask_copy(mm_cpumask(mm),
@@ -1688,10 +1687,7 @@ static void smp_flush_cache_range(struct vm_area_struct *vma,
struct mm_struct *mm = vma->vm_mm;

if (mm->context != NO_CONTEXT) {
- cpumask_t cpu_mask;
- cpumask_copy(&cpu_mask, mm_cpumask(mm));
- cpumask_clear_cpu(smp_processor_id(), &cpu_mask);
- if (!cpumask_empty(&cpu_mask))
+ if (cpumask_any_but_current(mm))
xc3(local_ops->cache_range, (unsigned long)vma, start,
end);
local_ops->cache_range(vma, start, end);
@@ -1705,10 +1701,7 @@ static void smp_flush_tlb_range(struct vm_area_struct *vma,
struct mm_struct *mm = vma->vm_mm;

if (mm->context != NO_CONTEXT) {
- cpumask_t cpu_mask;
- cpumask_copy(&cpu_mask, mm_cpumask(mm));
- cpumask_clear_cpu(smp_processor_id(), &cpu_mask);
- if (!cpumask_empty(&cpu_mask))
+ if (cpumask_any_but_current(mm))
xc3(local_ops->tlb_range, (unsigned long)vma, start,
end);
local_ops->tlb_range(vma, start, end);
@@ -1720,10 +1713,7 @@ static void smp_flush_cache_page(struct vm_area_struct *vma, unsigned long page)
struct mm_struct *mm = vma->vm_mm;

if (mm->context != NO_CONTEXT) {
- cpumask_t cpu_mask;
- cpumask_copy(&cpu_mask, mm_cpumask(mm));
- cpumask_clear_cpu(smp_processor_id(), &cpu_mask);
- if (!cpumask_empty(&cpu_mask))
+ if (cpumask_any_but_current(mm))
xc2(local_ops->cache_page, (unsigned long)vma, page);
local_ops->cache_page(vma, page);
}
@@ -1734,10 +1724,7 @@ static void smp_flush_tlb_page(struct vm_area_struct *vma, unsigned long page)
struct mm_struct *mm = vma->vm_mm;

if (mm->context != NO_CONTEXT) {
- cpumask_t cpu_mask;
- cpumask_copy(&cpu_mask, mm_cpumask(mm));
- cpumask_clear_cpu(smp_processor_id(), &cpu_mask);
- if (!cpumask_empty(&cpu_mask))
+ if (cpumask_any_but_current(mm))
xc2(local_ops->tlb_page, (unsigned long)vma, page);
local_ops->tlb_page(vma, page);
}
@@ -1759,10 +1746,7 @@ static void smp_flush_page_to_ram(unsigned long page)

static void smp_flush_sig_insns(struct mm_struct *mm, unsigned long insn_addr)
{
- cpumask_t cpu_mask;
- cpumask_copy(&cpu_mask, mm_cpumask(mm));
- cpumask_clear_cpu(smp_processor_id(), &cpu_mask);
- if (!cpumask_empty(&cpu_mask))
+ if (cpumask_any_but_current(mm))
xc2(local_ops->sig_insns, (unsigned long)mm, insn_addr);
local_ops->sig_insns(mm, insn_addr);
}
--
2.27.0

2024-04-20 08:00:18

by Sam Ravnborg

Subject: Re: [PATCH v2 1/7] sparc/srmmu: Remove on-stack cpumask var

Hi Dawei,
On Sat, Apr 20, 2024 at 01:15:41PM +0800, Dawei Li wrote:
> In general it's preferable to avoid placing cpumasks on the stack, as
> for large values of NR_CPUS these can consume significant amounts of
> stack space and make stack overflows more likely.
>
> Use cpumask_any_but() to avoid the need for a temporary cpumask on
> the stack.

Another good argument for this patch is the simplification of the code.

>
> Signed-off-by: Dawei Li <[email protected]>
> ---
> arch/sparc/mm/srmmu.c | 40 ++++++++++++----------------------------
> 1 file changed, 12 insertions(+), 28 deletions(-)
>
> diff --git a/arch/sparc/mm/srmmu.c b/arch/sparc/mm/srmmu.c
> index 852085ada368..86fd20c878ae 100644
> --- a/arch/sparc/mm/srmmu.c
> +++ b/arch/sparc/mm/srmmu.c
> @@ -1653,13 +1653,15 @@ static void smp_flush_tlb_all(void)
> local_ops->tlb_all();
> }
>
> +static bool cpumask_any_but_current(struct mm_struct *mm)
> +{
> + return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids;
> +}

This helper is not a cpumask helper - the name should reflect what it is
used for.

Something like:
static bool any_other_mm_cpus(struct mm_struct *mm)
{
return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids;
}

The implementation is fine - it is only the naming that should be
improve.
With this change (or a better name):
Reviewed-by: Sam Ravnborg <[email protected]>

Sam

2024-04-22 05:48:10

by Dawei Li

Subject: Re: [PATCH v2 1/7] sparc/srmmu: Remove on-stack cpumask var

Hi Sam,

Thanks for review.

On Sat, Apr 20, 2024 at 09:58:46AM +0200, Sam Ravnborg wrote:
> Hi Dawei,
> On Sat, Apr 20, 2024 at 01:15:41PM +0800, Dawei Li wrote:
> > In general it's preferable to avoid placing cpumasks on the stack, as
> > for large values of NR_CPUS these can consume significant amounts of
> > stack space and make stack overflows more likely.
> >
> > Use cpumask_any_but() to avoid the need for a temporary cpumask on
> > the stack.
>
> Another good argument for this patch is the simplification of the code.
>
> >
> > Signed-off-by: Dawei Li <[email protected]>
> > ---
> > arch/sparc/mm/srmmu.c | 40 ++++++++++++----------------------------
> > 1 file changed, 12 insertions(+), 28 deletions(-)
> >
> > diff --git a/arch/sparc/mm/srmmu.c b/arch/sparc/mm/srmmu.c
> > index 852085ada368..86fd20c878ae 100644
> > --- a/arch/sparc/mm/srmmu.c
> > +++ b/arch/sparc/mm/srmmu.c
> > @@ -1653,13 +1653,15 @@ static void smp_flush_tlb_all(void)
> > local_ops->tlb_all();
> > }
> >
> > +static bool cpumask_any_but_current(struct mm_struct *mm)
> > +{
> > + return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids;
> > +}
>
> This helper is not a cpumask helper - the name should reflect what it is
> used for.
>
> Something like:
> static bool any_other_mm_cpus(struct mm_struct *mm)
> {
> return cpumask_any_but(mm_cpumask(mm), smp_processor_id()) < nr_cpu_ids;
> }

Acked. I will rename the helper as you suggested.

>
> The implementation is fine - it is only the naming that should be
> improve.
> With this change (or a better name):
> Reviewed-by: Sam Ravnborg <[email protected]>
>
> Sam

Thanks,

Dawei
>