Re: [RFC PATCH 06/11] powerpc/tm: Refactor the __switch_to_tm code

On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote:

__switch_to_tm is the function that switches between two tasks which migh=

t

have TM enabled. This function is clearly split in two parts, the task th=

at

is leaving the CPU, known as 'prev' and the task that is being scheduled,
known as new.
=20
It starts checking if the previous task had TM enable, if so, it increase=

s

the load_tm (this is the only place we increment load_tm). It also saves
the TM SPRs here.
=20
If the previous task was scheduled out with a transaction active, the
failure cause needs to be updated, since it might contain the failure cau=

se

that caused the exception, as TM_CAUSE_MISC. In this case, since there wa=

s

a context switch, overwrite the failure cause.
=20
If the previous task has overflowed load_tm, disable TM, putting the
facility save/restore lazy mechanism at lazy mode.
=20
Regarding the new task, when loading it, it basically restore the SPRs, a=

nd

TIF_RESTORE_TM (already set by tm_reclaim_current if the transaction was
active) would invoke the recheckpoint process later in restore_tm_state()
if recheckpoint is somehow required.

This paragraph is a little awkwardly worded.  Can you rewrite?

On top of that, both tm_reclaim_task() and tm_recheckpoint_new_task()
functions are not used anymore, removing them.

What about tm_reclaim_current().  This is being used in places like signals
which I would have thought we could avoid with this series

c

Can we just drop the __ ?

+		struct task_struct *new)
 {
 	if (!cpu_has_feature(CPU_FTR_TM))
 		return;
=20
-	/* 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;
-
-	if (!MSR_TM_ACTIVE(new->thread.regs->msr)){
-		tm_restore_sprs(&new->thread);
-		return;
-	}
-	/* Recheckpoint to restore original checkpointed register state. */
-	TM_DEBUG("*** tm_recheckpoint of pid %d (new->msr 0x%lx)\n",
-		 new->pid, new->thread.regs->msr);
-
-	tm_recheckpoint(&new->thread);
-
-	/*
-	 * The checkpointed state has been restored but the live state has
-	 * not, ensure all the math functionality is turned off to trigger
-	 * restore_math() to reload.
-	 */
-	new->thread.regs->msr &=3D ~(MSR_FP | MSR_VEC | MSR_VSX);
+	/* The task leaving the CPU was using TM, let's handle it */
+	if (tm_enabled(prev)) {
+		/*
+		 * Load_tm is incremented only when the task is scheduled out
+		 */
+		prev->thread.load_tm++;
=20
-	TM_DEBUG("*** tm_recheckpoint of pid %d complete "
-		 "(kernel msr 0x%lx)\n",
-		 new->pid, mfmsr());
-}
+		/*
+		 * If TM is enabled for the thread, it needs to, at least,
+		 * save the SPRs
+		 */
+		tm_enable();
+		tm_save_sprs(&prev->thread);
=20
-static inline void __switch_to_tm(struct task_struct *prev,
-		struct task_struct *new)
-{
-	if (cpu_has_feature(CPU_FTR_TM)) {
-		if (tm_enabled(prev) || tm_enabled(new))
-			tm_enable();
+		/*
+		 * If we got here with an active transaction, then, it was
+		 * aborted by TM_KERNEL_ENTRY and the fix the failure case
+		 * needs to be fixed, so, indepedently how we arrived here, the
+		 * new TM abort case will be TM_CAUSE_RESCHED now.

What does "fix the failure case needs to be fixed" mean?

also s/indepedently/independently/

+		 */
+		if (MSR_TM_ACTIVE(prev->thread.regs->msr)) {
+			/*
+			 * If there was an IRQ during trecheckpoint, it will
+			 * cause an IRQ to be replayed. This replayed IRQ can
+			 * invoke SCHEDULE_USER, thus, we arrive here with a TM
+			 * active transaction.

I don't think this can happen. trecheckpoint (and treclaim) are called with=
 IRQs
hard off (since they change r1).

I think something else is going on here. I think this code and comment need=
s to
go but I assume it's here because you are seeing something.

truct

task_struct *src)
 	/*
 	 * Flush TM state out so we can copy it.  __switch_to_tm() does this
 	 * flush but it removes the checkpointed state from the current CPU and
-	 * 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.
+	 * transitions the CPU out of TM mode.  Hence we need to make sure
+	 * TIF_RESTORE_TM is set so restore_tm_state is called to restore the
+	 * checkpointed state and back to TM mode.
 	 *
 	 * Can't pass dst because it isn't ready. Doesn't matter, passing
 	 * dst is only important for __switch_to()