[PATCH] powerpc:Fix rheap alignment problem

[PATCH] powerpc:Fix rheap alignment problem

[Li Yang-r58472]

Honour alignment parameter in the rheap allocator.
Remove compile warning.

Signed-off-by: Pantelis Antoniou <pantelis@embeddedalley.com>
Signed-off-by: Li Yang <leoli@freescale.com>

---
 arch/powerpc/lib/Makefile |    1 +
 arch/powerpc/lib/rheap.c  |   24 ++++++++++++++++++++----
 include/asm-ppc/rheap.h   |    4 ++++
 3 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
index 34f5c2e..136a892 100644
--- a/arch/powerpc/lib/Makefile
+++ b/arch/powerpc/lib/Makefile
@@ -11,6 +11,7 @@ obj-y			+= bitops.o
 obj-$(CONFIG_PPC64)	+= checksum_64.o copypage_64.o copyuser_64.o \
 			   memcpy_64.o usercopy_64.o mem_64.o string.o \
 			   strcase.o
+obj-$(CONFIG_QUICC_ENGINE) += rheap.o
 obj-$(CONFIG_PPC_ISERIES) += e2a.o
 obj-$(CONFIG_XMON)	+= sstep.o
 
diff --git a/arch/powerpc/lib/rheap.c b/arch/powerpc/lib/rheap.c
index 31e5118..57bf991 100644
--- a/arch/powerpc/lib/rheap.c
+++ b/arch/powerpc/lib/rheap.c
@@ -423,17 +423,21 @@ void *rh_detach_region(rh_info_t * info,
 	return (void *)s;
 }
 
-void *rh_alloc(rh_info_t * info, int size, const char *owner)
+void *rh_alloc_align(rh_info_t * info, int size, int alignment, const char *owner)
 {
 	struct list_head *l;
 	rh_block_t *blk;
 	rh_block_t *newblk;
 	void *start;
 
-	/* Validate size */
-	if (size <= 0)
+	/* Validate size, (must be power of two) */
+	if (size <= 0 || (alignment & (alignment - 1)) != 0)
 		return ERR_PTR(-EINVAL);
 
+	/* given alignment larger that default rheap alignment */
+	if (alignment > info->alignment)
+		size += alignment - 1;
+
 	/* Align to configured alignment */
 	size = (size + (info->alignment - 1)) & ~(info->alignment - 1);
 
@@ -476,15 +480,27 @@ void *rh_alloc(rh_info_t * info, int siz
 
 	attach_taken_block(info, newblk);
 
+	/* for larger alignment return fixed up pointer  */
+	/* this is no problem with the deallocator since */
+	/* we scan for pointers that lie in the blocks   */
+	if (alignment > info->alignment)
+		start = (void *)(((unsigned long)start + alignment - 1) &
+				~(alignment - 1));
+
 	return start;
 }
 
+void *rh_alloc(rh_info_t * info, int size, const char *owner)
+{
+	return rh_alloc_align(info, size, info->alignment, owner);
+}
+
 /* allocate at precisely the given address */
 void *rh_alloc_fixed(rh_info_t * info, void *start, int size, const char *owner)
 {
 	struct list_head *l;
 	rh_block_t *blk, *newblk1, *newblk2;
-	unsigned long s, e, m, bs, be;
+	unsigned long s, e, m, bs = 0, be = 0;
 
 	/* Validate size */
 	if (size <= 0)
diff --git a/include/asm-ppc/rheap.h b/include/asm-ppc/rheap.h
index e6ca1f6..65b9322 100644
--- a/include/asm-ppc/rheap.h
+++ b/include/asm-ppc/rheap.h
@@ -62,6 +62,10 @@ extern int rh_attach_region(rh_info_t * 
 /* Detach a free region */
 extern void *rh_detach_region(rh_info_t * info, void *start, int size);
 
+/* Allocate the given size from the remote heap (with alignment) */
+extern void *rh_alloc_align(rh_info_t * info, int size, int alignment,
+		const char *owner);
+
 /* Allocate the given size from the remote heap */
 extern void *rh_alloc(rh_info_t * info, int size, const char *owner);

--
Leo Li
Freescale Semiconductor

LeoLi@freescale.com 

Re: [PATCH] powerpc:Fix rheap alignment problem

[Benjamin Herrenschmidt]

On Fri, 2006-06-30 at 21:02 +0800, Li Yang-r58472 wrote:
> Honour alignment parameter in the rheap allocator.
> Remove compile warning.

What is this used for ? This rheap allocator ? I see no user in
arch/powerpc at least and only two users apparently in arch/ppc... are
we sure we need something that complex for these ? Can't we just use a
running bitmap allocator or an idr ?

Cheers,
Ben.

> Signed-off-by: Pantelis Antoniou <pantelis@embeddedalley.com>
> Signed-off-by: Li Yang <leoli@freescale.com>
> 
> ---
>  arch/powerpc/lib/Makefile |    1 +
>  arch/powerpc/lib/rheap.c  |   24 ++++++++++++++++++++----
>  include/asm-ppc/rheap.h   |    4 ++++
>  3 files changed, 25 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/lib/Makefile b/arch/powerpc/lib/Makefile
> index 34f5c2e..136a892 100644
> --- a/arch/powerpc/lib/Makefile
> +++ b/arch/powerpc/lib/Makefile
> @@ -11,6 +11,7 @@ obj-y			+= bitops.o
>  obj-$(CONFIG_PPC64)	+= checksum_64.o copypage_64.o copyuser_64.o \
>  			   memcpy_64.o usercopy_64.o mem_64.o string.o \
>  			   strcase.o
> +obj-$(CONFIG_QUICC_ENGINE) += rheap.o
>  obj-$(CONFIG_PPC_ISERIES) += e2a.o
>  obj-$(CONFIG_XMON)	+= sstep.o
>  
> diff --git a/arch/powerpc/lib/rheap.c b/arch/powerpc/lib/rheap.c
> index 31e5118..57bf991 100644
> --- a/arch/powerpc/lib/rheap.c
> +++ b/arch/powerpc/lib/rheap.c
> @@ -423,17 +423,21 @@ void *rh_detach_region(rh_info_t * info,
>  	return (void *)s;
>  }
>  
> -void *rh_alloc(rh_info_t * info, int size, const char *owner)
> +void *rh_alloc_align(rh_info_t * info, int size, int alignment, const char *owner)
>  {
>  	struct list_head *l;
>  	rh_block_t *blk;
>  	rh_block_t *newblk;
>  	void *start;
>  
> -	/* Validate size */
> -	if (size <= 0)
> +	/* Validate size, (must be power of two) */
> +	if (size <= 0 || (alignment & (alignment - 1)) != 0)
>  		return ERR_PTR(-EINVAL);
>  
> +	/* given alignment larger that default rheap alignment */
> +	if (alignment > info->alignment)
> +		size += alignment - 1;
> +
>  	/* Align to configured alignment */
>  	size = (size + (info->alignment - 1)) & ~(info->alignment - 1);
>  
> @@ -476,15 +480,27 @@ void *rh_alloc(rh_info_t * info, int siz
>  
>  	attach_taken_block(info, newblk);
>  
> +	/* for larger alignment return fixed up pointer  */
> +	/* this is no problem with the deallocator since */
> +	/* we scan for pointers that lie in the blocks   */
> +	if (alignment > info->alignment)
> +		start = (void *)(((unsigned long)start + alignment - 1) &
> +				~(alignment - 1));
> +
>  	return start;
>  }
>  
> +void *rh_alloc(rh_info_t * info, int size, const char *owner)
> +{
> +	return rh_alloc_align(info, size, info->alignment, owner);
> +}
> +
>  /* allocate at precisely the given address */
>  void *rh_alloc_fixed(rh_info_t * info, void *start, int size, const char *owner)
>  {
>  	struct list_head *l;
>  	rh_block_t *blk, *newblk1, *newblk2;
> -	unsigned long s, e, m, bs, be;
> +	unsigned long s, e, m, bs = 0, be = 0;
>  
>  	/* Validate size */
>  	if (size <= 0)
> diff --git a/include/asm-ppc/rheap.h b/include/asm-ppc/rheap.h
> index e6ca1f6..65b9322 100644
> --- a/include/asm-ppc/rheap.h
> +++ b/include/asm-ppc/rheap.h
> @@ -62,6 +62,10 @@ extern int rh_attach_region(rh_info_t * 
>  /* Detach a free region */
>  extern void *rh_detach_region(rh_info_t * info, void *start, int size);
>  
> +/* Allocate the given size from the remote heap (with alignment) */
> +extern void *rh_alloc_align(rh_info_t * info, int size, int alignment,
> +		const char *owner);
> +
>  /* Allocate the given size from the remote heap */
>  extern void *rh_alloc(rh_info_t * info, int size, const char *owner);
> 
> --
> Leo Li
> Freescale Semiconductor
> 
> LeoLi@freescale.com 
> 
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

Re: [PATCH] powerpc:Fix rheap alignment problem

[Benjamin Herrenschmidt]

On Sat, 2006-07-01 at 14:35 +0800, Linux powerpc wrote:
> Yes, it was used for allocating dual port RAM for CPM.  And now we are
> adding QE support to powerpc arch which need to use rheap(QE is next
> generation for CPM).  Please see the patches I <leoli@freescale.com>
> just posted for 8360epb support.  Moreover, previous CPM support is
> adding to powerpc arch too. 

Ok, well, I don't have anything specifically against that code, I was
just wondering if it may not duplicate something we already have (yet
another space allocator basically)... 

Ben.

Re: [PATCH] powerpc:Fix rheap alignment problem

[Christoph Hellwig]

On Sat, Jul 01, 2006 at 05:21:06PM +1000, Benjamin Herrenschmidt wrote:
> On Sat, 2006-07-01 at 14:35 +0800, Linux powerpc wrote:
> > Yes, it was used for allocating dual port RAM for CPM.  And now we are
> > adding QE support to powerpc arch which need to use rheap(QE is next
> > generation for CPM).  Please see the patches I <leoli@freescale.com>
> > just posted for 8360epb support.  Moreover, previous CPM support is
> > adding to powerpc arch too. 
> 
> Ok, well, I don't have anything specifically against that code, I was
> just wondering if it may not duplicate something we already have (yet
> another space allocator basically)... 

Yepp.  Without looking at the rheap allocator in deatail, any reason
it can't use lib/genalloc.c?

Re: [PATCH] powerpc:Fix rheap alignment problem

[Kumar Gala]

On Jul 1, 2006, at 5:25 AM, Christoph Hellwig wrote:

> On Sat, Jul 01, 2006 at 05:21:06PM +1000, Benjamin Herrenschmidt  
> wrote:
>> On Sat, 2006-07-01 at 14:35 +0800, Linux powerpc wrote:
>>> Yes, it was used for allocating dual port RAM for CPM.  And now  
>>> we are
>>> adding QE support to powerpc arch which need to use rheap(QE is next
>>> generation for CPM).  Please see the patches I <leoli@freescale.com>
>>> just posted for 8360epb support.  Moreover, previous CPM support is
>>> adding to powerpc arch too.
>>
>> Ok, well, I don't have anything specifically against that code, I was
>> just wondering if it may not duplicate something we already have (yet
>> another space allocator basically)...
>
> Yepp.  Without looking at the rheap allocator in deatail, any reason
> it can't use lib/genalloc.c?

Doing a quick glance at lib/genalloc.c I dont see any reason we  
couldn't use it.  However, Panto will know best, since he wrote rheap.

- k

Re: [PATCH] powerpc:Fix rheap alignment problem

[Pantelis Antoniou]

On Saturday 01 July 2006 17:34, Kumar Gala wrote:
> 
> On Jul 1, 2006, at 5:25 AM, Christoph Hellwig wrote:
> 
> > On Sat, Jul 01, 2006 at 05:21:06PM +1000, Benjamin Herrenschmidt  
> > wrote:
> >> On Sat, 2006-07-01 at 14:35 +0800, Linux powerpc wrote:
> >>> Yes, it was used for allocating dual port RAM for CPM.  And now  
> >>> we are
> >>> adding QE support to powerpc arch which need to use rheap(QE is next
> >>> generation for CPM).  Please see the patches I <leoli@freescale.com>
> >>> just posted for 8360epb support.  Moreover, previous CPM support is
> >>> adding to powerpc arch too.
> >>
> >> Ok, well, I don't have anything specifically against that code, I was
> >> just wondering if it may not duplicate something we already have (yet
> >> another space allocator basically)...
> >
> > Yepp.  Without looking at the rheap allocator in deatail, any reason
> > it can't use lib/genalloc.c?
> 
> Doing a quick glance at lib/genalloc.c I dont see any reason we  
> couldn't use it.  However, Panto will know best, since he wrote rheap.
> 
> - k
> 

Hi there,

RHEAP started life long before on 2.4 before genalloc was included in the kernel.
The difference is only in the implementation, rheap uses double linked lists
while genalloc uses per pool bitmaps. RHEAP is faster & conserves a bit more space 
since it doesn't use a bitmap to track the chunks.

Since genalloc is the blessed linux thing it might be best to use that & remove
rheap completely. Oh well...

Regards

Pantelis

Re: [PATCH] powerpc:Fix rheap alignment problem

[Pantelis Antoniou]

On Sunday 02 July 2006 06:54, Rune Torgersen wrote:
> From: Pantelis Antoniou
> Sent: Sat 7/1/2006 9:50 AM
> >Since genalloc is the blessed linux thing it might be best to use that & remove
> >rheap completely. Oh well...
> 
> Two problems with genalloc that I can see (for CPM programming):
> 1) (minor) Does not have a way to specify alignment (genalloc does it for you)
> 2) (major problerm, at least for me) Does not have a way to allocate a specified address in the pool.
> 
> 2 is needed esp when programming MCC drivers, since a lot of the datastructures must be in DP RAM _and_ be in a specific spot. And if you cannot tell the allocator that I am using a specific address, then the allocator might very well give somebody else that portion of RAM. The only solution without a fixed allocator is to allocate ALL memory in the DP RAM and use your own allocator. 
> 

Yeah, that too.

Too bad there are no main tree drivers like that, but they do exist.

One could conceivably hack genalloc to do that, but will end up with
something complex too.

BTW, there are other uEngine based architectures with similar alignment
requirements.

So in conclusion, for the in-tree drivers genalloc is sufficient as an cpm memory allocator.
For some out of tree drivers, it is not.

Pantelis

Re: [PATCH] powerpc:Fix rheap alignment problem

[Benjamin Herrenschmidt]

On Sun, 2006-07-02 at 08:18 +0300, Pantelis Antoniou wrote:
> On Sunday 02 July 2006 06:54, Rune Torgersen wrote:
> > From: Pantelis Antoniou
> > Sent: Sat 7/1/2006 9:50 AM
> > >Since genalloc is the blessed linux thing it might be best to use that & remove
> > >rheap completely. Oh well...
> > 
> > Two problems with genalloc that I can see (for CPM programming):
> > 1) (minor) Does not have a way to specify alignment (genalloc does it for you)
> > 2) (major problerm, at least for me) Does not have a way to allocate a specified address in the pool.
> > 
> > 2 is needed esp when programming MCC drivers, since a lot of the datastructures must be in DP RAM _and_ be in a specific spot. And if you cannot tell the allocator that I am using a specific address, then the allocator might very well give somebody else that portion of RAM. The only solution without a fixed allocator is to allocate ALL memory in the DP RAM and use your own allocator. 
> > 
> 
> Yeah, that too.
> 
> Too bad there are no main tree drivers like that, but they do exist.
> 
> One could conceivably hack genalloc to do that, but will end up with
> something complex too.
> 
> BTW, there are other uEngine based architectures with similar alignment
> requirements.
> 
> So in conclusion, for the in-tree drivers genalloc is sufficient as an cpm memory allocator.
> For some out of tree drivers, it is not.

Sounds like a good enough justification to keep rheap for now then.

Ben.

RE: [PATCH] powerpc:Fix rheap alignment problem

[Li Yang-r58472]

Buddy allocation is good in general, but doesn't mean it fits best in any condition.  In this case for managing DPRAM/MURAM in Freescale soc, in most case we only put buffer descriptor in DPRAM.  That means the alloc/free only occurs on initialization and unloading of the driver.  So there are not supposed to be a lot of free operations.  Buddy allocation will cause more internal fragment, in my humble opinion.  And a free-list allocation is best fit in this case.


Best Regards,
Leo
> -----Original Message-----
> From: linuxppc-dev-bounces+leoli=freescale.com@ozlabs.org
> [mailto:linuxppc-dev-bounces+leoli=freescale.com@ozlabs.org] On Behalf Of
> Pantelis Antoniou
> Sent: Sunday, July 02, 2006 1:18 PM
> To: Rune Torgersen
> Cc: linuxppc-dev list; Paul Mackerras; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH] powerpc:Fix rheap alignment problem
> 
> On Sunday 02 July 2006 06:54, Rune Torgersen wrote:
> > From: Pantelis Antoniou
> > Sent: Sat 7/1/2006 9:50 AM
> > >Since genalloc is the blessed linux thing it might be best to use that &
> remove
> > >rheap completely. Oh well...
> >
> > Two problems with genalloc that I can see (for CPM programming):
> > 1) (minor) Does not have a way to specify alignment (genalloc does it for
> you)
> > 2) (major problerm, at least for me) Does not have a way to allocate a specified
> address in the pool.
> >
> > 2 is needed esp when programming MCC drivers, since a lot of the datastructures
> must be in DP RAM _and_ be in a specific spot. And if you cannot tell the
> allocator that I am using a specific address, then the allocator might very
> well give somebody else that portion of RAM. The only solution without a fixed
> allocator is to allocate ALL memory in the DP RAM and use your own allocator.
> >
> 
> Yeah, that too.
> 
> Too bad there are no main tree drivers like that, but they do exist.
> 
> One could conceivably hack genalloc to do that, but will end up with
> something complex too.
> 
> BTW, there are other uEngine based architectures with similar alignment
> requirements.
> 
> So in conclusion, for the in-tree drivers genalloc is sufficient as an cpm
> memory allocator.
> For some out of tree drivers, it is not.
> 
> Pantelis
> _______________________________________________
> Linuxppc-dev mailing list
> Linuxppc-dev@ozlabs.org
> https://ozlabs.org/mailman/listinfo/linuxppc-dev

RE: [PATCH] powerpc:Fix rheap alignment problem

[Li Yang-r58472]

> > > Two problems with genalloc that I can see (for CPM programming):
> > > 1) (minor) Does not have a way to specify alignment (genalloc does it for
> you)
> > > 2) (major problerm, at least for me) Does not have a way to allocate a
> specified address in the pool.
> > >
> > > 2 is needed esp when programming MCC drivers, since a lot of the
> datastructures must be in DP RAM _and_ be in a specific spot. And if you cannot
> tell the allocator that I am using a specific address, then the allocator might
> very well give somebody else that portion of RAM. The only solution without
> a fixed allocator is to allocate ALL memory in the DP RAM and use your own
> allocator.
> > >
> >
> > Yeah, that too.
> >
> > Too bad there are no main tree drivers like that, but they do exist.
> >
> > One could conceivably hack genalloc to do that, but will end up with
> > something complex too.
> >
> > BTW, there are other uEngine based architectures with similar alignment
> > requirements.
> >
> > So in conclusion, for the in-tree drivers genalloc is sufficient as an cpm
> memory allocator.
> > For some out of tree drivers, it is not.
> 
> Sounds like a good enough justification to keep rheap for now then.

As the reason I stated in the last mail, rheap should continue being used not only for this fix-address situation but also for CPM/QE buffer descriptor management.  Rheap and genalloc are two different implementations of dynamic memory allocator, which have different suitable cases.  Both of them should be kept for different applications.
> 
> Ben.