Messages in this thread | | | Date | Thu, 2 Mar 2023 14:21:53 +0800 | From | Leo Yan <> | Subject | Re: [PATCH v3 08/14] perf kvm: Add dimensions for KVM event statistics |
| |
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
| |