Messages in this thread | | | Date | Thu, 17 Sep 2020 17:20:59 -0300 | From | Arnaldo Carvalho de Melo <> | Subject | Re: [PATCH v6 1/7] perf util: Create streams |
| |
Em Fri, Sep 11, 2020 at 04:03:47PM +0800, Jin Yao escreveu: > We define the stream is the branch history which is aggregated by > the branch records from perf samples. For example, the callchains > aggregated from the branch records are considered as streams. > By browsing the hot stream, we can understand the hot code path. > > Now we only support the callchain for stream. For measuring the > hot level for a stream, we use the callchain_node->hit, higher > is hotter. > > There may be many callchains sampled so we only focus on the top > N hottest callchains. N is a user defined parameter or predefined > default value (nr_streams_max). > > This patch creates an evsel_streams array per event, and saves > the top N hottest streams in a stream array. > > So now we can get the per-event top N hottest streams. > > Signed-off-by: Jin Yao <yao.jin@linux.intel.com> > --- > v6: > - Rebase to perf/core > > v5: > - Remove enum stram_type > - Rebase to perf/core > > v4: > - Refactor the code > - Rename patch name from 'perf util: Create streams for managing > top N hottest callchains' to 'perf util: Create streams' > > v2: > - Use zfree in free_evsel_streams(). > > tools/perf/util/Build | 1 + > tools/perf/util/stream.c | 148 +++++++++++++++++++++++++++++++++++++++ > tools/perf/util/stream.h | 23 ++++++ > 3 files changed, 172 insertions(+) > create mode 100644 tools/perf/util/stream.c > create mode 100644 tools/perf/util/stream.h > > diff --git a/tools/perf/util/Build b/tools/perf/util/Build > index cd5e41960e64..6ffdf833cd51 100644 > --- a/tools/perf/util/Build > +++ b/tools/perf/util/Build > @@ -101,6 +101,7 @@ perf-y += call-path.o > perf-y += rwsem.o > perf-y += thread-stack.o > perf-y += spark.o > +perf-y += stream.o > perf-$(CONFIG_AUXTRACE) += auxtrace.o > perf-$(CONFIG_AUXTRACE) += intel-pt-decoder/ > perf-$(CONFIG_AUXTRACE) += intel-pt.o > diff --git a/tools/perf/util/stream.c b/tools/perf/util/stream.c > new file mode 100644 > index 000000000000..015c1d07ce3a > --- /dev/null > +++ b/tools/perf/util/stream.c > @@ -0,0 +1,148 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Compare and figure out the top N hottest streams > + * Copyright (c) 2020, Intel Corporation. > + * Author: Jin Yao > + */ > + > +#include <inttypes.h> > +#include <stdlib.h> > +#include <linux/zalloc.h> > +#include "debug.h" > +#include "hist.h" > +#include "sort.h" > +#include "stream.h" > +#include "evlist.h" > + > +static void free_evsel_streams(struct evsel_streams *es, int nr_evsel)
Please name the functions with the struct name first, followed by __ and then the method name, i.e. for this one:
static void evsel_streams__delete(struct evsel_streams *es, int nr_evsels)
One thing that made me curious is why nr_evsels (plural please) isn't in 'struct evsel_streams', why?
> +{ > + for (int i = 0; i < nr_evsel; i++) > + zfree(&es[i].streams); > + > + free(es); > +} > + > +static struct evsel_streams *create_evsel_streams(int nr_evsel, > + int nr_streams_max)
Oh, I see, you create just an array :-\
Please rename it to:
evsel_streams__new(int nr_evsels, int nr_streams_max)
> +{ > + struct evsel_streams *es; > + > + es = calloc(nr_evsel, sizeof(struct evsel_streams)); > + if (!es) > + return NULL; > + > + for (int i = 0; i < nr_evsel; i++) { > + struct evsel_streams *s = &es[i]; > + > + s->streams = calloc(nr_streams_max, sizeof(struct stream)); > + if (!s->streams) > + goto err; > + > + s->nr_streams_max = nr_streams_max; > + s->evsel_idx = -1; > + } > + > + return es; > + > +err: > + free_evsel_streams(es, nr_evsel); > + return NULL; > +} > + > +/* > + * The cnodes with high hit number are hot callchains. > + */ > +static void set_hot_cnode(struct evsel_streams *es, > + struct callchain_node *cnode)
Since it is static its kinda ok not to have the prefix, but I'd do it as:
static void evsel_streams__set_hot_cnode(...)
For ctags sake, for instance, i.e. navigate in vim using ctags.
> +{ > + int i, idx = 0; > + u64 hit; > + > + if (es->nr_streams < es->nr_streams_max) { > + i = es->nr_streams; > + es->streams[i].cnode = cnode; > + es->nr_streams++; > + return; > + } > + > + /* > + * Considering a few number of hot streams, only use simple > + * way to find the cnode with smallest hit number and replace. > + */ > + hit = (es->streams[0].cnode)->hit; > + for (i = 1; i < es->nr_streams; i++) { > + if ((es->streams[i].cnode)->hit < hit) { > + hit = (es->streams[i].cnode)->hit; > + idx = i; > + } > + } > + > + if (cnode->hit > hit) > + es->streams[idx].cnode = cnode; > +} > + > +static void update_hot_callchain(struct hist_entry *he, > + struct evsel_streams *es) > +{ > + struct rb_root *root = &he->sorted_chain; > + struct rb_node *rb_node = rb_first(root); > + struct callchain_node *cnode; > + > + while (rb_node) { > + cnode = rb_entry(rb_node, struct callchain_node, rb_node); > + set_hot_cnode(es, cnode); > + rb_node = rb_next(rb_node); > + } > +} > + > +static void init_hot_callchain(struct hists *hists, struct evsel_streams *es) > +{ > + struct rb_node *next = rb_first_cached(&hists->entries); > + > + while (next) { > + struct hist_entry *he; > + > + he = rb_entry(next, struct hist_entry, rb_node); > + update_hot_callchain(he, es); > + next = rb_next(&he->rb_node); > + } > +} > + > +static int evlist_init_callchain_streams(struct evlist *evlist, > + struct evsel_streams *es, int nr_es)
So here we miss just one extra _, i.e.:
+static int evlist__init_callchain_streams(struct evlist *evlist, + struct evsel_streams *es, int nr_es)
> +{ > + struct evsel *pos; > + int i = 0; > + > + BUG_ON(nr_es < evlist->core.nr_entries); > + > + evlist__for_each_entry(evlist, pos) { > + struct hists *hists = evsel__hists(pos); > + > + hists__output_resort(hists, NULL); > + init_hot_callchain(hists, &es[i]); > + es[i].evsel_idx = pos->idx; > + i++; > + } > + > + return 0; > +} > + > +struct evsel_streams *perf_evlist__create_streams(struct evlist *evlist, > + int nr_streams_max)
And here it should be:
+struct evsel_streams *evlist__create_streams(struct evlist *evlist, + int nr_streams_max)
I.e. without the perf_ prefix, since that is for 'struct perf_evlist' methods, that are in tools/lib/perf/, aka libperf
> +{ > + struct evsel_streams *es; > + int nr_evsel = evlist->core.nr_entries, ret = -1; > + > + es = create_evsel_streams(nr_evsel, nr_streams_max);
Minor nitpick, usually combine declaration with first attribution, if possible, i.e.:
+ struct evsel_streams *es = create_evsel_streams(nr_evsel, nr_streams_max);
> + if (!es) > + return NULL; > + > + ret = evlist_init_callchain_streams(evlist, es, nr_evsel); > + if (ret) { > + free_evsel_streams(es, nr_evsel); > + return NULL; > + } > + > + return es; > +} > diff --git a/tools/perf/util/stream.h b/tools/perf/util/stream.h > new file mode 100644 > index 000000000000..c6844c5787cb > --- /dev/null > +++ b/tools/perf/util/stream.h > @@ -0,0 +1,23 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#ifndef __PERF_STREAM_H > +#define __PERF_STREAM_H > + > +#include "callchain.h" > + > +struct stream { > + struct callchain_node *cnode; > +}; > + > +struct evsel_streams { > + struct stream *streams; > + int nr_streams_max; > + int nr_streams; > + int evsel_idx; > +}; > + > +struct evlist; > + > +struct evsel_streams *perf_evlist__create_streams(struct evlist *evlist, > + int nr_streams_max); > +
evlist__create_streams()
> +#endif /* __PERF_STREAM_H */ > -- > 2.17.1 >
--
- Arnaldo
| |