Patchwork [12/22] x86/fpu: Only write PKRU if it is different from current

login
register
mail settings
Submitter Sebastian Andrzej Siewior
Date Jan. 9, 2019, 11:47 a.m.
Message ID <20190109114744.10936-13-bigeasy@linutronix.de>
Download mbox | patch
Permalink /patch/695749/
State New
Headers show

Comments

Sebastian Andrzej Siewior - Jan. 9, 2019, 11:47 a.m.
Dave Hansen says that the `wrpkru' is more expensive than `rdpkru'. It
has a higher cycle cost and it's also practically a (light) speculation
barrier.

As an optimisation read the current PKRU value and only write the new
one if it is different.

Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 arch/x86/include/asm/special_insns.h | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)
Dave Hansen - Jan. 23, 2019, 6:09 p.m.
On 1/9/19 3:47 AM, Sebastian Andrzej Siewior wrote:
> +static inline void __write_pkru(u32 pkru)
> +{
> +	/*
> +	 * Writting PKRU is expensive. Only write the PKRU value if it is
> +	 * different from the current one.
> +	 */

I'd say:

	WRPKRU is relatively expensive compared to RDPKRU.
	Avoid WRPKRU when it would not change the value.

In the grand scheme of things, WRPKRU is cheap.  It's certainly not an
"expensive instruction" compared to things like WBINVD.

> +	if (pkru == __read_pkru())
> +		return;
> +	__write_pkru_insn(pkru);
> +}

Is there a case where we need __write_pkru_insn() directly?  Why not
just put the inline assembly in here?
Sebastian Andrzej Siewior - Feb. 7, 2019, 11:27 a.m.
On 2019-01-23 10:09:24 [-0800], Dave Hansen wrote:
> On 1/9/19 3:47 AM, Sebastian Andrzej Siewior wrote:
> > +static inline void __write_pkru(u32 pkru)
> > +{
> > +	/*
> > +	 * Writting PKRU is expensive. Only write the PKRU value if it is
> > +	 * different from the current one.
> > +	 */
> 
> I'd say:
> 
> 	WRPKRU is relatively expensive compared to RDPKRU.
> 	Avoid WRPKRU when it would not change the value.
> 
> In the grand scheme of things, WRPKRU is cheap.  It's certainly not an
> "expensive instruction" compared to things like WBINVD.

Okay.

> > +	if (pkru == __read_pkru())
> > +		return;
> > +	__write_pkru_insn(pkru);
> > +}
> 
> Is there a case where we need __write_pkru_insn() directly?  Why not
> just put the inline assembly in here?

There is no user of __write_pkru_insn(). I had one in the past I think.
Let me merge it for now.

Sebastian

Patch

diff --git a/arch/x86/include/asm/special_insns.h b/arch/x86/include/asm/special_insns.h
index 43c029cdc3fe8..c2ccf71b22dd6 100644
--- a/arch/x86/include/asm/special_insns.h
+++ b/arch/x86/include/asm/special_insns.h
@@ -107,7 +107,7 @@  static inline u32 __read_pkru(void)
 	return pkru;
 }
 
-static inline void __write_pkru(u32 pkru)
+static inline void __write_pkru_insn(u32 pkru)
 {
 	u32 ecx = 0, edx = 0;
 
@@ -118,6 +118,17 @@  static inline void __write_pkru(u32 pkru)
 	asm volatile(".byte 0x0f,0x01,0xef\n\t"
 		     : : "a" (pkru), "c"(ecx), "d"(edx));
 }
+
+static inline void __write_pkru(u32 pkru)
+{
+	/*
+	 * Writting PKRU is expensive. Only write the PKRU value if it is
+	 * different from the current one.
+	 */
+	if (pkru == __read_pkru())
+		return;
+	__write_pkru_insn(pkru);
+}
 #else
 static inline u32 __read_pkru(void)
 {