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@linaro.org>
Reviewed-by: James Clark <james.clark@arm.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,
+};
+
+#define EV_METRIC_CMP(metric)						\
+static int64_t ev_cmp_##metric(struct perf_hpp_fmt *fmt __maybe_unused,	\
+			       struct hist_entry *left,			\
+			       struct hist_entry *right)		\
+{									\
+	struct kvm_event *event_left;					\
+	struct kvm_event *event_right;					\
+	struct perf_kvm_stat *perf_kvm;					\
+									\
+	event_left  = container_of(left, struct kvm_event, he);		\
+	event_right = container_of(right, struct kvm_event, he);	\
+									\
+	perf_kvm = event_left->perf_kvm;				\
+	return cmp_event_##metric(event_left, event_right,		\
+				  perf_kvm->trace_vcpu);		\
+}
+
+EV_METRIC_CMP(time)
+EV_METRIC_CMP(count)
+EV_METRIC_CMP(max)
+EV_METRIC_CMP(min)
+EV_METRIC_CMP(mean)
+
+static struct kvm_dimension dim_time = {
+	.name		= "time",
+	.cmp		= ev_cmp_time,
+};
+
+static struct kvm_dimension dim_count = {
+	.name		= "sample",
+	.cmp		= ev_cmp_count,
+};
+
+static struct kvm_dimension dim_max_time = {
+	.name		= "max_t",
+	.cmp		= ev_cmp_max,
+};
+
+static struct kvm_dimension dim_min_time = {
+	.name		= "min_t",
+	.cmp		= ev_cmp_min,
+};
+
+static struct kvm_dimension dim_mean_time = {
+	.name		= "mean_t",
+	.cmp		= ev_cmp_mean,
+};
+
+static struct kvm_dimension *dimensions[] = {
+	&dim_event,
+	&dim_time,
+	&dim_count,
+	&dim_max_time,
+	&dim_min_time,
+	&dim_mean_time,
+	NULL,
+};
+
+static bool fmt_equal(struct perf_hpp_fmt *a, struct perf_hpp_fmt *b)
+{
+	struct kvm_fmt *kvm_fmt_a = container_of(a, struct kvm_fmt, fmt);
+	struct kvm_fmt *kvm_fmt_b = container_of(b, struct kvm_fmt, fmt);
+
+	return kvm_fmt_a->dim == kvm_fmt_b->dim;
+}
+
+static void fmt_free(struct perf_hpp_fmt *fmt)
+{
+	struct kvm_fmt *kvm_fmt;
+
+	kvm_fmt = container_of(fmt, struct kvm_fmt, fmt);
+	free(kvm_fmt);
+}
+
+static struct kvm_dimension *get_dimension(const char *name)
 {
+	unsigned int i;
+
+	for (i = 0; dimensions[i] != NULL; i++) {
+		if (!strcmp(dimensions[i]->name, name))
+			return dimensions[i];
+	}
+
+	return NULL;
+}
+
+static struct kvm_fmt *get_format(const char *name)
+{
+	struct kvm_dimension *dim = get_dimension(name);
+	struct kvm_fmt *kvm_fmt;
+	struct perf_hpp_fmt *fmt;
+
+	if (!dim)
+		return NULL;
+
+	kvm_fmt = zalloc(sizeof(*kvm_fmt));
+	if (!kvm_fmt)
+		return NULL;
+
+	kvm_fmt->dim = dim;
+
+	fmt = &kvm_fmt->fmt;
+	INIT_LIST_HEAD(&fmt->list);
+	INIT_LIST_HEAD(&fmt->sort_list);
+	fmt->cmp	= dim->cmp;
+	fmt->sort	= dim->cmp;
+	fmt->color	= NULL;
+	fmt->entry	= NULL;
+	fmt->header	= NULL;
+	fmt->width	= NULL;
+	fmt->collapse	= dim->cmp;
+	fmt->equal	= fmt_equal;
+	fmt->free	= fmt_free;
+
+	return kvm_fmt;
+}
+
+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);
+	}
+
+	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);
+	}
+
+	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);
+
+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

Hi Leo,

On Tue, Feb 28, 2023 at 3:53 AM Leo Yan <leo.yan@linaro.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@linaro.org>
> Reviewed-by: James Clark <james.clark@arm.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
>

Hi Namhyung,

On Tue, Feb 28, 2023 at 03:34:28PM -0800, Namhyung Kim wrote:

[...]

> > +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.

I will rename as 'ev_name' or 'event_name'; and it's good point to
compare the name with string order.  Will do it.

[...]

> [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.

So far, we don't need to add any generic output field, I will remove
reset_dimensions() / output_field_add(), alternatively, in next spin I
will simply print an error info and return -EINVAL.

> > +       }
> > +
> > +       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.

Will do the same change with the above statement.

[...]

> > +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.

Will do.

Thanks a lot for reviewing!

Leo