Re: [Patch] CPM allocation mechanism in 2.6.13

Re: [Patch] CPM allocation mechanism in 2.6.13

[Pantelis Antoniou]

Clement Fabien wrote:
> Hello
> 
> I found out that the rh_alloc mechanism used in 2.6.xx does not always
> provide aligned blocks. 
> I'm using a 8275 and try to start the SAR driver of Alex Zeffert.
> 
> Here is a trace of allocations at linux startup of the code that gives
> me problems:
> 
> <...>
> rh_alloc Required size = 8, align = 16
> rh_alloc Block available, blk->size = 00003f40 - Start = 000000c0
> rh_alloc Alloc size = 16, align = 16 - Start = 000000c0
> 
> rh_alloc Required size = 6144, align = 32 
> rh_alloc Block available, blk->size = 00003f30 - Start = 000000d0 
> rh_alloc Alloc size = 6144, align = 32 - Start = 000000d0 
> (!!!! WRONG ALIGNMENT !!!!)
> 
> rh_alloc Required size = 264, align = 64 
> rh_alloc Block available, blk->size = 00002730 - Start = 000018d0
> rh_alloc Alloc size = 320, align = 64 - Start = 000018d0  
> (!!!! WRONG ALIGNMENT !!!!)
> 
> rh_alloc Required size = 64, align = 16
> rh_alloc Block available, blk->size = 000025f0 - Start = 00001a10
> rh_alloc Alloc size = 64, align = 16 - Start = 00001a10
> <...>
> 
> If we observe rh_alloc (arch/ppc/lib/rheap.c) function
> 
> The following line only ensures that there is enough space to perform
> alignment:
> 1.	size = (size + (info->alignment - 1)) & ~(info->alignment - 1);
> 
> But with the block start modification mechanism you cannot ensure that
> the next block will be aligned : 
> 2.   	blk->start = (int8_t *)blk->start + size;
> 
> The allocation mechanism must take into account the starting address, as
> it was in Linux 2.4 following code (3. extracted from commproc.c)
> 3.     off = ((p->start_addr + align_mask) & (~align_mask)) -
> p->start_addr;
> 
> I've had an initial discussion with Pantelis who suggests that alignment
> shall be unique for this heap: 
> 
> 
>>>Excuse me, how do you modify the rheap's alignment at runtime?
>>>The allocators prototype is:
> 
> 
>>>void *rh_alloc(rh_info_t * info, int size, const char *owner)
> 
> 
>>>I don't see an alignment parameter.
> 
> 
>>>If you are modifying info->alignment by hand before calling you can't
> 
> 
>>>possibly expect this to work.
> 
> 
>>>The alignment is fixed after the creation of the rheap.
>>>If you need specific alignment you do it like this.
> 
> 
>>>u8 *p = rh_alloc(info, size + (align - 1), "owner");
> 
> 
>>>p = (void *)((unsigned long)p + (align - 1) & ~(align - 1));
> 
> 
> So I guess the problem is in the usage of the function in kernel: it is
> initialized with 1 byte alignment (in cpm2_dpinit in
> arch/ppc/syslib/cpm2_common.c), then the cpm_dpalloc manipulates the
> alignment like this:
> 
> 4.	cpm_dpmem_info.alignment = align;
> 	start = rh_alloc(&cpm_dpmem_info, size, "commproc");
> 
> 
> I do not have an exact idea whether to modify cpm_dpalloc or rh_alloc.
> Can someone provide inputs on this subject?
> 
> Please see also the patch I suggest to correct the rh_alloc function
> 
> Regards
> Fabien Clement
> 

I'm on it.

Regards

Pantelis

[Patch] CPM allocation mechanism in 2.6.13

[Clement Fabien]

[-- Attachment #1: Type: text/plain, Size: 2819 bytes --]

Hello

I found out that the rh_alloc mechanism used in 2.6.xx does not always
provide aligned blocks. 
I'm using a 8275 and try to start the SAR driver of Alex Zeffert.

Here is a trace of allocations at linux startup of the code that gives
me problems:

<...>
rh_alloc Required size = 8, align = 16
rh_alloc Block available, blk->size = 00003f40 - Start = 000000c0
rh_alloc Alloc size = 16, align = 16 - Start = 000000c0

rh_alloc Required size = 6144, align = 32 
rh_alloc Block available, blk->size = 00003f30 - Start = 000000d0 
rh_alloc Alloc size = 6144, align = 32 - Start = 000000d0 
(!!!! WRONG ALIGNMENT !!!!)

rh_alloc Required size = 264, align = 64 
rh_alloc Block available, blk->size = 00002730 - Start = 000018d0
rh_alloc Alloc size = 320, align = 64 - Start = 000018d0  
(!!!! WRONG ALIGNMENT !!!!)

rh_alloc Required size = 64, align = 16
rh_alloc Block available, blk->size = 000025f0 - Start = 00001a10
rh_alloc Alloc size = 64, align = 16 - Start = 00001a10
<...>

If we observe rh_alloc (arch/ppc/lib/rheap.c) function

The following line only ensures that there is enough space to perform
alignment:
1.	size = (size + (info->alignment - 1)) & ~(info->alignment - 1);

But with the block start modification mechanism you cannot ensure that
the next block will be aligned : 
2.   	blk->start = (int8_t *)blk->start + size;

The allocation mechanism must take into account the starting address, as
it was in Linux 2.4 following code (3. extracted from commproc.c)
3.     off = ((p->start_addr + align_mask) & (~align_mask)) -
p->start_addr;

I've had an initial discussion with Pantelis who suggests that alignment
shall be unique for this heap: 

>> Excuse me, how do you modify the rheap's alignment at runtime?
>> The allocators prototype is:

>> void *rh_alloc(rh_info_t * info, int size, const char *owner)

>> I don't see an alignment parameter.

>> If you are modifying info->alignment by hand before calling you can't

>> possibly expect this to work.

>> The alignment is fixed after the creation of the rheap.
>> If you need specific alignment you do it like this.

>> u8 *p = rh_alloc(info, size + (align - 1), "owner");

>> p = (void *)((unsigned long)p + (align - 1) & ~(align - 1));

So I guess the problem is in the usage of the function in kernel: it is
initialized with 1 byte alignment (in cpm2_dpinit in
arch/ppc/syslib/cpm2_common.c), then the cpm_dpalloc manipulates the
alignment like this:

4.	cpm_dpmem_info.alignment = align;
	start = rh_alloc(&cpm_dpmem_info, size, "commproc");


I do not have an exact idea whether to modify cpm_dpalloc or rh_alloc.
Can someone provide inputs on this subject?

Please see also the patch I suggest to correct the rh_alloc function

Regards
Fabien Clement


[-- Attachment #2: rheap.patch --]
[-- Type: application/octet-stream, Size: 1863 bytes --]

Index: arch/ppc/lib/rheap.c
===================================================================
RCS file: /home/conf/tnp/linuxppc/linux/arch/ppc/lib/rheap.c,v
retrieving revision 1.1.1.1
diff -b -u -r1.1.1.1 rheap.c
--- arch/ppc/lib/rheap.c	6 Dec 2004 16:15:22 -0000	1.1.1.1
+++ arch/ppc/lib/rheap.c	24 Nov 2005 09:42:37 -0000
@@ -431,13 +431,15 @@
 	rh_block_t *blk;
 	rh_block_t *newblk;
 	void *start;
+   unsigned int off;
+   unsigned int align_mask = (info->alignment - 1);
 
 	/* Validate size */
 	if (size <= 0)
 		return ERR_PTR(-EINVAL);
 
 	/* Align to configured alignment */
-	size = (size + (info->alignment - 1)) & ~(info->alignment - 1);
+   size = (size + align_mask) & (~align_mask);
 
 	if (assure_empty(info, 1) < 0)
 		return ERR_PTR(-ENOMEM);
@@ -446,7 +448,11 @@
 	list_for_each(l, &info->free_list) {
 		blk = list_entry(l, rh_block_t, list);
 		if (size <= blk->size)
+      {
+         off = ((int)(blk->start + align_mask) & (~align_mask)) - (int)blk->start;
+         if (blk->size >= size + off) 
 			break;
+      }
 		blk = NULL;
 	}
 
@@ -454,7 +460,7 @@
 		return ERR_PTR(-ENOMEM);
 
 	/* Just fits */
-	if (blk->size == size) {
+	if (blk->size == (size + off)) {
 		/* Move from free list to taken list */
 		list_del(&blk->list);
 		blk->owner = owner;
@@ -462,21 +468,23 @@
 
 		attach_taken_block(info, blk);
 
-		return start;
 	}
-
+   else
+   {  
 	newblk = get_slot(info);
 	newblk->start = blk->start;
-	newblk->size = size;
+   	newblk->size = size + off;
 	newblk->owner = owner;
 
 	/* blk still in free list, with updated start, size */
-	blk->start = (int8_t *)blk->start + size;
-	blk->size -= size;
+   	blk->start = (int8_t *)blk->start + size + off;
+   	blk->size -= (size + off);
 
-	start = newblk->start;
+   	start = newblk->start + off;
 
 	attach_taken_block(info, newblk);
+   }
+
 
 	return start;
 }