diff mbox

[V7,1/3] genalloc:support memory-allocation with bytes-alignment to genalloc

Message ID 1441011520-15424-1-git-send-email-qiang.zhao@freescale.com (mailing list archive)
State Superseded
Delegated to: Scott Wood
Headers show

Commit Message

Zhao Qiang Aug. 31, 2015, 8:58 a.m. UTC
Bytes alignment is required to manage some special RAM,
so add gen_pool_first_fit_align to genalloc,
meanwhile add gen_pool_alloc_data to pass data to
gen_pool_first_fit_align(modify gen_pool_alloc as a wrapper)

Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>
---
Changes for v6:
	- patches set v6 include a new patch because of using 
	- genalloc to manage QE MURAM, patch 0001 is the new 
	- patch, adding bytes alignment for allocation for use.
Changes for v7:
	- cpm muram also need to use genalloc to manage, it has 
	  a function to reserve a specific region of muram,
	  add offset to genpool_data for start addr to be allocated.

 include/linux/genalloc.h | 25 ++++++++++++++++----
 lib/genalloc.c           | 61 ++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 77 insertions(+), 9 deletions(-)

Comments

Scott Wood Sept. 2, 2015, 12:30 a.m. UTC | #1
On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:
> Bytes alignment is required to manage some special RAM,
> so add gen_pool_first_fit_align to genalloc,
> meanwhile add gen_pool_alloc_data to pass data to
> gen_pool_first_fit_align(modify gen_pool_alloc as a wrapper)
> 
> Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>
> ---
> Changes for v6:
>       - patches set v6 include a new patch because of using 
>       - genalloc to manage QE MURAM, patch 0001 is the new 
>       - patch, adding bytes alignment for allocation for use.
> Changes for v7:
>       - cpm muram also need to use genalloc to manage, it has 
>         a function to reserve a specific region of muram,
>         add offset to genpool_data for start addr to be allocated.

This seems to be describing more than just the changes in this patch.  What 
does also handling cpm have to do with this patch?  Are you adding support 
for reserving a specific region in this patch?  I don't see it, and in any 
case it should go in a different patch.

> +/*
> + *  gen_pool data descriptor for gen_pool_first_fit_align.
> + */
> +struct genpool_data_align {
> +     int align;              /* alignment by bytes for starting address */
> +     unsigned long offset;   /* the offset of allocation start addr*/
> +};

The offset belongs on the caller side, not here.

-Scott
Zhao Qiang Sept. 2, 2015, 2:10 a.m. UTC | #2
On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Wednesday, September 02, 2015 8:30 AM

> To: Zhao Qiang-B45475

> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li

> Yang-Leo-R58472; paulus@samba.org

> Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> bytes-alignment to genalloc

> 

> On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:

> > Bytes alignment is required to manage some special RAM, so add

> > gen_pool_first_fit_align to genalloc, meanwhile add

> > gen_pool_alloc_data to pass data to gen_pool_first_fit_align(modify

> > gen_pool_alloc as a wrapper)

> >

> > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>

> > ---

> > Changes for v6:

> >       - patches set v6 include a new patch because of using

> >       - genalloc to manage QE MURAM, patch 0001 is the new

> >       - patch, adding bytes alignment for allocation for use.

> > Changes for v7:

> >       - cpm muram also need to use genalloc to manage, it has

> >         a function to reserve a specific region of muram,

> >         add offset to genpool_data for start addr to be allocated.

> 

> This seems to be describing more than just the changes in this patch.

> What does also handling cpm have to do with this patch?  Are you adding

> support for reserving a specific region in this patch?  I don't see it,

> and in any case it should go in a different patch.


Yes, I added. The code below can support the function.
	offset_bit = (alignment->offset + (1UL << order) - 1) >> order;
      return bitmap_find_next_zero_area(map, size, start + offset_bit, nr,
                        align_mask);
CPM has an function cpm_muram_alloc_fixed, needing to allocate muram from a
Specific offset. So I add the code and add offset to struct data.
This patch is the first patch of this patch set, so I explain what changes about
Set v7 and why I add support for reserving a specific region in this patch.

If you really think a different patch needed, I can add a new patch to handle it.

> 

> > +/*

> > + *  gen_pool data descriptor for gen_pool_first_fit_align.

> > + */

> > +struct genpool_data_align {

> > +     int align;              /* alignment by bytes for starting

> address */

> > +     unsigned long offset;   /* the offset of allocation start addr*/

> > +};

> 

> The offset belongs on the caller side, not here.


So, how do I pass offset to gen_pool_alloc_data or pool->algo?

> 

> -Scott
Scott Wood Sept. 2, 2015, 2:18 a.m. UTC | #3
On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:
> On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, September 02, 2015 8:30 AM
> > To: Zhao Qiang-B45475
> > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li
> > Yang-Leo-R58472; paulus@samba.org
> > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > bytes-alignment to genalloc
> > 
> > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:
> > > Bytes alignment is required to manage some special RAM, so add
> > > gen_pool_first_fit_align to genalloc, meanwhile add
> > > gen_pool_alloc_data to pass data to gen_pool_first_fit_align(modify
> > > gen_pool_alloc as a wrapper)
> > > 
> > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>
> > > ---
> > > Changes for v6:
> > >       - patches set v6 include a new patch because of using
> > >       - genalloc to manage QE MURAM, patch 0001 is the new
> > >       - patch, adding bytes alignment for allocation for use.
> > > Changes for v7:
> > >       - cpm muram also need to use genalloc to manage, it has
> > >         a function to reserve a specific region of muram,
> > >         add offset to genpool_data for start addr to be allocated.
> > 
> > This seems to be describing more than just the changes in this patch.
> > What does also handling cpm have to do with this patch?  Are you adding
> > support for reserving a specific region in this patch?  I don't see it,
> > and in any case it should go in a different patch.
> 
> Yes, I added. The code below can support the function.
>       offset_bit = (alignment->offset + (1UL << order) - 1) >> order;
>       return bitmap_find_next_zero_area(map, size, start + offset_bit, nr,
>                         align_mask);
>
> CPM has an function cpm_muram_alloc_fixed, needing to allocate muram from a
> Specific offset. So I add the code and add offset to struct data.

I thought the offset was related to the previous discussion of checking for 
allocation failure.  Are you using it to implement alloc_fixed()?  If so, 
please don't.  Besides the awkward implementation (what does it logically 
have to do with gen_pool_first_fit_align?), it does not appear to be correct -
- what happens with multiple chunks?  What happens if part of the region the 
caller is trying to reserve is already taken?  Implement a proper function to 
reserve a fixed genalloc region.

> This patch is the first patch of this patch set, so I explain what changes 
> about
> Set v7 and why I add support for reserving a specific region in this patch.

If you want to provide commentary that covers the entire patchset, use a 
cover letter.

> > > +/*
> > > + *  gen_pool data descriptor for gen_pool_first_fit_align.
> > > + */
> > > +struct genpool_data_align {
> > > +     int align;              /* alignment by bytes for starting
> > address */
> > > +     unsigned long offset;   /* the offset of allocation start addr*/
> > > +};
> > 
> > The offset belongs on the caller side, not here.
> 
> So, how do I pass offset to gen_pool_alloc_data or pool->algo?

You don't.

-Scott
Zhao Qiang Sept. 2, 2015, 2:29 a.m. UTC | #4
On Wed, 2015-09-02 at 10:18AM -0500, Wood Scott-B07421 wrote:
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Wednesday, September 02, 2015 10:18 AM

> To: Zhao Qiang-B45475

> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li

> Yang-Leo-R58472; paulus@samba.org

> Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> bytes-alignment to genalloc

> 

> On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:

> > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:

> > > -----Original Message-----

> > > From: Wood Scott-B07421

> > > Sent: Wednesday, September 02, 2015 8:30 AM

> > > To: Zhao Qiang-B45475

> > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;

> > > Li Yang-Leo-R58472; paulus@samba.org

> > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> > > bytes-alignment to genalloc

> > >

> > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:

> > > > Bytes alignment is required to manage some special RAM, so add

> > > > gen_pool_first_fit_align to genalloc, meanwhile add

> > > > gen_pool_alloc_data to pass data to

> > > > gen_pool_first_fit_align(modify gen_pool_alloc as a wrapper)

> > > >

> > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>

> > > > ---

> > > > Changes for v6:

> > > >       - patches set v6 include a new patch because of using

> > > >       - genalloc to manage QE MURAM, patch 0001 is the new

> > > >       - patch, adding bytes alignment for allocation for use.

> > > > Changes for v7:

> > > >       - cpm muram also need to use genalloc to manage, it has

> > > >         a function to reserve a specific region of muram,

> > > >         add offset to genpool_data for start addr to be allocated.

> > >

> > > This seems to be describing more than just the changes in this patch.

> > > What does also handling cpm have to do with this patch?  Are you

> > > adding support for reserving a specific region in this patch?  I

> > > don't see it, and in any case it should go in a different patch.

> >

> > Yes, I added. The code below can support the function.

> >       offset_bit = (alignment->offset + (1UL << order) - 1) >> order;

> >       return bitmap_find_next_zero_area(map, size, start + offset_bit,

> nr,

> >                         align_mask);

> >

> > CPM has an function cpm_muram_alloc_fixed, needing to allocate muram

> > from a Specific offset. So I add the code and add offset to struct data.

> 

> I thought the offset was related to the previous discussion of checking

> for allocation failure.  Are you using it to implement alloc_fixed()?  If

> so, please don't.  Besides the awkward implementation (what does it

> logically have to do with gen_pool_first_fit_align?), it does not appear

> to be correct -

> - what happens with multiple chunks?  What happens if part of the region

> the caller is trying to reserve is already taken?  Implement a proper

> function to reserve a fixed genalloc region.


This offset is totally different with the workaround OFFSET!
This offset is the offset of the muram.
CPM need to allocate block from a specific offset due to hardware restriction.
So I must handle this offset in genalloc. 

> 

> > This patch is the first patch of this patch set, so I explain what

> > changes about Set v7 and why I add support for reserving a specific

> > region in this patch.

> 

> If you want to provide commentary that covers the entire patchset, use a

> cover letter.

> 

> > > > +/*

> > > > + *  gen_pool data descriptor for gen_pool_first_fit_align.

> > > > + */

> > > > +struct genpool_data_align {

> > > > +     int align;              /* alignment by bytes for starting

> > > address */

> > > > +     unsigned long offset;   /* the offset of allocation start

> addr*/

> > > > +};

> > >

> > > The offset belongs on the caller side, not here.

> >

> > So, how do I pass offset to gen_pool_alloc_data or pool->algo?

> 

> You don't.

> 

> -Scott

>
Scott Wood Sept. 2, 2015, 2:33 a.m. UTC | #5
On Tue, 2015-09-01 at 21:29 -0500, Zhao Qiang-B45475 wrote:
> On Wed, 2015-09-02 at 10:18AM -0500, Wood Scott-B07421 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, September 02, 2015 10:18 AM
> > To: Zhao Qiang-B45475
> > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li
> > Yang-Leo-R58472; paulus@samba.org
> > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > bytes-alignment to genalloc
> > 
> > On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:
> > > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, September 02, 2015 8:30 AM
> > > > To: Zhao Qiang-B45475
> > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;
> > > > Li Yang-Leo-R58472; paulus@samba.org
> > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > > > bytes-alignment to genalloc
> > > > 
> > > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:
> > > > > Bytes alignment is required to manage some special RAM, so add
> > > > > gen_pool_first_fit_align to genalloc, meanwhile add
> > > > > gen_pool_alloc_data to pass data to
> > > > > gen_pool_first_fit_align(modify gen_pool_alloc as a wrapper)
> > > > > 
> > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>
> > > > > ---
> > > > > Changes for v6:
> > > > >       - patches set v6 include a new patch because of using
> > > > >       - genalloc to manage QE MURAM, patch 0001 is the new
> > > > >       - patch, adding bytes alignment for allocation for use.
> > > > > Changes for v7:
> > > > >       - cpm muram also need to use genalloc to manage, it has
> > > > >         a function to reserve a specific region of muram,
> > > > >         add offset to genpool_data for start addr to be allocated.
> > > > 
> > > > This seems to be describing more than just the changes in this patch.
> > > > What does also handling cpm have to do with this patch?  Are you
> > > > adding support for reserving a specific region in this patch?  I
> > > > don't see it, and in any case it should go in a different patch.
> > > 
> > > Yes, I added. The code below can support the function.
> > >       offset_bit = (alignment->offset + (1UL << order) - 1) >> order;
> > >       return bitmap_find_next_zero_area(map, size, start + offset_bit,
> > nr,
> > >                         align_mask);
> > > 
> > > CPM has an function cpm_muram_alloc_fixed, needing to allocate muram
> > > from a Specific offset. So I add the code and add offset to struct data.
> > 
> > I thought the offset was related to the previous discussion of checking
> > for allocation failure.  Are you using it to implement alloc_fixed()?  If
> > so, please don't.  Besides the awkward implementation (what does it
> > logically have to do with gen_pool_first_fit_align?), it does not appear
> > to be correct -
> > - what happens with multiple chunks?  What happens if part of the region
> > the caller is trying to reserve is already taken?  Implement a proper
> > function to reserve a fixed genalloc region.
> 
> This offset is totally different with the workaround OFFSET!

There's a reason why we write changelogs that describe what the patch is 
doing, and avoid combining logically distinct changes in the same patch.

> This offset is the offset of the muram.

The offset of the muram relative to what?  Or do you mean the offset into 
muram?

> CPM need to allocate block from a specific offset due to hardware 
> restriction.
> So I must handle this offset in genalloc. 

Again, if you need to be able to mark a specific range reserved, add a 
function that does that properly.  Don't try to hack it in the way you did.

-Scott
Zhao Qiang Sept. 2, 2015, 3:05 a.m. UTC | #6
On Wed, 2015-09-02 at 10:33AM -0500, Wood Scott-B07421 wrote:

> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Wednesday, September 02, 2015 10:33 AM

> To: Zhao Qiang-B45475

> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li

> Yang-Leo-R58472; paulus@samba.org

> Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> bytes-alignment to genalloc

> 

> On Tue, 2015-09-01 at 21:29 -0500, Zhao Qiang-B45475 wrote:

> > On Wed, 2015-09-02 at 10:18AM -0500, Wood Scott-B07421 wrote:

> > > -----Original Message-----

> > > From: Wood Scott-B07421

> > > Sent: Wednesday, September 02, 2015 10:18 AM

> > > To: Zhao Qiang-B45475

> > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;

> > > Li Yang-Leo-R58472; paulus@samba.org

> > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> > > bytes-alignment to genalloc

> > >

> > > On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:

> > > > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:

> > > > > -----Original Message-----

> > > > > From: Wood Scott-B07421

> > > > > Sent: Wednesday, September 02, 2015 8:30 AM

> > > > > To: Zhao Qiang-B45475

> > > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > > > lauraa@codeaurora.org; Xie Xiaobo-R63061;

> > > > > benh@kernel.crashing.org; Li Yang-Leo-R58472; paulus@samba.org

> > > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation

> > > > > with bytes-alignment to genalloc

> > > > >

> > > > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:

> > > > > > Bytes alignment is required to manage some special RAM, so add

> > > > > > gen_pool_first_fit_align to genalloc, meanwhile add

> > > > > > gen_pool_alloc_data to pass data to

> > > > > > gen_pool_first_fit_align(modify gen_pool_alloc as a wrapper)

> > > > > >

> > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>

> > > > > > ---

> > > > > > Changes for v6:

> > > > > >       - patches set v6 include a new patch because of using

> > > > > >       - genalloc to manage QE MURAM, patch 0001 is the new

> > > > > >       - patch, adding bytes alignment for allocation for use.

> > > > > > Changes for v7:

> > > > > >       - cpm muram also need to use genalloc to manage, it has

> > > > > >         a function to reserve a specific region of muram,

> > > > > >         add offset to genpool_data for start addr to be

> allocated.

> > > > >

> > > > > This seems to be describing more than just the changes in this

> patch.

> > > > > What does also handling cpm have to do with this patch?  Are you

> > > > > adding support for reserving a specific region in this patch?  I

> > > > > don't see it, and in any case it should go in a different patch.

> > > >

> > > > Yes, I added. The code below can support the function.

> > > >       offset_bit = (alignment->offset + (1UL << order) - 1) >>

> order;

> > > >       return bitmap_find_next_zero_area(map, size, start +

> > > > offset_bit,

> > > nr,

> > > >                         align_mask);

> > > >

> > > > CPM has an function cpm_muram_alloc_fixed, needing to allocate

> > > > muram from a Specific offset. So I add the code and add offset to

> struct data.

> > >

> > > I thought the offset was related to the previous discussion of

> > > checking for allocation failure.  Are you using it to implement

> > > alloc_fixed()?  If so, please don't.  Besides the awkward

> > > implementation (what does it logically have to do with

> > > gen_pool_first_fit_align?), it does not appear to be correct -

> > > - what happens with multiple chunks?  What happens if part of the

> > > region the caller is trying to reserve is already taken?  Implement

> > > a proper function to reserve a fixed genalloc region.

> >

> > This offset is totally different with the workaround OFFSET!

> 

> There's a reason why we write changelogs that describe what the patch is

> doing, and avoid combining logically distinct changes in the same patch.

> 

> > This offset is the offset of the muram.

> 

> The offset of the muram relative to what?  Or do you mean the offset into

> muram?


Yes, the offset into muram.

> 

> > CPM need to allocate block from a specific offset due to hardware

> > restriction.

> > So I must handle this offset in genalloc.

> 

> Again, if you need to be able to mark a specific range reserved, add a

> function that does that properly.  Don't try to hack it in the way you

> did.


Add a function? Do you mean add a new alloc function or new algo?
If you mean new algo, CPM use both align algo and new algo, set 
Different algos in different muram_alloc func? 

> 

> -Scott
Scott Wood Sept. 2, 2015, 3:08 a.m. UTC | #7
On Tue, 2015-09-01 at 22:05 -0500, Zhao Qiang-B45475 wrote:
> On Wed, 2015-09-02 at 10:33AM -0500, Wood Scott-B07421 wrote:
> 
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, September 02, 2015 10:33 AM
> > To: Zhao Qiang-B45475
> > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li
> > Yang-Leo-R58472; paulus@samba.org
> > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > bytes-alignment to genalloc
> > 
> > On Tue, 2015-09-01 at 21:29 -0500, Zhao Qiang-B45475 wrote:
> > > On Wed, 2015-09-02 at 10:18AM -0500, Wood Scott-B07421 wrote:
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, September 02, 2015 10:18 AM
> > > > To: Zhao Qiang-B45475
> > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;
> > > > Li Yang-Leo-R58472; paulus@samba.org
> > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > > > bytes-alignment to genalloc
> > > > 
> > > > On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:
> > > > > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:
> > > > > > -----Original Message-----
> > > > > > From: Wood Scott-B07421
> > > > > > Sent: Wednesday, September 02, 2015 8:30 AM
> > > > > > To: Zhao Qiang-B45475
> > > > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > > > lauraa@codeaurora.org; Xie Xiaobo-R63061;
> > > > > > benh@kernel.crashing.org; Li Yang-Leo-R58472; paulus@samba.org
> > > > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation
> > > > > > with bytes-alignment to genalloc
> > > > > > 
> > > > > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:
> > > > > > > Bytes alignment is required to manage some special RAM, so add
> > > > > > > gen_pool_first_fit_align to genalloc, meanwhile add
> > > > > > > gen_pool_alloc_data to pass data to
> > > > > > > gen_pool_first_fit_align(modify gen_pool_alloc as a wrapper)
> > > > > > > 
> > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>
> > > > > > > ---
> > > > > > > Changes for v6:
> > > > > > >       - patches set v6 include a new patch because of using
> > > > > > >       - genalloc to manage QE MURAM, patch 0001 is the new
> > > > > > >       - patch, adding bytes alignment for allocation for use.
> > > > > > > Changes for v7:
> > > > > > >       - cpm muram also need to use genalloc to manage, it has
> > > > > > >         a function to reserve a specific region of muram,
> > > > > > >         add offset to genpool_data for start addr to be
> > allocated.
> > > > > > 
> > > > > > This seems to be describing more than just the changes in this
> > patch.
> > > > > > What does also handling cpm have to do with this patch?  Are you
> > > > > > adding support for reserving a specific region in this patch?  I
> > > > > > don't see it, and in any case it should go in a different patch.
> > > > > 
> > > > > Yes, I added. The code below can support the function.
> > > > >       offset_bit = (alignment->offset + (1UL << order) - 1) >>
> > order;
> > > > >       return bitmap_find_next_zero_area(map, size, start +
> > > > > offset_bit,
> > > > nr,
> > > > >                         align_mask);
> > > > > 
> > > > > CPM has an function cpm_muram_alloc_fixed, needing to allocate
> > > > > muram from a Specific offset. So I add the code and add offset to
> > struct data.
> > > > 
> > > > I thought the offset was related to the previous discussion of
> > > > checking for allocation failure.  Are you using it to implement
> > > > alloc_fixed()?  If so, please don't.  Besides the awkward
> > > > implementation (what does it logically have to do with
> > > > gen_pool_first_fit_align?), it does not appear to be correct -
> > > > - what happens with multiple chunks?  What happens if part of the
> > > > region the caller is trying to reserve is already taken?  Implement
> > > > a proper function to reserve a fixed genalloc region.
> > > 
> > > This offset is totally different with the workaround OFFSET!
> > 
> > There's a reason why we write changelogs that describe what the patch is
> > doing, and avoid combining logically distinct changes in the same patch.
> > 
> > > This offset is the offset of the muram.
> > 
> > The offset of the muram relative to what?  Or do you mean the offset into
> > muram?
> 
> Yes, the offset into muram.
> 
> > 
> > > CPM need to allocate block from a specific offset due to hardware
> > > restriction.
> > > So I must handle this offset in genalloc.
> > 
> > Again, if you need to be able to mark a specific range reserved, add a
> > function that does that properly.  Don't try to hack it in the way you
> > did.
> 
> Add a function? Do you mean add a new alloc function or new algo?
> If you mean new algo, CPM use both align algo and new algo, set 
> Different algos in different muram_alloc func? 

I was thinking that it was a sufficiently different operation that it 
warranted its own independent function, but I suppose you could do it as an 
algorithm that only accepts the requested range and returns failure for all 
other chunks (as well as if the range is unavailable).  It would not be 
related at all to the aligned-alloc algorithm.

-Scott
Zhao Qiang Sept. 2, 2015, 3:57 a.m. UTC | #8
On Wed, 2015-09-02 at 10:33AM -0500, Wood Scott-B07421 wrote:
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Wednesday, September 02, 2015 11:09 AM

> To: Zhao Qiang-B45475

> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li

> Yang-Leo-R58472; paulus@samba.org

> Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> bytes-alignment to genalloc

> 

> On Tue, 2015-09-01 at 22:05 -0500, Zhao Qiang-B45475 wrote:

> > On Wed, 2015-09-02 at 10:33AM -0500, Wood Scott-B07421 wrote:

> >

> > > -----Original Message-----

> > > From: Wood Scott-B07421

> > > Sent: Wednesday, September 02, 2015 10:33 AM

> > > To: Zhao Qiang-B45475

> > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;

> > > Li Yang-Leo-R58472; paulus@samba.org

> > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> > > bytes-alignment to genalloc

> > >

> > > On Tue, 2015-09-01 at 21:29 -0500, Zhao Qiang-B45475 wrote:

> > > > On Wed, 2015-09-02 at 10:18AM -0500, Wood Scott-B07421 wrote:

> > > > > -----Original Message-----

> > > > > From: Wood Scott-B07421

> > > > > Sent: Wednesday, September 02, 2015 10:18 AM

> > > > > To: Zhao Qiang-B45475

> > > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > > > lauraa@codeaurora.org; Xie Xiaobo-R63061;

> > > > > benh@kernel.crashing.org; Li Yang-Leo-R58472; paulus@samba.org

> > > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation

> > > > > with bytes-alignment to genalloc

> > > > >

> > > > > On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:

> > > > > > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:

> > > > > > > -----Original Message-----

> > > > > > > From: Wood Scott-B07421

> > > > > > > Sent: Wednesday, September 02, 2015 8:30 AM

> > > > > > > To: Zhao Qiang-B45475

> > > > > > > Cc: linux-kernel@vger.kernel.org;

> > > > > > > linuxppc-dev@lists.ozlabs.org; lauraa@codeaurora.org; Xie

> > > > > > > Xiaobo-R63061; benh@kernel.crashing.org; Li Yang-Leo-R58472;

> > > > > > > paulus@samba.org

> > > > > > > Subject: Re: [PATCH V7 1/3] genalloc:support

> > > > > > > memory-allocation with bytes-alignment to genalloc

> > > > > > >

> > > > > > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:

> > > > > > > > Bytes alignment is required to manage some special RAM, so

> > > > > > > > add gen_pool_first_fit_align to genalloc, meanwhile add

> > > > > > > > gen_pool_alloc_data to pass data to

> > > > > > > > gen_pool_first_fit_align(modify gen_pool_alloc as a

> > > > > > > > wrapper)

> > > > > > > >

> > > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>

> > > > > > > > ---

> > > > > > > > Changes for v6:

> > > > > > > >       - patches set v6 include a new patch because of using

> > > > > > > >       - genalloc to manage QE MURAM, patch 0001 is the new

> > > > > > > >       - patch, adding bytes alignment for allocation for

> use.

> > > > > > > > Changes for v7:

> > > > > > > >       - cpm muram also need to use genalloc to manage, it

> has

> > > > > > > >         a function to reserve a specific region of muram,

> > > > > > > >         add offset to genpool_data for start addr to be

> > > allocated.

> > > > > > >

> > > > > > > This seems to be describing more than just the changes in

> > > > > > > this

> > > patch.

> > > > > > > What does also handling cpm have to do with this patch?  Are

> > > > > > > you adding support for reserving a specific region in this

> > > > > > > patch?  I don't see it, and in any case it should go in a

> different patch.

> > > > > >

> > > > > > Yes, I added. The code below can support the function.

> > > > > >       offset_bit = (alignment->offset + (1UL << order) - 1) >>

> > > order;

> > > > > >       return bitmap_find_next_zero_area(map, size, start +

> > > > > > offset_bit,

> > > > > nr,

> > > > > >                         align_mask);

> > > > > >

> > > > > > CPM has an function cpm_muram_alloc_fixed, needing to allocate

> > > > > > muram from a Specific offset. So I add the code and add offset

> > > > > > to

> > > struct data.

> > > > >

> > > > > I thought the offset was related to the previous discussion of

> > > > > checking for allocation failure.  Are you using it to implement

> > > > > alloc_fixed()?  If so, please don't.  Besides the awkward

> > > > > implementation (what does it logically have to do with

> > > > > gen_pool_first_fit_align?), it does not appear to be correct -

> > > > > - what happens with multiple chunks?  What happens if part of

> > > > > the region the caller is trying to reserve is already taken?

> > > > > Implement a proper function to reserve a fixed genalloc region.

> > > >

> > > > This offset is totally different with the workaround OFFSET!

> > >

> > > There's a reason why we write changelogs that describe what the

> > > patch is doing, and avoid combining logically distinct changes in the

> same patch.

> > >

> > > > This offset is the offset of the muram.

> > >

> > > The offset of the muram relative to what?  Or do you mean the offset

> > > into muram?

> >

> > Yes, the offset into muram.

> >

> > >

> > > > CPM need to allocate block from a specific offset due to hardware

> > > > restriction.

> > > > So I must handle this offset in genalloc.

> > >

> > > Again, if you need to be able to mark a specific range reserved, add

> > > a function that does that properly.  Don't try to hack it in the way

> > > you did.

> >

> > Add a function? Do you mean add a new alloc function or new algo?

> > If you mean new algo, CPM use both align algo and new algo, set

> > Different algos in different muram_alloc func?

> 

> I was thinking that it was a sufficiently different operation that it

> warranted its own independent function, but I suppose you could do it as

> an algorithm that only accepts the requested range and returns failure

> for all other chunks (as well as if the range is unavailable).  It would

> not be related at all to the aligned-alloc algorithm.


If do so, I need set algo in different muram_alloc function, it is redundancy.
The algos has start para, but it set start_bit = 0 in gen_pool_alloc_data, can 
We pass a start addr para to gen_pool_alloc_data?

> 

> -Scott

>
Scott Wood Sept. 2, 2015, 4:51 a.m. UTC | #9
On Tue, 2015-09-01 at 22:57 -0500, Zhao Qiang-B45475 wrote:
> On Wed, 2015-09-02 at 10:33AM -0500, Wood Scott-B07421 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, September 02, 2015 11:09 AM
> > To: Zhao Qiang-B45475
> > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li
> > Yang-Leo-R58472; paulus@samba.org
> > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > bytes-alignment to genalloc
> > 
> > On Tue, 2015-09-01 at 22:05 -0500, Zhao Qiang-B45475 wrote:
> > > On Wed, 2015-09-02 at 10:33AM -0500, Wood Scott-B07421 wrote:
> > > 
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, September 02, 2015 10:33 AM
> > > > To: Zhao Qiang-B45475
> > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;
> > > > Li Yang-Leo-R58472; paulus@samba.org
> > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > > > bytes-alignment to genalloc
> > > > 
> > > > On Tue, 2015-09-01 at 21:29 -0500, Zhao Qiang-B45475 wrote:
> > > > > On Wed, 2015-09-02 at 10:18AM -0500, Wood Scott-B07421 wrote:
> > > > > > -----Original Message-----
> > > > > > From: Wood Scott-B07421
> > > > > > Sent: Wednesday, September 02, 2015 10:18 AM
> > > > > > To: Zhao Qiang-B45475
> > > > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > > > lauraa@codeaurora.org; Xie Xiaobo-R63061;
> > > > > > benh@kernel.crashing.org; Li Yang-Leo-R58472; paulus@samba.org
> > > > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation
> > > > > > with bytes-alignment to genalloc
> > > > > > 
> > > > > > On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:
> > > > > > > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:
> > > > > > > > -----Original Message-----
> > > > > > > > From: Wood Scott-B07421
> > > > > > > > Sent: Wednesday, September 02, 2015 8:30 AM
> > > > > > > > To: Zhao Qiang-B45475
> > > > > > > > Cc: linux-kernel@vger.kernel.org;
> > > > > > > > linuxppc-dev@lists.ozlabs.org; lauraa@codeaurora.org; Xie
> > > > > > > > Xiaobo-R63061; benh@kernel.crashing.org; Li Yang-Leo-R58472;
> > > > > > > > paulus@samba.org
> > > > > > > > Subject: Re: [PATCH V7 1/3] genalloc:support
> > > > > > > > memory-allocation with bytes-alignment to genalloc
> > > > > > > > 
> > > > > > > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:
> > > > > > > > > Bytes alignment is required to manage some special RAM, so
> > > > > > > > > add gen_pool_first_fit_align to genalloc, meanwhile add
> > > > > > > > > gen_pool_alloc_data to pass data to
> > > > > > > > > gen_pool_first_fit_align(modify gen_pool_alloc as a
> > > > > > > > > wrapper)
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>
> > > > > > > > > ---
> > > > > > > > > Changes for v6:
> > > > > > > > >       - patches set v6 include a new patch because of using
> > > > > > > > >       - genalloc to manage QE MURAM, patch 0001 is the new
> > > > > > > > >       - patch, adding bytes alignment for allocation for
> > use.
> > > > > > > > > Changes for v7:
> > > > > > > > >       - cpm muram also need to use genalloc to manage, it
> > has
> > > > > > > > >         a function to reserve a specific region of muram,
> > > > > > > > >         add offset to genpool_data for start addr to be
> > > > allocated.
> > > > > > > > 
> > > > > > > > This seems to be describing more than just the changes in
> > > > > > > > this
> > > > patch.
> > > > > > > > What does also handling cpm have to do with this patch?  Are
> > > > > > > > you adding support for reserving a specific region in this
> > > > > > > > patch?  I don't see it, and in any case it should go in a
> > different patch.
> > > > > > > 
> > > > > > > Yes, I added. The code below can support the function.
> > > > > > >       offset_bit = (alignment->offset + (1UL << order) - 1) >>
> > > > order;
> > > > > > >       return bitmap_find_next_zero_area(map, size, start +
> > > > > > > offset_bit,
> > > > > > nr,
> > > > > > >                         align_mask);
> > > > > > > 
> > > > > > > CPM has an function cpm_muram_alloc_fixed, needing to allocate
> > > > > > > muram from a Specific offset. So I add the code and add offset
> > > > > > > to
> > > > struct data.
> > > > > > 
> > > > > > I thought the offset was related to the previous discussion of
> > > > > > checking for allocation failure.  Are you using it to implement
> > > > > > alloc_fixed()?  If so, please don't.  Besides the awkward
> > > > > > implementation (what does it logically have to do with
> > > > > > gen_pool_first_fit_align?), it does not appear to be correct -
> > > > > > - what happens with multiple chunks?  What happens if part of
> > > > > > the region the caller is trying to reserve is already taken?
> > > > > > Implement a proper function to reserve a fixed genalloc region.
> > > > > 
> > > > > This offset is totally different with the workaround OFFSET!
> > > > 
> > > > There's a reason why we write changelogs that describe what the
> > > > patch is doing, and avoid combining logically distinct changes in the
> > same patch.
> > > > 
> > > > > This offset is the offset of the muram.
> > > > 
> > > > The offset of the muram relative to what?  Or do you mean the offset
> > > > into muram?
> > > 
> > > Yes, the offset into muram.
> > > 
> > > > 
> > > > > CPM need to allocate block from a specific offset due to hardware
> > > > > restriction.
> > > > > So I must handle this offset in genalloc.
> > > > 
> > > > Again, if you need to be able to mark a specific range reserved, add
> > > > a function that does that properly.  Don't try to hack it in the way
> > > > you did.
> > > 
> > > Add a function? Do you mean add a new alloc function or new algo?
> > > If you mean new algo, CPM use both align algo and new algo, set
> > > Different algos in different muram_alloc func?
> > 
> > I was thinking that it was a sufficiently different operation that it
> > warranted its own independent function, but I suppose you could do it as
> > an algorithm that only accepts the requested range and returns failure
> > for all other chunks (as well as if the range is unavailable).  It would
> > not be related at all to the aligned-alloc algorithm.
> 
> If do so, I need set algo in different muram_alloc function, it is 
> redundancy.

If you do it as a separate top-level function there would be no algorithm.

Using an algorithm would be simpler to implement, but a bit more awkward in 
the caller due to the need to swap out the algorithm (unless we change 
gen_pool_alloc_data to gen_pool_alloc_algo_data or similar...).

> The algos has start para, but it set start_bit = 0 in gen_pool_alloc_data, 
> can We pass a start addr para to gen_pool_alloc_data?

No.  Again, setting that "start" variable is not equivalent to what you're 
trying to accomplish, even if it happens to work in your test case.

-Scott
Zhao Qiang Sept. 6, 2015, 3:13 a.m. UTC | #10
On Wed, 2015-09-02 at 10:18AM +0800, Wood Scott-B07421 wrote:
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Wednesday, September 02, 2015 10:18 AM

> To: Zhao Qiang-B45475

> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li

> Yang-Leo-R58472; paulus@samba.org

> Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> bytes-alignment to genalloc

> 

> On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:

> > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:

> > > -----Original Message-----

> > > From: Wood Scott-B07421

> > > Sent: Wednesday, September 02, 2015 8:30 AM

> > > To: Zhao Qiang-B45475

> > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;

> > > Li Yang-Leo-R58472; paulus@samba.org

> > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> > > bytes-alignment to genalloc

> > >

> > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:

> > > > Bytes alignment is required to manage some special RAM, so add

> > > > gen_pool_first_fit_align to genalloc, meanwhile add

> > > > gen_pool_alloc_data to pass data to

> > > > gen_pool_first_fit_align(modify gen_pool_alloc as a wrapper)

> > > >

> > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>

> > > > ---

> > > > Changes for v6:

> > > >       - patches set v6 include a new patch because of using

> > > >       - genalloc to manage QE MURAM, patch 0001 is the new

> > > >       - patch, adding bytes alignment for allocation for use.

> > > > Changes for v7:

> > > >       - cpm muram also need to use genalloc to manage, it has

> > > >         a function to reserve a specific region of muram,

> > > >         add offset to genpool_data for start addr to be allocated.

> > >

> > > This seems to be describing more than just the changes in this patch.

> > > What does also handling cpm have to do with this patch?  Are you

> > > adding support for reserving a specific region in this patch?  I

> > > don't see it, and in any case it should go in a different patch.

> >

> > Yes, I added. The code below can support the function.

> >       offset_bit = (alignment->offset + (1UL << order) - 1) >> order;

> >       return bitmap_find_next_zero_area(map, size, start + offset_bit,

> nr,

> >                         align_mask);

> >

> > CPM has an function cpm_muram_alloc_fixed, needing to allocate muram

> > from a Specific offset. So I add the code and add offset to struct data.

> 

> I thought the offset was related to the previous discussion of checking

> for allocation failure.  Are you using it to implement alloc_fixed()?  If

> so, please don't.  Besides the awkward implementation (what does it

> logically have to do with gen_pool_first_fit_align?), it does not appear

> to be correct -

> - what happens with multiple chunks?  What happens if part of the region

> the caller is trying to reserve is already taken?  Implement a proper

> function to reserve a fixed genalloc region.

> 

The offset is which allocation block address need to be larger than, 
Not equal to, it really like the parameter start of the algo(the bitnumber
To start searching at).

> 

> > > > +/*

> > > > + *  gen_pool data descriptor for gen_pool_first_fit_align.

> > > > + */

> > > > +struct genpool_data_align {

> > > > +     int align;              /* alignment by bytes for starting

> > > address */

> > > > +     unsigned long offset;   /* the offset of allocation start

> addr*/

> > > > +};

> > >

> 

> -Scott

> 

-Zhao
Scott Wood Sept. 9, 2015, 4:37 p.m. UTC | #11
On Sat, 2015-09-05 at 22:13 -0500, Zhao Qiang-B45475 wrote:
> On Wed, 2015-09-02 at 10:18AM +0800, Wood Scott-B07421 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Wednesday, September 02, 2015 10:18 AM
> > To: Zhao Qiang-B45475
> > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li
> > Yang-Leo-R58472; paulus@samba.org
> > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > bytes-alignment to genalloc
> > 
> > On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:
> > > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, September 02, 2015 8:30 AM
> > > > To: Zhao Qiang-B45475
> > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;
> > > > Li Yang-Leo-R58472; paulus@samba.org
> > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > > > bytes-alignment to genalloc
> > > > 
> > > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:
> > > > > Bytes alignment is required to manage some special RAM, so add
> > > > > gen_pool_first_fit_align to genalloc, meanwhile add
> > > > > gen_pool_alloc_data to pass data to
> > > > > gen_pool_first_fit_align(modify gen_pool_alloc as a wrapper)
> > > > > 
> > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>
> > > > > ---
> > > > > Changes for v6:
> > > > >       - patches set v6 include a new patch because of using
> > > > >       - genalloc to manage QE MURAM, patch 0001 is the new
> > > > >       - patch, adding bytes alignment for allocation for use.
> > > > > Changes for v7:
> > > > >       - cpm muram also need to use genalloc to manage, it has
> > > > >         a function to reserve a specific region of muram,
> > > > >         add offset to genpool_data for start addr to be allocated.
> > > > 
> > > > This seems to be describing more than just the changes in this patch.
> > > > What does also handling cpm have to do with this patch?  Are you
> > > > adding support for reserving a specific region in this patch?  I
> > > > don't see it, and in any case it should go in a different patch.
> > > 
> > > Yes, I added. The code below can support the function.
> > >       offset_bit = (alignment->offset + (1UL << order) - 1) >> order;
> > >       return bitmap_find_next_zero_area(map, size, start + offset_bit,
> > nr,
> > >                         align_mask);
> > > 
> > > CPM has an function cpm_muram_alloc_fixed, needing to allocate muram
> > > from a Specific offset. So I add the code and add offset to struct data.
> > 
> > I thought the offset was related to the previous discussion of checking
> > for allocation failure.  Are you using it to implement alloc_fixed()?  If
> > so, please don't.  Besides the awkward implementation (what does it
> > logically have to do with gen_pool_first_fit_align?), it does not appear
> > to be correct -
> > - what happens with multiple chunks?  What happens if part of the region
> > the caller is trying to reserve is already taken?  Implement a proper
> > function to reserve a fixed genalloc region.
> > 
> The offset is which allocation block address need to be larger than, 
> Not equal to, it really like the parameter start of the algo(the bitnumber
> To start searching at).

cpm_muram_alloc_fixed() is not "search starting at this offset".  It is 
"reserve this exact range or fail".

-Scott
Zhao Qiang Sept. 10, 2015, 2:26 a.m. UTC | #12
On Wed, 2015-09-10 at 12:38AM -0500, Wood Scott-B07421 wrote:
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Thursday, September 10, 2015 12:38 AM

> To: Zhao Qiang-B45475

> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li

> Yang-Leo-R58472; paulus@samba.org

> Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> bytes-alignment to genalloc

> 

> On Sat, 2015-09-05 at 22:13 -0500, Zhao Qiang-B45475 wrote:

> > On Wed, 2015-09-02 at 10:18AM +0800, Wood Scott-B07421 wrote:

> > > -----Original Message-----

> > > From: Wood Scott-B07421

> > > Sent: Wednesday, September 02, 2015 10:18 AM

> > > To: Zhao Qiang-B45475

> > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;

> > > Li Yang-Leo-R58472; paulus@samba.org

> > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> > > bytes-alignment to genalloc

> > >

> > > On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:

> > > > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:

> > > > > -----Original Message-----

> > > > > From: Wood Scott-B07421

> > > > > Sent: Wednesday, September 02, 2015 8:30 AM

> > > > > To: Zhao Qiang-B45475

> > > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > > > lauraa@codeaurora.org; Xie Xiaobo-R63061;

> > > > > benh@kernel.crashing.org; Li Yang-Leo-R58472; paulus@samba.org

> > > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation

> > > > > with bytes-alignment to genalloc

> > > > >

> > > > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:

> > > > > > Bytes alignment is required to manage some special RAM, so add

> > > > > > gen_pool_first_fit_align to genalloc, meanwhile add

> > > > > > gen_pool_alloc_data to pass data to

> > > > > > gen_pool_first_fit_align(modify gen_pool_alloc as a wrapper)

> > > > > >

> > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>

> > > > > > ---

> > > > > > Changes for v6:

> > > > > >       - patches set v6 include a new patch because of using

> > > > > >       - genalloc to manage QE MURAM, patch 0001 is the new

> > > > > >       - patch, adding bytes alignment for allocation for use.

> > > > > > Changes for v7:

> > > > > >       - cpm muram also need to use genalloc to manage, it has

> > > > > >         a function to reserve a specific region of muram,

> > > > > >         add offset to genpool_data for start addr to be

> allocated.

> > > > >

> > > > > This seems to be describing more than just the changes in this

> patch.

> > > > > What does also handling cpm have to do with this patch?  Are you

> > > > > adding support for reserving a specific region in this patch?  I

> > > > > don't see it, and in any case it should go in a different patch.

> > > >

> > > > Yes, I added. The code below can support the function.

> > > >       offset_bit = (alignment->offset + (1UL << order) - 1) >>

> order;

> > > >       return bitmap_find_next_zero_area(map, size, start +

> > > > offset_bit,

> > > nr,

> > > >                         align_mask);

> > > >

> > > > CPM has an function cpm_muram_alloc_fixed, needing to allocate

> > > > muram from a Specific offset. So I add the code and add offset to

> struct data.

> > >

> > > I thought the offset was related to the previous discussion of

> > > checking for allocation failure.  Are you using it to implement

> > > alloc_fixed()?  If so, please don't.  Besides the awkward

> > > implementation (what does it logically have to do with

> > > gen_pool_first_fit_align?), it does not appear to be correct -

> > > - what happens with multiple chunks?  What happens if part of the

> > > region the caller is trying to reserve is already taken?  Implement

> > > a proper function to reserve a fixed genalloc region.

> > >

> > The offset is which allocation block address need to be larger than,

> > Not equal to, it really like the parameter start of the algo(the

> > bitnumber To start searching at).

> 

> cpm_muram_alloc_fixed() is not "search starting at this offset".  It is

> "reserve this exact range or fail".


Yes, you are right! How about to add a new algo into genalloc to search 
At offset, then handle it in muram layer, if the address return from genalloc
Is not equal to offset, return negative number?

> 

> -Scott
Scott Wood Sept. 10, 2015, 10:07 p.m. UTC | #13
On Wed, 2015-09-09 at 21:26 -0500, Zhao Qiang-B45475 wrote:
> On Wed, 2015-09-10 at 12:38AM -0500, Wood Scott-B07421 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Thursday, September 10, 2015 12:38 AM
> > To: Zhao Qiang-B45475
> > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li
> > Yang-Leo-R58472; paulus@samba.org
> > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > bytes-alignment to genalloc
> > 
> > On Sat, 2015-09-05 at 22:13 -0500, Zhao Qiang-B45475 wrote:
> > > On Wed, 2015-09-02 at 10:18AM +0800, Wood Scott-B07421 wrote:
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Wednesday, September 02, 2015 10:18 AM
> > > > To: Zhao Qiang-B45475
> > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;
> > > > Li Yang-Leo-R58472; paulus@samba.org
> > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > > > bytes-alignment to genalloc
> > > > 
> > > > On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:
> > > > > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:
> > > > > > -----Original Message-----
> > > > > > From: Wood Scott-B07421
> > > > > > Sent: Wednesday, September 02, 2015 8:30 AM
> > > > > > To: Zhao Qiang-B45475
> > > > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > > > lauraa@codeaurora.org; Xie Xiaobo-R63061;
> > > > > > benh@kernel.crashing.org; Li Yang-Leo-R58472; paulus@samba.org
> > > > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation
> > > > > > with bytes-alignment to genalloc
> > > > > > 
> > > > > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:
> > > > > > > Bytes alignment is required to manage some special RAM, so add
> > > > > > > gen_pool_first_fit_align to genalloc, meanwhile add
> > > > > > > gen_pool_alloc_data to pass data to
> > > > > > > gen_pool_first_fit_align(modify gen_pool_alloc as a wrapper)
> > > > > > > 
> > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>
> > > > > > > ---
> > > > > > > Changes for v6:
> > > > > > >       - patches set v6 include a new patch because of using
> > > > > > >       - genalloc to manage QE MURAM, patch 0001 is the new
> > > > > > >       - patch, adding bytes alignment for allocation for use.
> > > > > > > Changes for v7:
> > > > > > >       - cpm muram also need to use genalloc to manage, it has
> > > > > > >         a function to reserve a specific region of muram,
> > > > > > >         add offset to genpool_data for start addr to be
> > allocated.
> > > > > > 
> > > > > > This seems to be describing more than just the changes in this
> > patch.
> > > > > > What does also handling cpm have to do with this patch?  Are you
> > > > > > adding support for reserving a specific region in this patch?  I
> > > > > > don't see it, and in any case it should go in a different patch.
> > > > > 
> > > > > Yes, I added. The code below can support the function.
> > > > >       offset_bit = (alignment->offset + (1UL << order) - 1) >>
> > order;
> > > > >       return bitmap_find_next_zero_area(map, size, start +
> > > > > offset_bit,
> > > > nr,
> > > > >                         align_mask);
> > > > > 
> > > > > CPM has an function cpm_muram_alloc_fixed, needing to allocate
> > > > > muram from a Specific offset. So I add the code and add offset to
> > struct data.
> > > > 
> > > > I thought the offset was related to the previous discussion of
> > > > checking for allocation failure.  Are you using it to implement
> > > > alloc_fixed()?  If so, please don't.  Besides the awkward
> > > > implementation (what does it logically have to do with
> > > > gen_pool_first_fit_align?), it does not appear to be correct -
> > > > - what happens with multiple chunks?  What happens if part of the
> > > > region the caller is trying to reserve is already taken?  Implement
> > > > a proper function to reserve a fixed genalloc region.
> > > > 
> > > The offset is which allocation block address need to be larger than,
> > > Not equal to, it really like the parameter start of the algo(the
> > > bitnumber To start searching at).
> > 
> > cpm_muram_alloc_fixed() is not "search starting at this offset".  It is
> > "reserve this exact range or fail".
> 
> Yes, you are right! How about to add a new algo into genalloc to search 
> At offset, then handle it in muram layer, if the address return from 
> genalloc
> Is not equal to offset, return negative number?

If you're adding a new algorithm, why not make it actually do what you want 
rather than adding something different and fixing it up in the caller?

-Scott
Zhao Qiang Sept. 11, 2015, 2:09 a.m. UTC | #14
On Fri, 2015-09-11 at 06:07AM -0500, Wood Scott-B07421 wrote:
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Friday, September 11, 2015 6:07 AM

> To: Zhao Qiang-B45475

> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li

> Yang-Leo-R58472; paulus@samba.org

> Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> bytes-alignment to genalloc

> 

> On Wed, 2015-09-09 at 21:26 -0500, Zhao Qiang-B45475 wrote:

> > On Wed, 2015-09-10 at 12:38AM -0500, Wood Scott-B07421 wrote:

> > > -----Original Message-----

> > > From: Wood Scott-B07421

> > > Sent: Thursday, September 10, 2015 12:38 AM

> > > To: Zhao Qiang-B45475

> > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;

> > > Li Yang-Leo-R58472; paulus@samba.org

> > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> > > bytes-alignment to genalloc

> > >

> > > On Sat, 2015-09-05 at 22:13 -0500, Zhao Qiang-B45475 wrote:

> > > > On Wed, 2015-09-02 at 10:18AM +0800, Wood Scott-B07421 wrote:

> > > > > -----Original Message-----

> > > > > From: Wood Scott-B07421

> > > > > Sent: Wednesday, September 02, 2015 10:18 AM

> > > > > To: Zhao Qiang-B45475

> > > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > > > lauraa@codeaurora.org; Xie Xiaobo-R63061;

> > > > > benh@kernel.crashing.org; Li Yang-Leo-R58472; paulus@samba.org

> > > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation

> > > > > with bytes-alignment to genalloc

> > > > >

> > > > > On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:

> > > > > > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:

> > > > > > > -----Original Message-----

> > > > > > > From: Wood Scott-B07421

> > > > > > > Sent: Wednesday, September 02, 2015 8:30 AM

> > > > > > > To: Zhao Qiang-B45475

> > > > > > > Cc: linux-kernel@vger.kernel.org;

> > > > > > > linuxppc-dev@lists.ozlabs.org; lauraa@codeaurora.org; Xie

> > > > > > > Xiaobo-R63061; benh@kernel.crashing.org; Li Yang-Leo-R58472;

> > > > > > > paulus@samba.org

> > > > > > > Subject: Re: [PATCH V7 1/3] genalloc:support

> > > > > > > memory-allocation with bytes-alignment to genalloc

> > > > > > >

> > > > > > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:

> > > > > > > > Bytes alignment is required to manage some special RAM, so

> > > > > > > > add gen_pool_first_fit_align to genalloc, meanwhile add

> > > > > > > > gen_pool_alloc_data to pass data to

> > > > > > > > gen_pool_first_fit_align(modify gen_pool_alloc as a

> > > > > > > > wrapper)

> > > > > > > >

> > > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>

> > > > > > > > ---

> > > > > > > > Changes for v6:

> > > > > > > >       - patches set v6 include a new patch because of using

> > > > > > > >       - genalloc to manage QE MURAM, patch 0001 is the new

> > > > > > > >       - patch, adding bytes alignment for allocation for

> use.

> > > > > > > > Changes for v7:

> > > > > > > >       - cpm muram also need to use genalloc to manage, it

> has

> > > > > > > >         a function to reserve a specific region of muram,

> > > > > > > >         add offset to genpool_data for start addr to be

> > > allocated.

> > > > > > >

> > > > > > > This seems to be describing more than just the changes in

> > > > > > > this

> > > patch.

> > > > > > > What does also handling cpm have to do with this patch?  Are

> > > > > > > you adding support for reserving a specific region in this

> > > > > > > patch?  I don't see it, and in any case it should go in a

> different patch.

> > > > > >

> > > > > > Yes, I added. The code below can support the function.

> > > > > >       offset_bit = (alignment->offset + (1UL << order) - 1) >>

> > > order;

> > > > > >       return bitmap_find_next_zero_area(map, size, start +

> > > > > > offset_bit,

> > > > > nr,

> > > > > >                         align_mask);

> > > > > >

> > > > > > CPM has an function cpm_muram_alloc_fixed, needing to allocate

> > > > > > muram from a Specific offset. So I add the code and add offset

> > > > > > to

> > > struct data.

> > > > >

> > > > > I thought the offset was related to the previous discussion of

> > > > > checking for allocation failure.  Are you using it to implement

> > > > > alloc_fixed()?  If so, please don't.  Besides the awkward

> > > > > implementation (what does it logically have to do with

> > > > > gen_pool_first_fit_align?), it does not appear to be correct -

> > > > > - what happens with multiple chunks?  What happens if part of

> > > > > the region the caller is trying to reserve is already taken?

> > > > > Implement a proper function to reserve a fixed genalloc region.

> > > > >

> > > > The offset is which allocation block address need to be larger

> > > > than, Not equal to, it really like the parameter start of the

> > > > algo(the bitnumber To start searching at).

> > >

> > > cpm_muram_alloc_fixed() is not "search starting at this offset".  It

> > > is "reserve this exact range or fail".

> >

> > Yes, you are right! How about to add a new algo into genalloc to

> > search At offset, then handle it in muram layer, if the address return

> > from genalloc Is not equal to offset, return negative number?

> 

> If you're adding a new algorithm, why not make it actually do what you

> want rather than adding something different and fixing it up in the

> caller?


Because I'm not sure whether it is proper to add a "offset searching at" algo. 

> 

> -Scott
Scott Wood Sept. 11, 2015, 2:15 a.m. UTC | #15
On Thu, 2015-09-10 at 21:09 -0500, Zhao Qiang-B45475 wrote:
> On Fri, 2015-09-11 at 06:07AM -0500, Wood Scott-B07421 wrote:
> > -----Original Message-----
> > From: Wood Scott-B07421
> > Sent: Friday, September 11, 2015 6:07 AM
> > To: Zhao Qiang-B45475
> > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li
> > Yang-Leo-R58472; paulus@samba.org
> > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > bytes-alignment to genalloc
> > 
> > On Wed, 2015-09-09 at 21:26 -0500, Zhao Qiang-B45475 wrote:
> > > On Wed, 2015-09-10 at 12:38AM -0500, Wood Scott-B07421 wrote:
> > > > -----Original Message-----
> > > > From: Wood Scott-B07421
> > > > Sent: Thursday, September 10, 2015 12:38 AM
> > > > To: Zhao Qiang-B45475
> > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;
> > > > Li Yang-Leo-R58472; paulus@samba.org
> > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with
> > > > bytes-alignment to genalloc
> > > > 
> > > > On Sat, 2015-09-05 at 22:13 -0500, Zhao Qiang-B45475 wrote:
> > > > > On Wed, 2015-09-02 at 10:18AM +0800, Wood Scott-B07421 wrote:
> > > > > > -----Original Message-----
> > > > > > From: Wood Scott-B07421
> > > > > > Sent: Wednesday, September 02, 2015 10:18 AM
> > > > > > To: Zhao Qiang-B45475
> > > > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;
> > > > > > lauraa@codeaurora.org; Xie Xiaobo-R63061;
> > > > > > benh@kernel.crashing.org; Li Yang-Leo-R58472; paulus@samba.org
> > > > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation
> > > > > > with bytes-alignment to genalloc
> > > > > > 
> > > > > > On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:
> > > > > > > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421 wrote:
> > > > > > > > -----Original Message-----
> > > > > > > > From: Wood Scott-B07421
> > > > > > > > Sent: Wednesday, September 02, 2015 8:30 AM
> > > > > > > > To: Zhao Qiang-B45475
> > > > > > > > Cc: linux-kernel@vger.kernel.org;
> > > > > > > > linuxppc-dev@lists.ozlabs.org; lauraa@codeaurora.org; Xie
> > > > > > > > Xiaobo-R63061; benh@kernel.crashing.org; Li Yang-Leo-R58472;
> > > > > > > > paulus@samba.org
> > > > > > > > Subject: Re: [PATCH V7 1/3] genalloc:support
> > > > > > > > memory-allocation with bytes-alignment to genalloc
> > > > > > > > 
> > > > > > > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:
> > > > > > > > > Bytes alignment is required to manage some special RAM, so
> > > > > > > > > add gen_pool_first_fit_align to genalloc, meanwhile add
> > > > > > > > > gen_pool_alloc_data to pass data to
> > > > > > > > > gen_pool_first_fit_align(modify gen_pool_alloc as a
> > > > > > > > > wrapper)
> > > > > > > > > 
> > > > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>
> > > > > > > > > ---
> > > > > > > > > Changes for v6:
> > > > > > > > >       - patches set v6 include a new patch because of using
> > > > > > > > >       - genalloc to manage QE MURAM, patch 0001 is the new
> > > > > > > > >       - patch, adding bytes alignment for allocation for
> > use.
> > > > > > > > > Changes for v7:
> > > > > > > > >       - cpm muram also need to use genalloc to manage, it
> > has
> > > > > > > > >         a function to reserve a specific region of muram,
> > > > > > > > >         add offset to genpool_data for start addr to be
> > > > allocated.
> > > > > > > > 
> > > > > > > > This seems to be describing more than just the changes in
> > > > > > > > this
> > > > patch.
> > > > > > > > What does also handling cpm have to do with this patch?  Are
> > > > > > > > you adding support for reserving a specific region in this
> > > > > > > > patch?  I don't see it, and in any case it should go in a
> > different patch.
> > > > > > > 
> > > > > > > Yes, I added. The code below can support the function.
> > > > > > >       offset_bit = (alignment->offset + (1UL << order) - 1) >>
> > > > order;
> > > > > > >       return bitmap_find_next_zero_area(map, size, start +
> > > > > > > offset_bit,
> > > > > > nr,
> > > > > > >                         align_mask);
> > > > > > > 
> > > > > > > CPM has an function cpm_muram_alloc_fixed, needing to allocate
> > > > > > > muram from a Specific offset. So I add the code and add offset
> > > > > > > to
> > > > struct data.
> > > > > > 
> > > > > > I thought the offset was related to the previous discussion of
> > > > > > checking for allocation failure.  Are you using it to implement
> > > > > > alloc_fixed()?  If so, please don't.  Besides the awkward
> > > > > > implementation (what does it logically have to do with
> > > > > > gen_pool_first_fit_align?), it does not appear to be correct -
> > > > > > - what happens with multiple chunks?  What happens if part of
> > > > > > the region the caller is trying to reserve is already taken?
> > > > > > Implement a proper function to reserve a fixed genalloc region.
> > > > > > 
> > > > > The offset is which allocation block address need to be larger
> > > > > than, Not equal to, it really like the parameter start of the
> > > > > algo(the bitnumber To start searching at).
> > > > 
> > > > cpm_muram_alloc_fixed() is not "search starting at this offset".  It
> > > > is "reserve this exact range or fail".
> > > 
> > > Yes, you are right! How about to add a new algo into genalloc to
> > > search At offset, then handle it in muram layer, if the address return
> > > from genalloc Is not equal to offset, return negative number?
> > 
> > If you're adding a new algorithm, why not make it actually do what you
> > want rather than adding something different and fixing it up in the
> > caller?
> 
> Because I'm not sure whether it is proper to add a "offset searching at" 
> algo. 

Again, "offset searching at" is not what we want anyway.  What we want is 
"allocate this range or fail".  Why would it be improper to have such an 
algorithm?

-Scott
Zhao Qiang Sept. 11, 2015, 2:25 a.m. UTC | #16
On Fri, 2015-09-11 at 10:15AM -0500, Wood Scott-B07421 wrote:
> -----Original Message-----

> From: Wood Scott-B07421

> Sent: Friday, September 11, 2015 10:15 AM

> To: Zhao Qiang-B45475

> Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org; Li

> Yang-Leo-R58472; paulus@samba.org

> Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> bytes-alignment to genalloc

> 

> On Thu, 2015-09-10 at 21:09 -0500, Zhao Qiang-B45475 wrote:

> > On Fri, 2015-09-11 at 06:07AM -0500, Wood Scott-B07421 wrote:

> > > -----Original Message-----

> > > From: Wood Scott-B07421

> > > Sent: Friday, September 11, 2015 6:07 AM

> > > To: Zhao Qiang-B45475

> > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > lauraa@codeaurora.org; Xie Xiaobo-R63061; benh@kernel.crashing.org;

> > > Li Yang-Leo-R58472; paulus@samba.org

> > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation with

> > > bytes-alignment to genalloc

> > >

> > > On Wed, 2015-09-09 at 21:26 -0500, Zhao Qiang-B45475 wrote:

> > > > On Wed, 2015-09-10 at 12:38AM -0500, Wood Scott-B07421 wrote:

> > > > > -----Original Message-----

> > > > > From: Wood Scott-B07421

> > > > > Sent: Thursday, September 10, 2015 12:38 AM

> > > > > To: Zhao Qiang-B45475

> > > > > Cc: linux-kernel@vger.kernel.org; linuxppc-dev@lists.ozlabs.org;

> > > > > lauraa@codeaurora.org; Xie Xiaobo-R63061;

> > > > > benh@kernel.crashing.org; Li Yang-Leo-R58472; paulus@samba.org

> > > > > Subject: Re: [PATCH V7 1/3] genalloc:support memory-allocation

> > > > > with bytes-alignment to genalloc

> > > > >

> > > > > On Sat, 2015-09-05 at 22:13 -0500, Zhao Qiang-B45475 wrote:

> > > > > > On Wed, 2015-09-02 at 10:18AM +0800, Wood Scott-B07421 wrote:

> > > > > > > -----Original Message-----

> > > > > > > From: Wood Scott-B07421

> > > > > > > Sent: Wednesday, September 02, 2015 10:18 AM

> > > > > > > To: Zhao Qiang-B45475

> > > > > > > Cc: linux-kernel@vger.kernel.org;

> > > > > > > linuxppc-dev@lists.ozlabs.org; lauraa@codeaurora.org; Xie

> > > > > > > Xiaobo-R63061; benh@kernel.crashing.org; Li Yang-Leo-R58472;

> > > > > > > paulus@samba.org

> > > > > > > Subject: Re: [PATCH V7 1/3] genalloc:support

> > > > > > > memory-allocation with bytes-alignment to genalloc

> > > > > > >

> > > > > > > On Tue, 2015-09-01 at 21:10 -0500, Zhao Qiang-B45475 wrote:

> > > > > > > > On Wed, 2015-09-02 at 08:38AM +0800, Wood Scott-B07421

> wrote:

> > > > > > > > > -----Original Message-----

> > > > > > > > > From: Wood Scott-B07421

> > > > > > > > > Sent: Wednesday, September 02, 2015 8:30 AM

> > > > > > > > > To: Zhao Qiang-B45475

> > > > > > > > > Cc: linux-kernel@vger.kernel.org;

> > > > > > > > > linuxppc-dev@lists.ozlabs.org; lauraa@codeaurora.org;

> > > > > > > > > Xie Xiaobo-R63061; benh@kernel.crashing.org; Li

> > > > > > > > > Yang-Leo-R58472; paulus@samba.org

> > > > > > > > > Subject: Re: [PATCH V7 1/3] genalloc:support

> > > > > > > > > memory-allocation with bytes-alignment to genalloc

> > > > > > > > >

> > > > > > > > > On Mon, 2015-08-31 at 16:58 +0800, Zhao Qiang wrote:

> > > > > > > > > > Bytes alignment is required to manage some special

> > > > > > > > > > RAM, so add gen_pool_first_fit_align to genalloc,

> > > > > > > > > > meanwhile add gen_pool_alloc_data to pass data to

> > > > > > > > > > gen_pool_first_fit_align(modify gen_pool_alloc as a

> > > > > > > > > > wrapper)

> > > > > > > > > >

> > > > > > > > > > Signed-off-by: Zhao Qiang <qiang.zhao@freescale.com>

> > > > > > > > > > ---

> > > > > > > > > > Changes for v6:

> > > > > > > > > >       - patches set v6 include a new patch because of

> using

> > > > > > > > > >       - genalloc to manage QE MURAM, patch 0001 is the

> new

> > > > > > > > > >       - patch, adding bytes alignment for allocation

> > > > > > > > > > for

> > > use.

> > > > > > > > > > Changes for v7:

> > > > > > > > > >       - cpm muram also need to use genalloc to manage,

> > > > > > > > > > it

> > > has

> > > > > > > > > >         a function to reserve a specific region of

> muram,

> > > > > > > > > >         add offset to genpool_data for start addr to

> > > > > > > > > > be

> > > > > allocated.

> > > > > > > > >

> > > > > > > > > This seems to be describing more than just the changes

> > > > > > > > > in this

> > > > > patch.

> > > > > > > > > What does also handling cpm have to do with this patch?

> > > > > > > > > Are you adding support for reserving a specific region

> > > > > > > > > in this patch?  I don't see it, and in any case it

> > > > > > > > > should go in a

> > > different patch.

> > > > > > > >

> > > > > > > > Yes, I added. The code below can support the function.

> > > > > > > >       offset_bit = (alignment->offset + (1UL << order) -

> > > > > > > > 1) >>

> > > > > order;

> > > > > > > >       return bitmap_find_next_zero_area(map, size, start +

> > > > > > > > offset_bit,

> > > > > > > nr,

> > > > > > > >                         align_mask);

> > > > > > > >

> > > > > > > > CPM has an function cpm_muram_alloc_fixed, needing to

> > > > > > > > allocate muram from a Specific offset. So I add the code

> > > > > > > > and add offset to

> > > > > struct data.

> > > > > > >

> > > > > > > I thought the offset was related to the previous discussion

> > > > > > > of checking for allocation failure.  Are you using it to

> > > > > > > implement alloc_fixed()?  If so, please don't.  Besides the

> > > > > > > awkward implementation (what does it logically have to do

> > > > > > > with gen_pool_first_fit_align?), it does not appear to be

> > > > > > > correct -

> > > > > > > - what happens with multiple chunks?  What happens if part

> > > > > > > of the region the caller is trying to reserve is already

> taken?

> > > > > > > Implement a proper function to reserve a fixed genalloc

> region.

> > > > > > >

> > > > > > The offset is which allocation block address need to be larger

> > > > > > than, Not equal to, it really like the parameter start of the

> > > > > > algo(the bitnumber To start searching at).

> > > > >

> > > > > cpm_muram_alloc_fixed() is not "search starting at this offset".

> > > > > It is "reserve this exact range or fail".

> > > >

> > > > Yes, you are right! How about to add a new algo into genalloc to

> > > > search At offset, then handle it in muram layer, if the address

> > > > return from genalloc Is not equal to offset, return negative number?

> > >

> > > If you're adding a new algorithm, why not make it actually do what

> > > you want rather than adding something different and fixing it up in

> > > the caller?

> >

> > Because I'm not sure whether it is proper to add a "offset searching

> at"

> > algo.

> 

> Again, "offset searching at" is not what we want anyway.  What we want is

> "allocate this range or fail".  Why would it be improper to have such an

> algorithm?


Yes, I mean "allocate this range or fail", just type wrong.
I will push another version and modify it.

> 

> -Scott
diff mbox

Patch

diff --git a/include/linux/genalloc.h b/include/linux/genalloc.h
index 1ccaab4..1958053 100644
--- a/include/linux/genalloc.h
+++ b/include/linux/genalloc.h
@@ -30,10 +30,12 @@ 
 #ifndef __GENALLOC_H__
 #define __GENALLOC_H__
 
+#include <linux/types.h>
 #include <linux/spinlock_types.h>
 
 struct device;
 struct device_node;
+struct gen_pool;
 
 /**
  * Allocation callback function type definition
@@ -47,7 +49,7 @@  typedef unsigned long (*genpool_algo_t)(unsigned long *map,
 			unsigned long size,
 			unsigned long start,
 			unsigned int nr,
-			void *data);
+			void *data, struct gen_pool *pool);
 
 /*
  *  General purpose special memory pool descriptor.
@@ -73,6 +75,14 @@  struct gen_pool_chunk {
 	unsigned long bits[0];		/* bitmap for allocating memory chunk */
 };
 
+/*
+ *  gen_pool data descriptor for gen_pool_first_fit_align.
+ */
+struct genpool_data_align {
+	int align;		/* alignment by bytes for starting address */
+	unsigned long offset;	/* the offset of allocation start addr*/
+};
+
 extern struct gen_pool *gen_pool_create(int, int);
 extern phys_addr_t gen_pool_virt_to_phys(struct gen_pool *pool, unsigned long);
 extern int gen_pool_add_virt(struct gen_pool *, unsigned long, phys_addr_t,
@@ -96,6 +106,7 @@  static inline int gen_pool_add(struct gen_pool *pool, unsigned long addr,
 }
 extern void gen_pool_destroy(struct gen_pool *);
 extern unsigned long gen_pool_alloc(struct gen_pool *, size_t);
+extern unsigned long gen_pool_alloc_data(struct gen_pool *, size_t, void *data);
 extern void *gen_pool_dma_alloc(struct gen_pool *pool, size_t size,
 		dma_addr_t *dma);
 extern void gen_pool_free(struct gen_pool *, unsigned long, size_t);
@@ -108,14 +119,20 @@  extern void gen_pool_set_algo(struct gen_pool *pool, genpool_algo_t algo,
 		void *data);
 
 extern unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
-		unsigned long start, unsigned int nr, void *data);
+		unsigned long start, unsigned int nr, void *data,
+		struct gen_pool *pool);
+
+extern unsigned long gen_pool_first_fit_align(unsigned long *map,
+		unsigned long size, unsigned long start, unsigned int nr,
+		void *data, struct gen_pool *pool);
 
 extern unsigned long gen_pool_first_fit_order_align(unsigned long *map,
 		unsigned long size, unsigned long start, unsigned int nr,
-		void *data);
+		void *data, struct gen_pool *pool);
 
 extern unsigned long gen_pool_best_fit(unsigned long *map, unsigned long size,
-		unsigned long start, unsigned int nr, void *data);
+		unsigned long start, unsigned int nr, void *data,
+		struct gen_pool *pool);
 
 extern struct gen_pool *devm_gen_pool_create(struct device *dev,
 		int min_alloc_order, int nid);
diff --git a/lib/genalloc.c b/lib/genalloc.c
index d214866..b9f8344 100644
--- a/lib/genalloc.c
+++ b/lib/genalloc.c
@@ -269,6 +269,24 @@  EXPORT_SYMBOL(gen_pool_destroy);
  */
 unsigned long gen_pool_alloc(struct gen_pool *pool, size_t size)
 {
+	return gen_pool_alloc_data(pool, size, pool->data);
+}
+EXPORT_SYMBOL(gen_pool_alloc);
+
+/**
+ * gen_pool_alloc_data - allocate special memory from the pool
+ * @pool: pool to allocate from
+ * @size: number of bytes to allocate from the pool
+ * @data: data passed to algorithm
+ *
+ * Allocate the requested number of bytes from the specified pool.
+ * Uses the pool allocation function (with first-fit algorithm by default).
+ * Can not be used in NMI handler on architectures without
+ * NMI-safe cmpxchg implementation.
+ */
+unsigned long gen_pool_alloc_data(struct gen_pool *pool, size_t size,
+		void *data)
+{
 	struct gen_pool_chunk *chunk;
 	unsigned long addr = 0;
 	int order = pool->min_alloc_order;
@@ -290,7 +308,7 @@  unsigned long gen_pool_alloc(struct gen_pool *pool, size_t size)
 		end_bit = chunk_size(chunk) >> order;
 retry:
 		start_bit = pool->algo(chunk->bits, end_bit, start_bit, nbits,
-				pool->data);
+				data, pool);
 		if (start_bit >= end_bit)
 			continue;
 		remain = bitmap_set_ll(chunk->bits, start_bit, nbits);
@@ -309,7 +327,7 @@  retry:
 	rcu_read_unlock();
 	return addr;
 }
-EXPORT_SYMBOL(gen_pool_alloc);
+EXPORT_SYMBOL(gen_pool_alloc_data);
 
 /**
  * gen_pool_dma_alloc - allocate special memory from the pool for DMA usage
@@ -500,15 +518,45 @@  EXPORT_SYMBOL(gen_pool_set_algo);
  * @start: The bitnumber to start searching at
  * @nr: The number of zeroed bits we're looking for
  * @data: additional data - unused
+ * @pool: pool to find the fit region memory from
  */
 unsigned long gen_pool_first_fit(unsigned long *map, unsigned long size,
-		unsigned long start, unsigned int nr, void *data)
+		unsigned long start, unsigned int nr, void *data,
+		struct gen_pool *pool)
 {
 	return bitmap_find_next_zero_area(map, size, start, nr, 0);
 }
 EXPORT_SYMBOL(gen_pool_first_fit);
 
 /**
+ * gen_pool_first_fit_align - find the first available region
+ * of memory matching the size requirement (alignment constraint)
+ * @map: The address to base the search on
+ * @size: The bitmap size in bits
+ * @start: The bitnumber to start searching at
+ * @nr: The number of zeroed bits we're looking for
+ * @data: data for alignment
+ * @pool: pool to get order from
+ */
+unsigned long gen_pool_first_fit_align(unsigned long *map, unsigned long size,
+		unsigned long start, unsigned int nr, void *data,
+		struct gen_pool *pool)
+{
+	struct genpool_data_align *alignment;
+	unsigned long align_mask;
+	unsigned long offset_bit;
+	int order;
+
+	alignment = data;
+	order = pool->min_alloc_order;
+	align_mask = ((alignment->align + (1UL << order) - 1) >> order) - 1;
+	offset_bit = (alignment->offset + (1UL << order) - 1) >> order;
+	return bitmap_find_next_zero_area(map, size, start + offset_bit, nr,
+			align_mask);
+}
+EXPORT_SYMBOL(gen_pool_first_fit_align);
+
+/**
  * gen_pool_first_fit_order_align - find the first available region
  * of memory matching the size requirement. The region will be aligned
  * to the order of the size specified.
@@ -517,10 +565,11 @@  EXPORT_SYMBOL(gen_pool_first_fit);
  * @start: The bitnumber to start searching at
  * @nr: The number of zeroed bits we're looking for
  * @data: additional data - unused
+ * @pool: pool to find the fit region memory from
  */
 unsigned long gen_pool_first_fit_order_align(unsigned long *map,
 		unsigned long size, unsigned long start,
-		unsigned int nr, void *data)
+		unsigned int nr, void *data, struct gen_pool *pool)
 {
 	unsigned long align_mask = roundup_pow_of_two(nr) - 1;
 
@@ -536,12 +585,14 @@  EXPORT_SYMBOL(gen_pool_first_fit_order_align);
  * @start: The bitnumber to start searching at
  * @nr: The number of zeroed bits we're looking for
  * @data: additional data - unused
+ * @pool: pool to find the fit region memory from
  *
  * Iterate over the bitmap to find the smallest free region
  * which we can allocate the memory.
  */
 unsigned long gen_pool_best_fit(unsigned long *map, unsigned long size,
-		unsigned long start, unsigned int nr, void *data)
+		unsigned long start, unsigned int nr, void *data,
+		struct gen_pool *pool)
 {
 	unsigned long start_bit = size;
 	unsigned long len = size + 1;