Re: [Qemu-devel] [PATCH] Save memory allocation in the elf loader

[Paolo Bonzini]

Il 19/02/2013 15:41, Fabien Chouteau ha scritto:
> The current elf loader uses too much memory. For example, I have a
> executable with a bss section of 400 MB and I set the ram size to 512
> MB. Qemu uses about 780MB of RAM (which is fine), but there's a peak at
> 1.6 GB during initialization (this is not fine).
> 
> This patch fixes two things:
>  1) do not allocate each elf program twice.
>  2) do not allocate memory for areas that are only zeros.
> 
> For this we need a new field in Rom: "datasize" which is the size of the
> allocated data. If datasize is less than romsize, it means that the area
> from datasize to romsize is filled with zeros.

You haven't CCed the only two people that likely would review your patch. :)

Paolo

> Signed-off-by: Fabien Chouteau <address@hidden>
> ---
>  hw/elf_ops.h |   19 ++++++++-------
>  hw/loader.c  |   75 
> ++++++++++++++++++++++++++++++++++++++++++++++++----------
>  hw/loader.h  |    2 ++
>  3 files changed, 75 insertions(+), 21 deletions(-)
> 
> diff --git a/hw/elf_ops.h b/hw/elf_ops.h
> index 531a425..acc701e 100644
> --- a/hw/elf_ops.h
> +++ b/hw/elf_ops.h
> @@ -197,7 +197,7 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>      struct elfhdr ehdr;
>      struct elf_phdr *phdr = NULL, *ph;
>      int size, i, total_size;
> -    elf_word mem_size;
> +    elf_word mem_size, file_size;
>      uint64_t addr, low = (uint64_t)-1, high = 0;
>      uint8_t *data = NULL;
>      char label[128];
> @@ -252,14 +252,16 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>      for(i = 0; i < ehdr.e_phnum; i++) {
>          ph = &phdr[i];
>          if (ph->p_type == PT_LOAD) {
> -            mem_size = ph->p_memsz;
> -            /* XXX: avoid allocating */
> -            data = g_malloc0(mem_size);
> +            mem_size = ph->p_memsz; /* Size of the ROM */
> +            file_size = ph->p_filesz; /* Size of the allocated data */
> +            data = g_malloc0(file_size);
>              if (ph->p_filesz > 0) {
> -                if (lseek(fd, ph->p_offset, SEEK_SET) < 0)
> +                if (lseek(fd, ph->p_offset, SEEK_SET) < 0) {
>                      goto fail;
> -                if (read(fd, data, ph->p_filesz) != ph->p_filesz)
> +                }
> +                if (read(fd, data, file_size) != file_size) {
>                      goto fail;
> +                }
>              }
>              /* address_offset is hack for kernel images that are
>                 linked at the wrong physical address.  */
> @@ -281,7 +283,9 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>              }
>  
>              snprintf(label, sizeof(label), "phdr #%d: %s", i, name);
> -            rom_add_blob_fixed(label, data, mem_size, addr);
> +
> +            /* rom_add_elf_program() seize the ownership of 'data' */
> +            rom_add_elf_program(label, data, file_size, mem_size, addr);
>  
>              total_size += mem_size;
>              if (addr < low)
> @@ -289,7 +293,6 @@ static int glue(load_elf, SZ)(const char *name, int fd,
>              if ((addr + mem_size) > high)
>                  high = addr + mem_size;
>  
> -            g_free(data);
>              data = NULL;
>          }
>      }
> diff --git a/hw/loader.c b/hw/loader.c
> index 995edc3..bd2b52d 100644
> --- a/hw/loader.c
> +++ b/hw/loader.c
> @@ -533,7 +533,14 @@ typedef struct Rom Rom;
>  struct Rom {
>      char *name;
>      char *path;
> +
> +    /* datasize is the amount of memory allocated in "data". If datasize is 
> less
> +     * than romsize, it means that the area from datasize to romsize is 
> filled
> +     * with zeros.
> +     */
>      size_t romsize;
> +    size_t datasize;
> +
>      uint8_t *data;
>      int isrom;
>      char *fw_dir;
> @@ -589,14 +596,15 @@ int rom_add_file(const char *file, const char *fw_dir,
>          rom->fw_dir  = g_strdup(fw_dir);
>          rom->fw_file = g_strdup(file);
>      }
> -    rom->addr    = addr;
> -    rom->romsize = lseek(fd, 0, SEEK_END);
> -    rom->data    = g_malloc0(rom->romsize);
> +    rom->addr     = addr;
> +    rom->romsize  = lseek(fd, 0, SEEK_END);
> +    rom->datasize = rom->romsize;
> +    rom->data     = g_malloc0(rom->datasize);
>      lseek(fd, 0, SEEK_SET);
> -    rc = read(fd, rom->data, rom->romsize);
> -    if (rc != rom->romsize) {
> +    rc = read(fd, rom->data, rom->datasize);
> +    if (rc != rom->datasize) {
>          fprintf(stderr, "rom: file %-20s: read error: rc=%d (expected 
> %zd)\n",
> -                rom->name, rc, rom->romsize);
> +                rom->name, rc, rom->datasize);
>          goto err;
>      }
>      close(fd);
> @@ -637,16 +645,37 @@ int rom_add_blob(const char *name, const void *blob, 
> size_t len,
>  {
>      Rom *rom;
>  
> -    rom = g_malloc0(sizeof(*rom));
> -    rom->name    = g_strdup(name);
> -    rom->addr    = addr;
> -    rom->romsize = len;
> -    rom->data    = g_malloc0(rom->romsize);
> +    rom           = g_malloc0(sizeof(*rom));
> +    rom->name     = g_strdup(name);
> +    rom->addr     = addr;
> +    rom->romsize  = len;
> +    rom->datasize = len;
> +    rom->data     = g_malloc0(rom->datasize);
>      memcpy(rom->data, blob, len);
>      rom_insert(rom);
>      return 0;
>  }
>  
> +/* This function is specific for elf program because we don't need to 
> allocate
> + * all the rom. We just allocate the first part and the rest is just zeros. 
> This
> + * is why romsize and datasize are different. Also, this function seize the
> + * memory ownership of "data", so we don't have to allocate and copy the 
> buffer.
> + */
> +int rom_add_elf_program(const char *name, void *data, size_t datasize,
> +                        size_t romsize, hwaddr addr)
> +{
> +    Rom *rom;
> +
> +    rom           = g_malloc0(sizeof(*rom));
> +    rom->name     = g_strdup(name);
> +    rom->addr     = addr;
> +    rom->datasize = datasize;
> +    rom->romsize  = romsize;
> +    rom->data     = data;
> +    rom_insert(rom);
> +    return 0;
> +}
> +
>  int rom_add_vga(const char *file)
>  {
>      return rom_add_file(file, "vgaroms", 0, -1);
> @@ -668,7 +697,7 @@ static void rom_reset(void *unused)
>          if (rom->data == NULL) {
>              continue;
>          }
> -        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->romsize);
> +        cpu_physical_memory_write_rom(rom->addr, rom->data, rom->datasize);
>          if (rom->isrom) {
>              /* rom needs to be written only once */
>              g_free(rom->data);
> @@ -756,13 +785,33 @@ int rom_copy(uint8_t *dest, hwaddr addr, size_t size)
>  
>          d = dest + (rom->addr - addr);
>          s = rom->data;
> -        l = rom->romsize;
> +        l = rom->datasize;
>  
>          if ((d + l) > (dest + size)) {
>              l = dest - d;
>          }
>  
>          memcpy(d, s, l);
> +
> +        if (rom->romsize > rom->datasize) {
> +            /* If datasize is less than romsize, it means that we didn't
> +             * allocate all the ROM because the trailing data are only zeros.
> +             */
> +
> +            d += l;
> +            l = rom->romsize - rom->datasize;
> +
> +            if ((d + l) > (dest + size)) {
> +                /* Rom size doesn't fit in the destination area. Adjust to 
> avoid
> +                 * overflow.
> +                 */
> +                l = dest - d;
> +            }
> +
> +            if (l > 0) {
> +                memset(d, 0x0, l);
> +            }
> +        }
>      }
>  
>      return (d + l) - dest;
> diff --git a/hw/loader.h b/hw/loader.h
> index 5e61c95..0958f06 100644
> --- a/hw/loader.h
> +++ b/hw/loader.h
> @@ -27,6 +27,8 @@ int rom_add_file(const char *file, const char *fw_dir,
>                   hwaddr addr, int32_t bootindex);
>  int rom_add_blob(const char *name, const void *blob, size_t len,
>                   hwaddr addr);
> +int rom_add_elf_program(const char *name, void *data, size_t datasize,
> +                        size_t romsize, hwaddr addr);
>  int rom_load_all(void);
>  void rom_set_fw(void *f);
>  int rom_copy(uint8_t *dest, hwaddr addr, size_t size);
>