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

From: Peng Fan
Date: Mon Mar 04 2019 - 20:31:53 EST




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