Message-ID: <20160425151638.GL3430@twins.programming.kicks-ass.net>
Date:	Mon, 25 Apr 2016 17:16:38 +0200
From:	Peter Zijlstra <peterz@...radead.org>
To:	Andy Lutomirski <luto@...capital.net>
Cc:	Dmitry Safonov <dsafonov@...tuozzo.com>,
	Thomas Gleixner <tglx@...utronix.de>,
	Shuah Khan <shuahkh@....samsung.com>,
	Ingo Molnar <mingo@...hat.com>,
	Dave Hansen <dave.hansen@...ux.intel.com>,
	Borislav Petkov <bp@...en8.de>, khorenko@...tuozzo.com,
	X86 ML <x86@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>, xemul@...tuozzo.com,
	linux-kselftest@...r.kernel.org,
	Cyrill Gorcunov <gorcunov@...nvz.org>,
	Dmitry Safonov <0x7f454c46@...il.com>,
	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	"H. Peter Anvin" <hpa@...or.com>
Subject: Re: [PATCH 1/2] x86/arch_prctl: add ARCH_SET_{COMPAT,NATIVE} to
 change compatible mode

On Thu, Apr 21, 2016 at 04:27:19PM -0700, Andy Lutomirski wrote:
> > Did that help? Or did I confuse you moar?
> >
> 
> I think I'm starting to get it.  What if we rearrange slightly, like this:
> 
> perf_sample_data already has a struct perf_regs in it.  We could add a
> flags field to the first chunk of perf_sample_data:
> 
> u64 sample_flags;

I actually considered that for another problem. Didn't like it then, but
seeing how I still haven't figured out a better way and you're now
proposing this too, maybe...

Part of the problem is that this will completely exhaust that first
cacheline :/

> perf_sample_data_init sets sample_flags to zero.

And while we're on struct perf_sample_data, that thing has gotten
insanely large. We carry it on-stack!

It should be fairly easy to take regs_user_copy out and use a per-cpu
array of them things for this I think, see below.

> Now we rename perf_sample_regs_user to __perf_sample_regs_user and
> make it non-static.  We also teach it to set do data->sample_flags |=
> PERF_SAMPLE_FLAGS_HAS_REGS_USER.  We add:
> 
> static void perf_fetch_regs_user(struct perf_sample_data *data, struct
> pt_regs *interrupt_regs)
> {
>   if (data->sample_flags & PERF_SAMPLE_FLAGS_HAS_REGS_USER)
>     return;
> 
>   __perf_sample_regs_user(&data->regs_user, interrupt_regs,
> &data->regs_user_copy);
> }

I meant to change perf_prepare_sample() to do:

	u64 sample_type = event->attr.sample_type & ~data.sample_type;

or something similar, such that we can override/avoid some of the work
there.

> (Hmm.  This only really works well if we can guarantee that
> interrupt_regs remains valid for the life of the perf_sample_data
> object.  Could we perhaps move the interrupt_regs pointer *into*
> perf_sample_data and stop passing it all over the place?)

So the problem with that is that we'll now overflow the one cacheline,
and the last time I really looked at this that made samples that much
slower.

It might be time to re-evaluate this stuff, since pretty much everything
will eventually write into perf_sample_data::ip etc.. which is the
second line anyway.

Also, looking at it, we actually have a pointer in there for this,
perf_sample_data::regs_intr::regs, but its at the very tail of this
monster, 4 cachelines off the normal path.

> We change all the callers of perf_sample_regs_user to use
> perf_fetch_regs_user instead.

There's only the one site currently, but yeah.

> What do you think?  If you like it, I can probably find some time to
> give it a shot, but I don't guarantee that I won't miss some subtlety
> in its interaction with the rest of the event output code.

Sure give it a go, I'll stomp on it to fix the pebs-time issue (we need
to skip perf_prepare_sample's PERF_SAMPLE_TIME branch for that).

> On a vaguely related note, why is the big prebs-to-pt_regs copy
> conditional on (sample_type & PERF_SAMPLE_REGS_INTR)?  I bet it would
> be faster to make it unconditional, because you could avoid copying
> over the entire pt_regs struct if PERF_SAMPLE_REGS_INTR isn't set.

Hmm, yes.. that code did move about a bit, not sure what it looked like
originally.

In any case, That fully copy is overkill in the simple case as well, I
think that could get away with only copying cs,flags.


Compile tested only..

---
Subject: perf: Replace perf_sample_data::regs_user_copy with per-cpu storage

struct perf_sample_data is immense, and we carry it on stack, shrink it
some.

struct perf_sample_data {
        /* size: 384, cachelines: 6, members: 19 */
}

Signed-off-by: Peter Zijlstra (Intel) <peterz@...radead.org>
---
 include/linux/perf_event.h |  2 --
 kernel/events/core.c       | 23 +++++++++++++++++------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 85749ae8cb5f..dd2cab6c5bbb 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -795,8 +795,6 @@ struct perf_sample_data {
 	 * on arch details.
 	 */
 	struct perf_regs		regs_user;
-	struct pt_regs			regs_user_copy;
-
 	struct perf_regs		regs_intr;
 	u64				stack_user_size;
 } ____cacheline_aligned;
diff --git a/kernel/events/core.c b/kernel/events/core.c
index eabeb2aec00f..72754607d2cd 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -5146,15 +5146,27 @@ perf_output_sample_regs(struct perf_output_handle *handle,
 	}
 }
 
-static void perf_sample_regs_user(struct perf_regs *regs_user,
-				  struct pt_regs *regs,
-				  struct pt_regs *regs_user_copy)
+static DEFINE_PER_CPU(struct pt_regs, __regs_user[4]);
+
+static struct pt_regs *regs_user_ptr(void)
+{
+	if (in_nmi())
+		return this_cpu_ptr(&__regs_user[0]);
+	if (in_interrupt())
+		return this_cpu_ptr(&__regs_user[1]);
+	if (in_serving_softirq())
+		return this_cpu_ptr(&__regs_user[2]);
+	return this_cpu_ptr(&__regs_user[3]);
+}
+
+static void
+perf_sample_regs_user(struct perf_regs *regs_user, struct pt_regs *regs)
 {
 	if (user_mode(regs)) {
 		regs_user->abi = perf_reg_abi(current);
 		regs_user->regs = regs;
 	} else if (current->mm) {
-		perf_get_regs_user(regs_user, regs, regs_user_copy);
+		perf_get_regs_user(regs_user, regs, regs_user_ptr());
 	} else {
 		regs_user->abi = PERF_SAMPLE_REGS_ABI_NONE;
 		regs_user->regs = NULL;
@@ -5638,8 +5650,7 @@ void perf_prepare_sample(struct perf_event_header *header,
 	}
 
 	if (sample_type & (PERF_SAMPLE_REGS_USER | PERF_SAMPLE_STACK_USER))
-		perf_sample_regs_user(&data->regs_user, regs,
-				      &data->regs_user_copy);
+		perf_sample_regs_user(&data->regs_user, regs);
 
 	if (sample_type & PERF_SAMPLE_REGS_USER) {
 		/* regs dump ABI info */

Powered by blists - more mailing lists