Re: [PATCH v3 08/14] perf kvm: Add dimensions for KVM event statistics

[Leo Yan]

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