Re: [PATCH v3] perf pmu: Validate raw event with sysfs exported format bits

[Jin, Yao]

Hi Jiri,

On 3/8/2021 9:14 PM, Jiri Olsa wrote:
> On Mon, Mar 08, 2021 at 08:57:49PM +0800, Jin, Yao wrote:
> > Hi Jiri,
> >
> > On 3/8/2021 6:35 PM, Jiri Olsa wrote:
> > > On Mon, Mar 08, 2021 at 11:15:06AM +0800, Jin Yao wrote:
> > >
> > > SNIP
> > >
> > > > diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> > > > index 44ef28302fc7..03ab1e6d0418 100644
> > > > --- a/tools/perf/util/pmu.c
> > > > +++ b/tools/perf/util/pmu.c
> > > > @@ -1812,3 +1812,39 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
> > > >    	return nr_caps;
> > > >    }
> > > > +
> > > > +void perf_pmu__warn_invalid_config(struct perf_pmu *pmu, __u64 config,
> > > > +				   char *name)
> > > > +{
> > > > +	struct perf_pmu_format *format;
> > > > +	__u64 masks = 0, bits;
> > > > +	char buf[100];
> > > > +	unsigned int i;
> > > > +
> > > > +	list_for_each_entry(format, &pmu->format, list)	{
> > > > +		/*
> > > > +		 * Skip extra configs such as config1/config2.
> > > > +		 */
> > > > +		if (format->value > 0)
> > > > +			continue;
> > >
> > > sorry I did not notice before, but could you please use more direct
> > > approach like:
> > >
> > > 		if (format->value == PERF_PMU_FORMAT_VALUE_CONFIG) {
> > > 			break;
> > > 		}
> > >
> > > this will be more obvious, also no need for the comment.. I spent some
> > > time looking what's the value for ;-)
> > >
> > > thanks,
> > > jirka
> >
> > Oh, yes, using PERF_PMU_FORMAT_VALUE_CONFIG is much more obvious. Sorry about that!
> >
> > While it can't break the loop, because we need to iterate over the whole
> > list to get the total valid bits. So like:
> >
> > if (format->value != PERF_PMU_FORMAT_VALUE_CONFIG)
> > 	continue;
> >
> > Is it right?
>
> sure, what I meant was to process only PERF_PMU_FORMAT_VALUE_CONFIG
> and then call break, because there's no need to iterate further
>
> jirka

Sorry, maybe I still misunderstood what you suggested.

My understanding is we still need to iterate the whole formats list even we find a 
PERF_PMU_FORMAT_VALUE_CONFIG.

root@kbl-ppc:/sys/devices/cpu/format# ls
any  cmask  edge  event  frontend  in_tx  in_tx_cp  inv  ldlat  offcore_rsp  pc  umask
root@kbl-ppc:/sys/devices/cpu/format# cat any
config:21
root@kbl-ppc:/sys/devices/cpu/format# cat cmask
config:24-31
root@kbl-ppc:/sys/devices/cpu/format# cat edge
config:18
root@kbl-ppc:/sys/devices/cpu/format# cat edge
config:18
root@kbl-ppc:/sys/devices/cpu/format# cat event
config:0-7
root@kbl-ppc:/sys/devices/cpu/format# cat frontend
config1:0-23
root@kbl-ppc:/sys/devices/cpu/format# cat in_tx_cp
config:33
root@kbl-ppc:/sys/devices/cpu/format# cat inv
config:23
root@kbl-ppc:/sys/devices/cpu/format# cat ldlat
config1:0-15
root@kbl-ppc:/sys/devices/cpu/format# cat offcore_rsp
config1:0-63
root@kbl-ppc:/sys/devices/cpu/format# cat pc
config:19
root@kbl-ppc:/sys/devices/cpu/format# cat umask
config:8-15

If we break the loop when we get the first PERF_PMU_FORMAT_VALUE_CONFIG, we will only get the format 
'any', right?

Thanks
Jin Yao