| Message ID | 20190219052750.10694-1-jniethe5@gmail.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | core/device: Remove redundant malloc | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | master/apply_patch Successfully applied |
| snowpatch_ozlabs/snowpatch_job_snowpatch-skiboot | fail | Test snowpatch/job/snowpatch-skiboot on branch master |
On Tue, Feb 19, 2019 at 4:28 PM Jordan Niethe <jniethe5@gmail.com> wrote: > > From: jordan <jpn@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@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 >
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; + char name[max_name]; 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++; + + 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;