Perf user-space ABI sequence lock memory barriers

Perf user-space ABI sequence lock memory barriers

[Mathieu Desnoyers]

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;
                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 ?

I would be tempted to use a volatile semantic on all reads of the
lock field (ACCESS_ONCE()). 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().

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

Thanks,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: Perf user-space ABI sequence lock memory barriers

[Peter Zijlstra]

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.

Re: Perf user-space ABI sequence lock memory barriers

[Mathieu Desnoyers]

----- Original Message -----
> From: "Peter Zijlstra" <peterz@infradead.org>
> To: "Mathieu Desnoyers" <mathieu.desnoyers@efficios.com>
> Cc: tglx@linutronix.de, "Heinz Egger" <Heinz.Egger@linutronix.de>, bigeasy@linutronix.de, "Linux Kernel Mailing List"
> <linux-kernel@vger.kernel.org>, "Ingo Molnar" <mingo@kernel.org>, "rostedt" <rostedt@goodmis.org>, "Paul E.
> McKenney" <paulmck@linux.vnet.ibm.com>
> Sent: Wednesday, February 5, 2014 3:05:18 AM
> Subject: Re: Perf user-space ABI sequence lock memory barriers
> 
> 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);

Ah, yes, although since as you describe, the data structure
is per-thread, there would be no need to do this.

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

Yes, this is what I meant, but I'm not sure it's absolutely required.

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

The one things that confused me in the note:

         * NOTE: for obvious reason this only works on self-monitoring
         *       processes.

is the use of the word "process" for a user-space API, when it actually
means "thread" in user-space semantic. Yes, I must have been doing too much
userland stuff lately. ;-)

Thanks for the clarification,

Mathieu

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com