[U-Boot] [PATCH v9] [RFC] Add dmmalloc module for DM.
Graeme Russ
graeme.russ at gmail.com
Thu Oct 25 03:40:00 CEST 2012
Hi Tomas,
Overall impression - Very nice indeed :)
A couple of nit-picks (some of which may be wrong on my part) and one
lingering question around the switch over from early to late heap...
On Thu, Oct 25, 2012 at 10:49 AM, Tomas Hlavacek <tmshlvck at gmail.com> wrote:
> Add pointer to the first early heap into GD structure.
> Implement simple early_malloc and early_free functions.
> Prepare for additional heaps and automated heap initialization.
> Add temporary early_malloc_active function (to be replaced in future by
> more coarse DM init flags).
> Add DM specific malloc calls - dmmalloc, dmfree, dmrealloc and dmcalloc.
>
> Signed-off-by: Tomas Hlavacek <tmshlvck at gmail.com>
> ---
> Changes in v9:
> - Rework early_malloc to keep track of allocated block size.
> - Add early_free and dmfree functions.
> - Rework dmrealloc.
> - Add kerneldoc comments to dmmalloc.h.
>
> Changes in v8:
> - Add dmcalloc() implmentation.
> - Add comments to function prototypes in dmmalloc.h.
>
> Changes in v7:
> - Rework check of first heap in early_brk().
>
> Changes in v6:
> - Move dmmalloc() and all dm* functions to dmmalloc.h.
> - Fix bool expression in early_malloc_active().
>
> arch/arm/include/asm/global_data.h | 3 +
> arch/avr32/include/asm/global_data.h | 3 +
> arch/blackfin/include/asm/global_data.h | 3 +
> arch/m68k/include/asm/global_data.h | 3 +
> arch/microblaze/include/asm/global_data.h | 3 +
> arch/mips/include/asm/global_data.h | 3 +
> arch/nds32/include/asm/global_data.h | 3 +
> arch/nios2/include/asm/global_data.h | 3 +
> arch/openrisc/include/asm/global_data.h | 3 +
> arch/powerpc/include/asm/global_data.h | 3 +
> arch/sandbox/include/asm/global_data.h | 3 +
> arch/sh/include/asm/global_data.h | 3 +
> arch/sparc/include/asm/global_data.h | 3 +
> arch/x86/include/asm/global_data.h | 3 +
> common/Makefile | 1 +
> common/dmmalloc.c | 188 ++++++++++++++++++++++++++++
> include/dmmalloc.h | 194 +++++++++++++++++++++++++++++
> 17 files changed, 425 insertions(+)
> create mode 100644 common/dmmalloc.c
> create mode 100644 include/dmmalloc.h
>
> diff --git a/arch/arm/include/asm/global_data.h b/arch/arm/include/asm/global_data.h
> index 2b9af93..9045829 100644
> --- a/arch/arm/include/asm/global_data.h
> +++ b/arch/arm/include/asm/global_data.h
> @@ -82,6 +82,9 @@ typedef struct global_data {
> unsigned long post_log_res; /* success of POST test */
> unsigned long post_init_f_time; /* When post_init_f started */
> #endif
> +#ifdef CONFIG_SYS_EARLY_MALLOC
> + void *early_heap; /* heap for early_malloc */
> +#endif
Why not early_heap_header *early_heap; ?
diff --git a/common/Makefile b/common/Makefile
> index fdfead7..bfb4d7a 100644
> --- a/common/Makefile
> +++ b/common/Makefile
> @@ -209,6 +209,7 @@ COBJS-y += dlmalloc.o
> COBJS-y += image.o
> COBJS-y += memsize.o
> COBJS-y += stdio.o
> +COBJS-$(CONFIG_DM) += dmmalloc.o
COBJS-$(CONFIG_SYS_EARLY_MALLOC) += dmmalloc.o ?
> COBJS := $(sort $(COBJS-y))
> diff --git a/common/dmmalloc.c b/common/dmmalloc.c
> new file mode 100644
> index 0000000..41589dd
> --- /dev/null
> +++ b/common/dmmalloc.c
> @@ -0,0 +1,188 @@
> +/*
> + * (C) Copyright 2012
> + * Tomas Hlavacek (tmshlvck at gmail.com)
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#include <common.h> /* for ROUND_UP */
> +#include <asm/u-boot.h>
> +#include <asm/global_data.h> /* for gd_t and gd */
> +#include <asm/types.h> /* for phys_addr_t and size_addt_t */
> +
> +#include <dmmalloc.h>
> +#include <malloc.h>
> +
> +#include <linux/compiler.h>
> +
> +DECLARE_GLOBAL_DATA_PTR;
> +
> +#ifdef CONFIG_SYS_EARLY_MALLOC
If you use COBJS-$(CONFIG_SYS_EARLY_MALLOC) += dmmalloc.o in the Makefile,
you can drop this #ifdef
> +__weak struct early_heap_header *early_brk(size_t size)
> +{
> + struct early_heap_header *h;
> + struct early_block_header *b;
> +
> + if (gd->early_heap != NULL)
> + return NULL;
> +
> + h = (struct early_heap_header *)CONFIG_SYS_EARLY_HEAP_ADDR;
> + b = (struct early_block_header *)(h + 1);
Hmmm, does this really work? I would have thought:
b = (struct early_block_header *)(h + sizeof(struct early_heap_header));
but I could be mistaken
> +
> + size = CONFIG_SYS_EARLY_HEAP_SIZE;
> + h->size = size;
> + h->early_heap_next = NULL;
> + b->size = size - sizeof(struct early_heap_header) -
> + sizeof(struct early_block_header);
> + b->size = BLOCK_SET_FREE(b->size);
> +
> + return h;
> +}
> +
> +static struct early_block_header *find_free_space(struct early_heap_header *h,
> + size_t size)
> +{
> + struct early_block_header *b;
> +
> + b = (struct early_block_header *)(h+1);
> + while ((phys_addr_t)b + sizeof(struct early_block_header)
> + < (phys_addr_t)h + h->size) {
> + if (BLOCK_FREE(b->size) && (BLOCK_SIZE(b->size) >= size))
> + return b;
> + b = (struct early_block_header *)((phys_addr_t)b +
> + sizeof(struct early_block_header) +
> + BLOCK_SIZE(b->size));
> + }
> +
> + return NULL;
> +}
> +
> +static struct early_block_header *split_block(struct early_block_header *b,
> + size_t size)
> +{
> + struct early_block_header *nb;
> +
> + if ((BLOCK_SIZE(b->size) < size) || (BLOCK_USED(b->size)))
> + return NULL;
> +
> + if (BLOCK_SIZE(b->size) <= (size + sizeof(struct early_block_header)))
> + return b;
> +
> + nb = (struct early_block_header *)((phys_addr_t)b +
> + sizeof(struct early_block_header) + size);
> + nb->size = b->size - size - sizeof(struct early_block_header);
> + b->size = size;
> +
> + return b;
> +}
> +
> +void *early_malloc(size_t size)
> +{
> + struct early_heap_header *h;
> + struct early_block_header *b;
> +
> + size = roundup(size, sizeof(phys_addr_t));
> + if (size == 0)
> + return NULL;
> +
> + if (gd->early_heap == NULL)
> + gd->early_heap = early_brk(size);
> +
> + if (gd->early_heap == NULL) {
> + debug("early_brk failed to initialize heap\n");
> + return NULL;
> + }
> +
> + h = gd->early_heap;
> + while (1) {
> + b = find_free_space(h, size);
> + if (b != NULL)
> + break;
> +
> + if (h->early_heap_next != NULL)
> + h = h->early_heap_next;
> + else
> + break;
> + }
> +
> + if (b == NULL) {
> + h->early_heap_next = early_brk(size+
> + sizeof(struct early_heap_header)+
> + sizeof(struct early_block_header));
> + h = h->early_heap_next;
> + if (h == NULL) {
> + debug("early_brk failed to extend heap by %d B\n",
> + size);
> + return NULL;
> + }
> +
> + b = find_free_space(h, size);
> + if (b == NULL) {
> + debug("early_malloc failed to extend heap by %d B\n",
> + size);
> + return NULL;
> + }
> + }
> +
> + if (b->size != size)
> + b = split_block(b, size);
> + if (b == NULL) {
> + debug("early_malloc failed to split block to %d B\n", size);
> + return NULL;
> + }
> +
> + b->size = BLOCK_SET_USED(b->size);
> +
> + return BLOCK_DATA(b);
> +}
> +
> +void early_free(void *addr)
> +{
> + struct early_block_header *h = BLOCK_HEADER(addr);
> + assert(BLOCK_USED(h->size));
> + h->size = BLOCK_SET_FREE(h->size);
> +}
> +
> +int early_malloc_active(void)
> +{
> + return ((gd->flags & GD_FLG_RELOC) != GD_FLG_RELOC);
> +}
I think we need another flag - GD_FLG_RELOC gets set before the permanent
heap is initialised, so there is a window of opportunity where things may
break
> +
> +void early_heap_dump(struct early_heap_header *h)
> +{
> + struct early_block_header *b;
> +
> + debug("heap: h=%p, h->size=%d\n", h, h->size);
> +
> + b = (struct early_block_header *)(h+1);
> + while ((phys_addr_t)b + sizeof(struct early_block_header)
> + < (phys_addr_t)h + h->size) {
> + debug("block: h=%p h->size=%d b=%p b->size=%d b->(used)=%d\n",
> + h, h->size, b, BLOCK_SIZE(b->size),
> + BLOCK_USED(b->size));
> + assert(BLOCK_SIZE(b->size) > 0);
> + b = (struct early_block_header *)((phys_addr_t)b +
> + sizeof(struct early_block_header) +
> + BLOCK_SIZE(b->size));
> + }
> + debug("--- heap dump end ---\n");
> +}
Nice touch, but could we just iterate through all ealry heap chunks starting
from gd->early_heap?
> +
> +#endif /* CONFIG_SYS_EARLY_MALLOC */
> +
> diff --git a/include/dmmalloc.h b/include/dmmalloc.h
> new file mode 100644
> index 0000000..a241e19
> --- /dev/null
> +++ b/include/dmmalloc.h
> @@ -0,0 +1,194 @@
> +/*
> + * (C) Copyright 2012
> + * Tomas Hlavacek (tmshlvck at gmail.com)
> + *
> + * See file CREDITS for list of people who contributed to this
> + * project.
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License as
> + * published by the Free Software Foundation; either version 2 of
> + * the License, or (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 59 Temple Place, Suite 330, Boston,
> + * MA 02111-1307 USA
> + */
> +
> +#ifndef __INCLUDE_DMMALLOC_H
> +#define __INCLUDE_DMMALLOC_H
> +
> +#include <config.h>
> +#include <linux/stddef.h> /* for size_t */
> +#include <malloc.h>
> +
> +#if (!defined(CONFIG_SYS_EARLY_HEAP_ADDR)) || \
> + (!defined(CONFIG_SYS_EARLY_HEAP_SIZE))
> +#undef CONFIG_SYS_EARLY_MALLOC
> +#endif /* CONFIG_SYS_EARLY_HEAP_ADDR */
If a board implements early_brk() using non-fixed a address and/or size
then this is going to cause trouble
> +#ifdef CONFIG_SYS_EARLY_MALLOC
I see no need for the #ifdef
> +struct early_heap_header {
> + size_t size;
> + void *early_heap_next;
> +};
> +
> +struct early_block_header {
> + size_t size;
> +};
> +
> +#define BLOCK_DATA(header) ((void *)(((struct early_block_header *)header)+1))
> +#define BLOCK_HEADER(addr) (((struct early_block_header *)addr)-1)
Again, not sure if this is correct. I would have thought:
(struct early_block_header *)(addr - sizeof(struct early_block_header)
And again, not sure if I'm right
> +#define BLOCK_USED_FLAG 0x80000000
> +#define BLOCK_SIZE(size) (size & (~BLOCK_USED_FLAG))
> +#define BLOCK_USED(size) ((size & BLOCK_USED_FLAG) == BLOCK_USED_FLAG)
> +#define BLOCK_FREE(size) (!BLOCK_USED(size))
> +#define BLOCK_SET_FREE(size) BLOCK_SIZE(size)
> +#define BLOCK_SET_USED(size) (size | BLOCK_USED_FLAG)
Hmmm, I'm not sure what the general policy is about where to put 'stuff'
that is only used by the implementation and does not 'need' to be made
publically available. i.e. I don't know if these #defines and the
early_block_header struct belong here or in the .c file...
> +
> +/**
> + * early_brk() - obtain address of the heap
> + * @size: Minimal size of the new early heap to be allocated.
> + *
> + * Function returns a new heap pointer.
> + *
> + * Allocate and initialize early_heap at least size bytes long.
> + * This function can be platform dependent or board dependent but sensible
> + * default is provided.
> + */
> +struct early_heap_header *early_brk(size_t size);
> +
> +/**
> + * early_malloc() - malloc operating on the early_heap(s)
> + * @size: Size in bytes.
> + *
> + * Function returns a pointer to the allocated block.
> + */
> +void *early_malloc(size_t size);
> +
> +/**
> + * early_free() - free operating on the early_heap(s)
> + * @addr: Pointer to the allocated block to be released.
> + */
> +void early_free(void *addr);
> +
> +/**
> + * early_malloc_active() - indicate if the early mallocator is active
> + *
> + * Function returns true when the early_malloc and early_free are used and
> + * false otherwise.
> + */
> +int early_malloc_active(void);
> +
> +/**
> + * early_heap_dump() - print blocks contained in an early_heap
> + * @h: Address of the early heap.
> + */
> +void early_heap_dump(struct early_heap_header *h);
> +
> +#endif /* CONFIG_SYS_EARLY_MALLOC */
> +
> +#ifdef CONFIG_DM
> +
> +/*
> + * DM versions of malloc* functions. In early init it calls early_malloc.
> + * It wraps around normal malloc* functions afterwards.
> + */
I don't think these _need_ to be inline functions - The compiler should be
smart enough no (non-)inline them as appropriate.
> +
> +/**
> + * dmmalloc() - malloc working seamlessly in early as well as in RAM stages
> + * @size: Size of the block to be allocated.
> + *
> + * Function returns an address of the newly allocated block when successful
> + * or NULL otherwise.
> + */
> +static inline void *dmmalloc(size_t size)
> +{
> +#ifdef CONFIG_SYS_EARLY_MALLOC
> + if (early_malloc_active())
> + return early_malloc(size);
> +#endif /* CONFIG_SYS_EARLY_MALLOC */
> + return malloc(size);
> +}
> +
> +/**
> + * dmfree() - free working seamlessly in early as well as in RAM stages
> + * @ptr: Pointer to the allocated block to be released.
> + */
> +static inline void dmfree(void *ptr)
> +{
> +#ifdef CONFIG_SYS_EARLY_MALLOC
> + if (early_malloc_active()) {
> + early_free(ptr);
> + return;
> + }
> +#endif /* CONFIG_SYS_EARLY_MALLOC */
> + free(ptr);
> +}
> +
> +/**
> + * dmcalloc() - calloc working seamlessly in early as well as in RAM stages
> + * @n: Number of elements to be allocated.
> + * @elem_size: Size of elements to be allocated.
> + *
> + * Function returns a pointer to newly the allocated area (n*elem_size) long.
> + */
> +static inline void *dmcalloc(size_t n, size_t elem_size)
> +{
> +#ifdef CONFIG_SYS_EARLY_MALLOC
> + char *addr;
> + int size = elem_size * n;
> +
> + if (early_malloc_active()) {
> + addr = early_malloc(size);
> + memset(addr, 0, size);
> + return addr;
> + }
> +#endif /* CONFIG_SYS_EARLY_MALLOC */
> + return calloc(n, elem_size);
> +}
> +
> +/**
> + * dmrealloc() - realloc working seamlessly in early as well as in RAM stages
> + * @oldaddr: Pointer to the old memory block.
> + * @bytes: New size to of the block to be reallocated.
> + *
> + * Function returns an address of the newly allocated block when successful
> + * or NULL otherwise.
> + *
> + * Data are copied from the block specified by oldaddr to the new block.
> + */
> +static inline void *dmrealloc(void *oldaddr, size_t bytes)
> +{
> +#ifdef CONFIG_SYS_EARLY_MALLOC
> + char *addr;
> + struct early_block_header *h;
> + if (early_malloc_active()) {
> + addr = dmmalloc(bytes);
> + if (addr == NULL)
> + return NULL;
> +
> + h = BLOCK_HEADER(oldaddr);
> + if (BLOCK_FREE(h->size))
> + return NULL;
> +
> + if (bytes > BLOCK_SIZE(h->size))
> + bytes = BLOCK_SIZE(h->size);
> +
> + memcpy(addr, oldaddr, bytes);
> + dmfree(oldaddr);
> + return addr;
> + }
> +#endif /* CONFIG_SYS_EARLY_MALLOC */
> + return realloc(oldaddr, bytes);
> +}
Hmmm, we have a very intersting corner case to deal with. What if we use
dmrealloc() to move allocated data from the early malloc pool to the final
malloc pool?
At some point, we need to assume the developer is only ever going to pass
early malloc'd memory to dmrealloc()
The other option is to use the gd->flags...
Define two flags - something like:
GD_FLG_HEAP_INIT -> Final heap has been initialised
GD_FLG_EH_DONE -> free(), realloc() refer to final heap
(I don't like the names, I'm just not up to thinking of anything better)
This way we can use dmalloc() prior to the heap being initialised and then
set GD_FLG_HEAP_INIT. Once GD_FLG_HEAP_INIT has been set, do a bunch of
dmrealloc() calls to essentially move the data into the permanent heap (of
course pointers int the structures need to be fixed up by the drivers) and
finally set GD_FLG_EH_DONE (and call early_heap_dump)
One problem I see is what happens of you call malloc() and free() on the
same block between the setting of GD_FLG_HEAP_INIT and GD_FLG_EH_DONE? The
code will try to reference the block as if it is in the early heap, but it
won't be.
One solution is, on detection of GD_FLG_HEAP_INIT being set, dmfree() and
dmrealloc() could search the early heap chunks instead of just assuming
that the referenced block is in the early heap (if GD_FLG_HEAP_INIT has not
been set, it will be safe to assume the memory is on the early heap)
> +
> +#endif /* CONFIG_DM */
> +#endif /* __INCLUDE_DMMALLOC_H */
> +
> --
> 1.7.10.4
>
Regards,
Graeme
More information about the U-Boot
mailing list