On Fri, 26 Oct 2012 13:29:58 -0700, Andi Kleen wrote:
> From: Andi Kleen <a...@linux.intel.com>
>
> perf record has a new option -W that enables weightened sampling.
>
> Add sorting support in top/report for the average weight per sample and the
> total weight sum. This allows to both compare relative cost per event
> and the total cost over the measurement period.
I expected the weight is used for scaling a sample period somehow - and
wondered the *somehow* part - when reading previous patch descriptions
but you just added sort keys. Is it what you intended originally?
>
> Add the necessary glue to perf report, record and the library.
>
> v2: Merge with new hist refactoring.
> Rename global_weight to weight and weight to local_weight.
But I think (total_)weight and avg_weight looks more natural.
> Signed-off-by: Andi Kleen <a...@linux.intel.com>
> ---
[snip]
> diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
> index 618d411..3800fb5 100644
> --- a/tools/perf/util/evsel.c
> +++ b/tools/perf/util/evsel.c
> @@ -445,6 +445,9 @@ void perf_evsel__config(struct perf_evsel *evsel, struct
> perf_record_opts *opts,
> attr->mmap_data = track;
> }
>
> + if (opts->sample_weight)
> + attr->sample_type |= PERF_SAMPLE_WEIGHT;
> +
> if (opts->call_graph) {
> attr->sample_type |= PERF_SAMPLE_CALLCHAIN;
>
> @@ -870,6 +873,7 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel,
> union perf_event *event,
> data->cpu = data->pid = data->tid = -1;
> data->stream_id = data->id = data->time = -1ULL;
> data->period = 1;
> + data->weight = 0;
Why not set it to 1 at first and ...
>
> if (event->header.type != PERF_RECORD_SAMPLE) {
> if (!evsel->attr.sample_id_all)
> @@ -941,6 +945,12 @@ int perf_evsel__parse_sample(struct perf_evsel *evsel,
> union perf_event *event,
> array++;
> }
>
> + data->weight = 0;
(It seems unnecessary)
> + if (type & PERF_SAMPLE_WEIGHT) {
> + data->weight = *array;
> + array++;
> + }
> +
> if (type & PERF_SAMPLE_READ) {
> fprintf(stderr, "PERF_SAMPLE_READ is unsupported for now\n");
> return -1;
[snip]
> @@ -268,13 +272,17 @@ static u8 symbol__parent_filter(const struct symbol
> *parent)
> static struct hist_entry *add_hist_entry(struct hists *hists,
> struct hist_entry *entry,
> struct addr_location *al,
> - u64 period)
> + u64 period,
> + u64 weight)
> {
> struct rb_node **p;
> struct rb_node *parent = NULL;
> struct hist_entry *he;
> int cmp;
>
> + if (weight == 0)
> + weight = 1;
> +
... get rid of the above?
> pthread_mutex_lock(&hists->lock);
>
> p = &hists->entries_in->rb_node;
> @@ -286,7 +294,7 @@ static struct hist_entry *add_hist_entry(struct hists
> *hists,
> cmp = hist_entry__cmp(entry, he);
>
> if (!cmp) {
> - he_stat__add_period(&he->stat, period);
> + he_stat__add_period(&he->stat, period, weight);
>
> /* If the map of an existing hist_entry has
> * become out-of-date due to an exec() or
> @@ -314,6 +322,7 @@ static struct hist_entry *add_hist_entry(struct hists
> *hists,
>
> rb_link_node(&he->rb_node_in, parent, p);
> rb_insert_color(&he->rb_node_in, hists->entries_in);
> + he->stat.weight += weight;
I'd suggest that the weight should be set to the 'entry' so that it can
be added when hist_entry__new() called.
Thanks,
Namhyung
> out:
> hist_entry__add_cpumode_period(he, al->cpumode, period);
> out_unlock:
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
