From: Peter Zijlstra <peterz@infradead.org>
To: Adrian Hunter <adrian.hunter@intel.com>
Cc: Leo Yan <leo.yan@linaro.org>,
Arnaldo Carvalho de Melo <acme@kernel.org>,
Ingo Molnar <mingo@redhat.com>,
Mark Rutland <mark.rutland@arm.com>,
Alexander Shishkin <alexander.shishkin@linux.intel.com>,
Namhyung Kim <namhyung@kernel.org>,
Andi Kleen <ak@linux.intel.com>,
linux-perf-users@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1 1/2] perf auxtrace: Change to use SMP memory barriers
Date: Thu, 27 May 2021 10:11:02 +0200 [thread overview]
Message-ID: <YK9UFl7Grv/mwmVc@hirez.programming.kicks-ass.net> (raw)
In-Reply-To: <d1fc3dd3-e79a-4e93-1083-6b08e0cabe59@intel.com>
On Thu, May 27, 2021 at 10:54:56AM +0300, Adrian Hunter wrote:
> On 19/05/21 5:03 pm, Leo Yan wrote:
> > The AUX ring buffer's head and tail can be accessed from multiple CPUs
> > on SMP system, so changes to use SMP memory barriers to replace the
> > uniprocessor barriers.
>
> I don't think user space should attempt to be SMP-aware.
Uhh, what? It pretty much has to. Since userspace cannot assume UP, it
must assume SMP.
> For perf tools, on __x86_64__ it looks like smp_rmb() is only a compiler barrier, whereas
> rmb() is a "lfence" memory barrier instruction, so this patch does not
> seem to do what the commit message says at least for x86.
The commit message is somewhat confused; *mb() are not UP barriers
(although they are available and useful on UP). They're device/dma
barriers.
> With regard to the AUX area, we don't know in general how data gets there,
> so using memory barriers seems sensible.
IIRC (but I ddn't check) the rule was that the kernel needs to ensure
the AUX area is complete before it updates the head pointer. So if
userspace can observe the head pointer, it must then also be able to
observe the data. This is not something userspace can fix up anyway.
The ordering here is between the head pointer and the data, and from a
userspace perspective that's a regular smp ordering. Similar for the
tail update, that's between our reading the data and writing the tail,
regular cache coherent smp ordering.
So ACK on the patch, it's sane and an optimization for both x86 and ARM.
Just the Changelog needs work.
> > Signed-off-by: Leo Yan <leo.yan@linaro.org>
> > ---
> > tools/perf/util/auxtrace.h | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/tools/perf/util/auxtrace.h b/tools/perf/util/auxtrace.h
> > index 472c0973b1f1..8bed284ccc82 100644
> > --- a/tools/perf/util/auxtrace.h
> > +++ b/tools/perf/util/auxtrace.h
> > @@ -452,7 +452,7 @@ static inline u64 auxtrace_mmap__read_snapshot_head(struct auxtrace_mmap *mm)
> > u64 head = READ_ONCE(pc->aux_head);
> >
> > /* Ensure all reads are done after we read the head */
> > - rmb();
> > + smp_rmb();
> > return head;
> > }
> >
> > @@ -466,7 +466,7 @@ static inline u64 auxtrace_mmap__read_head(struct auxtrace_mmap *mm)
> > #endif
> >
> > /* Ensure all reads are done after we read the head */
> > - rmb();
> > + smp_rmb();
> > return head;
> > }
> >
> > @@ -478,7 +478,7 @@ static inline void auxtrace_mmap__write_tail(struct auxtrace_mmap *mm, u64 tail)
> > #endif
> >
> > /* Ensure all reads are done before we write the tail out */
> > - mb();
> > + smp_mb();
> > #if BITS_PER_LONG == 64 || !defined(HAVE_SYNC_COMPARE_AND_SWAP_SUPPORT)
> > pc->aux_tail = tail;
> > #else
> >
>
next prev parent reply other threads:[~2021-05-27 8:12 UTC|newest]
Thread overview: 22+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-05-19 14:03 [PATCH v1 1/2] perf auxtrace: Change to use SMP memory barriers Leo Yan
2021-05-19 14:03 ` [PATCH v1 2/2] perf auxtrace: Optimize barriers with load-acquire and store-release Leo Yan
2021-05-31 15:10 ` Leo Yan
2021-05-31 15:55 ` Peter Zijlstra
2021-05-31 19:03 ` Adrian Hunter
2021-06-01 6:33 ` Leo Yan
2021-06-01 6:58 ` Peter Zijlstra
2021-06-01 9:07 ` Adrian Hunter
2021-06-01 9:17 ` Peter Zijlstra
2021-06-01 9:45 ` Adrian Hunter
2021-06-01 9:48 ` Peter Zijlstra
2021-06-01 14:56 ` Leo Yan
2021-06-01 6:48 ` Peter Zijlstra
2021-05-27 7:54 ` [PATCH v1 1/2] perf auxtrace: Change to use SMP memory barriers Adrian Hunter
2021-05-27 8:11 ` Peter Zijlstra [this message]
2021-05-27 8:25 ` Adrian Hunter
2021-05-27 9:24 ` Adrian Hunter
2021-05-27 9:57 ` Peter Zijlstra
2021-05-31 14:53 ` Leo Yan
2021-05-31 15:48 ` Peter Zijlstra
2021-06-01 3:21 ` Leo Yan
2021-05-27 9:45 ` Peter Zijlstra
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=YK9UFl7Grv/mwmVc@hirez.programming.kicks-ass.net \
--to=peterz@infradead.org \
--cc=acme@kernel.org \
--cc=adrian.hunter@intel.com \
--cc=ak@linux.intel.com \
--cc=alexander.shishkin@linux.intel.com \
--cc=leo.yan@linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-perf-users@vger.kernel.org \
--cc=mark.rutland@arm.com \
--cc=mingo@redhat.com \
--cc=namhyung@kernel.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 read-only IMAP folder(s) and NNTP newsgroup(s).