From: Masami Hiramatsu <mhiramat@kernel.org>
To: Andi Kleen <andi@firstfloor.org>
Cc: acme@kernel.org, jolsa@kernel.org, mhiramat@kernel.org,
adrian.hunter@intel.com, linux-kernel@vger.kernel.org,
Andi Kleen <ak@linux.intel.com>
Subject: Re: [PATCH 03/12] perf, tools: Support storing additional data in strlist
Date: Tue, 28 Nov 2017 22:31:31 +0900 [thread overview]
Message-ID: <20171128223131.95f6d6229b4f6cabcdcc5bb8@kernel.org> (raw)
In-Reply-To: <20171128002321.2878-4-andi@firstfloor.org>
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>
next prev parent reply other threads:[~2017-11-28 13:31 UTC|newest]
Thread overview: 23+ messages / expand[flat|nested] mbox.gz Atom feed top
2017-11-28 0:23 Implement dwarf variable/type resolving for perf script Andi Kleen
2017-11-28 0:23 ` [PATCH 01/12] perf, tools, pt: Clear instruction for ptwrite samples Andi Kleen
2017-11-28 0:23 ` [PATCH 02/12] perf, tools, script: Print insn/insnlen for non PT sample Andi Kleen
2017-11-28 0:23 ` [PATCH 03/12] perf, tools: Support storing additional data in strlist Andi Kleen
2017-11-28 13:31 ` Masami Hiramatsu [this message]
2017-11-28 0:23 ` [PATCH 04/12] perf, tools: Store variable name and register for dwarf variable lists Andi Kleen
2017-11-28 0:23 ` [PATCH 05/12] perf, tools, probe: Print location for resolved variables Andi Kleen
2017-11-29 1:19 ` Masami Hiramatsu
2017-11-28 0:23 ` [PATCH 06/12] perf, tools, probe: Support a quiet argument for debug info open Andi Kleen
2017-11-29 3:14 ` Masami Hiramatsu
2017-11-29 3:39 ` Andi Kleen
2017-11-30 2:36 ` Masami Hiramatsu
2017-11-28 0:23 ` [PATCH 07/12] perf, tools, script: Resolve variable names for registers Andi Kleen
2017-11-28 0:23 ` [PATCH 08/12] perf, tools: Always print probe finder warnings with -v Andi Kleen
2017-11-29 3:16 ` Masami Hiramatsu
2017-11-28 0:23 ` [PATCH 09/12] perf, tools: Downgrade register mapping message to warning Andi Kleen
2017-11-29 5:56 ` Masami Hiramatsu
2017-11-28 0:23 ` [PATCH 10/12] perf, tools: Add args and gprs shortcut for registers Andi Kleen
2017-11-28 0:23 ` [PATCH 11/12] perf, tools: Print probe warnings for binaries only once per binary Andi Kleen
2017-11-30 2:38 ` Masami Hiramatsu
2017-11-28 0:23 ` [PATCH 12/12] perf, tools, script: Implement dwarf resolving of instructions Andi Kleen
2017-12-01 2:36 ` Masami Hiramatsu
2017-11-28 5:31 ` Implement dwarf variable/type resolving for perf script Masami Hiramatsu
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20171128223131.95f6d6229b4f6cabcdcc5bb8@kernel.org \
--to=mhiramat@kernel.org \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=ak@linux.intel.com \
--cc=andi@firstfloor.org \
--cc=jolsa@kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
Powered by JetHome