Re: [Qemu-devel] [PATCH 0/1] Fix large memory chunks allocation with tcg_malloc

[Evgeny Voevodin]

On 02.03.2012 13:22, Kirill Batuzov wrote:
> Currently large memory chunk allocation with tcg_malloc is broken.  An attempt
> to allocate such chunk when pool_current field of TCGContext is not NULL will
> result in circular links in list of memory pools:
>
> p = new pool;
> s->pool_current->next = p;
> p->next = s->pool_current;
> (in tcg_malloc_internal)
>
> Later p became a current pool, and current pool became next pool.  Next
> tcg_malloc will switch current pool to next pool ('previous' current pool)
> and will start allocating memory from it's beginning.  But some memory in
> the beginning of this pool was already allocated and will be used twice
> for different arrays.
>
> At the end of this cover letter there is a patch that demonstrates the
> problem.  It breaks current trunk on the first translation block containing
> labels.
>
> Large memory pools can not be reused by memory allocator for big allocations
> and an attempt to reuse them for small allocations may result in an infinite
> increase of memory consumption during run time.  Memory consumption would
> increase every time a new large chunk of memory is allocated.  If code
> allocates such chunk on every translation block (like patch at the end of this
> letter do) then memory consumption would increase with every new block
> translated.
>
> My fix for the problems mentioned above is in the second e-mail.  I moved large
> memory pools to a separate list and free them on pool_reset.

As I understand, your approach removes linking back to the previous 
allocated chunk to avoid usage of already allocated and used memory 
again. Also you added g_free() to tcg_pool_reset(). Wouldn't it slow 
down emulation?
Maybe such linkage to previous chunk was made with assumption that big 
allocations will not happen twice one after another? For example, while 
loading kernel on realview and exynos, the code never reaches this 
string in tcg_malloc_internal():

p->next = s->pool_current;

If this assumption is correct, maybe it's better to just insert a 
verification that allocated chunk has enough space to hold requested block?

+   if (s->pool_cur - p->data + size > p->size) {
+       goto new_pool;
+   }

    s->pool_current = p;
    s->pool_cur = p->data + size;
    s->pool_end = p->data + p->size;
    return p->data;


--
Kind regards,
Evgeny Voevodin,
Leading Software Engineer,
ASWG, Moscow R&D center, Samsung Electronics
e-mail: address@hidden