| Message ID | 1529362784-14194-4-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [v2,1/4] powerpc/tm: Remove msr_tm_active() | expand |
On Mon, 2018-06-18 at 19:59 -0300, Breno Leitao wrote: > If __switch_to() tries to context switch from task A to task B, and task A > had task->thread->regs->msr[TM] enabled, then __switch_to_tm() will call > tm_recheckpoint_new_task(), which will call trecheckpoint, for task B, which > is clearly wrong since task B might not be an active TM user. > > This does not cause a lot of damage because tm_recheckpoint() will abort > the call since it realize that the current task does not have msr[TM] bit > set. > > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > arch/powerpc/kernel/process.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c > index f8beee03f00a..d26a150766ef 100644 > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -1036,7 +1036,8 @@ static inline void __switch_to_tm(struct task_struct > *prev, > prev->thread.regs->msr &= ~MSR_TM; > } > > - tm_recheckpoint_new_task(new); > + if (tm_enabled(new)) > + tm_recheckpoint_new_task(new); I'm not sure we need this patch as tm_recheckpoint_new_task() does this itself. --- static inline void tm_recheckpoint_new_task(struct task_struct *new) { if (!cpu_has_feature(CPU_FTR_TM)) return; /* Recheckpoint the registers of the thread we're about to switch to. * * If the task was using FP, we non-lazily reload both the original and * the speculative FP register states. This is because the kernel * doesn't see if/when a TM rollback occurs, so if we take an FP * unavailable later, we are unable to determine which set of FP regs * need to be restored. */ if (!tm_enabled(new)) return; --- Mikey
Hey Mikey, Thanks for the review. On 08/15/2018 08:50 PM, Michael Neuling wrote: > On Mon, 2018-06-18 at 19:59 -0300, Breno Leitao wrote: >> If __switch_to() tries to context switch from task A to task B, and task A >> had task->thread->regs->msr[TM] enabled, then __switch_to_tm() will call >> tm_recheckpoint_new_task(), which will call trecheckpoint, for task B, which >> is clearly wrong since task B might not be an active TM user. >> >> This does not cause a lot of damage because tm_recheckpoint() will abort >> the call since it realize that the current task does not have msr[TM] bit >> set. >> >> Signed-off-by: Breno Leitao <leitao@debian.org> >> --- >> arch/powerpc/kernel/process.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c >> index f8beee03f00a..d26a150766ef 100644 >> --- a/arch/powerpc/kernel/process.c >> +++ b/arch/powerpc/kernel/process.c >> @@ -1036,7 +1036,8 @@ static inline void __switch_to_tm(struct task_struct >> *prev, >> prev->thread.regs->msr &= ~MSR_TM; >> } >> >> - tm_recheckpoint_new_task(new); >> + if (tm_enabled(new)) >> + tm_recheckpoint_new_task(new); > > I'm not sure we need this patch as tm_recheckpoint_new_task() does this itself. My plan is to move this check prior to calling these TM functions, doing early checking and avoiding calling tm_recheckpoint on a non-tm enabled task. It is very weird when you see, during a tracing, a kernel thread (PF_KTHREAD) being tm_recheckpointed. :-/ That said, the TM function would do the operation other than "check and do it" mode. Ideally I would like to check the thread before calling any TM functions, and warning (WARN_ON) if we detect, later in the game, that a thread is not TM enabled. This helps on two different fronts, in my opinion: * Code readability * Understanding tracing (ftrace) outputs.
diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index f8beee03f00a..d26a150766ef 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -1036,7 +1036,8 @@ static inline void __switch_to_tm(struct task_struct *prev, prev->thread.regs->msr &= ~MSR_TM; } - tm_recheckpoint_new_task(new); + if (tm_enabled(new)) + tm_recheckpoint_new_task(new); } }
If __switch_to() tries to context switch from task A to task B, and task A had task->thread->regs->msr[TM] enabled, then __switch_to_tm() will call tm_recheckpoint_new_task(), which will call trecheckpoint, for task B, which is clearly wrong since task B might not be an active TM user. This does not cause a lot of damage because tm_recheckpoint() will abort the call since it realize that the current task does not have msr[TM] bit set. Signed-off-by: Breno Leitao <leitao@debian.org> --- arch/powerpc/kernel/process.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)