Re: Perf user-space ABI sequence lock memory barriers

[Peter Zijlstra <peterz@infradead.org>]

On Tue, Feb 04, 2014 at 10:56:24PM +0000, Mathieu Desnoyers wrote:
> Hi,
> 
> I'm currently integrating user-space performance counters from
> Perf into LTTng-UST, and I'm noticing something odd regarding
> the home-made sequence lock found at:
> 
> kernel/events/core.c: perf_event_update_userpage()
> 
>         ++userpg->lock;
>         barrier();
> [...]
>         barrier();
>         ++userpg->lock;
> 
> This goes in pair with something like this at user-level:
> 
>         do {
>                 seq = pc->lock;

You could make that:

		while ((seq = pc->lock) & 1);

>                 barrier();
> 
>                 idx = pc->index;
>                 count = pc->offset;
>                 if (idx)
>                         count += rdpmc(idx - 1);
> 
>                 barrier();
>         } while (pc->lock != seq);
> 
> As we see, only compiler barrier() are protecting all this.
> First question, is it possible that the update be performed
> by a thread running on a different CPU than the thread reading
> the info in user-space ?

You can make that so, but that is not a 'supported' case. This all
assumes you're monitoring yourself, in which case the event is ran on
the cpu you are running on too and the updates are matched on cpu, or
separated by schedule() which includes the required memory barriers to
make it appear its all on the same cpu anyway.

> I would be tempted to use a volatile semantic on all reads of the
> lock field (ACCESS_ONCE()).

Since its all separated by the compiler barrier all the reads should be
contained and the compiler is not allowed to re-read once outside.

So I don't see the point of volatile/ACCESS_ONCE here.

You could make an argument for ACCESS_ONCE(pc->lock) though.

> Secondly, read sequence locks usually use a
> smp_rmb() at the end of the seqcount_begin(), and at the beginning
> of the seqcount_retry(). Moreover, this is usually matched
> by smp_wmb() in write_seqcount begin/end().

Given this is all for self-monitoring and hard assuming the event runs
on the same cpu, smp barriers are pointless.

> Am I missing something special about this lock that makes these
> barriers unnecessary ?

The self-monitoring aspect perhaps? But there's a NOTE in struct
perf_event_mmap_page() that's rather a dead give-away on that though.