[Skiboot] [PATCH] core/device: Remove redundant malloc

[Oliver]

On Tue, Feb 19, 2019 at 4:28 PM Jordan Niethe <jniethe5 at gmail.com> wrote:
>
> From: jordan <jpn at pasglop.ozlabs.ibm.com>
>
> Currently dt_new_addr() / dt_new_2addr() malloc a scatch buffer for the
> node name. This is then duplicated in new_node(). This makes the initial
> allocation redundant. To address this the allocation was removed from
> dt_new_addr() / dt_new_2addr() and new_node() was changed to a varadic
> function to accomodate the different name format.
>
> Ideally vsnprintf() would be used to calculate the length of the
> resulting name so a buffer could be dynamically allocated.
> This would then be filled with another call to vsnprintf(). However the
> implemenation of vsnprintf() does not behave correctly when called with
> a size of 0 so this is not possible.
> Instead a statically allocated buffer is used. This assumes a maximum
> length for a name.

> The optimization for ro memory names in take_name() is no longer
> peformed. This is because the arguements to new_node() are intended for
> vsnprintf() and can not be readly checked using is_rodata().
> This does not significantly impact memory usage.

So on P9 systems we have a lot of cores and each core has a seperate
DT node which is populated with static strings. It might be worth
looking into how this affects memory usage there.

Also, we could check if there is a format string indicator (the %) in
the name and still do the rodata trick if we can be sure that it's a
static string.

>
> Signed-off-by: Jordan Niethe <jniethe5 at gmail.com>
> ---
>  core/device.c | 41 ++++++++++++++++++++---------------------
>  1 file changed, 20 insertions(+), 21 deletions(-)
>
> diff --git a/core/device.c b/core/device.c
> index 6364a60e93ec..7f8bb929a184 100644
> --- a/core/device.c
> +++ b/core/device.c
> @@ -44,15 +44,31 @@ static void free_name(const char *name)
>                 free((char *)name);
>  }
>
> -static struct dt_node *new_node(const char *name)
> +static struct dt_node *new_node(const char *fmt, ...)
>  {
> +       const size_t max_name = 256;

you can drop this and use sizeof(name) as the param for vsnprintf()

> +       char name[max_name];

bring this down one line for REVERSE-CHRISTMAS-TREE variable declarations

>         struct dt_node *node = malloc(sizeof *node);
> +       size_t len = 0;
> +       va_list args;
> +
>         if (!node) {
>                 prerror("Failed to allocate node\n");
>                 abort();
>         }
>
> -       node->name = take_name(name);
> +       va_start(args, fmt);
> +       len = vsnprintf(name, max_name, fmt, args);
> +       va_end(args);
> +
> +       len++;

is len being used at all here?

> +
> +       node->name = strdup(name);
> +       if (!node->name) {
> +               prerror("Failed to allocate copy of name");
> +               abort();
> +       }
> +
>         node->parent = NULL;
>         list_head_init(&node->properties);
>         list_head_init(&node->children);
> @@ -208,18 +224,10 @@ struct dt_node *dt_find_by_name_addr(struct dt_node *parent, const char *name,
>  struct dt_node *dt_new_addr(struct dt_node *parent, const char *name,
>                             uint64_t addr)
>  {
> -       char *lname;
>         struct dt_node *new;
> -       size_t len;
>
>         assert(parent);
> -       len = strlen(name) + STR_MAX_CHARS(addr) + 2;
> -       lname = malloc(len);
> -       if (!lname)
> -               return NULL;
> -       snprintf(lname, len, "%s@%llx", name, (long long)addr);
> -       new = new_node(lname);
> -       free(lname);
> +       new = new_node("%s@%llx", name, (long long)addr);
>         if (!dt_attach_root(parent, new)) {
>                 dt_destroy(new);
>                 return NULL;
> @@ -230,19 +238,10 @@ struct dt_node *dt_new_addr(struct dt_node *parent, const char *name,
>  struct dt_node *dt_new_2addr(struct dt_node *parent, const char *name,
>                              uint64_t addr0, uint64_t addr1)
>  {
> -       char *lname;
>         struct dt_node *new;
> -       size_t len;
>         assert(parent);
>
> -       len = strlen(name) + 2*STR_MAX_CHARS(addr0) + 3;
> -       lname = malloc(len);
> -       if (!lname)
> -               return NULL;
> -       snprintf(lname, len, "%s@%llx,%llx",
> -                name, (long long)addr0, (long long)addr1);
> -       new = new_node(lname);
> -       free(lname);
> +       new = new_node("%s@%llx,%llx", name, (long long)addr0, (long long)addr1);
>         if (!dt_attach_root(parent, new)) {
>                 dt_destroy(new);
>                 return NULL;
> --
> 2.19.1
>