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

[Adrian Hunter]

On 02/07/15 12:43, Ingo Molnar wrote:
> 
> * Adrian Hunter <adrian.hunter@xxxxxxxxx> wrote:
> 
>> How is this?
>>
>> From: Adrian Hunter <adrian.hunter@xxxxxxxxx>
>> 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@xxxxxxxxx>
>> ---
>>  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).

Don't have time sorry.

> 
> 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?

Normally the warts of syscalls are hidden by libraries.  I am not sure a
library couldn't do just as well with the advantage that it would work for
old kernels too.  A library could also have information about multiple
architectures, not just the one that is actually running e.g. your BTS
example won't work on ARM.

A library could provide a similar API to the one you described.  Either it
just does the syscall and returns, or if extended error information is
requested, it probes the API with various options, checks
perf_event_paranoid, and generally uses whatever information it can to best
figure out what went wrong.

> 
> 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.

That presumes the user understands what is or is not available on their
architecture, because on a non-x86 architecture they still get nothing
x86-specific.

It also runs a bit against the way config works e.g. even if the PMU driver
is config'ed out it still has to provide a sysfs interface.

So, isn't the patch I proposed sufficient for now?

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/