RE: [PATCH 2/2] percpu: pcpu_next_md_free_region: inclusive check for PCPU_BITMAP_BLOCK_BITS

[Peng Fan]

> -----Original Message-----
> From: dennis@xxxxxxxxxx [mailto:dennis@xxxxxxxxxx]
> Sent: 2019年3月5日 2:57
> To: Peng Fan <peng.fan@xxxxxxx>
> Cc: tj@xxxxxxxxxx; cl@xxxxxxxxx; linux-mm@xxxxxxxxx;
> linux-kernel@xxxxxxxxxxxxxxx; van.freenix@xxxxxxxxx
> Subject: Re: [PATCH 2/2] percpu: pcpu_next_md_free_region: inclusive check
> for PCPU_BITMAP_BLOCK_BITS
> 
> Hi Peng,
> 
> On Mon, Mar 04, 2019 at 10:33:55AM +0000, Peng Fan wrote:
> > If the block [contig_hint_start, contig_hint_start + contig_hint)
> > matches block->right_free area, need use "<=", not "<".
> >
> > Signed-off-by: Peng Fan <peng.fan@xxxxxxx>
> > ---
> >
> > V1:
> >   Based on
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpatc
> hwork.kernel.org%2Fcover%2F10832459%2F&amp;data=02%7C01%7Cpeng.f
> an%40nxp.com%7C6546dfcc85f0492d7c7508d6a0d33076%7C686ea1d3bc2b
> 4c6fa92cd99c5c301635%7C0%7C0%7C636873226241185534&amp;sdata=9
> azIw8vXJ8eqqd0T0znmEN6jR2cWhFghKBfg0zIJMDM%3D&amp;reserved=0
> applied linux-next
> >   boot test on qemu aarch64
> >
> >  mm/percpu.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/mm/percpu.c b/mm/percpu.c index
> > 5ee90fc34ea3..0f91f1d883c6 100644
> > --- a/mm/percpu.c
> > +++ b/mm/percpu.c
> > @@ -390,7 +390,8 @@ static void pcpu_next_md_free_region(struct
> pcpu_chunk *chunk, int *bit_off,
> >  		 */
> >  		*bits = block->contig_hint;
> >  		if (*bits && block->contig_hint_start >= block_off &&
> > -		    *bits + block->contig_hint_start < PCPU_BITMAP_BLOCK_BITS)
> {
> > +		    *bits + block->contig_hint_start <=
> > +		    PCPU_BITMAP_BLOCK_BITS) {
> >  			*bit_off = pcpu_block_off_to_off(i,
> >  					block->contig_hint_start);
> >  			return;
> > --
> > 2.16.4
> >
> 
> This is wrong. This iterator is for updating contig hints and not for
> finding fit.

I missed to consider the case the when contig_hint_start matches
right_free area, the right_free area will be take into consideration
into next loop.

> 
> Have you tried reproducing and proving the issue you are seeing? In
> general, making changes to percpu carries a lot of risk. I really only
> want to be taking code that is provably solving a problem and not
> supported by just code inspection. Boot testing for a change like this
> is really not enough as we need to be sure changes like these are
> correct.

I'll be careful for future patches.

Thanks,
Peng.

> 
> Thanks,
> Dennis