Message-ID: <Yq3Xzz2XebvEJFcJ@FVFF77S0Q05N>
Date:   Sat, 18 Jun 2022 14:49:03 +0100
From:   Mark Rutland <mark.rutland@....com>
To:     Vince Weaver <vincent.weaver@...ne.edu>
Cc:     linux-kernel@...r.kernel.org, linux-perf-users@...r.kernel.org,
        Peter Zijlstra <peterz@...radead.org>,
        Arnaldo Carvalho de Melo <acme@...nel.org>,
        Namhyung Kim <namhyung@...nel.org>,
        Will Deacon <will@...nel.org>,
        Alexander Shishkin <alexander.shishkin@...ux.intel.com>,
        Ingo Molnar <mingo@...hat.com>
Subject: Re: READ_ONCE() usage in perf_mmap_read_self()

On Fri, Jun 17, 2022 at 04:13:22PM -0400, Vince Weaver wrote:
> Hello

Hi Vince,

> Is the perf_mmap__read_self() interface as found in tools/lib/perf/mmap.c
> the "official" interface for accessing the perf mmap info from
> userspace?

The tools/lib/perf stuff is *a* library for accessing it, but no-one's mandated
to use that, and in that sense it's not the "official" interface. The
"official" interface is whatever's documented in the UAPI headers, e.g.

  https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/include/uapi/linux/perf_event.h?h=v5.19-rc2#n517

... though I admit that's not as clear as it ideally would be, and we could
improve that and/or add some other documentation that's earier to consume.

> Are the READ_ONCE() calls required?  If they are left out, will accessing 
> the mmap interface potentially fail?  Has this ever been seen in practice?

There's a bunch of subtlety here. The gist is:
 
1) Here we use READ_ONCE() to make the loads single-copy-atomic (i.e. not torn
   into separate accesses). There are other ways that can be achieved without
   using READ_ONCE().

   In practice, compilers don't generally tear loads of less than the machine
   word size when used in "simple" sequences, but they can often replay loads
   assuming that the value hasn't changed. For the sequences here, I suspect
   it's very unlikely the compiler would tear accesses or replay them.

2) The reads of pc->lock need to be single-copy-atomic, or the loop may not
   correctly protect reads within the sequence. In the absnece of that, the
   result could be bogus (and as below, accesses to the counters could trigger
   unexpected exceptions).

3) Compiler barriers are necessary to ensure that loads within the loop are
   protected by pc->lock and not pulled out of the loop or cached over multiple
   iterations of the loop.

4) Data loaded *and consumed* within the loop may need to be loaded with
   single-copy-atomic accesses depending on how they are used. For example,
   pc->index needs to be loaded in a single-copt-atomic way or an erroeous
   counter index could be used, and the access to that counter might cause an
   exception.

   In contrast, dor data where we just perform some maths without side-effects,
   that wouldn't be important.

5) Data merely loaded within the loop doesn't strictly need to be loaded in a
   single-copy-atomic way. If there's concurrent modification, pc->lock will
   have changed, causing it to be diescarded, and if there's no concurrent
   modification the tearing is benign.

IIUC we marked everything with READ_ONCE() because that was simpler to reason
about (and very unlikely to be detrimental) rather than it being strictly
necessary.

Does that make sense and does that answer your questions?

> Part of why I am asking is both
>  	tools/lib/perf/mmap.c
> and
>  	tools/linux/compiler.h (which defines READ_ONCE)
> have SPDX headers indicating they are GPL-2.0 licensed, which means it 
> seems like it would not be possible to easily use the perf mmap interface 
> from BSD-licensed code.  Would it be possible to get those two files 
> re-licensed?

I suspect that's likely to be difficult since (IIUC) those are derived from
other kernel code licensed as GPL-2.0, and we'd need to get all relevant
contributors to agree to the relicensing.

> The (BSD-licensed) PAPI is currently using a mmap reading interface 
> based on early documentation for the feature, but it isn't 100% the same 
> as the version from libperf (and isn't using READ_ONCE).  Life would be 
> easier if we could use the perf version of the code because then we would 
> have one less variable to deal with when trying to track down issues.

Would clarifying the documentation be helpful here? We might be able to
document this more clearly and precisely, then align the tools/lib/perf/ code
with the documentation, even if we can't necessarily relicense that code as-is.

Thanks,
Mark.

Powered by blists - more mailing lists