[PATCH 1/2] perpcu: correct pcpu_find_block_fit comments

[PATCH 1/2] perpcu: correct pcpu_find_block_fit comments

[Peng Fan]

pcpu_find_block_fit is not find block index, it is to find
the bitmap off in a chunk.

Signed-off-by: Peng Fan <peng.fan@nxp.com>
---

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

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

diff --git a/mm/percpu.c b/mm/percpu.c
index 7f630d5469e8..5ee90fc34ea3 100644
--- a/mm/percpu.c
+++ b/mm/percpu.c
@@ -1061,7 +1061,7 @@ static bool pcpu_is_populated(struct pcpu_chunk *chunk, int bit_off, int bits,
 }
 
 /**
- * pcpu_find_block_fit - finds the block index to start searching
+ * pcpu_find_block_fit - finds the offset in chunk bitmap to start searching
  * @chunk: chunk of interest
  * @alloc_bits: size of request in allocation units
  * @align: alignment of area (max PAGE_SIZE bytes)
-- 
2.16.4

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

[Peng Fan]

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

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

[dennis]

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

Re: [PATCH 1/2] perpcu: correct pcpu_find_block_fit comments

[Dennis Zhou]

On Mon, Mar 04, 2019 at 10:33:52AM +0000, Peng Fan wrote:
> pcpu_find_block_fit is not find block index, it is to find
> the bitmap off in a chunk.
> 
> Signed-off-by: Peng Fan <peng.fan@nxp.com>
> ---
> 
> V1:
>   Based on https://patchwork.kernel.org/cover/10832459/ applied linux-next
> 
>  mm/percpu.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/percpu.c b/mm/percpu.c
> index 7f630d5469e8..5ee90fc34ea3 100644
> --- a/mm/percpu.c
> +++ b/mm/percpu.c
> @@ -1061,7 +1061,7 @@ static bool pcpu_is_populated(struct pcpu_chunk *chunk, int bit_off, int bits,
>  }
>  
>  /**
> - * pcpu_find_block_fit - finds the block index to start searching
> + * pcpu_find_block_fit - finds the offset in chunk bitmap to start searching
>   * @chunk: chunk of interest
>   * @alloc_bits: size of request in allocation units
>   * @align: alignment of area (max PAGE_SIZE bytes)
> -- 
> 2.16.4
> 

So really the block index is encoded in the bit offset. I'm not super
happy with either wording because the point of the function really is to
find a block(s) that can support this allocation and it happens the
output is a chunk offset.

Thanks,
Dennis

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

[Peng Fan]

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

RE: [PATCH 1/2] perpcu: correct pcpu_find_block_fit comments

[Peng Fan]

> -----Original Message-----
> From: Dennis Zhou [mailto:dennis@kernel.org]
> Sent: 2019年3月5日 3:14
> 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 1/2] perpcu: correct pcpu_find_block_fit comments
> 
> On Mon, Mar 04, 2019 at 10:33:52AM +0000, Peng Fan wrote:
> > pcpu_find_block_fit is not find block index, it is to find the bitmap
> > off in a chunk.
> >
> > Signed-off-by: Peng Fan <peng.fan@nxp.com>
> > ---
> >
> > V1:
> >   Based on
> >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fpat
> >
> chwork.kernel.org%2Fcover%2F10832459%2F&amp;data=02%7C01%7Cpeng.
> fan%40
> >
> nxp.com%7Cebb4d3d31dca4c76ec2e08d6a0d587fd%7C686ea1d3bc2b4c6fa9
> 2cd99c5
> >
> c301635%7C0%7C0%7C636873236308905172&amp;sdata=POM0aLB5P7g3y
> p6oD5s8uDY
> > 629jfIeo4hko4PcqNA70%3D&amp;reserved=0 applied linux-next
> >
> >  mm/percpu.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/percpu.c b/mm/percpu.c index
> > 7f630d5469e8..5ee90fc34ea3 100644
> > --- a/mm/percpu.c
> > +++ b/mm/percpu.c
> > @@ -1061,7 +1061,7 @@ static bool pcpu_is_populated(struct pcpu_chunk
> > *chunk, int bit_off, int bits,  }
> >
> >  /**
> > - * pcpu_find_block_fit - finds the block index to start searching
> > + * pcpu_find_block_fit - finds the offset in chunk bitmap to start
> > + searching
> >   * @chunk: chunk of interest
> >   * @alloc_bits: size of request in allocation units
> >   * @align: alignment of area (max PAGE_SIZE bytes)
> > --
> > 2.16.4
> >
> 
> So really the block index is encoded in the bit offset. I'm not super
> happy with either wording because the point of the function really is to
> find a block(s) that can support this allocation and it happens the
> output is a chunk offset.

I just think the comments is confusing, because block_index is not used in
this function or returned. However the function returns a bit offset
in a chunk for caller's. I understand block index is encoded in the bit offset as following
            block_index     
              |
              v
Block   |----|----|----|----|----|----|----|----|----|----|
Chunk  |------------------------------------------------------|
                |
                V
               bit_off in a chunk
and bit_off = block_index * PCPU_BITMAP_BLOCK_BITS + offset in a block

I'll leave it as is, since you not prefer.

Thanks,
Peng.
> 
> Thanks,
> Dennis