| Message ID | 1529362784-14194-1-git-send-email-leitao@debian.org (mailing list archive) |
|---|---|
| State | Superseded |
| 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: > 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. Mikey > +#else > return; > +#endif > + } > > msr = regs->msr; > msr_check_and_set(msr_all_available);
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
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!
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
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);
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(-)