[PATCH] mm/percpu.c: Modify calculation of size of populated bitmap of chunk for memory allocation

[PATCH] mm/percpu.c: Modify calculation of size of populated bitmap of chunk for memory allocation

[mcsmonk]

From: Sunghyun Jin <mcsmonk@gmail.com>

Variable populated, which is a member of struct pcpu_chunk, is used as a
unit of size of unsigned long.
However, size of populated is miscounted. So, I fix this minor part.

Signed-off-by: Sunghyun Jin <mcsmonk@gmail.com>
---
 mm/percpu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/percpu.c b/mm/percpu.c
index f4709629e6de..1ed1a349eab8 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -1316,7 +1316,7 @@ static struct pcpu_chunk * __init pcpu_alloc_first_chunk(unsigned long tmp_addr,
 
 	/* allocate chunk */
 	alloc_size = sizeof(struct pcpu_chunk) +
-		BITS_TO_LONGS(region_size >> PAGE_SHIFT);
+		BITS_TO_LONGS(region_size >> PAGE_SHIFT) * sizeof(unsigned long);
 	chunk = memblock_alloc(alloc_size, SMP_CACHE_BYTES);
 	if (!chunk)
 		panic("%s: Failed to allocate %zu bytes\n", __func__,
-- 
2.17.1

Re: [PATCH] mm/percpu.c: Modify calculation of size of populated bitmap of chunk for memory allocation

[Vlastimil Babka]

On 9/3/20 2:41 PM, mcsmonk wrote:
> From: Sunghyun Jin <mcsmonk@gmail.com>
> 
> Variable populated, which is a member of struct pcpu_chunk, is used as a
> unit of size of unsigned long.
> However, size of populated is miscounted. So, I fix this minor part.

+CC folks who touched it last

Nice find! Did you observe e.g. a panic that can be used in the commit log? Or
were we always lucky thanks to alignment?
Is there perhaps a commit that introduced the bug and we can use it as Fixes:?
My brief look suggests 8ab16c43ea79 ("percpu: change the number of pages marked
in the first_chunk pop bitmap")

Thanks!

> Signed-off-by: Sunghyun Jin <mcsmonk@gmail.com>
> ---
>  mm/percpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/percpu.c b/mm/percpu.c
> index f4709629e6de..1ed1a349eab8 100644
> --- a/mm/percpu.c
> +++ b/mm/percpu.c
> @@ -1316,7 +1316,7 @@ static struct pcpu_chunk * __init pcpu_alloc_first_chunk(unsigned long tmp_addr,
>  
>  	/* allocate chunk */
>  	alloc_size = sizeof(struct pcpu_chunk) +
> -		BITS_TO_LONGS(region_size >> PAGE_SHIFT);
> +		BITS_TO_LONGS(region_size >> PAGE_SHIFT) * sizeof(unsigned long);
>  	chunk = memblock_alloc(alloc_size, SMP_CACHE_BYTES);
>  	if (!chunk)
>  		panic("%s: Failed to allocate %zu bytes\n", __func__,
> 

Re: [PATCH] mm/percpu.c: Modify calculation of size of populated bitmap of chunk for memory allocation

[Dennis Zhou]

Hello Vlastimil and Sunghyun,

On Thu, Sep 03, 2020 at 03:46:33PM +0200, Vlastimil Babka wrote:
> On 9/3/20 2:41 PM, mcsmonk wrote:
> > From: Sunghyun Jin <mcsmonk@gmail.com>
> > 
> > Variable populated, which is a member of struct pcpu_chunk, is used as a
> > unit of size of unsigned long.
> > However, size of populated is miscounted. So, I fix this minor part.
> 
> +CC folks who touched it last
> 

Thanks for CCing me.

> Nice find! Did you observe e.g. a panic that can be used in the commit log? Or
> were we always lucky thanks to alignment?

Well that is indeed awkward. Luckily the first chunk is a bit special
and only holds 7 dynamic pages. Additionally, the allocation rounds to
SMP_CACHE_BYTES so that would give us 8 bytes to play with as
struct pcpu_chunk is 120 bytes. So, while technically (wrong) the
1 byte was sufficient and the additional buffer is why at least I never
got a panic report to today.

> Is there perhaps a commit that introduced the bug and we can use it as Fixes:?
> My brief look suggests 8ab16c43ea79 ("percpu: change the number of pages marked
> in the first_chunk pop bitmap")
> 

That is the right commit. I'll pick this up and add the fixes line.

> Thanks!
> 

Thanks,
Dennis

> > Signed-off-by: Sunghyun Jin <mcsmonk@gmail.com>
> > ---
> >  mm/percpu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/mm/percpu.c b/mm/percpu.c
> > index f4709629e6de..1ed1a349eab8 100644
> > --- a/mm/percpu.c
> > +++ b/mm/percpu.c
> > @@ -1316,7 +1316,7 @@ static struct pcpu_chunk * __init pcpu_alloc_first_chunk(unsigned long tmp_addr,
> >  
> >  	/* allocate chunk */
> >  	alloc_size = sizeof(struct pcpu_chunk) +
> > -		BITS_TO_LONGS(region_size >> PAGE_SHIFT);
> > +		BITS_TO_LONGS(region_size >> PAGE_SHIFT) * sizeof(unsigned long);
> >  	chunk = memblock_alloc(alloc_size, SMP_CACHE_BYTES);
> >  	if (!chunk)
> >  		panic("%s: Failed to allocate %zu bytes\n", __func__,
> > 
>