Message-ID: <20150702190059.GB15416@gmail.com>
Date:	Thu, 2 Jul 2015 21:00:59 +0200
From:	Ingo Molnar <mingo@...nel.org>
To:	Adrian Hunter <adrian.hunter@...el.com>
Cc:	Arnaldo Carvalho de Melo <acme@...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>,
	Arnaldo Carvalho de Melo <acme@...hat.com>,
	Vince Weaver <vince@...ter.net>,
	Alexander Shishkin <alexander.shishkin@...ux.intel.com>
Subject: Re: [GIT PULL 0/8] perf/pt -> Intel PT/BTS


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

> > 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 who on the Intel side has time to finish Intel/PT support properly?

As a first step I think we need to disable the Intel/PT kernel side - it was
merged with the express promise that proper tooling would come along promptly,
but if that's not happening due to lack of resources, then the kernel side is
obviously in limbo as well.

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

The method I suggested works _both_ with old and new kernels, with the new kernel 
simply giving better error feedback.

And these are not warts really, it's simply a threshold issue: above a certain 
complexity of features it makes sense to introduce a qualitatively better error 
reporting interface. When I tested Intel PT support I happened to go past that 
threshold and it became obvious that we need it.

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

So I think eventually a proper libperf will crystalize out of perf's syscall 
wrappers (I suggested it for a long time), but this has very little impact on the 
end user who doesn't care whether it's done in perf, or in some intermediate 
library.

So it can be done in the current perf code just as much - when libperf gets 
introduced it will be factored out into tools/lib/perf/ without much problem and 
then other tools can use it too.

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

I partially agree - but it's really a problem that we currently only get two 
states:

  - the PMU driver is there in sysfs
  - the PMU driver is not there in sysfs

while we really want to a lot more about why it's not there - and it's the kernel 
that knows this best, not tooling.

So maybe instead of having a separate directory, we could have a sysfs file that 
listed all the PMU drivers that the kernel knows about, with a status line that 
tells us whether they are:

  - not configured
  - configured but runtime disabled due to reason X or Y or Z
  - configured and enabled

OTOH having the directories with a single file in them is a close substitute, and 
better meets the sysfs 'one file, one value' principle.

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

Well, no, because it really does the check in the wrong place and introduces 
possible friction between what tooling thinks is the supported environment and 
what the kernel thinks - and thus postpones the problem.

I could live with this current solution as the initial version if a subsequent 
effort fixes it up properly by adding much better error reporting infrastructure, 
but the 'no time' comment makes me doubt the whole Intel/PT feature set.

Thanks,

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