Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Thu, 4 Mar 2021 21:17:51 +0100
From:   Jiri Olsa <jolsa@redhat.com>
To:     Jin Yao <yao.jin@linux.intel.com>
Cc:     acme@kernel.org, jolsa@kernel.org, peterz@infradead.org,
        mingo@redhat.com, alexander.shishkin@linux.intel.com,
        Linux-kernel@vger.kernel.org, ak@linux.intel.com,
        kan.liang@intel.com, yao.jin@intel.com
Subject: Re: [PATCH] perf pmu: Validate raw event with sysfs exported format
 bits
Message-ID: <YEFAb7Q7yhkw6g9s@krava>
References: <20210303051736.26974-1-yao.jin@linux.intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20210303051736.26974-1-yao.jin@linux.intel.com>
Precedence: bulk

On Wed, Mar 03, 2021 at 01:17:36PM +0800, Jin Yao wrote:

SNIP

> The set bits in 'bits' indicate the invalid bits used in config.
> Finally use strbuf to report the invalid bits.
> 
> Some architectures may not export supported bits through sysfs,
> so if masks is 0, perf_pmu__config_valid just returns true.
> 
> After:
> 
> Single event:
> 
>   # ./perf stat -e cpu/r031234/ -a -- sleep 1
>   WARNING: event config '31234' not valid (bits 16 17 not supported by kernel)!
> 
>    Performance counter stats for 'system wide':
> 
>                    0      cpu/r031234/
> 
>          1.001403757 seconds time elapsed
> 
> Multiple events:
> 
>   # ./perf stat -e cpu/rf01234/,cpu/r031234/ -a -- sleep 1
>   WARNING: event config 'f01234' not valid (bits 20 22 not supported by kernel)!
>   WARNING: event config '31234' not valid (bits 16 17 not supported by kernel)!

right, seems useful

> 
>    Performance counter stats for 'system wide':
> 
>                    0      cpu/rf01234/
>                    0      cpu/r031234/
> 
> The warning is reported for invalid bits.
> 
> Signed-off-by: Jin Yao <yao.jin@linux.intel.com>
> ---
>  tools/perf/util/parse-events.c | 11 ++++++++++
>  tools/perf/util/pmu.c          | 38 ++++++++++++++++++++++++++++++++++
>  tools/perf/util/pmu.h          |  4 ++++
>  3 files changed, 53 insertions(+)
> 
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 42c84adeb2fb..1820b2c0a241 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -356,6 +356,17 @@ __add_event(struct list_head *list, int *idx,
>  	struct perf_cpu_map *cpus = pmu ? perf_cpu_map__get(pmu->cpus) :
>  			       cpu_list ? perf_cpu_map__new(cpu_list) : NULL;
>  

if we want it just for raw/numeric, we could add it earlier on,
like to parse_events_add_numeric call

but perhaps it might be helpful to check any pmu event,
could perhaps reveal some broken format 

> +	if (pmu && attr->type == PERF_TYPE_RAW) {
> +		struct strbuf buf = STRBUF_INIT;
> +
> +		if (!perf_pmu__config_valid(pmu, attr->config, &buf)) {
> +			pr_warning("WARNING: event config '%llx' not valid ("
> +				   "bits%s not supported by kernel)!\n",
> +				   attr->config, buf.buf);
> +		}
> +		strbuf_release(&buf);
> +	}
> +
>  	if (init_attr)
>  		event_attr_init(attr);
>  
> diff --git a/tools/perf/util/pmu.c b/tools/perf/util/pmu.c
> index 44ef28302fc7..5e361adb2698 100644
> --- a/tools/perf/util/pmu.c
> +++ b/tools/perf/util/pmu.c
> @@ -1812,3 +1812,41 @@ int perf_pmu__caps_parse(struct perf_pmu *pmu)
>  
>  	return nr_caps;
>  }
> +
> +bool perf_pmu__config_valid(struct perf_pmu *pmu, __u64 config,
> +			    struct strbuf *buf)
> +{
> +	struct perf_pmu_format *format;
> +	__u64 masks = 0, bits;
> +	int i;
> +
> +	list_for_each_entry(format, &pmu->format, list)	{
> +		/*
> +		 * Skip extra configs such as config1/config2.
> +		 */
> +		if (format->value > 0)
> +			continue;
> +
> +		for_each_set_bit(i, format->bits, PERF_PMU_FORMAT_BITS)
> +			masks |= 1ULL << i;
> +	}
> +
> +	/*
> +	 * Kernel doesn't export any valid format bits.
> +	 */
> +	if (masks == 0)
> +		return true;
> +
> +	bits = config & ~masks;
> +	if (bits == 0)
> +		return true;

so in here you now that there's something wrong, so why
bother with the outside strbuf, when we can easily just
do all the printing in here?

> +
> +	for (i = 0; i < PERF_PMU_FORMAT_BITS; i++) {
> +		if (bits & 0x1)
> +			strbuf_addf(buf, " %d", i);
> +
> +		bits >>= 1;

could you use the for_each_set_bit in here?

thanks,
jirka