| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <CAM9d7cgEZ4ugAfef8hDKOr11xF7QP1w7-Ax3mnDr4XXmYr0DoQ@mail.gmail.com>
Date: Tue, 28 Feb 2023 15:34:28 -0800
From: Namhyung Kim <namhyung@...nel.org>
To: Leo Yan <leo.yan@...aro.org>
Cc: Arnaldo Carvalho de Melo <acme@...nel.org>,
Peter Zijlstra <peterz@...radead.org>,
Ingo Molnar <mingo@...hat.com>,
Mark Rutland <mark.rutland@....com>,
Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
Jiri Olsa <jolsa@...nel.org>, Ian Rogers <irogers@...gle.com>,
Adrian Hunter <adrian.hunter@...el.com>,
James Clark <james.clark@....com>,
linux-perf-users@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 08/14] perf kvm: Add dimensions for KVM event statistics
Hi Leo,
On Tue, Feb 28, 2023 at 3:53 AM Leo Yan <leo.yan@...aro.org> wrote:
>
> To support KVM event statistics, this patch firstly registers histograms
> columns and sorting fields; every column or field has its own format
> structure, the format structure is dereferenced to access the dimension,
> finally the dimension provides the comparison callback for sorting
> result.
>
> Signed-off-by: Leo Yan <leo.yan@...aro.org>
> Reviewed-by: James Clark <james.clark@....com>
> ---
> tools/perf/builtin-kvm.c | 239 +++++++++++++++++++++++++++++++++++--
> tools/perf/util/kvm-stat.h | 2 +
> 2 files changed, 234 insertions(+), 7 deletions(-)
>
> diff --git a/tools/perf/builtin-kvm.c b/tools/perf/builtin-kvm.c
> index a9f467926bdd..da84f5063d4d 100644
> --- a/tools/perf/builtin-kvm.c
> +++ b/tools/perf/builtin-kvm.c
> @@ -70,9 +70,9 @@ static int cmp_event_ ## func(struct kvm_event *one, \
> get_event_ ##func(two, vcpu); \
> }
>
> -GET_EVENT_KEY(time, time);
> -GET_EVENT_KEY(max, stats.max);
> -GET_EVENT_KEY(min, stats.min);
> +COMPARE_EVENT_KEY(time, time);
> +COMPARE_EVENT_KEY(max, stats.max);
> +COMPARE_EVENT_KEY(min, stats.min);
> COMPARE_EVENT_KEY(count, stats.n);
> COMPARE_EVENT_KEY(mean, stats.mean);
>
> @@ -90,13 +90,238 @@ struct kvm_hists {
> struct perf_hpp_list list;
> };
>
> +struct kvm_dimension {
> + const char *name;
> + int64_t (*cmp)(struct perf_hpp_fmt *fmt, struct hist_entry *left,
> + struct hist_entry *right);
> +};
> +
> +struct kvm_fmt {
> + struct perf_hpp_fmt fmt;
> + struct kvm_dimension *dim;
> +};
> +
> static struct kvm_hists kvm_hists;
>
> -static int kvm_hists__init(void)
> +static int64_t
> +empty_cmp(struct perf_hpp_fmt *fmt __maybe_unused,
> + struct hist_entry *left __maybe_unused,
> + struct hist_entry *right __maybe_unused)
> +{
> + return 0;
> +}
> +
> +static struct kvm_dimension dim_event = {
> + .name = "name",
> + .cmp = empty_cmp,
> +};
I guess you can name it as 'dim_name' to reduce the confusion.
Also it can compare event names using strcmp() in case users
want to see event names in alphabetical order.
[SNIP]
> +
> +static int kvm_hists__init_output(struct perf_hpp_list *hpp_list, char *name)
> +{
> + struct kvm_fmt *kvm_fmt = get_format(name);
> +
> + if (!kvm_fmt) {
> + reset_dimensions();
> + return output_field_add(hpp_list, name);
Hmm.. do you plan to support these generic output fields too?
I'm not sure you need reset_dimensions() here.
> + }
> +
> + perf_hpp_list__column_register(hpp_list, &kvm_fmt->fmt);
> + return 0;
> +}
> +
> +static int kvm_hists__init_sort(struct perf_hpp_list *hpp_list, char *name)
> +{
> + struct kvm_fmt *kvm_fmt = get_format(name);
> +
> + if (!kvm_fmt) {
> + reset_dimensions();
> + return sort_dimension__add(hpp_list, name, NULL, 0);
Ditto.
> + }
> +
> + perf_hpp_list__register_sort_field(hpp_list, &kvm_fmt->fmt);
> + return 0;
> +}
> +
> +static int kvm_hpp_list__init(char *list,
> + struct perf_hpp_list *hpp_list,
> + int (*fn)(struct perf_hpp_list *hpp_list,
> + char *name))
> +{
> + char *tmp, *tok;
> + int ret;
> +
> + if (!list || !fn)
> + return 0;
> +
> + for (tok = strtok_r(list, ", ", &tmp); tok;
> + tok = strtok_r(NULL, ", ", &tmp)) {
> + ret = fn(hpp_list, tok);
> + if (!ret)
> + continue;
> +
> + /* Handle errors */
> + if (ret == -EINVAL)
> + pr_err("Invalid field key: '%s'", tok);
> + else if (ret == -ESRCH)
> + pr_err("Unknown field key: '%s'", tok);
> + else
> + pr_err("Fail to initialize for field key: '%s'", tok);
> +
> + break;
> + }
> +
> + return ret;
> +}
> +
> +static int kvm_hpp_list__parse(struct perf_hpp_list *hpp_list,
> + const char *output_, const char *sort_)
> +{
> + char *output = output_ ? strdup(output_) : NULL;
> + char *sort = sort_ ? strdup(sort_) : NULL;
> + int ret;
> +
> + ret = kvm_hpp_list__init(output, hpp_list, kvm_hists__init_output);
> + if (ret)
> + goto out;
> +
> + ret = kvm_hpp_list__init(sort, hpp_list, kvm_hists__init_sort);
> + if (ret)
> + goto out;
> +
> + /* Copy sort keys to output fields */
> + perf_hpp__setup_output_field(hpp_list);
I think you also need perf_hpp__append_sort_keys() as in
setup_sorting() to have secondary sort keys in case the
given sort key cannot determine the ordering.
Thanks,
Namhyung
> +
> +out:
> + free(output);
> + free(sort);
> + return ret;
> +}
> +
> +static int kvm_hists__init(struct perf_kvm_stat *kvm)
> +{
> + const char *output_columns = "name,sample,time,max_t,min_t,mean_t";
> +
> __hists__init(&kvm_hists.hists, &kvm_hists.list);
> perf_hpp_list__init(&kvm_hists.list);
> - return 0;
> + return kvm_hpp_list__parse(&kvm_hists.list, output_columns,
> + kvm->sort_key);
> }
>
> static const char *get_filename_for_perf_kvm(void)
> @@ -979,7 +1204,7 @@ static int kvm_events_live_report(struct perf_kvm_stat *kvm)
> set_term_quiet_input(&save);
> init_kvm_event_record(kvm);
>
> - kvm_hists__init();
> + kvm_hists__init(kvm);
>
> signal(SIGINT, sig_handler);
> signal(SIGTERM, sig_handler);
> @@ -1176,7 +1401,7 @@ static int kvm_events_report_vcpu(struct perf_kvm_stat *kvm)
> init_kvm_event_record(kvm);
> setup_pager();
>
> - kvm_hists__init();
> + kvm_hists__init(kvm);
>
> ret = read_events(kvm);
> if (ret)
> diff --git a/tools/perf/util/kvm-stat.h b/tools/perf/util/kvm-stat.h
> index 80d5c5a9ae31..ca5796959f66 100644
> --- a/tools/perf/util/kvm-stat.h
> +++ b/tools/perf/util/kvm-stat.h
> @@ -40,6 +40,8 @@ struct kvm_event {
> #define DEFAULT_VCPU_NUM 8
> int max_vcpu;
> struct kvm_event_stats *vcpu;
> +
> + struct hist_entry he;
> };
>
> typedef int (*key_cmp_fun)(struct kvm_event*, struct kvm_event*, int);
> --
> 2.34.1
>
Powered by blists - more mailing lists