| Message ID | 1507803439-12862-3-git-send-email-mpe@ellerman.id.au (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 54820530c5faa9fd78e1c08cb6449100b1a19157 |
| Headers | show |
| Series | [1/4] powerpc/tm: Add commandline option to disable hardware transactional memory | expand |
On 10/12/2017 12:17 PM, Michael Ellerman wrote: > + pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n"); > + cur_cpu_spec->cpu_features |= CPU_FTR_TM; > + cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND; > + tm_suspend_disabled = true; This doesn't look right because if suspend is not available, you need to clear the original PPC_FEATURE2_HTM flag because the semantics are not right, so that applications can use fallback code. Otherwise, applications may incorrectly select the HTM code and break if running on a system which supports HTM, but without the suspend state. The new flag should say that HTM is supported, but without the suspend state, and it should be always set if PPC_FEATURE2_HTM is set. Thanks, Florian
Florian Weimer <fweimer@redhat.com> writes: > On 10/12/2017 12:17 PM, Michael Ellerman wrote: >> + pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n"); >> + cur_cpu_spec->cpu_features |= CPU_FTR_TM; >> + cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND; >> + tm_suspend_disabled = true; > > This doesn't look right because if suspend is not available, you need to > clear the original PPC_FEATURE2_HTM flag because the semantics are not > right, so that applications can use fallback code. Otherwise, > applications may incorrectly select the HTM code and break if running on > a system which supports HTM, but without the suspend state. Just clarifying: it won't break an application, but abort the transaction, which can cause a performance hit. > The new flag should say that HTM is supported, but without the suspend > state, and it should be always set if PPC_FEATURE2_HTM is set. If we change the semantics of this bit, old applications that don't suspend transactions won't run on these processors, even if they're safe to run. If we adopt the current semantics, only applications that enter in suspend state will have to be modified in order to not get a performance regression.
On 10/19/2017 02:04 PM, Tulio Magno Quites Machado Filho wrote: > Florian Weimer <fweimer@redhat.com> writes: > >> On 10/12/2017 12:17 PM, Michael Ellerman wrote: >>> + pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n"); >>> + cur_cpu_spec->cpu_features |= CPU_FTR_TM; >>> + cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND; >>> + tm_suspend_disabled = true; >> >> This doesn't look right because if suspend is not available, you need to >> clear the original PPC_FEATURE2_HTM flag because the semantics are not >> right, so that applications can use fallback code. Otherwise, >> applications may incorrectly select the HTM code and break if running on >> a system which supports HTM, but without the suspend state. > > Just clarifying: it won't break an application, but abort the transaction, > which can cause a performance hit. And in this case, it is better not HTM at all. >> The new flag should say that HTM is supported, but without the suspend >> state, and it should be always set if PPC_FEATURE2_HTM is set. > > If we change the semantics of this bit, old applications that don't suspend > transactions won't run on these processors, even if they're safe to run. > > If we adopt the current semantics, only applications that enter in suspend > state will have to be modified in order to not get a performance regression. In this light, the trade-off implied by the posted patch seems to be the right one. But the other discussion of an ABI change still makes me a bit nervous. Thanks, Florian
Forwarding some comments from Adhemerval sent to libc-alpha [1]... Adhemerval Zanella <adhemerval.zanella@linaro.org> writes: >Florian Weimer <fweimer@redhat.com> writes: > >> On 10/12/2017 12:17 PM, Michael Ellerman wrote: >>> + pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n"); >>> + cur_cpu_spec->cpu_features |= CPU_FTR_TM; >>> + cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND; >>> + tm_suspend_disabled = true; >> >> This doesn't look right because if suspend is not available, you need to >> clear the original PPC_FEATURE2_HTM flag because the semantics are not >> right, so that applications can use fallback code. Otherwise, >> applications may incorrectly select the HTM code and break if running on >> a system which supports HTM, but without the suspend state. >> >> The new flag should say that HTM is supported, but without the suspend >> state, and it should be always set if PPC_FEATURE2_HTM is set. > > Will it also change TEXARS with the abort information? It should, with a permanent error cause so that old applications entering suspended state can adopt another technique. Michael, could you clarify if this is indeed happening, please? > I completely agree with Florian here, this is as *ABI* change > and the kernel need to advertise a different TM ABI instead > of as an extension. Adhemerval, could you elaborate which problems you're foreseeing, please?
On 19/10/2017 11:34, Tulio Magno Quites Machado Filho wrote: > Forwarding some comments from Adhemerval sent to libc-alpha [1]... > > Adhemerval Zanella <adhemerval.zanella@linaro.org> writes: >> Florian Weimer <fweimer@redhat.com> writes: >> >>> On 10/12/2017 12:17 PM, Michael Ellerman wrote: >>>> + pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n"); >>>> + cur_cpu_spec->cpu_features |= CPU_FTR_TM; >>>> + cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND; >>>> + tm_suspend_disabled = true; >>> >>> This doesn't look right because if suspend is not available, you need to >>> clear the original PPC_FEATURE2_HTM flag because the semantics are not >>> right, so that applications can use fallback code. Otherwise, >>> applications may incorrectly select the HTM code and break if running on >>> a system which supports HTM, but without the suspend state. >>> >>> The new flag should say that HTM is supported, but without the suspend >>> state, and it should be always set if PPC_FEATURE2_HTM is set. >> >> Will it also change TEXARS with the abort information? > > It should, with a permanent error cause so that old applications entering > suspended state can adopt another technique. > Michael, could you clarify if this is indeed happening, please? > >> I completely agree with Florian here, this is as *ABI* change >> and the kernel need to advertise a different TM ABI instead >> of as an extension. > > Adhemerval, could you elaborate which problems you're foreseeing, please? > Pretty much the same Florian already stated: an application can not any more assume for instance: tsr. 0 mfcr r9,128 andis. r10,r9,0x4000 be cr0,L(suspend) andis. r10,r9,0x2000 be cr0,L(transactional) However thinking more about it I am not sure if this should be really a problem: on default HTM mode the program must handle self-induced failures as the tbegin. failure path and I assume trying to suspend/resume in this case will trigger this. For instance: if (__builtin_tbegin (0)) { /* some transactional stuff. */ __builtin_tsuspend (); /* non transactional stuff. */ __builtin_tresume (); /* more transactional stuff. */ } else { /* fall-out code. */ } So I assume for these chips without suspend/resume support the example code will always run the fall-out code.
On Thu, 2017-10-19 at 11:34 -0200, Tulio Magno Quites Machado Filho wrote: > Forwarding some comments from Adhemerval sent to libc-alpha [1]... > > Adhemerval Zanella <adhemerval.zanella@linaro.org> writes: > > Florian Weimer <fweimer@redhat.com> writes: > > > > > On 10/12/2017 12:17 PM, Michael Ellerman wrote: > > > > + pr_info("Enabling TM (Transactional Memory) with Suspend > > > > Disabled\n"); > > > > + cur_cpu_spec->cpu_features |= CPU_FTR_TM; > > > > + cur_cpu_spec->cpu_user_features2 |= > > > > PPC_FEATURE2_HTM_NO_SUSPEND; > > > > + tm_suspend_disabled = true; > > > > > > This doesn't look right because if suspend is not available, you need to > > > clear the original PPC_FEATURE2_HTM flag because the semantics are not > > > right, so that applications can use fallback code. Otherwise, > > > applications may incorrectly select the HTM code and break if running on > > > a system which supports HTM, but without the suspend state. > > > > > > The new flag should say that HTM is supported, but without the suspend > > > state, and it should be always set if PPC_FEATURE2_HTM is set. > > > > Will it also change TEXARS with the abort information? > > It should, with a permanent error cause so that old applications entering > suspended state can adopt another technique. > Michael, could you clarify if this is indeed happening, please? It will. If we hit a tsuspend and we are in this mode, the hardware will abort us and set the Failure Persistent bit (unless there is some other exception pending at the same time which causes the abort). Mikey
Hi all, Tulio Magno Quites Machado Filho <tuliom@linux.vnet.ibm.com> writes: > Forwarding some comments from Adhemerval sent to libc-alpha [1]... > > Adhemerval Zanella <adhemerval.zanella@linaro.org> writes: >>Florian Weimer <fweimer@redhat.com> writes: >> >>> On 10/12/2017 12:17 PM, Michael Ellerman wrote: >>>> + pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n"); >>>> + cur_cpu_spec->cpu_features |= CPU_FTR_TM; >>>> + cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND; >>>> + tm_suspend_disabled = true; >>> >>> This doesn't look right because if suspend is not available, you need to >>> clear the original PPC_FEATURE2_HTM flag because the semantics are not >>> right, so that applications can use fallback code. Otherwise, >>> applications may incorrectly select the HTM code and break if running on >>> a system which supports HTM, but without the suspend state. ... >> I completely agree with Florian here, this is as *ABI* change >> and the kernel need to advertise a different TM ABI instead >> of as an extension. Sorry for the slow reply, I'm travelling. You're all misreading the patch, when we enter into this hunk of code TM is disabled, so PPC_FEATURE2_HTM is *not set*. If we can configure no suspend mode, then we turn on *only* the new HTM_NO_SUSPEND bit. So yes it is an ABI change, and we advertise it as such. User space code that wants to use "no suspend mode" has to opt-in by checking for the new feature bit. I've updated the patch to make this clearer. I've also added HTM_NOSC because that should still be set (it describes kernel behaviour not hardware). cheers diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c index cf52d53da460..d23f148a11f0 100644 --- a/arch/powerpc/platforms/powernv/setup.c +++ b/arch/powerpc/platforms/powernv/setup.c @@ -304,6 +305,28 @@ static int __init pnv_probe(void) return 1; } +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM +void __init pnv_tm_init(void) +{ + if (!firmware_has_feature(FW_FEATURE_OPAL) || + !pvr_version_is(PVR_POWER9) || + early_cpu_has_feature(CPU_FTR_TM)) + return; + + if (opal_reinit_cpus(OPAL_REINIT_CPUS_TM_SUSPEND_DISABLED) != OPAL_SUCCESS) + return; + + pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n"); + cur_cpu_spec->cpu_features |= CPU_FTR_TM; + /* Make sure "normal" HTM is off (it should be) */ + cur_cpu_spec->cpu_user_features2 &= ~PPC_FEATURE2_HTM; + /* Turn on no suspend mode, and HTM no SC */ + cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND | \ + PPC_FEATURE2_HTM_NOSC; + tm_suspend_disabled = true; +} +#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */ + /* * Returns the cpu frequency for 'cpu' in Hz. This is used by * /proc/cpuinfo
Florian Weimer <fweimer@redhat.com> writes: > On 10/12/2017 12:17 PM, Michael Ellerman wrote: >> + pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n"); >> + cur_cpu_spec->cpu_features |= CPU_FTR_TM; >> + cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND; >> + tm_suspend_disabled = true; > > This doesn't look right because if suspend is not available, you need to > clear the original PPC_FEATURE2_HTM flag because the semantics are not > right, so that applications can use fallback code. Yes I agree, but the PPC_FEATURE2_HTM bit is already clear. I've updated the patch to clear it explicitly, see my other mail. > Otherwise, > applications may incorrectly select the HTM code and break if running on > a system which supports HTM, but without the suspend state. Yes. > The new flag should say that HTM is supported, but without the suspend > state, and it should be always set if PPC_FEATURE2_HTM is set. I don't agree with that. HTM and HTM_NO_SUSPEND are mutually exclusive. I don't understand how it would work if HTM_NO_SUSPEND was "always set if PPC_FEATURE2_HTM is set"? That would tell apps that HTM with suspend is supported and HTM without suspend is supported, which is meaningless AFAICS. cheers
Adhemerval Zanella <adhemerval.zanella@linaro.org> writes: > On 19/10/2017 11:34, Tulio Magno Quites Machado Filho wrote: >> Forwarding some comments from Adhemerval sent to libc-alpha [1]... >> >> Adhemerval Zanella <adhemerval.zanella@linaro.org> writes: >>> Florian Weimer <fweimer@redhat.com> writes: >>> >>>> On 10/12/2017 12:17 PM, Michael Ellerman wrote: >>>>> + pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n"); >>>>> + cur_cpu_spec->cpu_features |= CPU_FTR_TM; >>>>> + cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND; >>>>> + tm_suspend_disabled = true; >>>> >>>> This doesn't look right because if suspend is not available, you need to >>>> clear the original PPC_FEATURE2_HTM flag because the semantics are not >>>> right, so that applications can use fallback code. Otherwise, >>>> applications may incorrectly select the HTM code and break if running on >>>> a system which supports HTM, but without the suspend state. >>>> >>>> The new flag should say that HTM is supported, but without the suspend >>>> state, and it should be always set if PPC_FEATURE2_HTM is set. >>> >>> Will it also change TEXARS with the abort information? >> >> It should, with a permanent error cause so that old applications entering >> suspended state can adopt another technique. >> Michael, could you clarify if this is indeed happening, please? >> >>> I completely agree with Florian here, this is as *ABI* change >>> and the kernel need to advertise a different TM ABI instead >>> of as an extension. >> >> Adhemerval, could you elaborate which problems you're foreseeing, please? >> > > Pretty much the same Florian already stated: an application can not any > more assume for instance: > > tsr. 0 > mfcr r9,128 > andis. r10,r9,0x4000 > be cr0,L(suspend) > andis. r10,r9,0x2000 > be cr0,L(transactional) > > However thinking more about it I am not sure if this should be really a > problem: on default HTM mode the program must handle self-induced failures > as the tbegin. failure path and I assume trying to suspend/resume in this > case will trigger this. You're right, it *probably* shouldn't be a problem for most code that is written well and doesn't use suspend mode for anything except trace/debug etc. But that's not a strong enough guarantee to just make the two modes equivalent and punt the problems to userspace. Disabling suspend changes the programming model in ways that are not clearly documented, and which have not been widely tested, so it has to be controlled by a new feature bit. cheers
diff --git a/arch/powerpc/include/asm/opal-api.h b/arch/powerpc/include/asm/opal-api.h index 9d191ebea706..233c7504b1f2 100644 --- a/arch/powerpc/include/asm/opal-api.h +++ b/arch/powerpc/include/asm/opal-api.h @@ -896,6 +896,8 @@ enum { */ OPAL_REINIT_CPUS_MMU_HASH = (1 << 2), OPAL_REINIT_CPUS_MMU_RADIX = (1 << 3), + + OPAL_REINIT_CPUS_TM_SUSPEND_DISABLED = (1 << 4), }; typedef struct oppanel_line { diff --git a/arch/powerpc/include/asm/powernv.h b/arch/powerpc/include/asm/powernv.h index f62797702300..dc5f6a5d4575 100644 --- a/arch/powerpc/include/asm/powernv.h +++ b/arch/powerpc/include/asm/powernv.h @@ -22,6 +22,8 @@ extern void pnv_npu2_destroy_context(struct npu_context *context, extern int pnv_npu2_handle_fault(struct npu_context *context, uintptr_t *ea, unsigned long *flags, unsigned long *status, int count); + +void pnv_tm_init(void); #else static inline void powernv_set_nmmu_ptcr(unsigned long ptcr) { } static inline struct npu_context *pnv_npu2_init_context(struct pci_dev *gpdev, @@ -36,6 +38,8 @@ static inline int pnv_npu2_handle_fault(struct npu_context *context, unsigned long *status, int count) { return -ENODEV; } + +static inline void pnv_tm_init(void) { } #endif #endif /* _ASM_POWERNV_H */ diff --git a/arch/powerpc/include/asm/tm.h b/arch/powerpc/include/asm/tm.h index 82e06ca3a49b..ad19fe41931b 100644 --- a/arch/powerpc/include/asm/tm.h +++ b/arch/powerpc/include/asm/tm.h @@ -19,4 +19,6 @@ extern void tm_abort(uint8_t cause); extern void tm_save_sprs(struct thread_struct *thread); extern void tm_restore_sprs(struct thread_struct *thread); +extern bool tm_suspend_disabled; + #endif /* __ASSEMBLY__ */ diff --git a/arch/powerpc/kernel/process.c b/arch/powerpc/kernel/process.c index 166145b18728..b02807ea54dc 100644 --- a/arch/powerpc/kernel/process.c +++ b/arch/powerpc/kernel/process.c @@ -77,6 +77,13 @@ extern unsigned long _get_SP(void); #ifdef CONFIG_PPC_TRANSACTIONAL_MEM +/* + * Are we running in "Suspend disabled" mode? If so we have to block any + * sigreturn that would get us into suspended state, and we also warn in some + * other paths that we should never reach with suspend disabled. + */ +bool tm_suspend_disabled __ro_after_init = false; + static void check_if_tm_restore_required(struct task_struct *tsk) { /* diff --git a/arch/powerpc/kernel/prom.c b/arch/powerpc/kernel/prom.c index d9bd6555f980..101822be525a 100644 --- a/arch/powerpc/kernel/prom.c +++ b/arch/powerpc/kernel/prom.c @@ -47,6 +47,7 @@ #include <asm/mmu.h> #include <asm/paca.h> #include <asm/pgtable.h> +#include <asm/powernv.h> #include <asm/iommu.h> #include <asm/btext.h> #include <asm/sections.h> @@ -681,7 +682,10 @@ static void __init tm_init(void) cur_cpu_spec->cpu_user_features2 &= ~(PPC_FEATURE2_HTM_NOSC | PPC_FEATURE2_HTM); cur_cpu_spec->cpu_features &= ~CPU_FTR_TM; + return; } + + pnv_tm_init(); } #else static void tm_init(void) { } diff --git a/arch/powerpc/platforms/powernv/setup.c b/arch/powerpc/platforms/powernv/setup.c index cf52d53da460..8096c3352e2b 100644 --- a/arch/powerpc/platforms/powernv/setup.c +++ b/arch/powerpc/platforms/powernv/setup.c @@ -36,6 +36,7 @@ #include <asm/opal.h> #include <asm/kexec.h> #include <asm/smp.h> +#include <asm/tm.h> #include "powernv.h" @@ -304,6 +305,24 @@ static int __init pnv_probe(void) return 1; } +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM +void __init pnv_tm_init(void) +{ + if (!firmware_has_feature(FW_FEATURE_OPAL) || + !pvr_version_is(PVR_POWER9) || + early_cpu_has_feature(CPU_FTR_TM)) + return; + + if (opal_reinit_cpus(OPAL_REINIT_CPUS_TM_SUSPEND_DISABLED) != OPAL_SUCCESS) + return; + + pr_info("Enabling TM (Transactional Memory) with Suspend Disabled\n"); + cur_cpu_spec->cpu_features |= CPU_FTR_TM; + cur_cpu_spec->cpu_user_features2 |= PPC_FEATURE2_HTM_NO_SUSPEND; + tm_suspend_disabled = true; +} +#endif /* CONFIG_PPC_TRANSACTIONAL_MEM */ + /* * Returns the cpu frequency for 'cpu' in Hz. This is used by * /proc/cpuinfo
Some Power9 revisions can run in a mode where TM operates without suspended state. If we find ourself on a CPU that might be in this mode, we query OPAL to check, and if so we reenable TM in CPU features, and enable a new user feature to signal to userspace that we are in this mode. Signed-off-by: Michael Ellerman <mpe@ellerman.id.au> --- arch/powerpc/include/asm/opal-api.h | 2 ++ arch/powerpc/include/asm/powernv.h | 4 ++++ arch/powerpc/include/asm/tm.h | 2 ++ arch/powerpc/kernel/process.c | 7 +++++++ arch/powerpc/kernel/prom.c | 4 ++++ arch/powerpc/platforms/powernv/setup.c | 19 +++++++++++++++++++ 6 files changed, 38 insertions(+)