Messages in this thread | | | Date | Tue, 28 Nov 2017 22:31:31 +0900 | From | Masami Hiramatsu <> | Subject | Re: [PATCH 03/12] perf, tools: Support storing additional data in strlist |
| |
On Mon, 27 Nov 2017 16:23:12 -0800 Andi Kleen <andi@firstfloor.org> wrote:
> From: Andi Kleen <ak@linux.intel.com> > > Add a configurable node size to strlist, which allows users > to store additional data in a str_node. Also add a new interface > to add a new strlist node, and return the node, so additional > data can be added. > > Signed-off-by: Andi Kleen <ak@linux.intel.com> > --- > tools/perf/util/rblist.c | 16 ++++++++++++++-- > tools/perf/util/rblist.h | 2 ++ > tools/perf/util/strlist.c | 15 ++++++++++++++- > tools/perf/util/strlist.h | 8 ++++++++ > 4 files changed, 38 insertions(+), 3 deletions(-) > > diff --git a/tools/perf/util/rblist.c b/tools/perf/util/rblist.c > index 0dfe27d99458..fa221e6b0932 100644 > --- a/tools/perf/util/rblist.c > +++ b/tools/perf/util/rblist.c > @@ -11,11 +11,13 @@ > > #include "rblist.h" > > -int rblist__add_node(struct rblist *rblist, const void *new_entry) > +int rblist__add_node_ptr(struct rblist *rblist, const void *new_entry, > + struct rb_node **nodep) > { > struct rb_node **p = &rblist->entries.rb_node; > struct rb_node *parent = NULL, *new_node; > > + *nodep = NULL; > while (*p != NULL) { > int rc; > > @@ -26,13 +28,16 @@ int rblist__add_node(struct rblist *rblist, const void *new_entry) > p = &(*p)->rb_left; > else if (rc < 0) > p = &(*p)->rb_right; > - else > + else { > + *nodep = parent; > return -EEXIST; > + } > } > > new_node = rblist->node_new(rblist, new_entry); > if (new_node == NULL) > return -ENOMEM; > + *nodep = new_node; > > rb_link_node(new_node, parent, p); > rb_insert_color(new_node, &rblist->entries); > @@ -41,6 +46,13 @@ int rblist__add_node(struct rblist *rblist, const void *new_entry) > return 0; > } > > +int rblist__add_node(struct rblist *rblist, const void *new_entry) > +{ > + struct rb_node *nd; > + > + return rblist__add_node_ptr(rblist, new_entry, &nd); > +} > + > void rblist__remove_node(struct rblist *rblist, struct rb_node *rb_node) > { > rb_erase(rb_node, &rblist->entries); > diff --git a/tools/perf/util/rblist.h b/tools/perf/util/rblist.h > index 4c8638a22571..2941e4295f63 100644 > --- a/tools/perf/util/rblist.h > +++ b/tools/perf/util/rblist.h > @@ -31,6 +31,8 @@ struct rblist { > void rblist__init(struct rblist *rblist); > void rblist__delete(struct rblist *rblist); > int rblist__add_node(struct rblist *rblist, const void *new_entry); > +int rblist__add_node_ptr(struct rblist *rblist, const void *new_entry, > + struct rb_node **nodep); > void rblist__remove_node(struct rblist *rblist, struct rb_node *rb_node); > struct rb_node *rblist__find(struct rblist *rblist, const void *entry); > struct rb_node *rblist__findnew(struct rblist *rblist, const void *entry); > diff --git a/tools/perf/util/strlist.c b/tools/perf/util/strlist.c > index 9de5434bb49e..68ef21c3797c 100644 > --- a/tools/perf/util/strlist.c > +++ b/tools/perf/util/strlist.c > @@ -18,7 +18,7 @@ struct rb_node *strlist__node_new(struct rblist *rblist, const void *entry) > const char *s = entry; > struct rb_node *rc = NULL; > struct strlist *strlist = container_of(rblist, struct strlist, rblist); > - struct str_node *snode = malloc(sizeof(*snode)); > + struct str_node *snode = malloc(strlist->node_size); > > if (snode != NULL) { > if (strlist->dupstr) { > @@ -66,6 +66,14 @@ int strlist__add(struct strlist *slist, const char *new_entry) > return rblist__add_node(&slist->rblist, new_entry); > } > > +struct str_node *strlist__add_node(struct strlist *slist, const char *new_entry) > +{ > + struct rb_node *nd; > + > + rblist__add_node_ptr(&slist->rblist, new_entry, &nd); > + return container_of(nd, struct str_node, rb_node); > +}
It should check the result of rblist__add_node_ptr() and return NULL if an error.
> + > int strlist__load(struct strlist *slist, const char *filename) > { > char entry[1024]; > @@ -165,11 +173,15 @@ struct strlist *strlist__new(const char *list, const struct strlist_config *conf > bool dupstr = true; > bool file_only = false; > const char *dirname = NULL; > + size_t node_size = sizeof(struct str_node); > > if (config) { > dupstr = !config->dont_dupstr; > dirname = config->dirname; > file_only = config->file_only; > + node_size = config->node_size; > + if (!node_size) > + node_size = sizeof(struct str_node);
This is dangerous, node_size can be smaller than sizeof(struct str_node). Instead, why don't we use "data_size" and set additional data size?
Also, we can add "u8 data[0]" at the end of struct str_node, so that user can easily access the appended data.
> } > > rblist__init(&slist->rblist); > @@ -179,6 +191,7 @@ struct strlist *strlist__new(const char *list, const struct strlist_config *conf > > slist->dupstr = dupstr; > slist->file_only = file_only; > + slist->node_size = node_size; > > if (list && strlist__parse_list(slist, list, dirname) != 0) > goto out_error; > diff --git a/tools/perf/util/strlist.h b/tools/perf/util/strlist.h > index d58f1e08b170..fd407e11e124 100644 > --- a/tools/perf/util/strlist.h > +++ b/tools/perf/util/strlist.h > @@ -16,25 +16,33 @@ struct strlist { > struct rblist rblist; > bool dupstr; > bool file_only; > + size_t node_size; > }; > > /* > * @file_only: When dirname is present, only consider entries as filenames, > * that should not be added to the list if dirname/entry is not > * found > + * @node_size: Allocate extra space after str_node which can be used for other > + * data. This is the complete size including str_node > */ > struct strlist_config { > bool dont_dupstr; > bool file_only; > const char *dirname; > + size_t node_size; > }; > > +#define STRLIST_CONFIG_DEFAULT \ > + { false, false, NULL, sizeof(struct str_node) } > + > struct strlist *strlist__new(const char *slist, const struct strlist_config *config); > void strlist__delete(struct strlist *slist); > > void strlist__remove(struct strlist *slist, struct str_node *sn); > int strlist__load(struct strlist *slist, const char *filename); > int strlist__add(struct strlist *slist, const char *str); > +struct str_node *strlist__add_node(struct strlist *slist, const char *str);
Hmm, I see this is much more efficient, but from the programming point of view, we can also use the combination of strlist__add() and strlist__find() for that purpose.
BTW, what happen if we already have same string on strlist?
Thank you,
> > struct str_node *strlist__entry(const struct strlist *slist, unsigned int idx); > struct str_node *strlist__find(struct strlist *slist, const char *entry); > -- > 2.13.6 >
-- Masami Hiramatsu <mhiramat@kernel.org>
| |