On 20.7.2012 15:40, Jiří Zárevúcky wrote:
> On 20 July 2012 14:00, Jakub Jermar <[email protected]> wrote:
>> Hi Jiri,
>>
>> On 19.7.2012 20:48, Jiří Zárevúcky wrote:
>>> Have you even figured out what is wrong with memalign()?
>>
>> Unfortunately, we have never really figured out what the underlying
>> rootcause was and hence we didn't know what exactly needs to be fixed.
>> This is partly because no one really cared enough for this particular bug.
>>
>
> Well, I did some digging yesterday (see the patch) and it would seem
> that the calculation that was supposed to ensure big-enough grow was
> not correct. Since the original area does not need to be aligned to
> the requirement, one must either add the entire alignment size to
> provide for the margin, or compute the needed addition based on
> alignment of the end of the existing area. If that is not done, the
> grow is successful, but allocation can't proceed because there is not
> enough space to align it.
> In my patch I basically do both in different cases, but I think the
> latter is still not entirely correct and will sometimes extend more
> than necessary.
Well, yes, the following patch seems to fix the problem too (it's a
variation on the previous Vojta's patch and also a bit similar to yours):
=== modified file 'uspace/lib/c/generic/malloc.c'
--- uspace/lib/c/generic/malloc.c 2012-06-13 13:16:19 +0000
+++ uspace/lib/c/generic/malloc.c 2012-07-20 13:49:33 +0000
@@ -678,7 +678,7 @@
return NULL;
size_t falign = lcm(align, BASE_ALIGN);
- size_t real_size = GROSS_SIZE(ALIGN_UP(size, falign));
+ size_t real_size = GROSS_SIZE(size);
bool retry = false;
heap_block_head_t *split;
@@ -711,7 +711,7 @@
if (!retry) {
/* Try to grow the heap space */
- if (heap_grow(real_size)) {
+ if (heap_grow(real_size + falign)) {
retry = true;
goto loop;
}
If, for example, the call was memalign(32, 32), the current code would
simply use real_size of GROSS_SIZE(32). But that would not work because
in fact we need to grow the heap by at maximum GROSS_SIZE(32 + 32)
(assuming we will use an existing area) to guarantee the request will be
satisfied.
In my opinion, the other struct overheads do not need to be added
(contrary to the comment in Vojta's old patch), because the respective
overheads are added as needed (eg. in heap_grow()).
In your patch, why don't you simply add size and align in area_grow(),
just as you do for the new areas?
Also, will the following:
+ void *aligned_end = ((void*)ALIGN_UP((uintptr_t)area->end + 1, align));
work if we have the following hypothetical situation?
area->start = 0
area->end = 30
memaling(32,32)
So the alignment will move the end to offset 31 (32-byte aligned). In
addition to that, we will add GROSS_SIZE(32), which could be 20 + 32 +
16. Unfortunately, there will not be enough new space to make the user
buffer 32-bytes aligned. For that, you really need to add the full
alignment. Or am I missing something?
Jakub
_______________________________________________
HelenOS-devel mailing list
[email protected]
http://lists.modry.cz/cgi-bin/listinfo/helenos-devel
