| Message ID | 1536781219-13938-11-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
| Series | New TM Model | expand |
| Context | Check | Description |
|---|---|---|
| snowpatch_ozlabs/apply_patch | success | next/apply_patch Successfully applied |
| snowpatch_ozlabs/checkpatch | success | Test checkpatch on branch next |
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); >
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
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
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);
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(-)