[PATCH 0/2] Enable MSR_TM lazily

[PATCH 0/2] Enable MSR_TM lazily

[Cyril Bur]

Currently the kernel checks to see if the hardware is transactional
memory capable and always enables the MSR_TM bit. The problem with
this is that the TM related SPRs become available to userspace,
requiring them to be switched between processes. It turns out these
SPRs are expensive to read and write and if a thread doesn't use TM
(or worse yet isn't even TM aware) then context switching incurs this
penalty for nothing.

The solution here is to leave the MSR_TM bit disabled and enable it
more 'on demand'. Leaving MSR_TM disabled cause a thread to take a
facility unavailable fault if and when it does decide to use TM. As
with recent updates to the FPU, VMX and VSX units the MSR_TM bit will
be enabled upon taking the fault and left on for some time afterwards
as the assumption is that if a thread used TM ones it may well use it
again. The kernel will turn the MSR_TM bit off after some number of
context switches of that thread.

Performance numbers haven't been completely gathered as yet but early
runs of tools/testing/selftests/powerpc/benchmarks/context_switch
(which doesn't use TM) yields a jump from ~160000 switches per second
to ~180000 switches per second with patch 3/3 applied.

These patches will need to be applied on top of my recent rework of
TM: http://patchwork.ozlabs.org/patch/666094/ 
I have pushed a branch to github to help with reviews:
https://github.com/cyrilbur-ibm/linux/tree/tm_lazy_v1

Changes since RFC:
	Fixed my unability to use & and | correctly.
	 - Spotted by Laurent Dufour, thanks
	Dropped the selftest patch as it was merged as part of a previous
	  series

Cyril Bur (2):
  powerpc: tm: Add TM Unavailable Exception
  powerpc: tm: Enable transactional memory (TM) lazily for userspace

 arch/powerpc/include/asm/processor.h |  1 +
 arch/powerpc/kernel/process.c        | 28 +++++++++++++++++++++++-----
 arch/powerpc/kernel/traps.c          | 34 ++++++++++++++++++++++++++++++++++
 3 files changed, 58 insertions(+), 5 deletions(-)

-- 
2.9.3

[PATCH 1/2] powerpc: tm: Add TM Unavailable Exception

[Cyril Bur]

If the kernel disables transactional memory (TM) and userspace still
tries TM related actions (TM instructions or TM SPR accesses) TM aware
hardware will cause the kernel to take a facility unavailable
exception.

Add checks for the exception being caused by illegal TM access in
userspace.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 arch/powerpc/kernel/traps.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index 0eba74b..cd40130 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1372,6 +1372,13 @@ void vsx_unavailable_exception(struct pt_regs *regs)
 }
 
 #ifdef CONFIG_PPC64
+static void tm_unavailable(struct pt_regs *regs)
+{
+	pr_emerg("Unrecoverable TM Unavailable Exception "
+			"%lx at %lx\n", regs->trap, regs->nip);
+	die("Unrecoverable TM Unavailable Exception", regs, SIGABRT);
+}
+
 void facility_unavailable_exception(struct pt_regs *regs)
 {
 	static char *facility_strings[] = {
@@ -1451,6 +1458,23 @@ void facility_unavailable_exception(struct pt_regs *regs)
 		return;
 	}
 
+	/*
+	 * TM Unavailable
+	 *
+	 * If
+	 *  - firmware bits say don't do TM or
+	 *  - CONFIG_PPC_TRANSACTIONAL_MEM was not set and
+	 *  - hardware is actually TM aware
+	 * Then userspace can spam the console (even with the use of
+	 * _ratelimited), just send the SIGILL.
+	 */
+	if (status == FSCR_TM_LG) {
+		if (!cpu_has_feature(CPU_FTR_TM))
+			goto out;
+		tm_unavailable(regs);
+		return;
+	}
+
 	if ((status < ARRAY_SIZE(facility_strings)) &&
 	    facility_strings[status])
 		facility = facility_strings[status];
@@ -1463,6 +1487,7 @@ void facility_unavailable_exception(struct pt_regs *regs)
 		"%sFacility '%s' unavailable, exception at 0x%lx, MSR=%lx\n",
 		hv ? "Hypervisor " : "", facility, regs->nip, regs->msr);
 
+out:
 	if (user_mode(regs)) {
 		_exception(SIGILL, regs, ILL_ILLOPC, regs->nip);
 		return;
-- 
2.9.3

[PATCH 2/2] powerpc: tm: Enable transactional memory (TM) lazily for userspace

[Cyril Bur]

Currently the MSR TM bit is always set if the hardware is TM capable.
This adds extra overhead as it means the TM SPRS (TFHAR, TEXASR and
TFAIR) must be swapped for each process regardless of if they use TM.

For processes that don't use TM the TM MSR bit can be turned off
allowing the kernel to avoid the expensive swap of the TM registers.

A TM unavailable exception will occur if a thread does use TM and the
kernel will enable MSR_TM and leave it so for some time afterwards.

Signed-off-by: Cyril Bur <cyrilbur@gmail.com>
---
 arch/powerpc/include/asm/processor.h |  1 +
 arch/powerpc/kernel/process.c        | 28 +++++++++++++++++++++++-----
 arch/powerpc/kernel/traps.c          |  9 +++++++++
 3 files changed, 33 insertions(+), 5 deletions(-)

diff --git a/arch/powerpc/include/asm/processor.h b/arch/powerpc/include/asm/processor.h
index b3e0cfc..c07c31b 100644
--- a/arch/powerpc/include/asm/processor.h
+++ b/arch/powerpc/include/asm/processor.h
@@ -257,6 +257,7 @@ struct thread_struct {
 	int		used_spe;	/* set if process has used spe */
 #endif /* CONFIG_SPE */
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+	u8	load_tm;
 	u64		tm_tfhar;	/* Transaction fail handler addr */
 	u64		tm_texasr;	/* Transaction exception & summary */
 	u64		tm_tfiar;	/* Transaction fail instr address reg */
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 11f7a64..cd81dd4 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -811,6 +811,12 @@ static inline bool hw_brk_match(struct arch_hw_breakpoint *a,
 }
 
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+
+static inline bool tm_enabled(struct task_struct *tsk)
+{
+	return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM);
+}
+
 static void tm_reclaim_thread(struct thread_struct *thr,
 			      struct thread_info *ti, uint8_t cause)
 {
@@ -891,6 +897,9 @@ void tm_recheckpoint(struct thread_struct *thread,
 {
 	unsigned long flags;
 
+	if (!(thread->regs->msr & MSR_TM))
+		return;
+
 	/* We really can't be interrupted here as the TEXASR registers can't
 	 * change and later in the trecheckpoint code, we have a userspace R1.
 	 * So let's hard disable over this region.
@@ -923,7 +932,7 @@ static inline void tm_recheckpoint_new_task(struct task_struct *new)
 	 * unavailable later, we are unable to determine which set of FP regs
 	 * need to be restored.
 	 */
-	if (!new->thread.regs)
+	if (!tm_enabled(new))
 		return;
 
 	if (!MSR_TM_ACTIVE(new->thread.regs->msr)){
@@ -954,8 +963,16 @@ static inline void __switch_to_tm(struct task_struct *prev,
 		struct task_struct *new)
 {
 	if (cpu_has_feature(CPU_FTR_TM)) {
-		tm_enable();
-		tm_reclaim_task(prev);
+		if (tm_enabled(prev) || tm_enabled(new))
+			tm_enable();
+
+		if (tm_enabled(prev)) {
+			prev->thread.load_tm++;
+			tm_reclaim_task(prev);
+			if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
+				prev->thread.regs->msr &= ~MSR_TM;
+		}
+
 		tm_recheckpoint_new_task(new);
 	}
 }
@@ -1392,6 +1409,9 @@ int arch_dup_task_struct(struct task_struct *dst, struct task_struct *src)
 	 * transitions the CPU out of TM mode.  Hence we need to call
 	 * tm_recheckpoint_new_task() (on the same task) to restore the
 	 * checkpointed state back and the TM mode.
+	 *
+	 * Can't pass dst because it isn't ready. Doesn't matter, passing
+	 * dst is only important for __switch_to()
 	 */
 	__switch_to_tm(src, src);
 
@@ -1635,8 +1655,6 @@ void start_thread(struct pt_regs *regs, unsigned long start, unsigned long sp)
 	current->thread.used_spe = 0;
 #endif /* CONFIG_SPE */
 #ifdef CONFIG_PPC_TRANSACTIONAL_MEM
-	if (cpu_has_feature(CPU_FTR_TM))
-		regs->msr |= MSR_TM;
 	current->thread.tm_tfhar = 0;
 	current->thread.tm_texasr = 0;
 	current->thread.tm_tfiar = 0;
diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c
index cd40130..9bb3895 100644
--- a/arch/powerpc/kernel/traps.c
+++ b/arch/powerpc/kernel/traps.c
@@ -1374,6 +1374,15 @@ void vsx_unavailable_exception(struct pt_regs *regs)
 #ifdef CONFIG_PPC64
 static void tm_unavailable(struct pt_regs *regs)
 {
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+	if (user_mode(regs)) {
+		current->thread.load_tm++;
+		regs->msr |= MSR_TM;
+		tm_enable();
+		tm_restore_sprs(&current->thread);
+		return;
+	}
+#endif
 	pr_emerg("Unrecoverable TM Unavailable Exception "
 			"%lx at %lx\n", regs->trap, regs->nip);
 	die("Unrecoverable TM Unavailable Exception", regs, SIGABRT);
-- 
2.9.3

Re: [PATCH 0/2] Enable MSR_TM lazily

[Nicholas Piggin]

Cc'ing Carlos

On Wed, 14 Sep 2016 18:02:14 +1000
Cyril Bur <cyrilbur@gmail.com> wrote:

> Currently the kernel checks to see if the hardware is transactional
> memory capable and always enables the MSR_TM bit. The problem with
> this is that the TM related SPRs become available to userspace,
> requiring them to be switched between processes. It turns out these
> SPRs are expensive to read and write and if a thread doesn't use TM
> (or worse yet isn't even TM aware) then context switching incurs this
> penalty for nothing.
> 
> The solution here is to leave the MSR_TM bit disabled and enable it
> more 'on demand'. Leaving MSR_TM disabled cause a thread to take a
> facility unavailable fault if and when it does decide to use TM. As
> with recent updates to the FPU, VMX and VSX units the MSR_TM bit will
> be enabled upon taking the fault and left on for some time afterwards
> as the assumption is that if a thread used TM ones it may well use it
> again. The kernel will turn the MSR_TM bit off after some number of
> context switches of that thread.
> 
> Performance numbers haven't been completely gathered as yet but early
> runs of tools/testing/selftests/powerpc/benchmarks/context_switch
> (which doesn't use TM) yields a jump from ~160000 switches per second
> to ~180000 switches per second with patch 3/3 applied.

Cool!

Question: glibc when built with lock elision seems like it will
execute tabort. before every syscall, to work around old kernel
behaviour. That's always going to fault TM on, isn't it?

How common it is for glibc to be built with elision?

We should probably be testing PPC_FEATURE2_HTM_NOSC to skip the
tabort.

(BTW, this is a pretty good writeup, would you consider adding
a bit more of it to patch 2 so it gets into the changelog?)

Thanks,
Nick

Re: [PATCH 0/2] Enable MSR_TM lazily

[Michael Neuling]

On Wed, 2016-09-14 at 21:28 +1000, Nicholas Piggin wrote:
> Cc'ing Carlos
>=20
> On Wed, 14 Sep 2016 18:02:14 +1000
> Cyril Bur <cyrilbur@gmail.com> wrote:
>=20
> >=20
> > Currently the kernel checks to see if the hardware is transactional
> > memory capable and always enables the MSR_TM bit. The problem with
> > this is that the TM related SPRs become available to userspace,
> > requiring them to be switched between processes. It turns out these
> > SPRs are expensive to read and write and if a thread doesn't use TM
> > (or worse yet isn't even TM aware) then context switching incurs this
> > penalty for nothing.
> >=20
> > The solution here is to leave the MSR_TM bit disabled and enable it
> > more 'on demand'. Leaving MSR_TM disabled cause a thread to take a
> > facility unavailable fault if and when it does decide to use TM. As
> > with recent updates to the FPU, VMX and VSX units the MSR_TM bit will
> > be enabled upon taking the fault and left on for some time afterwards
> > as the assumption is that if a thread used TM ones it may well use it
> > again. The kernel will turn the MSR_TM bit off after some number of
> > context switches of that thread.
> >=20
> > Performance numbers haven't been completely gathered as yet but early
> > runs of tools/testing/selftests/powerpc/benchmarks/context_switch
> > (which doesn't use TM) yields a jump from ~160000 switches per second
> > to ~180000 switches per second with patch 3/3 applied.
> Cool!
>=20
> Question: glibc when built with lock elision seems like it will
> execute tabort. before every syscall, to work around old kernel
> behaviour. That's always going to fault TM on, isn't it?

I think we might be able to detect this case in the kernel. If it's a tabor=
t
that's trapped on, we can't have been transactional. =C2=A0Hence we can saf=
ely PC+=3D4
and leave off TM off.=C2=A0

It would cost us a get_user(inst, regs->nip); but it might be worth it for =
this
special but common case.

> How common it is for glibc to be built with elision?

IIRC Ubuntu uses it on 16.04 (and maybe 15.10).

> We should probably be testing PPC_FEATURE2_HTM_NOSC to skip the
> tabort.

Agree, that would be idea. Binary patching glic at runtime.

> (BTW, this is a pretty good writeup, would you consider adding
> a bit more of it to patch 2 so it gets into the changelog?)

Agreed.

Mikey

Re: [PATCH 0/2] Enable MSR_TM lazily

[Nicholas Piggin]

On Wed, 14 Sep 2016 21:46:39 +1000
Michael Neuling <mikey@neuling.org> wrote:

> On Wed, 2016-09-14 at 21:28 +1000, Nicholas Piggin wrote:
> > Cc'ing Carlos
> >=20
> > On Wed, 14 Sep 2016 18:02:14 +1000
> > Cyril Bur <cyrilbur@gmail.com> wrote:
> >  =20
> > >=20
> > > Currently the kernel checks to see if the hardware is transactional
> > > memory capable and always enables the MSR_TM bit. The problem with
> > > this is that the TM related SPRs become available to userspace,
> > > requiring them to be switched between processes. It turns out these
> > > SPRs are expensive to read and write and if a thread doesn't use TM
> > > (or worse yet isn't even TM aware) then context switching incurs this
> > > penalty for nothing.
> > >=20
> > > The solution here is to leave the MSR_TM bit disabled and enable it
> > > more 'on demand'. Leaving MSR_TM disabled cause a thread to take a
> > > facility unavailable fault if and when it does decide to use TM. As
> > > with recent updates to the FPU, VMX and VSX units the MSR_TM bit will
> > > be enabled upon taking the fault and left on for some time afterwards
> > > as the assumption is that if a thread used TM ones it may well use it
> > > again. The kernel will turn the MSR_TM bit off after some number of
> > > context switches of that thread.
> > >=20
> > > Performance numbers haven't been completely gathered as yet but early
> > > runs of tools/testing/selftests/powerpc/benchmarks/context_switch
> > > (which doesn't use TM) yields a jump from ~160000 switches per second
> > > to ~180000 switches per second with patch 3/3 applied. =20
> > Cool!
> >=20
> > Question: glibc when built with lock elision seems like it will
> > execute tabort. before every syscall, to work around old kernel
> > behaviour. That's always going to fault TM on, isn't it? =20
>=20
> I think we might be able to detect this case in the kernel. If it's a tab=
ort
> that's trapped on, we can't have been transactional. =C2=A0Hence we can s=
afely PC+=3D4
> and leave off TM off.=C2=A0
>=20
> It would cost us a get_user(inst, regs->nip); but it might be worth it fo=
r this
> special but common case.

That would take an extra trap for every syscall, I think.


> > How common it is for glibc to be built with elision? =20
>=20
> IIRC Ubuntu uses it on 16.04 (and maybe 15.10).

Ah yes, but I was wrong: it also has to be linked against -lpthread
because it depends on r13 !=3D 0. That's why I couldn't see it in my
trace. Now I do when using -lpthread. On 16.04.


> > We should probably be testing PPC_FEATURE2_HTM_NOSC to skip the
> > tabort. =20
>=20
> Agree, that would be idea. Binary patching glic at runtime.

That would be nice. Does glibc support binary patching? I'm not very
familiar with the code. Current syscall code ends up something like
this:

    cmpdi    13,0
    beq      1f
    lwz      0,TM_CAPABLE(13)
    cmpwi    0,0
    beq      1f
    li       11,_ABORT_SYSCALL
    tabort.  11
    .align 4
1:
    li 0,syscall
    sc

Without runtime patching, then if we had another variable that meant
we are TM capable *and* need to issue a tabort., then we can do the
same sequence without extra instructions. That might be the first
step.

Thanks,
Nick

Re: [PATCH 0/2] Enable MSR_TM lazily

[Michael Neuling]

[-- Attachment #1: Type: text/plain, Size: 699 bytes --]

On 14 Sep. 2016 10:12 pm, "Nicholas Piggin" <npiggin@gmail.com> wrote:
>
> On Wed, 14 Sep 2016 21:46:39 +1000
> Michael Neuling <mikey@neuling.org> wrote:
>
> > On Wed, 2016-09-14 at 21:28 +1000, Nicholas Piggin wrote:
> > > Cc'ing Carlos
> > >
> > > On Wed, 14 Sep 2016 18:02:14 +1000

> > I think we might be able to detect this case in the kernel. If it's a
tabort
> > that's trapped on, we can't have been transactional.  Hence we can
safely PC+=4
> > and leave off TM off.
> >
> > It would cost us a get_user(inst, regs->nip); but it might be worth it
for this
> > special but common case.
>
> That would take an extra trap for every syscall, I think.

You're right. That wouldn't work.

Mikey

[-- Attachment #2: Type: text/html, Size: 1083 bytes --]

Re: [PATCH 0/2] Enable MSR_TM lazily

[Carlos Eduardo Seo]

On 9/14/16 8:28 AM, Nicholas Piggin wrote:
>
> How common it is for glibc to be built with elision?
>

Not that common. We have it built with TLE support in Ubuntu (starting 
in 15.04), SLES 12 (since SP2) and AT 9.0-0.

However, it is only enabled by default in Ubuntu. For SLES and AT 9.0, 
the user has to set an env var to enable it (it's a hack).

There is some work upstream to add a tunables framework to glibc. That 
will allow us to properly provide a way to users enable/disable TLE as 
they wish. That patch is almost in, and as soon as it's committed, we'll 
start working on the tunable for TLE.

-- 
Carlos Eduardo Seo
Software Engineer - Linux on Power Toolchain
cseo@linux.vnet.ibm.com

Re: [PATCH 0/2] Enable MSR_TM lazily

[Nicholas Piggin]

On Wed, 14 Sep 2016 11:10:22 -0300
Carlos Eduardo Seo <cseo@linux.vnet.ibm.com> wrote:

> On 9/14/16 8:28 AM, Nicholas Piggin wrote:
> >
> > How common it is for glibc to be built with elision?
> >  
> 
> Not that common. We have it built with TLE support in Ubuntu (starting 
> in 15.04), SLES 12 (since SP2) and AT 9.0-0.
> 
> However, it is only enabled by default in Ubuntu. For SLES and AT 9.0, 
> the user has to set an env var to enable it (it's a hack).
> 
> There is some work upstream to add a tunables framework to glibc. That 
> will allow us to properly provide a way to users enable/disable TLE as 
> they wish. That patch is almost in, and as soon as it's committed, we'll 
> start working on the tunable for TLE.

Okay, but for TLE-enabled case, we still want to skip the tabort
before syscall on recent kernels, so we could add that now with a 
relatively small patch couldn't we?

Thanks,
Nick

Re: [PATCH 2/2] powerpc: tm: Enable transactional memory (TM) lazily for userspace

[Simon Guo]

On Wed, Sep 14, 2016 at 06:02:16PM +1000, Cyril Bur wrote:
> @@ -954,8 +963,16 @@ static inline void __switch_to_tm(struct task_struct *prev,
>  		struct task_struct *new)
>  {
>  	if (cpu_has_feature(CPU_FTR_TM)) {
> -		tm_enable();
> -		tm_reclaim_task(prev);
> +		if (tm_enabled(prev) || tm_enabled(new))
> +			tm_enable();
> +
> +		if (tm_enabled(prev)) {
> +			prev->thread.load_tm++;
> +			tm_reclaim_task(prev);
> +			if (!MSR_TM_ACTIVE(prev->thread.regs->msr) && prev->thread.load_tm == 0)
> +				prev->thread.regs->msr &= ~MSR_TM;
> +		}
Hi Cyril,

If MSR_TM_ACTIVE(), is it better to reset load_tm to 0?
Other looks good to me.

Thanks,
- Simon

Re: [PATCH 2/2] powerpc: tm: Enable transactional memory (TM) lazily for userspace

[Cyril Bur]

On Mon, 2016-09-19 at 12:47 +0800, Simon Guo wrote:
> On Wed, Sep 14, 2016 at 06:02:16PM +1000, Cyril Bur wrote:
> > 
> > @@ -954,8 +963,16 @@ static inline void __switch_to_tm(struct
> > task_struct *prev,
> >  		struct task_struct *new)
> >  {
> >  	if (cpu_has_feature(CPU_FTR_TM)) {
> > -		tm_enable();
> > -		tm_reclaim_task(prev);
> > +		if (tm_enabled(prev) || tm_enabled(new))
> > +			tm_enable();
> > +
> > +		if (tm_enabled(prev)) {
> > +			prev->thread.load_tm++;
> > +			tm_reclaim_task(prev);
> > +			if (!MSR_TM_ACTIVE(prev->thread.regs->msr) 
> > && prev->thread.load_tm == 0)
> > +				prev->thread.regs->msr &= ~MSR_TM;
> > +		}
> Hi Cyril,
> 
> If MSR_TM_ACTIVE(), is it better to reset load_tm to 0?
> Other looks good to me.
> 

Doing so would extend the window that we keep TM enabled for when we
might not need to. It is possible that we could assume that if
MSR_TM_ACTIVE() then they're in codepathes that will reuse TM again
soon so load_tm = 0 could be a good idea but there's really no way to
know. Food for thought I guess...

Maybe?

Good thought,
Cyril

> Thanks,
> - Simon

Re: [1/2] powerpc: tm: Add TM Unavailable Exception

[Michael Ellerman]

On Wed, 2016-14-09 at 08:02:15 UTC, Cyril Bur wrote:
> If the kernel disables transactional memory (TM) and userspace still
> tries TM related actions (TM instructions or TM SPR accesses) TM aware
> hardware will cause the kernel to take a facility unavailable
> exception.
> 
> Add checks for the exception being caused by illegal TM access in
> userspace.
> 
> Signed-off-by: Cyril Bur <cyrilbur@gmail.com>

Series applied to powerpc next, thanks.

https://git.kernel.org/powerpc/c/172f7aaa75d0eaae167edde25c08aa

cheers