From: Frederic Weisbecker <fweisbec@gmail.com>
To: Michael Neuling <mikey@neuling.org>,
Peter Zijlstra <peterz@infradead.org>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>,
Anton Blanchard <anton@samba.org>,
LKML <linux-kernel@vger.kernel.org>,
Linux PPC dev <linuxppc-dev@ozlabs.org>,
Victor Kaplansky <VICTORK@il.ibm.com>,
Mathieu Desnoyers <mathieu.desnoyers@polymtl.ca>,
michael <michael@ellerman.id.au>
Subject: Re: perf events ring buffer memory barrier on powerpc
Date: Wed, 23 Oct 2013 15:25:44 +0100 [thread overview]
Message-ID: <CAFTL4hxGxSqWSaX0s7kSdXXY0sgSmM3intiKpM7H_4RhRP5DBg@mail.gmail.com> (raw)
In-Reply-To: <20131023141948.GB3566@localhost.localdomain>
2013/10/23 Frederic Weisbecker <fweisbec@gmail.com>:
> On Wed, Oct 23, 2013 at 10:54:54AM +1100, Michael Neuling wrote:
>> Frederic,
>>
>> In the perf ring buffer code we have this in perf_output_get_handle():
>>
>> if (!local_dec_and_test(&rb->nest))
>> goto out;
>>
>> /*
>> * Publish the known good head. Rely on the full barrier implied
>> * by atomic_dec_and_test() order the rb->head read and this
>> * write.
>> */
>> rb->user_page->data_head = head;
>>
>> The comment says atomic_dec_and_test() but the code is
>> local_dec_and_test().
>>
>> On powerpc, local_dec_and_test() doesn't have a memory barrier but
>> atomic_dec_and_test() does. Is the comment wrong, or is
>> local_dec_and_test() suppose to imply a memory barrier too and we have
>> it wrongly implemented in powerpc?
>>
>> My guess is that local_dec_and_test() is correct but we to add an
>> explicit memory barrier like below:
>>
>> (Kudos to Victor Kaplansky for finding this)
>>
>> Mikey
>>
>> diff --git a/kernel/events/ring_buffer.c b/kernel/events/ring_buffer.c
>> index cd55144..95768c6 100644
>> --- a/kernel/events/ring_buffer.c
>> +++ b/kernel/events/ring_buffer.c
>> @@ -87,10 +87,10 @@ again:
>> goto out;
>>
>> /*
>> - * Publish the known good head. Rely on the full barrier implied
>> - * by atomic_dec_and_test() order the rb->head read and this
>> - * write.
>> + * Publish the known good head. We need a memory barrier to order the
>> + * order the rb->head read and this write.
>> */
>> + smp_mb ();
>> rb->user_page->data_head = head;
>>
>> /*
>
>
> I'm adding Peter in Cc since he wrote that code.
> I agree that local_dec_and_test() doesn't need to imply an smp barrier.
> All it has to provide as a guarantee is the atomicity against local concurrent
> operations (interrupts, preemption, ...).
>
> Now I'm a bit confused about this barrier.
>
> I think we want this ordering:
>
> Kernel User
>
> READ rb->user_page->data_tail READ rb->user_page->data_head
> smp_mb() smp_mb()
> WRITE rb data READ rb data
> smp_mb() smp_mb()
> rb->user_page->data_head WRITE rb->user_page->data_tail
^^ I meant a write above for data_head.
next prev parent reply other threads:[~2013-10-23 14:25 UTC|newest]
Thread overview: 120+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-10-22 23:54 perf events ring buffer memory barrier on powerpc Michael Neuling
2013-10-23 7:39 ` Victor Kaplansky
2013-10-23 14:19 ` Frederic Weisbecker
2013-10-23 14:25 ` Frederic Weisbecker [this message]
2013-10-25 17:37 ` Peter Zijlstra
2013-10-25 20:31 ` Michael Neuling
2013-10-27 9:00 ` Victor Kaplansky
2013-10-28 9:22 ` Peter Zijlstra
2013-10-28 10:02 ` Frederic Weisbecker
2013-10-28 12:38 ` Victor Kaplansky
2013-10-28 13:26 ` Peter Zijlstra
2013-10-28 16:34 ` Paul E. McKenney
2013-10-28 20:17 ` Oleg Nesterov
2013-10-28 20:58 ` Victor Kaplansky
2013-10-29 10:21 ` Peter Zijlstra
2013-10-29 10:30 ` Peter Zijlstra
2013-10-29 10:35 ` Peter Zijlstra
2013-10-29 20:15 ` Oleg Nesterov
2013-10-29 19:27 ` Vince Weaver
2013-10-30 10:42 ` Peter Zijlstra
2013-10-30 11:48 ` James Hogan
2013-10-30 12:48 ` Peter Zijlstra
2013-11-06 13:19 ` [tip:perf/core] tools/perf: Add required memory barriers tip-bot for Peter Zijlstra
2013-11-06 13:50 ` Vince Weaver
2013-11-06 14:00 ` Peter Zijlstra
2013-11-06 14:28 ` Peter Zijlstra
2013-11-06 14:55 ` Vince Weaver
2013-11-06 15:10 ` Peter Zijlstra
2013-11-06 15:23 ` Peter Zijlstra
2013-11-06 14:44 ` Peter Zijlstra
2013-11-06 16:07 ` Peter Zijlstra
2013-11-06 17:31 ` Vince Weaver
2013-11-06 18:24 ` Peter Zijlstra
2013-11-07 8:21 ` Ingo Molnar
2013-11-07 14:27 ` Vince Weaver
2013-11-07 15:55 ` Ingo Molnar
2013-11-11 16:24 ` Peter Zijlstra
2013-11-11 21:10 ` Ingo Molnar
2013-10-29 21:23 ` perf events ring buffer memory barrier on powerpc Michael Neuling
2013-10-30 9:27 ` Paul E. McKenney
2013-10-30 11:25 ` Peter Zijlstra
2013-10-30 14:52 ` Victor Kaplansky
2013-10-30 15:39 ` Peter Zijlstra
2013-10-30 17:14 ` Victor Kaplansky
2013-10-30 17:44 ` Peter Zijlstra
2013-10-31 6:16 ` Paul E. McKenney
2013-11-01 13:12 ` Victor Kaplansky
2013-11-02 16:36 ` Paul E. McKenney
2013-11-02 17:26 ` Paul E. McKenney
2013-10-31 6:40 ` Paul E. McKenney
2013-11-01 14:25 ` Victor Kaplansky
2013-11-02 17:28 ` Paul E. McKenney
2013-11-01 14:56 ` Peter Zijlstra
2013-11-02 17:32 ` Paul E. McKenney
2013-11-03 14:40 ` Paul E. McKenney
2013-11-03 15:17 ` [RFC] arch: Introduce new TSO memory barrier smp_tmb() Peter Zijlstra
2013-11-03 18:08 ` Linus Torvalds
2013-11-03 20:01 ` Peter Zijlstra
2013-11-03 22:42 ` Paul E. McKenney
2013-11-03 23:34 ` Linus Torvalds
2013-11-04 10:51 ` Paul E. McKenney
2013-11-04 11:22 ` Peter Zijlstra
2013-11-04 16:27 ` Paul E. McKenney
2013-11-04 16:48 ` Peter Zijlstra
2013-11-04 19:11 ` Peter Zijlstra
2013-11-04 19:18 ` Peter Zijlstra
2013-11-04 20:54 ` Paul E. McKenney
2013-11-04 20:53 ` Paul E. McKenney
2013-11-05 14:05 ` Will Deacon
2013-11-05 14:49 ` Paul E. McKenney
2013-11-05 18:49 ` Peter Zijlstra
2013-11-06 11:00 ` Will Deacon
2013-11-06 12:39 ` Peter Zijlstra
2013-11-06 12:51 ` Geert Uytterhoeven
2013-11-06 13:57 ` Peter Zijlstra
2013-11-06 18:48 ` Paul E. McKenney
2013-11-06 19:42 ` Peter Zijlstra
2013-11-07 11:17 ` Will Deacon
2013-11-07 13:36 ` Peter Zijlstra
2013-11-07 23:50 ` Mathieu Desnoyers
2013-11-04 11:05 ` Will Deacon
2013-11-04 16:34 ` Paul E. McKenney
2013-11-03 20:59 ` Benjamin Herrenschmidt
2013-11-03 22:43 ` Paul E. McKenney
2013-11-03 17:07 ` perf events ring buffer memory barrier on powerpc Will Deacon
2013-11-03 22:47 ` Paul E. McKenney
2013-11-04 9:57 ` Will Deacon
2013-11-04 10:52 ` Paul E. McKenney
2013-11-01 16:11 ` Peter Zijlstra
2013-11-02 17:46 ` Paul E. McKenney
2013-11-01 16:18 ` Peter Zijlstra
2013-11-02 17:49 ` Paul E. McKenney
2013-10-30 13:28 ` Victor Kaplansky
2013-10-30 15:51 ` Peter Zijlstra
2013-10-30 18:29 ` Peter Zijlstra
2013-10-30 19:11 ` Peter Zijlstra
2013-10-31 4:33 ` Paul E. McKenney
2013-10-31 4:32 ` Paul E. McKenney
2013-10-31 9:04 ` Peter Zijlstra
2013-10-31 15:07 ` Paul E. McKenney
2013-10-31 15:19 ` Peter Zijlstra
2013-11-01 9:28 ` Paul E. McKenney
2013-11-01 10:30 ` Peter Zijlstra
2013-11-02 15:20 ` Paul E. McKenney
2013-11-04 9:07 ` Peter Zijlstra
2013-11-04 10:00 ` Paul E. McKenney
2013-10-31 9:59 ` Victor Kaplansky
2013-10-31 12:28 ` David Laight
2013-10-31 12:55 ` Victor Kaplansky
2013-10-31 15:25 ` Paul E. McKenney
2013-11-01 16:06 ` Victor Kaplansky
2013-11-01 16:25 ` David Laight
2013-11-01 16:30 ` Victor Kaplansky
2013-11-03 20:57 ` Benjamin Herrenschmidt
2013-11-02 15:46 ` Paul E. McKenney
2013-10-28 19:09 ` Oleg Nesterov
2013-10-29 14:06 ` [tip:perf/urgent] perf: Fix perf ring buffer memory ordering tip-bot for Peter Zijlstra
2014-05-08 20:46 perf events ring buffer memory barrier on powerpc Mikulas Patocka
[not found] ` <OF667059AA.7F151BCC-ONC2257CD3.0036CFEB-C2257CD3.003BBF01@il.ibm.com>
2014-05-09 12:20 ` Mikulas Patocka
2014-05-09 13:47 ` Paul E. McKenney
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAFTL4hxGxSqWSaX0s7kSdXXY0sgSmM3intiKpM7H_4RhRP5DBg@mail.gmail.com \
--to=fweisbec@gmail.com \
--cc=VICTORK@il.ibm.com \
--cc=anton@samba.org \
--cc=benh@kernel.crashing.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linuxppc-dev@ozlabs.org \
--cc=mathieu.desnoyers@polymtl.ca \
--cc=michael@ellerman.id.au \
--cc=mikey@neuling.org \
--cc=peterz@infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).