Re: [RFC PATCH 09/11] powerpc/tm: Do not restore default DSCR
On Thu, 2018-09-27 at 17:51 -0300, Breno Leitao wrote: > Hi Mikey, > > On 09/18/2018 02:41 AM, Michael Neuling wrote: > > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote: > > > In the previous TM code, trecheckpoint was being executed in the middle of > > > an exception, thus, DSCR was being restored to default kernel DSCR value > > > after trecheckpoint was done. > > > > > > With this current patchset, trecheckpoint is executed just before getting > > > to userspace, at ret_from_except_lite, for example. Thus, we do not need > > > to > > > set default kernel DSCR value anymore, as we are leaving kernel space. It > > > is OK to keep the checkpointed DSCR value into the live SPR, mainly > > > because > > > the transaction is doomed and it will fail soon (after RFID), > > > > What if we are going back to a suspended transaction? It will remain live > > until > > userspace does a tresume > > Hmm, I understand that once we get in kernel space, and call > treclaim/trecheckpoint, the transaction will be doomed and it will abort and > rollback when we leave kernel space. I.e., if we can treclaim/trecheckpointn > in kernel space, the transaction will *always* abort just after RFID, so, a > possible tresume will never be executed. Is my understanding wrong? Your understanding is wrong. We don't roll back until MSR[TS] = T. There are two cases for the RFID. 1) if you RFID back to userspace that is transactional (ie MSR[TS] = T), then it will immediately rollback as soon as the RFID happens since we are going directly to T. 2) If you RFID back to userspace that is suspended (ie MSR[TS] = S), then it won't roll back until userspace does a tresume. It doesn't rollback until we go MSR[TS] = S -> T). > > > > > so, > > > continuing with the pre-checkpointed DSCR value is what seems correct. > > > > Reading this description suggests this patch isn't really needed. Right? > > Maybe the description is not clear, but I understand this patch is required, > otherwise we will leave userspace with a default DSCR value. > > By the way, do you know if there is a change in DSCR inside a transaction, > will it be reverted if the transaction is aborted? Yes it will be reverted. We even have a selftest for it in tools/testing/selftests/powerpc/tm/tm-resched-dscr.c There are a bunch of checkpointed SPRs. From the arch: Checkpointed registers: The set of registers that are saved to the “checkpoint area” when a transaction is initiated, and restored upon transaction failure, is a subset of the architected register state, consisting of the General Purpose Registers, Floating-Point Regis- ters, Vector Registers, Vector-Scalar Registers, and the following Special Registers and fields: CR fields other than CR0, XER, LR, CTR, FPSCR, AMR, PPR, VRSAVE, VSCR, DSCR, and TAR. Mikey
Re: [RFC PATCH 09/11] powerpc/tm: Do not restore default DSCR
Hi Mikey, On 09/18/2018 02:41 AM, Michael Neuling wrote: > On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote: >> In the previous TM code, trecheckpoint was being executed in the middle of >> an exception, thus, DSCR was being restored to default kernel DSCR value >> after trecheckpoint was done. >> >> With this current patchset, trecheckpoint is executed just before getting >> to userspace, at ret_from_except_lite, for example. Thus, we do not need to >> set default kernel DSCR value anymore, as we are leaving kernel space. It >> is OK to keep the checkpointed DSCR value into the live SPR, mainly because >> the transaction is doomed and it will fail soon (after RFID), > > What if we are going back to a suspended transaction? It will remain live > until > userspace does a tresume Hmm, I understand that once we get in kernel space, and call treclaim/trecheckpoint, the transaction will be doomed and it will abort and rollback when we leave kernel space. I.e., if we can treclaim/trecheckpointn in kernel space, the transaction will *always* abort just after RFID, so, a possible tresume will never be executed. Is my understanding wrong? > >> so, >> continuing with the pre-checkpointed DSCR value is what seems correct. > > Reading this description suggests this patch isn't really needed. Right? Maybe the description is not clear, but I understand this patch is required, otherwise we will leave userspace with a default DSCR value. By the way, do you know if there is a change in DSCR inside a transaction, will it be reverted if the transaction is aborted? Thank you
Re: [RFC PATCH 09/11] powerpc/tm: Do not restore default DSCR
On Wed, 2018-09-12 at 16:40 -0300, Breno Leitao wrote: > In the previous TM code, trecheckpoint was being executed in the middle of > an exception, thus, DSCR was being restored to default kernel DSCR value > after trecheckpoint was done. > > With this current patchset, trecheckpoint is executed just before getting > to userspace, at ret_from_except_lite, for example. Thus, we do not need to > set default kernel DSCR value anymore, as we are leaving kernel space. It > is OK to keep the checkpointed DSCR value into the live SPR, mainly because > the transaction is doomed and it will fail soon (after RFID), What if we are going back to a suspended transaction? It will remain live until userspace does a tresume > so, > continuing with the pre-checkpointed DSCR value is what seems correct. Reading this description suggests this patch isn't really needed. Right? Mikey > That said, we must set the DSCR value that will be used in userspace now. > Current trecheckpoint() function sets it to the pre-checkpointed value > prior to lines being changed in this patch, so, removing these lines would > keep the pre-checkpointed values. > > Important to say that we do not need to do the same thing with tm_reclaim, > since it already set the DSCR to the default value, after TRECLAIM is > called, in the following lines: > > /* Load CPU's default DSCR */ > ld r0, PACA_DSCR_DEFAULT(r13) > mtspr SPRN_DSCR, r0 > > Signed-off-by: Breno Leitao > --- > arch/powerpc/kernel/tm.S | 4 > 1 file changed, 4 deletions(-) > > diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S > index 6bffbc5affe7..5427eda69846 100644 > --- a/arch/powerpc/kernel/tm.S > +++ b/arch/powerpc/kernel/tm.S > @@ -493,10 +493,6 @@ restore_gprs: > mtlrr0 > ld r2, STK_GOT(r1) > > - /* Load CPU's default DSCR */ > - ld r0, PACA_DSCR_DEFAULT(r13) > - mtspr SPRN_DSCR, r0 > - > blr > > /* ** */
