Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS

[Ingo Molnar <mingo@kernel.org>]

* Adrian Hunter <adrian.hunter@intel.com> wrote:

> How is this?
> 
> From: Adrian Hunter <adrian.hunter@intel.com>
> Date: Wed, 1 Jul 2015 11:14:50 +0300
> Subject: [PATCH] perf tools: Add error messages for missing intel_bts and
>  intel_pt support
> 
> Add error messages to assist users in determining why there is
> no intel_bts or intel_pt support.
> 
> Signed-off-by: Adrian Hunter <adrian.hunter@intel.com>
> ---
>  tools/perf/arch/x86/util/header.c | 15 ++++++++++++++
>  tools/perf/util/header.h          |  3 ++-
>  tools/perf/util/parse-events.c    | 41 ++++++++++++++++++++++++++++++++++++++-
>  3 files changed, 57 insertions(+), 2 deletions(-)
> 
> diff --git a/tools/perf/arch/x86/util/header.c b/tools/perf/arch/x86/util/header.c
> index 146d12a1cec0..afc5bdfd2d15 100644
> --- a/tools/perf/arch/x86/util/header.c
> +++ b/tools/perf/arch/x86/util/header.c
> @@ -57,3 +57,18 @@ get_cpuid(char *buffer, size_t sz)
>  	}
>  	return -1;
>  }
> +
> +int have_intel_cpu(void)
> +{
> +	char buffer[64];
> +	int ret;
> +
> +	ret = get_cpuid(buffer, sizeof(buffer));
> +	if (ret)
> +		return -1;
> +
> +	if (!strncmp(buffer, "GenuineIntel,", 13))
> +		return 1;
> +
> +	return 0;
> +}
> diff --git a/tools/perf/util/header.h b/tools/perf/util/header.h
> index d4d57962c591..f6eab49d13d1 100644
> --- a/tools/perf/util/header.h
> +++ b/tools/perf/util/header.h
> @@ -153,8 +153,9 @@ bool is_perf_magic(u64 magic);
>  int write_padded(int fd, const void *bf, size_t count, size_t count_aligned);
>  
>  /*
> - * arch specific callback
> + * arch specific callbacks
>   */
>  int get_cpuid(char *buffer, size_t sz);
> +int have_intel_cpu(void);
>  
>  #endif /* __PERF_HEADER_H */
> diff --git a/tools/perf/util/parse-events.c b/tools/perf/util/parse-events.c
> index 09f8d2357108..23fb777b40fa 100644
> --- a/tools/perf/util/parse-events.c
> +++ b/tools/perf/util/parse-events.c
> @@ -656,6 +656,45 @@ static char *pmu_event_name(struct list_head *head_terms)
>  	return NULL;
>  }
>  
> +int __weak have_intel_cpu(void)
> +{
> +	return 0;
> +}
> +
> +static int pmu_not_found_error(char *name, struct parse_events_error *err)
> +{
> +	int ret;
> +
> +	if (!err)
> +		goto out;
> +
> +	if (!strcmp(name, "intel_bts")) {
> +		ret = have_intel_cpu();
> +		if (ret < 0)
> +			goto out;
> +		if (!ret) {
> +			err->str = strdup("intel_bts requires an Intel CPU");
> +			goto out;
> +		}
> +		err->str = strdup("kernel does not support intel_bts (requires 64-bit v4.1 kernel or later and BTS hardware support)");
> +		goto out;
> +	}
> +
> +	if (!strcmp(name, "intel_pt")) {
> +		ret = have_intel_cpu();
> +		if (ret < 0)
> +			goto out;
> +		if (!ret) {
> +			err->str = strdup("intel_pt requires an Intel CPU (Core 5th generation or later)");
> +			goto out;
> +		}
> +		err->str = strdup("kernel does not support intel_pt (requires v4.1 kernel or later and 5th generation Intel Core processor or later)");
> +		goto out;
> +	}
> +out:
> +	return -EINVAL;
> +}
> +
>  int parse_events_add_pmu(struct parse_events_evlist *data,
>  			 struct list_head *list, char *name,
>  			 struct list_head *head_config)
> @@ -667,7 +706,7 @@ int parse_events_add_pmu(struct parse_events_evlist *data,
>  
>  	pmu = perf_pmu__find(name);
>  	if (!pmu)
> -		return -EINVAL;
> +		return pmu_not_found_error(name, data->error);
>  
>  	if (pmu->default_config) {
>  		memcpy(&attr, pmu->default_config,

So I really think we need an extended error reporting feature on the perf kernel 
side, so that a 'natural' error (plus a string) is reported back to tooling, 
instead of the current -EINVAL.

No need to do it for everything, doing it for BTS and related functionality would 
be a good first step to start this.

If you are interested you could try this, or I can try to write something (after 
the merge window).

So the idea would be to convert such opaque error returns:

        if (attr->config == PERF_COUNT_HW_BRANCH_INSTRUCTIONS &&
            !attr->freq && hwc->sample_period == 1) {
                /* BTS is not supported by this architecture. */
                if (!x86_pmu.bts_active)
                        return -EOPNOTSUPP;

into:

			return perf_err(event, -EOPNOTSUPP, "The BTS hardware feature is not available on this CPU.");

Where perf_err() is a function that on one hand returns -EOPNOTSUPP - so 'legacy' 
error handling works as before: the syscall will return -EOPNOTSUPP.

But if a new 'extended error reporting' feature bit is set in the perf_attr, then 
perf_err() also does the following:

 - it copies the error string either back to a user-space pointer which is in the 
   perf_attr (plus a max length field)

 - or an alternative implementation would be to puts the string into the event's 
   ring buffer, with a special (new) event marker - where it can be recovered by 
   tooling.

(I think the first approach is better, because it would work fine for events 
without ring-buffers as well.)

Old tooling won't have the feature flag set and won't have the string pointer in 
perf_attr, so nothing will happen on that case.

New tooling that supports 'extended error reporting' has the feature flag set and 
the kernel will copy the error string into the provided user-space string buffer, 
where tooling could use that string to generate more meaningful error messages:

  perf syscall error: The BTS hardware feature is not available on this CPU.

Does this make sense to you?

Now an additional complication is the fact that BTS can now also be a separate 
PMU, listed under /sys/bus/event_source/devices/intel_bts/.

If it's not listed there, we don't know the exact reason: is it not available 
because it's an old kernel? Or is it the wrong CPU?

We could solve that by extending the sysfs interface and adding an "error" file to 
the PMU directory: which would contain the reason why the driver was not created.

I.e. if the BTS driver was not created, we'd still have 
/sys/bus/event_source/devices/intel_bts/error (and no other file), which gives 
tooling a good way to discover why a particular PMU is not available. This adds a 
tiny bit more RAM overhead, but it's for the better I think, because tooling could 
be a lot more certain about what the capabilities of the kernel are.

Thanks,

	Ingo