[3/4] powerpc/powernv: Enable TM without suspend if possible

Message ID 1507803439-12862-3-git-send-email-mpe@ellerman.id.au
State Accepted
Commit 54820530c5faa9fd78e1c08cb6449100b1a19157
Headers show
Series
  • [1/4] powerpc/tm: Add commandline option to disable hardware transactional memory
Related show

Commit Message

Michael Ellerman Oct. 12, 2017, 10:17 a.m.
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(+)

Comments

Florian Weimer Oct. 19, 2017, 10:07 a.m. | #1
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
Tulio Magno Quites Machado Filho Oct. 19, 2017, 12:04 p.m. | #2
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.
Florian Weimer Oct. 19, 2017, 12:45 p.m. | #3
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
Tulio Magno Quites Machado Filho Oct. 19, 2017, 1:34 p.m. | #4
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?
Adhemerval Zanella Oct. 19, 2017, 3:13 p.m. | #5
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.
Michael Neuling Oct. 20, 2017, 2:47 a.m. | #6
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
Michael Ellerman Oct. 22, 2017, 9:48 a.m. | #7
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
Michael Ellerman Oct. 22, 2017, 9:54 a.m. | #8
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
Michael Ellerman Oct. 22, 2017, 9:59 a.m. | #9
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

Patch

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