[RFC,10/11] powerpc/tm: Set failure summary

Message ID 1536781219-13938-11-git-send-email-leitao@debian.org
State RFC
Headers show
Series
  • New TM Model
Related show

Checks

Context Check Description
snowpatch_ozlabs/checkpatch success Test checkpatch on branch next
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied

Commit Message

Breno Leitao Sept. 12, 2018, 7:40 p.m.
Since the transaction will be doomed with treckpt., the TEXASR[FS]
should be set, to reflect that the transaction is a failure. This patch
ensures it before recheckpointing, and remove changes from other places
that were calling recheckpoint.

Signed-off-by: Breno Leitao <leitao@debian.org>
---
 arch/powerpc/kernel/process.c   | 6 ++++++
 arch/powerpc/kernel/signal_32.c | 2 --
 arch/powerpc/kernel/signal_64.c | 2 --
 3 files changed, 6 insertions(+), 4 deletions(-)

Comments

Michael Neuling Sept. 18, 2018, 5:50 a.m. | #1
On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> Since the transaction will be doomed with treckpt., the TEXASR[FS]
> should be set, to reflect that the transaction is a failure. This patch
> ensures it before recheckpointing, and remove changes from other places
> that were calling recheckpoint.

TEXASR[FS] should be set by the reclaim. I don't know why you'd need to set this
explicitly in process.c. The only case is when the user supplies a bad signal
context, but we should check that in the signals code, not process.c

Hence I think this patch is wrong.

Also, according to the architecture, TEXASR[FS] HAS TO BE SET on trecheckpoint
otherwise you'll get a TM Bad Thing. You should say that rather than suggesting
it's because the transaction is doomed. It's illegal to not do it. That's why we
have this check in arch/powerpc/kernel/tm.S.


	/* Do final sanity check on TEXASR to make sure FS is set.  Do this
	 * here before we load up the userspace r1 so any bugs we hit will get
	 * a call chain */
	mfspr	r5, SPRN_TEXASR
	srdi	r5, r5, 16
	li	r6, (TEXASR_FS)@h
	and	r6, r6, r5
1:	tdeqi	r6, 0
	EMIT_BUG_ENTRY 1b,__FILE__,__LINE__,0


Mikey

> Signed-off-by: Breno Leitao <leitao@debian.org>
> ---
>  arch/powerpc/kernel/process.c   | 6 ++++++
>  arch/powerpc/kernel/signal_32.c | 2 --
>  arch/powerpc/kernel/signal_64.c | 2 --
>  3 files changed, 6 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
> index 5cace1b744b1..77725b2e4dc1 100644
> --- a/arch/powerpc/kernel/process.c
> +++ b/arch/powerpc/kernel/process.c
> @@ -937,6 +937,12 @@ void tm_recheckpoint(struct thread_struct *thread)
>  	local_irq_save(flags);
>  	hard_irq_disable();
>  
> +	/*
> +	 * Make sure the failure summary is set, since the transaction will be
> +	 * doomed.
> +	 */
> +	thread->tm_texasr |= TEXASR_FS;
> +
>  	/* The TM SPRs are restored here, so that TEXASR.FS can be set
>  	 * before the trecheckpoint and no explosion occurs.
>  	 */
> diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
> index 4a1b17409bf3..96956d50538e 100644
> --- a/arch/powerpc/kernel/signal_32.c
> +++ b/arch/powerpc/kernel/signal_32.c
> @@ -851,8 +851,6 @@ static long restore_tm_user_regs(struct pt_regs *regs,
>  	/* Pull in the MSR TM bits from the user context */
>  	regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr_hi & MSR_TS_MASK);
>  
> -	/* Make sure the transaction is marked as failed */
> -	current->thread.tm_texasr |= TEXASR_FS;
>  	/* Make sure restore_tm_state will be called */
>  	set_thread_flag(TIF_RESTORE_TM);
>  
> diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
> index 32402aa23a5e..c84501711b14 100644
> --- a/arch/powerpc/kernel/signal_64.c
> +++ b/arch/powerpc/kernel/signal_64.c
> @@ -569,8 +569,6 @@ static long restore_tm_sigcontexts(struct task_struct
> *tsk,
>  		}
>  	}
>  #endif
> -	/* Make sure the transaction is marked as failed */
> -	tsk->thread.tm_texasr |= TEXASR_FS;
>  	/* Guarantee that restore_tm_state() will be called */
>  	set_thread_flag(TIF_RESTORE_TM);
>
Breno Leitao Sept. 27, 2018, 8:52 p.m. | #2
Hi Mikey,

On 09/18/2018 02:50 AM, Michael Neuling wrote:
> On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
>> Since the transaction will be doomed with treckpt., the TEXASR[FS]
>> should be set, to reflect that the transaction is a failure. This patch
>> ensures it before recheckpointing, and remove changes from other places
>> that were calling recheckpoint.
> 
> TEXASR[FS] should be set by the reclaim. 

Do you mean that the CPU should set TEXASR[FS] when treclaim is called, or,
that the tm_reclaim?

Looking at the ISA, I didn't see TEXASR[FS] being set automatically when a
reclaim happens, although, I see it needs TEXASR[FS] to be set when executing
a trecheckpoint, otherwise it will cause a TM Bad Thing.

That is why I am forcing TEXASR[FS]=1 to doom the transaction so we can
recheckpoint it, but it seems I understood this wrongly.

> I don't know why you'd need to set this
> explicitly in process.c. The only case is when the user supplies a bad signal
> context, but we should check that in the signals code, not process.c
> 
> Hence I think this patch is wrong.
> 
> Also, according to the architecture, TEXASR[FS] HAS TO BE SET on trecheckpoint
> otherwise you'll get a TM Bad Thing. You should say that rather than suggesting
> it's because the transaction is doomed. It's ilqlegal to not do it. That's why we
> have this check in arch/powerpc/kernel/tm.S.

When you say "HAS TO BE SET", do you mean that the hardware will set it and
we shouldn't care about this flag? Thus, if I am hitting EMIT_BUG_ENTRY, it
means my TEXASR was messed somehow?

Thank you
Michael Neuling Sept. 28, 2018, 5:17 a.m. | #3
On Thu, 2018-09-27 at 17:52 -0300, Breno Leitao wrote:
> Hi Mikey,
> 
> On 09/18/2018 02:50 AM, Michael Neuling wrote:
> > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:
> > > Since the transaction will be doomed with treckpt., the TEXASR[FS]
> > > should be set, to reflect that the transaction is a failure. This patch
> > > ensures it before recheckpointing, and remove changes from other places
> > > that were calling recheckpoint.
> > 
> > TEXASR[FS] should be set by the reclaim. 
> 
> Do you mean that the CPU should set TEXASR[FS] when treclaim is called, or,
> that the tm_reclaim?
> 
> Looking at the ISA, I didn't see TEXASR[FS] being set automatically when a
> reclaim happens, 
treclaim in ISA talks about "TMRecordFailure(cause)" and that macro sets
TEXASR[FS]=1. 

So yes treclaim always sets TEXASR[FS]=1.

> although, I see it needs TEXASR[FS] to be set when executing a 
> trecheckpoint, otherwise it will cause a TM Bad Thing.

Yep

> That is why I am forcing TEXASR[FS]=1 to doom the transaction so we can
> recheckpoint it, but it seems I understood this wrongly.

You shouldn't need to.  We do a bug_on() just before the trecheckpoint to make
sure it's set, but you shouldn't need to set it (other than the signals case).

> > I don't know why you'd need to set this
> > explicitly in process.c. The only case is when the user supplies a bad
> > signal
> > context, but we should check that in the signals code, not process.c
> > 
> > Hence I think this patch is wrong.
> > 
> > Also, according to the architecture, TEXASR[FS] HAS TO BE SET on
> > trecheckpoint
> > otherwise you'll get a TM Bad Thing. You should say that rather than
> > suggesting
> > it's because the transaction is doomed. It's ilqlegal to not do it. That's
> > why we
> > have this check in arch/powerpc/kernel/tm.S.
> 
> When you say "HAS TO BE SET", do you mean that the hardware will set it and
> we shouldn't care about this flag? Thus, if I am hitting EMIT_BUG_ENTRY, it
> means my TEXASR was messed somehow?

I'm just saying what you said before about the tm bad thing.  We have to make
sure it's set before we do the trecheckpoint otherwise we end up in the TM bad
thing. The treclaim should handle this for us (but again the signals case need
to be checked).

Mikey

Patch

diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c
index 5cace1b744b1..77725b2e4dc1 100644
--- a/arch/powerpc/kernel/process.c
+++ b/arch/powerpc/kernel/process.c
@@ -937,6 +937,12 @@  void tm_recheckpoint(struct thread_struct *thread)
 	local_irq_save(flags);
 	hard_irq_disable();
 
+	/*
+	 * Make sure the failure summary is set, since the transaction will be
+	 * doomed.
+	 */
+	thread->tm_texasr |= TEXASR_FS;
+
 	/* The TM SPRs are restored here, so that TEXASR.FS can be set
 	 * before the trecheckpoint and no explosion occurs.
 	 */
diff --git a/arch/powerpc/kernel/signal_32.c b/arch/powerpc/kernel/signal_32.c
index 4a1b17409bf3..96956d50538e 100644
--- a/arch/powerpc/kernel/signal_32.c
+++ b/arch/powerpc/kernel/signal_32.c
@@ -851,8 +851,6 @@  static long restore_tm_user_regs(struct pt_regs *regs,
 	/* Pull in the MSR TM bits from the user context */
 	regs->msr = (regs->msr & ~MSR_TS_MASK) | (msr_hi & MSR_TS_MASK);
 
-	/* Make sure the transaction is marked as failed */
-	current->thread.tm_texasr |= TEXASR_FS;
 	/* Make sure restore_tm_state will be called */
 	set_thread_flag(TIF_RESTORE_TM);
 
diff --git a/arch/powerpc/kernel/signal_64.c b/arch/powerpc/kernel/signal_64.c
index 32402aa23a5e..c84501711b14 100644
--- a/arch/powerpc/kernel/signal_64.c
+++ b/arch/powerpc/kernel/signal_64.c
@@ -569,8 +569,6 @@  static long restore_tm_sigcontexts(struct task_struct *tsk,
 		}
 	}
 #endif
-	/* Make sure the transaction is marked as failed */
-	tsk->thread.tm_texasr |= TEXASR_FS;
 	/* Guarantee that restore_tm_state() will be called */
 	set_thread_flag(TIF_RESTORE_TM);