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

From: Dennis Zhou <dennis@kernel.org>
To: Vlastimil Babka <vbabka@suse.cz>
Cc: mcsmonk <mcsmonk@gmail.com>,
	linux-mm@kvack.org, Mike Rapoport <rppt@linux.ibm.com>,
	Dennis Zhou <dennis@kernel.org>, Tejun Heo <tj@kernel.org>
Subject: Re: [PATCH] mm/percpu.c: Modify calculation of size of populated bitmap of chunk for memory allocation
Date: Thu, 3 Sep 2020 15:46:26 +0000	[thread overview]
Message-ID: <20200903154626.GA1665298@google.com> (raw)
In-Reply-To: <a314b5ff-599f-b580-9bf2-e6c9b95183ea@suse.cz>

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__,
> > 
> 