Re: [RFCv2 4/4] perf: util: support sysfs supported_cpumask file

[Mark Rutland <mark.rutland@arm.com>]

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

The difference in naming allows new userspace to do the right thing
while not breaking existing userspace, though I agree it's somewhat
clunky.

> > and perf stat may unexpectedly block at exit time. In the absence of a
> > cpumask, perf stat is functional.
> > 
> > To address this, this patch adds support for a new file,
> > supported_cpumask, which can be used to describe heterogeneous CPUs,
> > without the risk of breaking existing userspace binaries.
> 
> is there kernel patch adding supported_cpumask support?

Modulo the naming, a patch exists [1]. We were holding off adding that
until we'd figured out how to address breaking existing userspace [2].

Thanks,
Mark.

[1] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-June/438239.html
[2] http://lists.infradead.org/pipermail/linux-arm-kernel/2016-July/441953.html