Date:	Thu, 21 Jul 2016 10:49:29 +0100
From:	Mark Rutland <mark.rutland@....com>
To:	Jiri Olsa <jolsa@...hat.com>
Cc:	linux-kernel@...r.kernel.org, acme@...nel.org,
	adrian.hunter@...el.com, alexander.shishkin@...ux.intel.com,
	hekuang@...wei.com, jolsa@...nel.org, kan.liang@...el.com,
	mingo@...hat.com, peterz@...radead.org, wangnan0@...wei.com
Subject: Re: [RFCv2 4/4] perf: util: support sysfs supported_cpumask file

On Thu, Jul 21, 2016 at 10:10:35AM +0200, Jiri Olsa wrote:
> On Mon, Jul 18, 2016 at 04:00:45PM +0100, Mark Rutland wrote:
> > On Mon, Jul 18, 2016 at 04:30:18PM +0200, Jiri Olsa wrote:
> > > On Fri, Jul 15, 2016 at 11:08:13AM +0100, Mark Rutland wrote:
> > > > For system PMUs, the perf tools have long expected a cpumask file under
> > > > sysfs, describing the single CPU which they support events being
> > > 
> > > single cpu? it's cpumask.. 
> > 
> > Indeed.
> > 
> > The issue is that in practice, due to an internal inconsistency the
> > perf tools only work work when a single CPU is described in the mask.
> > More details below (and in patch 1).
> > 
> > > > opened/handled on. Prior patches in this series have reworked this
> > > > support to support multiple CPUs in a mask, as is required to handle
> > > > heterogeneous CPU PMUs.
> > > > 
> > > > Unfortunately, adding a cpumask file to CPU PMUs would break existing
> > > > userspace. Prior to this series, perf record will refuse to open events,
> > > 
> > > I'm lost.. we already have 'cpumask' file under pmu..
> > 
> > Sorry, I should spell out the problem more concretely:
> > 
> > When manipulating events, the tools sometimes use evsel->cpus, and other
> > times evlist->cpus. Sometimes, the two are used inconsistently, which
> > only works if they are the same size and/or describe the same CPUs.
> > Patch 1 fixes an instance of this, where the inconsistency results in
> > treating uninitialised memory as perf event FDs.
> > 
> > In the absence of a PMU cpumask file, the evsel's cpumask is initialised
> > to that of the evlist, so things line up.
> > 
> > Currently the only PMUs which happen to expose a cpumask are uncore
> > PMUs, which in practice only describe a single CPU.
> > 
> > When recording system-wide, various parts of the perf tools assume a
> > single CPU, regardless of evlist->cpus, for the purpose of manipulating
> > events. This happens to make uncore PMUs work, avoiding the
> > inconsistency.
> > 
> > Were we to just add a 'cpumask' file to our CPU PMUs, we would break
> > existing userspace (e.g. hitting the issue fixed in patch 1).
> 
> so you're saying that perf is broken once pmu's cpumask
> contains more than single cpu, is that right?

Yes.

> we should fix that, not make workarounds.. I'll go check,
> I might be still missing something ;-)

I certainly agree that this should be fixed in the perf tool; hence
patches 1-3. ;)

The problem the workaround is trying to solve is kernel compatibility
with existing binaries, for which (prior to this series):

- perf record doesn't work by default in heterogeneous systems in the
  *absence* of a cpumask.

- perf stat doesn't work by default in heterogeneous systems in the
  *presence* of a cpumask.

The kernel doesn't *currently* expose a cpumask for the ARM CPU PMUs, so
we'd need to add one. While new userspace should work as of these
patches, I can't add a file called 'cpumask' kernel-side without
breaking existing perf existing binaries (in the case of perf stat).

If it's possible to solve this without exposing a cpumask file at all,
that would be ideal, but so far I haven't been able to make that work.
Any ideas welcome!

> would be great to have some automated test for this stuff 

Good point. I will take a look into that.

Thanks,
Mark.

[1] http://lkml.kernel.org/r/1468577293-19667-1-git-send-email-mark.rutland@arm.com

Powered by blists - more mailing lists