diff mbox series

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

Message ID 20190304104541.25745-2-peng.fan@nxp.com (mailing list archive)
State New, archived
Headers show
Series [1/2] perpcu: correct pcpu_find_block_fit comments | expand

Commit Message

Peng Fan March 4, 2019, 10:33 a.m. UTC 
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@nxp.com>
---

V1:
  Based on https://patchwork.kernel.org/cover/10832459/ applied linux-next
  boot test on qemu aarch64

 mm/percpu.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

Comments

Dennis Zhou March 4, 2019, 6:56 p.m. UTC | #1 
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@nxp.com>
> ---
> 
> V1:
>   Based on https://patchwork.kernel.org/cover/10832459/ 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.

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.

Thanks,
Dennis
Peng Fan March 5, 2019, 1:31 a.m. UTC | #2 
> -----Original Message-----
> From: dennis@kernel.org [mailto:dennis@kernel.org]
> Sent: 2019年3月5日 2:57
> To: Peng Fan <peng.fan@nxp.com>
> Cc: tj@kernel.org; cl@linux.com; linux-mm@kvack.org;
> linux-kernel@vger.kernel.org; van.freenix@gmail.com
> 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@nxp.com>
> > ---
> >
> > 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
diff mbox series

Patch

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;