Re: [PATCH v4 08/10] powerpc/smp: Allocate cpumask only after searching thread group
Srikar Dronamraju writes: > * Michael Ellerman [2020-07-31 17:52:15]: > >> Srikar Dronamraju writes: >> > If allocated earlier and the search fails, then cpumask need to be >> > freed. However cpu_l1_cache_map can be allocated after we search thread >> > group. >> >> It's not freed anywhere AFAICS? > > Yes, its never freed. Infact we are never checking if > zalloc_cpumask_var_node fails. Its not just this cpumask, but historically > all the other existing cpumasks in arch/powerpc/kernel/smp.c are never > freed/checked. I did dig into this a bit and it appears that .. > (Please do correct me if I am wrong!! ) That's correct. > Powerpc using cpumask_var_t for all of the percpu variables. And it dont seem > to enable CONFIG_CPUMASK_OFFSTACK even from the MAXSMP config. I remember Rusty adding that code, but I don't know if we ever considered enabling CPUMASK_OFFSTACK. Probably we meant to but never got around to doing it. > So from include/linux/cpumask.h > > typedef struct cpumask cpumask_var_t[1]; > and > zalloc_cpumask_var_node ends up being cpumask_clear > > So I think we are historically we seem to assume we are always > !CPUMASK_OFFSTACK and hence we dont need to check for return as well as > free.. Right. > I would look forward to your comments on how we should handle this going > forward. But I would keep this the same for this patchset. Agreed, just clarify in the change log that it's not freed at the moment because of CPU_MASK_OFFSTACK=n > One of the questions that I have is if we most likely are to be in > !CONFIG_CPUMASK_OFFSTACK, then should be migrate to cpumask_t for percpu > variables. I don't think so, cpumask_t is semi-deprecated AIUI. > The reason being we end up using NR_CPU cpumask for each percpu cpumask > variable instead of using NR_CPU cpumask_t pointer. Our current defconfigs have NR_CPUS=2048, which is probably just small enough to continue using OFFSTACK=n. But we allow configuring NR_CPUS up to 8192, which surely would need OFFSTACK=y in order to work. So I think we need to stick with cpumask_var_t, but we should test with OFFSTACK=y, and should probably be a bit more careful with checking the allocations succeed. And then we should select OFFSTACK=y for NR_CPUS above some threshold. cheers
Re: [PATCH v4 08/10] powerpc/smp: Allocate cpumask only after searching thread group
* Michael Ellerman [2020-07-31 17:52:15]:
> Srikar Dronamraju writes:
> > If allocated earlier and the search fails, then cpumask need to be
> > freed. However cpu_l1_cache_map can be allocated after we search thread
> > group.
>
> It's not freed anywhere AFAICS?
>
Yes, its never freed. Infact we are never checking if
zalloc_cpumask_var_node fails. Its not just this cpumask, but historically
all the other existing cpumasks in arch/powerpc/kernel/smp.c are never
freed/checked. I did dig into this a bit and it appears that ..
(Please do correct me if I am wrong!! )
Powerpc using cpumask_var_t for all of the percpu variables. And it dont seem
to enable CONFIG_CPUMASK_OFFSTACK even from the MAXSMP config.
So from include/linux/cpumask.h
typedef struct cpumask cpumask_var_t[1];
and
zalloc_cpumask_var_node ends up being cpumask_clear
So I think we are historically we seem to assume we are always
!CPUMASK_OFFSTACK and hence we dont need to check for return as well as
free..
I would look forward to your comments on how we should handle this going
forward. But I would keep this the same for this patchset.
One of the questions that I have is if we most likely are to be in
!CONFIG_CPUMASK_OFFSTACK, then should be migrate to cpumask_t for percpu
variables.
The reason being we end up using NR_CPU cpumask for each percpu cpumask
variable instead of using NR_CPU cpumask_t pointer.
> And even after this change there's still an error path that doesn't free
> it, isn't there?
>
> cheers
>
> > Cc: linuxppc-dev
> > Cc: LKML
> > Cc: Michael Ellerman
> > Cc: Nicholas Piggin
> > Cc: Anton Blanchard
> > Cc: Oliver O'Halloran
> > Cc: Nathan Lynch
> > Cc: Michael Neuling
> > Cc: Gautham R Shenoy
> > Cc: Ingo Molnar
> > Cc: Peter Zijlstra
> > Cc: Valentin Schneider
> > Cc: Jordan Niethe
> > Reviewed-by: Gautham R. Shenoy
> > Signed-off-by: Srikar Dronamraju
> > ---
> > arch/powerpc/kernel/smp.c | 7 +++
> > 1 file changed, 3 insertions(+), 4 deletions(-)
> >
> > diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> > index 698000c7f76f..dab96a1203ec 100644
> > --- a/arch/powerpc/kernel/smp.c
> > +++ b/arch/powerpc/kernel/smp.c
> > @@ -797,10 +797,6 @@ static int init_cpu_l1_cache_map(int cpu)
> > if (err)
> > goto out;
> >
> > - zalloc_cpumask_var_node(_cpu(cpu_l1_cache_map, cpu),
> > - GFP_KERNEL,
> > - cpu_to_node(cpu));
> > -
> > cpu_group_start = get_cpu_thread_group_start(cpu, );
> >
> > if (unlikely(cpu_group_start == -1)) {
> > @@ -809,6 +805,9 @@ static int init_cpu_l1_cache_map(int cpu)
> > goto out;
> > }
> >
> > + zalloc_cpumask_var_node(_cpu(cpu_l1_cache_map, cpu),
> > + GFP_KERNEL, cpu_to_node(cpu));
> > +
> > for (i = first_thread; i < first_thread + threads_per_core; i++) {
> > int i_group_start = get_cpu_thread_group_start(i, );
> >
> > --
> > 2.17.1
--
Thanks and Regards
Srikar Dronamraju
Re: [PATCH v4 08/10] powerpc/smp: Allocate cpumask only after searching thread group
Srikar Dronamraju writes:
> If allocated earlier and the search fails, then cpumask need to be
> freed. However cpu_l1_cache_map can be allocated after we search thread
> group.
It's not freed anywhere AFAICS?
And even after this change there's still an error path that doesn't free
it, isn't there?
cheers
> Cc: linuxppc-dev
> Cc: LKML
> Cc: Michael Ellerman
> Cc: Nicholas Piggin
> Cc: Anton Blanchard
> Cc: Oliver O'Halloran
> Cc: Nathan Lynch
> Cc: Michael Neuling
> Cc: Gautham R Shenoy
> Cc: Ingo Molnar
> Cc: Peter Zijlstra
> Cc: Valentin Schneider
> Cc: Jordan Niethe
> Reviewed-by: Gautham R. Shenoy
> Signed-off-by: Srikar Dronamraju
> ---
> arch/powerpc/kernel/smp.c | 7 +++
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
> index 698000c7f76f..dab96a1203ec 100644
> --- a/arch/powerpc/kernel/smp.c
> +++ b/arch/powerpc/kernel/smp.c
> @@ -797,10 +797,6 @@ static int init_cpu_l1_cache_map(int cpu)
> if (err)
> goto out;
>
> - zalloc_cpumask_var_node(_cpu(cpu_l1_cache_map, cpu),
> - GFP_KERNEL,
> - cpu_to_node(cpu));
> -
> cpu_group_start = get_cpu_thread_group_start(cpu, );
>
> if (unlikely(cpu_group_start == -1)) {
> @@ -809,6 +805,9 @@ static int init_cpu_l1_cache_map(int cpu)
> goto out;
> }
>
> + zalloc_cpumask_var_node(_cpu(cpu_l1_cache_map, cpu),
> + GFP_KERNEL, cpu_to_node(cpu));
> +
> for (i = first_thread; i < first_thread + threads_per_core; i++) {
> int i_group_start = get_cpu_thread_group_start(i, );
>
> --
> 2.17.1
[PATCH v4 08/10] powerpc/smp: Allocate cpumask only after searching thread group
If allocated earlier and the search fails, then cpumask need to be
freed. However cpu_l1_cache_map can be allocated after we search thread
group.
Cc: linuxppc-dev
Cc: LKML
Cc: Michael Ellerman
Cc: Nicholas Piggin
Cc: Anton Blanchard
Cc: Oliver O'Halloran
Cc: Nathan Lynch
Cc: Michael Neuling
Cc: Gautham R Shenoy
Cc: Ingo Molnar
Cc: Peter Zijlstra
Cc: Valentin Schneider
Cc: Jordan Niethe
Reviewed-by: Gautham R. Shenoy
Signed-off-by: Srikar Dronamraju
---
arch/powerpc/kernel/smp.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 698000c7f76f..dab96a1203ec 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -797,10 +797,6 @@ static int init_cpu_l1_cache_map(int cpu)
if (err)
goto out;
- zalloc_cpumask_var_node(_cpu(cpu_l1_cache_map, cpu),
- GFP_KERNEL,
- cpu_to_node(cpu));
-
cpu_group_start = get_cpu_thread_group_start(cpu, );
if (unlikely(cpu_group_start == -1)) {
@@ -809,6 +805,9 @@ static int init_cpu_l1_cache_map(int cpu)
goto out;
}
+ zalloc_cpumask_var_node(_cpu(cpu_l1_cache_map, cpu),
+ GFP_KERNEL, cpu_to_node(cpu));
+
for (i = first_thread; i < first_thread + threads_per_core; i++) {
int i_group_start = get_cpu_thread_group_start(i, );
--
2.17.1
[PATCH v4 08/10] powerpc/smp: Allocate cpumask only after searching thread group
If allocated earlier and the search fails, then cpumask need to be
freed. However cpu_l1_cache_map can be allocated after we search thread
group.
Cc: linuxppc-dev
Cc: LKML
Cc: Michael Ellerman
Cc: Nicholas Piggin
Cc: Anton Blanchard
Cc: Oliver O'Halloran
Cc: Nathan Lynch
Cc: Michael Neuling
Cc: Gautham R Shenoy
Cc: Ingo Molnar
Cc: Peter Zijlstra
Cc: Valentin Schneider
Cc: Jordan Niethe
Reviewed-by: Gautham R. Shenoy
Signed-off-by: Srikar Dronamraju
---
arch/powerpc/kernel/smp.c | 7 +++
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/arch/powerpc/kernel/smp.c b/arch/powerpc/kernel/smp.c
index 698000c7f76f..dab96a1203ec 100644
--- a/arch/powerpc/kernel/smp.c
+++ b/arch/powerpc/kernel/smp.c
@@ -797,10 +797,6 @@ static int init_cpu_l1_cache_map(int cpu)
if (err)
goto out;
- zalloc_cpumask_var_node(_cpu(cpu_l1_cache_map, cpu),
- GFP_KERNEL,
- cpu_to_node(cpu));
-
cpu_group_start = get_cpu_thread_group_start(cpu, );
if (unlikely(cpu_group_start == -1)) {
@@ -809,6 +805,9 @@ static int init_cpu_l1_cache_map(int cpu)
goto out;
}
+ zalloc_cpumask_var_node(_cpu(cpu_l1_cache_map, cpu),
+ GFP_KERNEL, cpu_to_node(cpu));
+
for (i = first_thread; i < first_thread + threads_per_core; i++) {
int i_group_start = get_cpu_thread_group_start(i, );
--
2.17.1
