Message-ID: <20150702130226.GF8882@kernel.org>
Date:	Thu, 2 Jul 2015 10:02:26 -0300
From:	Arnaldo Carvalho de Melo <acme@...nel.org>
To:	Adrian Hunter <adrian.hunter@...el.com>
Cc:	Ingo Molnar <mingo@...nel.org>, linux-kernel@...r.kernel.org,
	Jiri Olsa <jolsa@...hat.com>, David Ahern <dsahern@...il.com>,
	Namhyung Kim <namhyung@...nel.org>,
	Peter Zijlstra <a.p.zijlstra@...llo.nl>,
	Stephane Eranian <eranian@...gle.com>,
	Vince Weaver <vince@...ter.net>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Subject: Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS

Em Thu, Jul 02, 2015 at 03:35:17PM +0300, Adrian Hunter escreveu:
> On 02/07/15 12:43, Ingo Molnar wrote:
> > 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.

Agreed, that is what tools/perf/util/evsel.c does, e.g.:

static struct {
        bool sample_id_all;
        bool exclude_guest;
        bool mmap2;
        bool cloexec;
        bool clockid;
        bool clockid_wrong;
} perf_missing_features;

And in __perf_evsel__open():

        /*
         * Must probe features in the order they were added to the
         * perf_event_attr interface.
         */
        if (!perf_missing_features.clockid_wrong && evsel->attr.use_clockid) {
                perf_missing_features.clockid_wrong = true;
                goto fallback_missing_features;
        } else if (!perf_missing_features.clockid && evsel->attr.use_clockid) {
                perf_missing_features.clockid = true;
                goto fallback_missing_features;
        } else if (!perf_missing_features.cloexec && (flags & PERF_FLAG_FD_CLOEXEC)) {
                perf_missing_features.cloexec = true;
                goto fallback_missing_features;
        } else if (!perf_missing_features.mmap2 && evsel->attr.mmap2) {
                perf_missing_features.mmap2 = true;
                goto fallback_missing_features;
        } else if (!perf_missing_features.exclude_guest &&
                   (evsel->attr.exclude_guest || evsel->attr.exclude_host)) {
                perf_missing_features.exclude_guest = true;
                goto fallback_missing_features;
        } else if (!perf_missing_features.sample_id_all) {
                perf_missing_features.sample_id_all = true;
                goto retry_sample_id;
        }

So it does already probe for features available in the kernel and reacts
as best as imagined so far to kernels lacking those features.

The whole point here is: who writes the library? Those who need it to
support some feature they want to have merged?

We can use the existing infrastructure to the fullest extent possible,
but at some point we need to extend it, who does it?
 
> > 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?

I haven't tested it so far, but it seems to improve things, given the
current infrastructure.

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

Powered by blists - more mailing lists