* [PATCH 1/3] powerpc/tm: Remove msr_tm_active() @ 2018-06-15 17:37 Breno Leitao 2018-06-15 17:42 ` [PATCH 2/3] powerpc/tm: Fix HTM documentation Breno Leitao 2018-06-15 20:06 ` [PATCH 1/3] powerpc/tm: Remove msr_tm_active() kbuild test robot 0 siblings, 2 replies; 19+ messages in thread From: Breno Leitao @ 2018-06-15 17:37 UTC (permalink / raw) To: linuxppc-dev; +Cc: Breno Leitao Currently msr_tm_active() is a wrapper around MSR_TM_ACTIVE() if CONFIG_PPC_TRANSACTIONAL_MEM is set, or it is just a function that returns false if CONFIG_PPC_TRANSACTIONAL_MEM is not set. This function is not necessary, since MSR_TM_ACTIVE() just do the same, checking for the TS bits and does not require any TM facility. This patchset remove every instance of msr_tm_active() and replaced it by MSR_TM_ACTIVE(); Signed-off-by: Breno Leitao <leitao@debian.org> --- arch/powerpc/kernel/process.c | 18 ++++++------------ 1 file changed, 6 insertions(+), 12 deletions(-) diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 9ef4aea9fffe..6b73d74793c2 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -102,24 +102,18 @@ static void check_if_tm_restore_required(struct task_struct *tsk) } } -static inline bool msr_tm_active(unsigned long msr) -{ - return MSR_TM_ACTIVE(msr); -} - static bool tm_active_with_fp(struct task_struct *tsk) { - return msr_tm_active(tsk->thread.regs->msr) && + return MSR_TM_ACTIVE(tsk->thread.regs->msr) && (tsk->thread.ckpt_regs.msr & MSR_FP); } static bool tm_active_with_altivec(struct task_struct *tsk) { - return msr_tm_active(tsk->thread.regs->msr) && + return MSR_TM_ACTIVE(tsk->thread.regs->msr) && (tsk->thread.ckpt_regs.msr & MSR_VEC); } #else -static inline bool msr_tm_active(unsigned long msr) { return false; } static inline void check_if_tm_restore_required(struct task_struct *tsk) { } static inline bool tm_active_with_fp(struct task_struct *tsk) { return false; } static inline bool tm_active_with_altivec(struct task_struct *tsk) { return false; } @@ -247,7 +241,7 @@ void enable_kernel_fp(void) * giveup as this would save to the 'live' structure not the * checkpointed structure. */ - if(!msr_tm_active(cpumsr) && msr_tm_active(current->thread.regs->msr)) + if(!MSR_TM_ACTIVE(cpumsr) && MSR_TM_ACTIVE(current->thread.regs->msr)) return; __giveup_fpu(current); } @@ -311,7 +305,7 @@ void enable_kernel_altivec(void) * giveup as this would save to the 'live' structure not the * checkpointed structure. */ - if(!msr_tm_active(cpumsr) && msr_tm_active(current->thread.regs->msr)) + if(!MSR_TM_ACTIVE(cpumsr) && MSR_TM_ACTIVE(current->thread.regs->msr)) return; __giveup_altivec(current); } @@ -397,7 +391,7 @@ void enable_kernel_vsx(void) * giveup as this would save to the 'live' structure not the * checkpointed structure. */ - if(!msr_tm_active(cpumsr) && msr_tm_active(current->thread.regs->msr)) + if(!MSR_TM_ACTIVE(cpumsr) && MSR_TM_ACTIVE(current->thread.regs->msr)) return; __giveup_vsx(current); } @@ -530,7 +524,7 @@ void restore_math(struct pt_regs *regs) { unsigned long msr; - if (!msr_tm_active(regs->msr) && + if (!MSR_TM_ACTIVE(regs->msr) && !current->thread.load_fp && !loadvec(current->thread)) return; -- 2.16.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 2/3] powerpc/tm: Fix HTM documentation 2018-06-15 17:37 [PATCH 1/3] powerpc/tm: Remove msr_tm_active() Breno Leitao @ 2018-06-15 17:42 ` Breno Leitao 2018-06-15 17:42 ` [PATCH 3/3] powerpc/tm: Remove struct thread_info param from tm_reclaim_thread() Breno Leitao 2018-06-15 20:06 ` [PATCH 1/3] powerpc/tm: Remove msr_tm_active() kbuild test robot 1 sibling, 1 reply; 19+ messages in thread From: Breno Leitao @ 2018-06-15 17:42 UTC (permalink / raw) To: linuxppc-dev; +Cc: Breno Leitao This patch simply fix part of the documentation on the HTM code. This fixes reference to old fields that were renamed in commit 000ec280e3dd ("powerpc: tm: Rename transct_(*) to ck(\1)_state"). It also documents better the flow after commit eb5c3f1c8647 ("powerpc: Always save/restore checkpointed regs during treclaim/trecheckpoint"), where tm_recheckpoint can recheckpoint what is in ck{fp,vr}_state blindly. Signed-off-by: Breno Leitao <leitao@debian.org> --- arch/powerpc/kernel/tm.S | 10 +++++----- arch/powerpc/kernel/traps.c | 15 +++++++++------ 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S index ff12f47a96b6..019d73053cd3 100644 --- a/arch/powerpc/kernel/tm.S +++ b/arch/powerpc/kernel/tm.S @@ -95,9 +95,9 @@ EXPORT_SYMBOL_GPL(tm_abort); * uint8_t cause) * * - Performs a full reclaim. This destroys outstanding - * transactions and updates thread->regs.tm_ckpt_* with the - * original checkpointed state. Note that thread->regs is - * unchanged. + * transactions and updates thread.ckpt_regs, thread.ckfp_state and + * thread.ckvr_state with the original checkpointed state. Note that + * thread->regs is unchanged. * * Purpose is to both abort transactions of, and preserve the state of, * a transactions at a context switch. We preserve/restore both sets of process @@ -260,7 +260,7 @@ _GLOBAL(tm_reclaim) /* Altivec (VEC/VMX/VR)*/ addi r7, r3, THREAD_CKVRSTATE - SAVE_32VRS(0, r6, r7) /* r6 scratch, r7 transact vr state */ + SAVE_32VRS(0, r6, r7) /* r6 scratch, r7 ckvr_state */ mfvscr v0 li r6, VRSTATE_VSCR stvx v0, r7, r6 @@ -271,7 +271,7 @@ _GLOBAL(tm_reclaim) /* Floating Point (FP) */ addi r7, r3, THREAD_CKFPSTATE - SAVE_32FPRS_VSRS(0, R6, R7) /* r6 scratch, r7 transact fp state */ + SAVE_32FPRS_VSRS(0, R6, R7) /* r6 scratch, r7 ckfp_state */ mffs fr0 stfd fr0,FPSTATE_FPSCR(r7) diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c index 0e17dcb48720..6742b6b3eb37 100644 --- a/arch/powerpc/kernel/traps.c +++ b/arch/powerpc/kernel/traps.c @@ -1719,16 +1719,19 @@ void fp_unavailable_tm(struct pt_regs *regs) * checkpointed FP registers need to be loaded. */ tm_reclaim_current(TM_CAUSE_FAC_UNAV); - /* Reclaim didn't save out any FPRs to transact_fprs. */ + + /* Reclaim initially saved out bogus (lazy) FPRs to ckfp_state, and + * then it was overwrite by the thr->fp_state by tm_reclaim_thread(). + * + * At this point, ck{fp,vr}_state contains the exact values we want to + * recheckpoint. + */ /* Enable FP for the task: */ current->thread.load_fp = 1; - /* This loads and recheckpoints the FP registers from - * thread.fpr[]. They will remain in registers after the - * checkpoint so we don't need to reload them after. - * If VMX is in use, the VRs now hold checkpointed values, - * so we don't want to load the VRs from the thread_struct. + /* + * Recheckpoint all the checkpointed ckpt, ck{fp, vr}_state registers. */ tm_recheckpoint(¤t->thread); } -- 2.16.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH 3/3] powerpc/tm: Remove struct thread_info param from tm_reclaim_thread() 2018-06-15 17:42 ` [PATCH 2/3] powerpc/tm: Fix HTM documentation Breno Leitao @ 2018-06-15 17:42 ` Breno Leitao 0 siblings, 0 replies; 19+ messages in thread From: Breno Leitao @ 2018-06-15 17:42 UTC (permalink / raw) To: linuxppc-dev; +Cc: Cyril Bur, Breno Leitao From: Cyril Bur <cyrilbur@gmail.com> tm_reclaim_thread() doesn't use the parameter anymore, both callers have to bother getting it as they have no need for a struct thread_info either. It was previously used but became unused in dc3106690b20 ("powerpc: tm: Always use fp_state and vr_state to store live registers") Just remove it and adjust the callers. Signed-off-by: Cyril Bur <cyrilbur@gmail.com> Signed-off-by: Breno Leitao <leitao@debian.org> --- arch/powerpc/kernel/process.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 6b73d74793c2..061a1c4cb3a8 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -860,8 +860,7 @@ static inline bool tm_enabled(struct task_struct *tsk) return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM); } -static void tm_reclaim_thread(struct thread_struct *thr, - struct thread_info *ti, uint8_t cause) +static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause) { /* * Use the current MSR TM suspended bit to track if we have @@ -908,7 +907,7 @@ static void tm_reclaim_thread(struct thread_struct *thr, void tm_reclaim_current(uint8_t cause) { tm_enable(); - tm_reclaim_thread(¤t->thread, current_thread_info(), cause); + tm_reclaim_thread(¤t->thread, cause); } static inline void tm_reclaim_task(struct task_struct *tsk) @@ -939,7 +938,7 @@ static inline void tm_reclaim_task(struct task_struct *tsk) thr->regs->ccr, thr->regs->msr, thr->regs->trap); - tm_reclaim_thread(thr, task_thread_info(tsk), TM_CAUSE_RESCHED); + tm_reclaim_thread(thr, TM_CAUSE_RESCHED); TM_DEBUG("--- tm_reclaim on pid %d complete\n", tsk->pid); -- 2.16.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH 1/3] powerpc/tm: Remove msr_tm_active() 2018-06-15 17:37 [PATCH 1/3] powerpc/tm: Remove msr_tm_active() Breno Leitao 2018-06-15 17:42 ` [PATCH 2/3] powerpc/tm: Fix HTM documentation Breno Leitao @ 2018-06-15 20:06 ` kbuild test robot 2018-06-18 22:59 ` [PATCH v2 1/4] " Breno Leitao 1 sibling, 1 reply; 19+ messages in thread From: kbuild test robot @ 2018-06-15 20:06 UTC (permalink / raw) To: Breno Leitao; +Cc: kbuild-all, linuxppc-dev, Breno Leitao [-- Attachment #1: Type: text/plain, Size: 7752 bytes --] Hi Breno, Thank you for the patch! Yet something to improve: [auto build test ERROR on powerpc/next] [also build test ERROR on v4.17 next-20180615] [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] url: https://github.com/0day-ci/linux/commits/Breno-Leitao/powerpc-tm-Remove-msr_tm_active/20180616-015124 base: https://git.kernel.org/pub/scm/linux/kernel/git/powerpc/linux.git next config: powerpc-mpc8272_ads_defconfig (attached as .config) compiler: powerpc-linux-gnu-gcc (Debian 7.2.0-11) 7.2.0 reproduce: wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross chmod +x ~/bin/make.cross # save the attached .config to linux build tree GCC_VERSION=7.2.0 make.cross ARCH=powerpc All error/warnings (new ones prefixed by >>): In file included from arch/powerpc/include/asm/processor.h:13:0, from arch/powerpc/include/asm/thread_info.h:26, from include/linux/thread_info.h:38, from arch/powerpc/include/asm/ptrace.h:158, from arch/powerpc/include/asm/hw_irq.h:12, from arch/powerpc/include/asm/irqflags.h:12, from include/linux/irqflags.h:16, from include/asm-generic/cmpxchg-local.h:6, from arch/powerpc/include/asm/cmpxchg.h:537, from arch/powerpc/include/asm/atomic.h:11, from include/linux/atomic.h:5, from include/linux/rcupdate.h:38, from include/linux/rculist.h:11, from include/linux/pid.h:5, from include/linux/sched.h:14, from arch/powerpc/kernel/process.c:18: arch/powerpc/kernel/process.c: In function 'enable_kernel_fp': >> arch/powerpc/include/asm/reg.h:65:23: error: left shift count >= width of type [-Werror=shift-count-overflow] #define __MASK(X) (1UL<<(X)) ^ >> arch/powerpc/include/asm/reg.h:117:18: note: in expansion of macro '__MASK' #define MSR_TS_T __MASK(MSR_TS_T_LG) /* Transaction Transactional */ ^~~~~~ >> arch/powerpc/include/asm/reg.h:118:22: note: in expansion of macro 'MSR_TS_T' #define MSR_TS_MASK (MSR_TS_T | MSR_TS_S) /* Transaction State bits */ ^~~~~~~~ >> arch/powerpc/include/asm/reg.h:119:34: note: in expansion of macro 'MSR_TS_MASK' #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */ ^~~~~~~~~~~ >> arch/powerpc/kernel/process.c:244:7: note: in expansion of macro 'MSR_TM_ACTIVE' if(!MSR_TM_ACTIVE(cpumsr) && MSR_TM_ACTIVE(current->thread.regs->msr)) ^~~~~~~~~~~~~ >> arch/powerpc/include/asm/reg.h:65:23: error: left shift count >= width of type [-Werror=shift-count-overflow] #define __MASK(X) (1UL<<(X)) ^ arch/powerpc/include/asm/reg.h:116:18: note: in expansion of macro '__MASK' #define MSR_TS_S __MASK(MSR_TS_S_LG) /* Transaction Suspended */ ^~~~~~ >> arch/powerpc/include/asm/reg.h:118:33: note: in expansion of macro 'MSR_TS_S' #define MSR_TS_MASK (MSR_TS_T | MSR_TS_S) /* Transaction State bits */ ^~~~~~~~ >> arch/powerpc/include/asm/reg.h:119:34: note: in expansion of macro 'MSR_TS_MASK' #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */ ^~~~~~~~~~~ >> arch/powerpc/kernel/process.c:244:7: note: in expansion of macro 'MSR_TM_ACTIVE' if(!MSR_TM_ACTIVE(cpumsr) && MSR_TM_ACTIVE(current->thread.regs->msr)) ^~~~~~~~~~~~~ >> arch/powerpc/include/asm/reg.h:65:23: error: left shift count >= width of type [-Werror=shift-count-overflow] #define __MASK(X) (1UL<<(X)) ^ >> arch/powerpc/include/asm/reg.h:117:18: note: in expansion of macro '__MASK' #define MSR_TS_T __MASK(MSR_TS_T_LG) /* Transaction Transactional */ ^~~~~~ >> arch/powerpc/include/asm/reg.h:118:22: note: in expansion of macro 'MSR_TS_T' #define MSR_TS_MASK (MSR_TS_T | MSR_TS_S) /* Transaction State bits */ ^~~~~~~~ >> arch/powerpc/include/asm/reg.h:119:34: note: in expansion of macro 'MSR_TS_MASK' #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */ ^~~~~~~~~~~ arch/powerpc/kernel/process.c:244:32: note: in expansion of macro 'MSR_TM_ACTIVE' if(!MSR_TM_ACTIVE(cpumsr) && MSR_TM_ACTIVE(current->thread.regs->msr)) ^~~~~~~~~~~~~ >> arch/powerpc/include/asm/reg.h:65:23: error: left shift count >= width of type [-Werror=shift-count-overflow] #define __MASK(X) (1UL<<(X)) ^ arch/powerpc/include/asm/reg.h:116:18: note: in expansion of macro '__MASK' #define MSR_TS_S __MASK(MSR_TS_S_LG) /* Transaction Suspended */ ^~~~~~ >> arch/powerpc/include/asm/reg.h:118:33: note: in expansion of macro 'MSR_TS_S' #define MSR_TS_MASK (MSR_TS_T | MSR_TS_S) /* Transaction State bits */ ^~~~~~~~ >> arch/powerpc/include/asm/reg.h:119:34: note: in expansion of macro 'MSR_TS_MASK' #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */ ^~~~~~~~~~~ arch/powerpc/kernel/process.c:244:32: note: in expansion of macro 'MSR_TM_ACTIVE' if(!MSR_TM_ACTIVE(cpumsr) && MSR_TM_ACTIVE(current->thread.regs->msr)) ^~~~~~~~~~~~~ arch/powerpc/kernel/process.c: In function 'restore_math': >> arch/powerpc/include/asm/reg.h:65:23: error: left shift count >= width of type [-Werror=shift-count-overflow] #define __MASK(X) (1UL<<(X)) ^ >> arch/powerpc/include/asm/reg.h:117:18: note: in expansion of macro '__MASK' #define MSR_TS_T __MASK(MSR_TS_T_LG) /* Transaction Transactional */ ^~~~~~ >> arch/powerpc/include/asm/reg.h:118:22: note: in expansion of macro 'MSR_TS_T' #define MSR_TS_MASK (MSR_TS_T | MSR_TS_S) /* Transaction State bits */ ^~~~~~~~ >> arch/powerpc/include/asm/reg.h:119:34: note: in expansion of macro 'MSR_TS_MASK' #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */ ^~~~~~~~~~~ arch/powerpc/kernel/process.c:527:7: note: in expansion of macro 'MSR_TM_ACTIVE' if (!MSR_TM_ACTIVE(regs->msr) && ^~~~~~~~~~~~~ vim +/MSR_TM_ACTIVE +244 arch/powerpc/kernel/process.c 226 227 void enable_kernel_fp(void) 228 { 229 unsigned long cpumsr; 230 231 WARN_ON(preemptible()); 232 233 cpumsr = msr_check_and_set(MSR_FP); 234 235 if (current->thread.regs && (current->thread.regs->msr & MSR_FP)) { 236 check_if_tm_restore_required(current); 237 /* 238 * If a thread has already been reclaimed then the 239 * checkpointed registers are on the CPU but have definitely 240 * been saved by the reclaim code. Don't need to and *cannot* 241 * giveup as this would save to the 'live' structure not the 242 * checkpointed structure. 243 */ > 244 if(!MSR_TM_ACTIVE(cpumsr) && MSR_TM_ACTIVE(current->thread.regs->msr)) 245 return; 246 __giveup_fpu(current); 247 } 248 } 249 EXPORT_SYMBOL(enable_kernel_fp); 250 --- 0-DAY kernel test infrastructure Open Source Technology Center https://lists.01.org/pipermail/kbuild-all Intel Corporation [-- Attachment #2: .config.gz --] [-- Type: application/gzip, Size: 12632 bytes --] ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 1/4] powerpc/tm: Remove msr_tm_active() 2018-06-15 20:06 ` [PATCH 1/3] powerpc/tm: Remove msr_tm_active() kbuild test robot @ 2018-06-18 22:59 ` Breno Leitao 2018-06-18 22:59 ` [PATCH v2 2/4] powerpc/tm: Fix HTM documentation Breno Leitao ` (3 more replies) 0 siblings, 4 replies; 19+ messages in thread From: Breno Leitao @ 2018-06-18 22:59 UTC (permalink / raw) To: linuxppc-dev; +Cc: Breno Leitao Currently msr_tm_active() is a wrapper around MSR_TM_ACTIVE() if CONFIG_PPC_TRANSACTIONAL_MEM is set, or it is just a function that returns false if CONFIG_PPC_TRANSACTIONAL_MEM is not set. This function is not necessary, since MSR_TM_ACTIVE() just do the same, checking for the TS bits and does not require any TM facility. This patchset remove every instance of msr_tm_active() and replaced it by MSR_TM_ACTIVE(). Signed-off-by: Breno Leitao <leitao@debian.org> --- arch/powerpc/kernel/process.c | 34 +++++++++++++++++++++------------- 1 file changed, 21 insertions(+), 13 deletions(-) diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index d26a150766ef..661e4ed5f628 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -102,24 +102,18 @@ static void check_if_tm_restore_required(struct task_struct *tsk) } } -static inline bool msr_tm_active(unsigned long msr) -{ - return MSR_TM_ACTIVE(msr); -} - static bool tm_active_with_fp(struct task_struct *tsk) { - return msr_tm_active(tsk->thread.regs->msr) && + return MSR_TM_ACTIVE(tsk->thread.regs->msr) && (tsk->thread.ckpt_regs.msr & MSR_FP); } static bool tm_active_with_altivec(struct task_struct *tsk) { - return msr_tm_active(tsk->thread.regs->msr) && + return MSR_TM_ACTIVE(tsk->thread.regs->msr) && (tsk->thread.ckpt_regs.msr & MSR_VEC); } #else -static inline bool msr_tm_active(unsigned long msr) { return false; } static inline void check_if_tm_restore_required(struct task_struct *tsk) { } static inline bool tm_active_with_fp(struct task_struct *tsk) { return false; } static inline bool tm_active_with_altivec(struct task_struct *tsk) { return false; } @@ -239,6 +233,7 @@ void enable_kernel_fp(void) cpumsr = msr_check_and_set(MSR_FP); if (current->thread.regs && (current->thread.regs->msr & MSR_FP)) { +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM check_if_tm_restore_required(current); /* * If a thread has already been reclaimed then the @@ -247,8 +242,10 @@ void enable_kernel_fp(void) * giveup as this would save to the 'live' structure not the * checkpointed structure. */ - if(!msr_tm_active(cpumsr) && msr_tm_active(current->thread.regs->msr)) + if (!MSR_TM_ACTIVE(cpumsr) && + MSR_TM_ACTIVE(current->thread.regs->msr)) return; +#endif __giveup_fpu(current); } } @@ -303,6 +300,7 @@ void enable_kernel_altivec(void) cpumsr = msr_check_and_set(MSR_VEC); if (current->thread.regs && (current->thread.regs->msr & MSR_VEC)) { +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM check_if_tm_restore_required(current); /* * If a thread has already been reclaimed then the @@ -311,8 +309,10 @@ void enable_kernel_altivec(void) * giveup as this would save to the 'live' structure not the * checkpointed structure. */ - if(!msr_tm_active(cpumsr) && msr_tm_active(current->thread.regs->msr)) + if (!MSR_TM_ACTIVE(cpumsr) && + MSR_TM_ACTIVE(current->thread.regs->msr)) return; +#endif __giveup_altivec(current); } } @@ -389,6 +389,7 @@ void enable_kernel_vsx(void) if (current->thread.regs && (current->thread.regs->msr & (MSR_VSX|MSR_VEC|MSR_FP))) { +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM check_if_tm_restore_required(current); /* * If a thread has already been reclaimed then the @@ -397,8 +398,10 @@ void enable_kernel_vsx(void) * giveup as this would save to the 'live' structure not the * checkpointed structure. */ - if(!msr_tm_active(cpumsr) && msr_tm_active(current->thread.regs->msr)) + if (!MSR_TM_ACTIVE(cpumsr) && + MSR_TM_ACTIVE(current->thread.regs->msr)) return; +#endif __giveup_vsx(current); } } @@ -530,9 +533,14 @@ void restore_math(struct pt_regs *regs) { unsigned long msr; - if (!msr_tm_active(regs->msr) && - !current->thread.load_fp && !loadvec(current->thread)) + if (!current->thread.load_fp && !loadvec(current->thread)) { +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM + if (!MSR_TM_ACTIVE(regs->msr)) + return; +#else return; +#endif + } msr = regs->msr; msr_check_and_set(msr_all_available); -- 2.16.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* [PATCH v2 2/4] powerpc/tm: Fix HTM documentation 2018-06-18 22:59 ` [PATCH v2 1/4] " Breno Leitao @ 2018-06-18 22:59 ` Breno Leitao 2018-08-15 23:46 ` Michael Neuling 2018-09-20 4:20 ` [v2,2/4] " Michael Ellerman 2018-06-18 22:59 ` [PATCH v2 3/4] powerpc/tm: Adjust tm_reclaim_thread() parameters Breno Leitao ` (2 subsequent siblings) 3 siblings, 2 replies; 19+ messages in thread From: Breno Leitao @ 2018-06-18 22:59 UTC (permalink / raw) To: linuxppc-dev; +Cc: Breno Leitao This patch simply fix part of the documentation on the HTM code. This fixes reference to old fields that were renamed in commit 000ec280e3dd ("powerpc: tm: Rename transct_(*) to ck(\1)_state") It also documents better the flow after commit eb5c3f1c8647 ("powerpc: Always save/restore checkpointed regs during treclaim/trecheckpoint"), where tm_recheckpoint can recheckpoint what is in ck{fp,vr}_state blindly. Signed-off-by: Breno Leitao <leitao@debian.org> --- arch/powerpc/kernel/tm.S | 10 +++++----- arch/powerpc/kernel/traps.c | 15 +++++++++------ 2 files changed, 14 insertions(+), 11 deletions(-) diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S index ff12f47a96b6..019d73053cd3 100644 --- a/arch/powerpc/kernel/tm.S +++ b/arch/powerpc/kernel/tm.S @@ -95,9 +95,9 @@ EXPORT_SYMBOL_GPL(tm_abort); * uint8_t cause) * * - Performs a full reclaim. This destroys outstanding - * transactions and updates thread->regs.tm_ckpt_* with the - * original checkpointed state. Note that thread->regs is - * unchanged. + * transactions and updates thread.ckpt_regs, thread.ckfp_state and + * thread.ckvr_state with the original checkpointed state. Note that + * thread->regs is unchanged. * * Purpose is to both abort transactions of, and preserve the state of, * a transactions at a context switch. We preserve/restore both sets of process @@ -260,7 +260,7 @@ _GLOBAL(tm_reclaim) /* Altivec (VEC/VMX/VR)*/ addi r7, r3, THREAD_CKVRSTATE - SAVE_32VRS(0, r6, r7) /* r6 scratch, r7 transact vr state */ + SAVE_32VRS(0, r6, r7) /* r6 scratch, r7 ckvr_state */ mfvscr v0 li r6, VRSTATE_VSCR stvx v0, r7, r6 @@ -271,7 +271,7 @@ _GLOBAL(tm_reclaim) /* Floating Point (FP) */ addi r7, r3, THREAD_CKFPSTATE - SAVE_32FPRS_VSRS(0, R6, R7) /* r6 scratch, r7 transact fp state */ + SAVE_32FPRS_VSRS(0, R6, R7) /* r6 scratch, r7 ckfp_state */ mffs fr0 stfd fr0,FPSTATE_FPSCR(r7) diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c index 0e17dcb48720..6742b6b3eb37 100644 --- a/arch/powerpc/kernel/traps.c +++ b/arch/powerpc/kernel/traps.c @@ -1719,16 +1719,19 @@ void fp_unavailable_tm(struct pt_regs *regs) * checkpointed FP registers need to be loaded. */ tm_reclaim_current(TM_CAUSE_FAC_UNAV); - /* Reclaim didn't save out any FPRs to transact_fprs. */ + + /* Reclaim initially saved out bogus (lazy) FPRs to ckfp_state, and + * then it was overwrite by the thr->fp_state by tm_reclaim_thread(). + * + * At this point, ck{fp,vr}_state contains the exact values we want to + * recheckpoint. + */ /* Enable FP for the task: */ current->thread.load_fp = 1; - /* This loads and recheckpoints the FP registers from - * thread.fpr[]. They will remain in registers after the - * checkpoint so we don't need to reload them after. - * If VMX is in use, the VRs now hold checkpointed values, - * so we don't want to load the VRs from the thread_struct. + /* + * Recheckpoint all the checkpointed ckpt, ck{fp, vr}_state registers. */ tm_recheckpoint(¤t->thread); } -- 2.16.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 2/4] powerpc/tm: Fix HTM documentation 2018-06-18 22:59 ` [PATCH v2 2/4] powerpc/tm: Fix HTM documentation Breno Leitao @ 2018-08-15 23:46 ` Michael Neuling 2018-09-20 4:20 ` [v2,2/4] " Michael Ellerman 1 sibling, 0 replies; 19+ messages in thread From: Michael Neuling @ 2018-08-15 23:46 UTC (permalink / raw) To: Breno Leitao, linuxppc-dev On Mon, 2018-06-18 at 19:59 -0300, Breno Leitao wrote: > This patch simply fix part of the documentation on the HTM code. >=20 > This fixes reference to old fields that were renamed in commit > 000ec280e3dd ("powerpc: tm: Rename transct_(*) to ck(\1)_state") >=20 > It also documents better the flow after commit eb5c3f1c8647 ("powerpc: > Always save/restore checkpointed regs during treclaim/trecheckpoint"), > where tm_recheckpoint can recheckpoint what is in ck{fp,vr}_state blindly= . >=20 > Signed-off-by: Breno Leitao <leitao@debian.org> Acked-By: Michael Neuling <mikey@neuling.org> Thanks > --- > arch/powerpc/kernel/tm.S | 10 +++++----- > arch/powerpc/kernel/traps.c | 15 +++++++++------ > 2 files changed, 14 insertions(+), 11 deletions(-) >=20 > diff --git a/arch/powerpc/kernel/tm.S b/arch/powerpc/kernel/tm.S > index ff12f47a96b6..019d73053cd3 100644 > --- a/arch/powerpc/kernel/tm.S > +++ b/arch/powerpc/kernel/tm.S > @@ -95,9 +95,9 @@ EXPORT_SYMBOL_GPL(tm_abort); > * uint8_t cause) > * > * - Performs a full reclaim. This destroys outstanding > - * transactions and updates thread->regs.tm_ckpt_* with the > - * original checkpointed state. Note that thread->regs is > - * unchanged. > + * transactions and updates thread.ckpt_regs, thread.ckfp_state and > + * thread.ckvr_state with the original checkpointed state. Note that > + * thread->regs is unchanged. > * > * Purpose is to both abort transactions of, and preserve the state of, > * a transactions at a context switch. We preserve/restore both sets of > process > @@ -260,7 +260,7 @@ _GLOBAL(tm_reclaim) > =20 > /* Altivec (VEC/VMX/VR)*/ > addi r7, r3, THREAD_CKVRSTATE > - SAVE_32VRS(0, r6, r7) /* r6 scratch, r7 transact vr state */ > + SAVE_32VRS(0, r6, r7) /* r6 scratch, r7 ckvr_state */ > mfvscr v0 > li r6, VRSTATE_VSCR > stvx v0, r7, r6 > @@ -271,7 +271,7 @@ _GLOBAL(tm_reclaim) > =20 > /* Floating Point (FP) */ > addi r7, r3, THREAD_CKFPSTATE > - SAVE_32FPRS_VSRS(0, R6, R7) /* r6 scratch, r7 transact fp state */ > + SAVE_32FPRS_VSRS(0, R6, R7) /* r6 scratch, r7 ckfp_state */ > mffs fr0 > stfd fr0,FPSTATE_FPSCR(r7) > =20 > diff --git a/arch/powerpc/kernel/traps.c b/arch/powerpc/kernel/traps.c > index 0e17dcb48720..6742b6b3eb37 100644 > --- a/arch/powerpc/kernel/traps.c > +++ b/arch/powerpc/kernel/traps.c > @@ -1719,16 +1719,19 @@ void fp_unavailable_tm(struct pt_regs *regs) > * checkpointed FP registers need to be loaded. > */ > tm_reclaim_current(TM_CAUSE_FAC_UNAV); > - /* Reclaim didn't save out any FPRs to transact_fprs. */ > + > + /* Reclaim initially saved out bogus (lazy) FPRs to ckfp_state, and > + * then it was overwrite by the thr->fp_state by tm_reclaim_thread(). > + * > + * At this point, ck{fp,vr}_state contains the exact values we want to > + * recheckpoint. > + */ > =20 > /* Enable FP for the task: */ > current->thread.load_fp =3D 1; > =20 > - /* This loads and recheckpoints the FP registers from > - * thread.fpr[]. They will remain in registers after the > - * checkpoint so we don't need to reload them after. > - * If VMX is in use, the VRs now hold checkpointed values, > - * so we don't want to load the VRs from the thread_struct. > + /* > + * Recheckpoint all the checkpointed ckpt, ck{fp, vr}_state registers. > */ > tm_recheckpoint(¤t->thread); > } ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [v2,2/4] powerpc/tm: Fix HTM documentation 2018-06-18 22:59 ` [PATCH v2 2/4] powerpc/tm: Fix HTM documentation Breno Leitao 2018-08-15 23:46 ` Michael Neuling @ 2018-09-20 4:20 ` Michael Ellerman 1 sibling, 0 replies; 19+ messages in thread From: Michael Ellerman @ 2018-09-20 4:20 UTC (permalink / raw) To: Breno Leitao, linuxppc-dev; +Cc: Breno Leitao On Mon, 2018-06-18 at 22:59:42 UTC, Breno Leitao wrote: > This patch simply fix part of the documentation on the HTM code. > > This fixes reference to old fields that were renamed in commit > 000ec280e3dd ("powerpc: tm: Rename transct_(*) to ck(\1)_state") > > It also documents better the flow after commit eb5c3f1c8647 ("powerpc: > Always save/restore checkpointed regs during treclaim/trecheckpoint"), > where tm_recheckpoint can recheckpoint what is in ck{fp,vr}_state blindly. > > Signed-off-by: Breno Leitao <leitao@debian.org> > Acked-By: Michael Neuling <mikey@neuling.org> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/96695563cebfb810b09479a9951ebb cheers ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 3/4] powerpc/tm: Adjust tm_reclaim_thread() parameters 2018-06-18 22:59 ` [PATCH v2 1/4] " Breno Leitao 2018-06-18 22:59 ` [PATCH v2 2/4] powerpc/tm: Fix HTM documentation Breno Leitao @ 2018-06-18 22:59 ` Breno Leitao 2018-08-15 23:48 ` Michael Neuling 2018-06-18 22:59 ` [PATCH v2 4/4] powerpc/tm: Do not recheckpoint non-tm task Breno Leitao 2018-08-15 23:46 ` [PATCH v2 1/4] powerpc/tm: Remove msr_tm_active() Michael Neuling 3 siblings, 1 reply; 19+ messages in thread From: Breno Leitao @ 2018-06-18 22:59 UTC (permalink / raw) To: linuxppc-dev; +Cc: Cyril Bur, Breno Leitao From: Cyril Bur <cyrilbur@gmail.com> tm_reclaim_thread() doesn't use the parameter anymore, both callers have to bother getting it as they have no need for a struct thread_info either. It was previously used but became unused in commit dc3106690b20 ("powerpc: tm: Always use fp_state and vr_state to store live registers") Just remove it and adjust the callers. Signed-off-by: Cyril Bur <cyrilbur@gmail.com> Signed-off-by: Breno Leitao <leitao@debian.org> --- arch/powerpc/kernel/process.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 9ef4aea9fffe..f8beee03f00a 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -866,8 +866,7 @@ static inline bool tm_enabled(struct task_struct *tsk) return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM); } -static void tm_reclaim_thread(struct thread_struct *thr, - struct thread_info *ti, uint8_t cause) +static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause) { /* * Use the current MSR TM suspended bit to track if we have @@ -914,7 +913,7 @@ static void tm_reclaim_thread(struct thread_struct *thr, void tm_reclaim_current(uint8_t cause) { tm_enable(); - tm_reclaim_thread(¤t->thread, current_thread_info(), cause); + tm_reclaim_thread(¤t->thread, cause); } static inline void tm_reclaim_task(struct task_struct *tsk) @@ -945,7 +944,7 @@ static inline void tm_reclaim_task(struct task_struct *tsk) thr->regs->ccr, thr->regs->msr, thr->regs->trap); - tm_reclaim_thread(thr, task_thread_info(tsk), TM_CAUSE_RESCHED); + tm_reclaim_thread(thr, TM_CAUSE_RESCHED); TM_DEBUG("--- tm_reclaim on pid %d complete\n", tsk->pid); -- 2.16.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 3/4] powerpc/tm: Adjust tm_reclaim_thread() parameters 2018-06-18 22:59 ` [PATCH v2 3/4] powerpc/tm: Adjust tm_reclaim_thread() parameters Breno Leitao @ 2018-08-15 23:48 ` Michael Neuling 0 siblings, 0 replies; 19+ messages in thread From: Michael Neuling @ 2018-08-15 23:48 UTC (permalink / raw) To: Breno Leitao, linuxppc-dev; +Cc: Cyril Bur On Mon, 2018-06-18 at 19:59 -0300, Breno Leitao wrote: > From: Cyril Bur <cyrilbur@gmail.com> >=20 > tm_reclaim_thread() doesn't use the parameter anymore, both callers have > to bother getting it as they have no need for a struct thread_info > either. >=20 > It was previously used but became unused in commit > dc3106690b20 ("powerpc: tm: Always use fp_state and vr_state to store liv= e > registers") >=20 > Just remove it and adjust the callers. >=20 > Signed-off-by: Cyril Bur <cyrilbur@gmail.com> > Signed-off-by: Breno Leitao <leitao@debian.org> Acked-by: Michael Neuling <mikey@neuling.org.> > --- > arch/powerpc/kernel/process.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) >=20 > diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.= c > index 9ef4aea9fffe..f8beee03f00a 100644 > --- a/arch/powerpc/kernel/process.c > +++ b/arch/powerpc/kernel/process.c > @@ -866,8 +866,7 @@ static inline bool tm_enabled(struct task_struct *tsk= ) > return tsk && tsk->thread.regs && (tsk->thread.regs->msr & MSR_TM); > } > =20 > -static void tm_reclaim_thread(struct thread_struct *thr, > - struct thread_info *ti, uint8_t cause) > +static void tm_reclaim_thread(struct thread_struct *thr, uint8_t cause) > { > /* > * Use the current MSR TM suspended bit to track if we have > @@ -914,7 +913,7 @@ static void tm_reclaim_thread(struct thread_struct *t= hr, > void tm_reclaim_current(uint8_t cause) > { > tm_enable(); > - tm_reclaim_thread(¤t->thread, current_thread_info(), cause); > + tm_reclaim_thread(¤t->thread, cause); > } > =20 > static inline void tm_reclaim_task(struct task_struct *tsk) > @@ -945,7 +944,7 @@ static inline void tm_reclaim_task(struct task_struct > *tsk) > thr->regs->ccr, thr->regs->msr, > thr->regs->trap); > =20 > - tm_reclaim_thread(thr, task_thread_info(tsk), TM_CAUSE_RESCHED); > + tm_reclaim_thread(thr, TM_CAUSE_RESCHED); > =20 > TM_DEBUG("--- tm_reclaim on pid %d complete\n", > tsk->pid); ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v2 4/4] powerpc/tm: Do not recheckpoint non-tm task 2018-06-18 22:59 ` [PATCH v2 1/4] " Breno Leitao 2018-06-18 22:59 ` [PATCH v2 2/4] powerpc/tm: Fix HTM documentation Breno Leitao 2018-06-18 22:59 ` [PATCH v2 3/4] powerpc/tm: Adjust tm_reclaim_thread() parameters Breno Leitao @ 2018-06-18 22:59 ` Breno Leitao 2018-08-15 23:50 ` Michael Neuling 2018-08-15 23:46 ` [PATCH v2 1/4] powerpc/tm: Remove msr_tm_active() Michael Neuling 3 siblings, 1 reply; 19+ messages in thread From: Breno Leitao @ 2018-06-18 22:59 UTC (permalink / raw) To: linuxppc-dev; +Cc: Breno Leitao 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); } } -- 2.16.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] powerpc/tm: Do not recheckpoint non-tm task 2018-06-18 22:59 ` [PATCH v2 4/4] powerpc/tm: Do not recheckpoint non-tm task Breno Leitao @ 2018-08-15 23:50 ` Michael Neuling 2018-08-16 14:19 ` Breno Leitao 0 siblings, 1 reply; 19+ messages in thread From: Michael Neuling @ 2018-08-15 23:50 UTC (permalink / raw) To: Breno Leitao, linuxppc-dev 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, wh= ich > is clearly wrong since task B might not be an active TM user. >=20 > 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. >=20 > Signed-off-by: Breno Leitao <leitao@debian.org> > --- > arch/powerpc/kernel/process.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) >=20 > 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_struc= t > *prev, > prev->thread.regs->msr &=3D ~MSR_TM; > } > =20 > - 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 its= elf. --- 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 ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 4/4] powerpc/tm: Do not recheckpoint non-tm task 2018-08-15 23:50 ` Michael Neuling @ 2018-08-16 14:19 ` Breno Leitao 0 siblings, 0 replies; 19+ messages in thread From: Breno Leitao @ 2018-08-16 14:19 UTC (permalink / raw) To: Michael Neuling, linuxppc-dev 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. ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] powerpc/tm: Remove msr_tm_active() 2018-06-18 22:59 ` [PATCH v2 1/4] " Breno Leitao ` (2 preceding siblings ...) 2018-06-18 22:59 ` [PATCH v2 4/4] powerpc/tm: Do not recheckpoint non-tm task Breno Leitao @ 2018-08-15 23:46 ` Michael Neuling 2018-08-16 17:21 ` [PATCH v3] " Breno Leitao 2018-08-17 0:49 ` [PATCH v2 1/4] " Michael Ellerman 3 siblings, 2 replies; 19+ messages in thread From: Michael Neuling @ 2018-08-15 23:46 UTC (permalink / raw) To: Breno Leitao, linuxppc-dev On Mon, 2018-06-18 at 19:59 -0300, Breno Leitao wrote: > Currently msr_tm_active() is a wrapper around MSR_TM_ACTIVE() if > CONFIG_PPC_TRANSACTIONAL_MEM is set, or it is just a function that > returns false if CONFIG_PPC_TRANSACTIONAL_MEM is not set. >=20 > This function is not necessary, since MSR_TM_ACTIVE() just do the same, > checking for the TS bits and does not require any TM facility. >=20 > This patchset remove every instance of msr_tm_active() and replaced it > by MSR_TM_ACTIVE(). >=20 > Signed-off-by: Breno Leitao <leitao@debian.org> >=20 Patch looks good... one minor nit below... > =20 > - if (!msr_tm_active(regs->msr) && > - !current->thread.load_fp && !loadvec(current->thread)) > + if (!current->thread.load_fp && !loadvec(current->thread)) { > +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM > + if (!MSR_TM_ACTIVE(regs->msr)) > + return; Can you make a MSR_TM_ACTIVE() that returns false when !CONFIG_PPC_TRANSACTIONAL_MEM. Then you don't need this inline #ifdef. Mikey > +#else > return; > +#endif > + } > =20 > msr =3D regs->msr; > msr_check_and_set(msr_all_available); ^ permalink raw reply [flat|nested] 19+ messages in thread
* [PATCH v3] powerpc/tm: Remove msr_tm_active() 2018-08-15 23:46 ` [PATCH v2 1/4] powerpc/tm: Remove msr_tm_active() Michael Neuling @ 2018-08-16 17:21 ` Breno Leitao 2018-10-04 6:14 ` [v3] " Michael Ellerman 2018-08-17 0:49 ` [PATCH v2 1/4] " Michael Ellerman 1 sibling, 1 reply; 19+ messages in thread From: Breno Leitao @ 2018-08-16 17:21 UTC (permalink / raw) To: linuxppc-dev; +Cc: mikey, Breno Leitao Currently msr_tm_active() is a wrapper around MSR_TM_ACTIVE() if CONFIG_PPC_TRANSACTIONAL_MEM is set, or it is just a function that returns false if CONFIG_PPC_TRANSACTIONAL_MEM is not set. This function is not necessary, since MSR_TM_ACTIVE() just do the same and could be used, removing the dualism and simplifying the code. This patchset remove every instance of msr_tm_active() and replaced it by MSR_TM_ACTIVE(). Signed-off-by: Breno Leitao <leitao@debian.org> --- arch/powerpc/include/asm/reg.h | 7 ++++++- arch/powerpc/kernel/process.c | 21 +++++++++------------ 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/arch/powerpc/include/asm/reg.h b/arch/powerpc/include/asm/reg.h index 562568414cf4..58393bc6c964 100644 --- a/arch/powerpc/include/asm/reg.h +++ b/arch/powerpc/include/asm/reg.h @@ -116,11 +116,16 @@ #define MSR_TS_S __MASK(MSR_TS_S_LG) /* Transaction Suspended */ #define MSR_TS_T __MASK(MSR_TS_T_LG) /* Transaction Transactional */ #define MSR_TS_MASK (MSR_TS_T | MSR_TS_S) /* Transaction State bits */ -#define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */ #define MSR_TM_RESV(x) (((x) & MSR_TS_MASK) == MSR_TS_MASK) /* Reserved */ #define MSR_TM_TRANSACTIONAL(x) (((x) & MSR_TS_MASK) == MSR_TS_T) #define MSR_TM_SUSPENDED(x) (((x) & MSR_TS_MASK) == MSR_TS_S) +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM +#define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */ +#else +#define MSR_TM_ACTIVE(x) 0 +#endif + #if defined(CONFIG_PPC_BOOK3S_64) #define MSR_64BIT MSR_SF diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 9ef4aea9fffe..f18dba07ea16 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -102,24 +102,18 @@ static void check_if_tm_restore_required(struct task_struct *tsk) } } -static inline bool msr_tm_active(unsigned long msr) -{ - return MSR_TM_ACTIVE(msr); -} - static bool tm_active_with_fp(struct task_struct *tsk) { - return msr_tm_active(tsk->thread.regs->msr) && + return MSR_TM_ACTIVE(tsk->thread.regs->msr) && (tsk->thread.ckpt_regs.msr & MSR_FP); } static bool tm_active_with_altivec(struct task_struct *tsk) { - return msr_tm_active(tsk->thread.regs->msr) && + return MSR_TM_ACTIVE(tsk->thread.regs->msr) && (tsk->thread.ckpt_regs.msr & MSR_VEC); } #else -static inline bool msr_tm_active(unsigned long msr) { return false; } static inline void check_if_tm_restore_required(struct task_struct *tsk) { } static inline bool tm_active_with_fp(struct task_struct *tsk) { return false; } static inline bool tm_active_with_altivec(struct task_struct *tsk) { return false; } @@ -247,7 +241,8 @@ void enable_kernel_fp(void) * giveup as this would save to the 'live' structure not the * checkpointed structure. */ - if(!msr_tm_active(cpumsr) && msr_tm_active(current->thread.regs->msr)) + if (!MSR_TM_ACTIVE(cpumsr) && + MSR_TM_ACTIVE(current->thread.regs->msr)) return; __giveup_fpu(current); } @@ -311,7 +306,8 @@ void enable_kernel_altivec(void) * giveup as this would save to the 'live' structure not the * checkpointed structure. */ - if(!msr_tm_active(cpumsr) && msr_tm_active(current->thread.regs->msr)) + if (!MSR_TM_ACTIVE(cpumsr) && + MSR_TM_ACTIVE(current->thread.regs->msr)) return; __giveup_altivec(current); } @@ -397,7 +393,8 @@ void enable_kernel_vsx(void) * giveup as this would save to the 'live' structure not the * checkpointed structure. */ - if(!msr_tm_active(cpumsr) && msr_tm_active(current->thread.regs->msr)) + if (!MSR_TM_ACTIVE(cpumsr) && + MSR_TM_ACTIVE(current->thread.regs->msr)) return; __giveup_vsx(current); } @@ -530,7 +527,7 @@ void restore_math(struct pt_regs *regs) { unsigned long msr; - if (!msr_tm_active(regs->msr) && + if (!MSR_TM_ACTIVE(regs->msr) && !current->thread.load_fp && !loadvec(current->thread)) return; -- 2.16.3 ^ permalink raw reply related [flat|nested] 19+ messages in thread
* Re: [v3] powerpc/tm: Remove msr_tm_active() 2018-08-16 17:21 ` [PATCH v3] " Breno Leitao @ 2018-10-04 6:14 ` Michael Ellerman 0 siblings, 0 replies; 19+ messages in thread From: Michael Ellerman @ 2018-10-04 6:14 UTC (permalink / raw) To: Breno Leitao, linuxppc-dev; +Cc: Breno Leitao, mikey On Thu, 2018-08-16 at 17:21:07 UTC, Breno Leitao wrote: > Currently msr_tm_active() is a wrapper around MSR_TM_ACTIVE() if > CONFIG_PPC_TRANSACTIONAL_MEM is set, or it is just a function that > returns false if CONFIG_PPC_TRANSACTIONAL_MEM is not set. > > This function is not necessary, since MSR_TM_ACTIVE() just do the same and > could be used, removing the dualism and simplifying the code. > > This patchset remove every instance of msr_tm_active() and replaced it > by MSR_TM_ACTIVE(). > > Signed-off-by: Breno Leitao <leitao@debian.org> Applied to powerpc next, thanks. https://git.kernel.org/powerpc/c/5c784c8414fba11b62e12439f11e10 cheers ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] powerpc/tm: Remove msr_tm_active() 2018-08-15 23:46 ` [PATCH v2 1/4] powerpc/tm: Remove msr_tm_active() Michael Neuling 2018-08-16 17:21 ` [PATCH v3] " Breno Leitao @ 2018-08-17 0:49 ` Michael Ellerman 2018-08-17 12:26 ` Breno Leitao 1 sibling, 1 reply; 19+ messages in thread From: Michael Ellerman @ 2018-08-17 0:49 UTC (permalink / raw) To: Michael Neuling, Breno Leitao, linuxppc-dev Michael Neuling <mikey@neuling.org> writes: > On Mon, 2018-06-18 at 19:59 -0300, Breno Leitao wrote: >> Currently msr_tm_active() is a wrapper around MSR_TM_ACTIVE() if >> CONFIG_PPC_TRANSACTIONAL_MEM is set, or it is just a function that >> returns false if CONFIG_PPC_TRANSACTIONAL_MEM is not set. >> >> This function is not necessary, since MSR_TM_ACTIVE() just do the same, >> checking for the TS bits and does not require any TM facility. >> >> This patchset remove every instance of msr_tm_active() and replaced it >> by MSR_TM_ACTIVE(). >> >> Signed-off-by: Breno Leitao <leitao@debian.org> >> > > Patch looks good... one minor nit below... > >> >> - if (!msr_tm_active(regs->msr) && >> - !current->thread.load_fp && !loadvec(current->thread)) >> + if (!current->thread.load_fp && !loadvec(current->thread)) { >> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM >> + if (!MSR_TM_ACTIVE(regs->msr)) >> + return; > > Can you make a MSR_TM_ACTIVE() that returns false when > !CONFIG_PPC_TRANSACTIONAL_MEM. Then you don't need this inline #ifdef. Is that safe? I see ~50 callers of MSR_TM_ACTIVE(), are they all inside #ifdef TM ? cheers ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] powerpc/tm: Remove msr_tm_active() 2018-08-17 0:49 ` [PATCH v2 1/4] " Michael Ellerman @ 2018-08-17 12:26 ` Breno Leitao 2018-08-21 6:33 ` Michael Ellerman 0 siblings, 1 reply; 19+ messages in thread From: Breno Leitao @ 2018-08-17 12:26 UTC (permalink / raw) To: Michael Ellerman, linuxppc-dev; +Cc: Michael Neuling Hi Michael, On 08/16/2018 09:49 PM, Michael Ellerman wrote: > Michael Neuling <mikey@neuling.org> writes: > >> On Mon, 2018-06-18 at 19:59 -0300, Breno Leitao wrote: >>> Currently msr_tm_active() is a wrapper around MSR_TM_ACTIVE() if >>> CONFIG_PPC_TRANSACTIONAL_MEM is set, or it is just a function that >>> returns false if CONFIG_PPC_TRANSACTIONAL_MEM is not set. >>> >>> This function is not necessary, since MSR_TM_ACTIVE() just do the same, >>> checking for the TS bits and does not require any TM facility. >>> >>> This patchset remove every instance of msr_tm_active() and replaced it >>> by MSR_TM_ACTIVE(). >>> >>> Signed-off-by: Breno Leitao <leitao@debian.org> >>> >> >> Patch looks good... one minor nit below... >> >>> >>> - if (!msr_tm_active(regs->msr) && >>> - !current->thread.load_fp && !loadvec(current->thread)) >>> + if (!current->thread.load_fp && !loadvec(current->thread)) { >>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM >>> + if (!MSR_TM_ACTIVE(regs->msr)) >>> + return; >> >> Can you make a MSR_TM_ACTIVE() that returns false when >> !CONFIG_PPC_TRANSACTIONAL_MEM. Then you don't need this inline #ifdef. > > Is that safe? > > I see ~50 callers of MSR_TM_ACTIVE(), are they all inside #ifdef TM ? I checked all of them, and the only two that are not called inside a #ifdef are at kvm/book3s_hv_tm.c. They are: kvm/book3s_hv_tm.c: if (!MSR_TM_ACTIVE(msr)) { kvm/book3s_hv_tm.c: if (MSR_TM_ACTIVE(msr) || !(vcpu->arch.texasr & TEXASR_FS)) { All the others are being called inside the #ifdef Other than that, I do not see why it would be a problem in the way I implemented it, since it will return false for the two cases above, which seems correct. Take a look on how the definition became: #ifdef CONFIG_PPC_TRANSACTIONAL_MEM #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */ #else #define MSR_TM_ACTIVE(x) 0 #endif I also tested it with different config files, and I didn't see any complain. These are the platforms I built for. * powernv_defconfig * pseries_le_defconfig * pseries_defconfig * ppc64_defconfig * ppc64e_defconfig * pmac32_defconfig * ppc44x_defconfig * mpc85xx_smp_defconfig * mpc85xx_defconfig * ps3_defconfig Anyway, if you have any other suggestion I can follow in order to guarantee that I am not causing any regression, I would be happy. Touching these core kernel macros is scary! Thanks! ^ permalink raw reply [flat|nested] 19+ messages in thread
* Re: [PATCH v2 1/4] powerpc/tm: Remove msr_tm_active() 2018-08-17 12:26 ` Breno Leitao @ 2018-08-21 6:33 ` Michael Ellerman 0 siblings, 0 replies; 19+ messages in thread From: Michael Ellerman @ 2018-08-21 6:33 UTC (permalink / raw) To: Breno Leitao, linuxppc-dev; +Cc: Michael Neuling Breno Leitao <leitao@debian.org> writes: > On 08/16/2018 09:49 PM, Michael Ellerman wrote: >> Michael Neuling <mikey@neuling.org> writes: >> >>> On Mon, 2018-06-18 at 19:59 -0300, Breno Leitao wrote: >>>> Currently msr_tm_active() is a wrapper around MSR_TM_ACTIVE() if >>>> CONFIG_PPC_TRANSACTIONAL_MEM is set, or it is just a function that >>>> returns false if CONFIG_PPC_TRANSACTIONAL_MEM is not set. >>>> >>>> This function is not necessary, since MSR_TM_ACTIVE() just do the same, >>>> checking for the TS bits and does not require any TM facility. >>>> >>>> This patchset remove every instance of msr_tm_active() and replaced it >>>> by MSR_TM_ACTIVE(). >>>> >>>> Signed-off-by: Breno Leitao <leitao@debian.org> >>>> >>> >>> Patch looks good... one minor nit below... >>> >>>> >>>> - if (!msr_tm_active(regs->msr) && >>>> - !current->thread.load_fp && !loadvec(current->thread)) >>>> + if (!current->thread.load_fp && !loadvec(current->thread)) { >>>> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM >>>> + if (!MSR_TM_ACTIVE(regs->msr)) >>>> + return; >>> >>> Can you make a MSR_TM_ACTIVE() that returns false when >>> !CONFIG_PPC_TRANSACTIONAL_MEM. Then you don't need this inline #ifdef. >> >> Is that safe? >> >> I see ~50 callers of MSR_TM_ACTIVE(), are they all inside #ifdef TM ? > > I checked all of them, and the only two that are not called inside a #ifdef > are at kvm/book3s_hv_tm.c. They are: > > kvm/book3s_hv_tm.c: if (!MSR_TM_ACTIVE(msr)) { > kvm/book3s_hv_tm.c: if (MSR_TM_ACTIVE(msr) || !(vcpu->arch.texasr & TEXASR_FS)) { That whole file is only built if TM=y: kvm-hv-$(CONFIG_PPC_TRANSACTIONAL_MEM) += \ book3s_hv_tm.o > All the others are being called inside the #ifdef > > Other than that, I do not see why it would be a problem in the way I > implemented it, since it will return false for the two cases above, which > seems correct. Take a look on how the definition became: > > #ifdef CONFIG_PPC_TRANSACTIONAL_MEM > #define MSR_TM_ACTIVE(x) (((x) & MSR_TS_MASK) != 0) /* Transaction active? */ > #else > #define MSR_TM_ACTIVE(x) 0 > #endif Imagine we had some code somewhere that checked for TM being active in a non-TM aware kernel, that would break with this change, because now the MSR check does nothing when TM=n. eg. we might check at boot time that we're not transactional, eg. in case we came from a kdump kernel that was in a transaction. So if all the call-sites are already inside an #ifdef I'd be inclined to not add the #ifdef around the MSR_TM_ACTIVE macro. That way the macro can always be used to check the MSR value, whether TM is compiled in or out. cheers ^ permalink raw reply [flat|nested] 19+ messages in thread
end of thread, other threads:[~2018-10-04 6:24 UTC | newest] Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-06-15 17:37 [PATCH 1/3] powerpc/tm: Remove msr_tm_active() Breno Leitao 2018-06-15 17:42 ` [PATCH 2/3] powerpc/tm: Fix HTM documentation Breno Leitao 2018-06-15 17:42 ` [PATCH 3/3] powerpc/tm: Remove struct thread_info param from tm_reclaim_thread() Breno Leitao 2018-06-15 20:06 ` [PATCH 1/3] powerpc/tm: Remove msr_tm_active() kbuild test robot 2018-06-18 22:59 ` [PATCH v2 1/4] " Breno Leitao 2018-06-18 22:59 ` [PATCH v2 2/4] powerpc/tm: Fix HTM documentation Breno Leitao 2018-08-15 23:46 ` Michael Neuling 2018-09-20 4:20 ` [v2,2/4] " Michael Ellerman 2018-06-18 22:59 ` [PATCH v2 3/4] powerpc/tm: Adjust tm_reclaim_thread() parameters Breno Leitao 2018-08-15 23:48 ` Michael Neuling 2018-06-18 22:59 ` [PATCH v2 4/4] powerpc/tm: Do not recheckpoint non-tm task Breno Leitao 2018-08-15 23:50 ` Michael Neuling 2018-08-16 14:19 ` Breno Leitao 2018-08-15 23:46 ` [PATCH v2 1/4] powerpc/tm: Remove msr_tm_active() Michael Neuling 2018-08-16 17:21 ` [PATCH v3] " Breno Leitao 2018-10-04 6:14 ` [v3] " Michael Ellerman 2018-08-17 0:49 ` [PATCH v2 1/4] " Michael Ellerman 2018-08-17 12:26 ` Breno Leitao 2018-08-21 6:33 ` Michael Ellerman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.