[PATCH 0/3] powerpc/tm: Abort syscalls in active transactions

[PATCH 0/3] powerpc/tm: Abort syscalls in active transactions

[Sam Bobroff]

See the first patch for a description of the reasoning behind this
change.

This set includes the change, a kernel selftest for it and
some slight refactoring of the selftest code.


Sam Bobroff (3):
  powerpc/tm: Abort syscalls in active transactions
  selftests/powerpc: Move get_auxv_entry() to harness.c
  selftests/powerpc: Add transactional syscall test

 Documentation/powerpc/transactional_memory.txt  |   33 +++----
 arch/powerpc/include/uapi/asm/tm.h              |    2 +-
 arch/powerpc/kernel/entry_64.S                  |   19 ++++
 tools/testing/selftests/powerpc/harness.c       |   47 ++++++++++
 tools/testing/selftests/powerpc/pmu/lib.c       |   47 ----------
 tools/testing/selftests/powerpc/pmu/lib.h       |    1 -
 tools/testing/selftests/powerpc/tm/Makefile     |    3 +-
 tools/testing/selftests/powerpc/tm/tm-syscall.c |  113 +++++++++++++++++++++++
 tools/testing/selftests/powerpc/utils.h         |    2 +-
 9 files changed, 200 insertions(+), 67 deletions(-)
 create mode 100644 tools/testing/selftests/powerpc/tm/tm-syscall.c

-- 
1.7.10.4

[PATCH 1/3] powerpc/tm: Abort syscalls in active transactions

[Sam Bobroff]

This patch changes the syscall handler to doom (tabort) active
transactions when a syscall is made and return immediately without
performing the syscall.

Currently, the system call instruction automatically suspends an
active transaction which causes side effects to persist when an active
transaction fails.

This does change the kernel's behaviour, but in a way that was
documented as unsupported. It doesn't reduce functionality because
syscalls will still be performed after tsuspend. It also provides a
consistent interface and makes the behaviour of user code
substantially the same across powerpc and platforms that do not
support suspended transactions (e.g. x86 and s390).

Performance measurements using
http://ozlabs.org/~anton/junkcode/null_syscall.c
indicate the cost of a system call increases by about 0.5%.

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
 Documentation/powerpc/transactional_memory.txt |   33 ++++++++++++------------
 arch/powerpc/include/uapi/asm/tm.h             |    2 +-
 arch/powerpc/kernel/entry_64.S                 |   19 ++++++++++++++
 3 files changed, 37 insertions(+), 17 deletions(-)

diff --git a/Documentation/powerpc/transactional_memory.txt b/Documentation/powerpc/transactional_memory.txt
index 9791e98..4167bc2 100644
--- a/Documentation/powerpc/transactional_memory.txt
+++ b/Documentation/powerpc/transactional_memory.txt
@@ -74,22 +74,23 @@ Causes of transaction aborts
 Syscalls
 ========
 
-Performing syscalls from within transaction is not recommended, and can lead
-to unpredictable results.
-
-Syscalls do not by design abort transactions, but beware: The kernel code will
-not be running in transactional state.  The effect of syscalls will always
-remain visible, but depending on the call they may abort your transaction as a
-side-effect, read soon-to-be-aborted transactional data that should not remain
-invisible, etc.  If you constantly retry a transaction that constantly aborts
-itself by calling a syscall, you'll have a livelock & make no progress.
-
-Simple syscalls (e.g. sigprocmask()) "could" be OK.  Even things like write()
-from, say, printf() should be OK as long as the kernel does not access any
-memory that was accessed transactionally.
-
-Consider any syscalls that happen to work as debug-only -- not recommended for
-production use.  Best to queue them up till after the transaction is over.
+Syscalls made from within an active transaction will not be performed and the
+transaction will be doomed by the kernel with the failure code TM_CAUSE_SYSCALL
+| TM_CAUSE_PERSISTENT.
+
+Syscalls made from within a suspended transaction are performed as normal and
+the transaction is not explicitly doomed by the kernel.  However, what the
+kernel does to perform the syscall may result in the transaction being doomed
+by the hardware.  The syscall is performed in suspended mode so any side
+effects will be persistent, independent of transaction success or failure.  No
+guarantees are provided by the kernel about which syscalls will affect
+transaction success.
+
+Care must be taken when relying on syscalls to abort during active transactions
+if the calls are made via a library.  Libraries may cache values (which may
+give the appearence of success) or perform operations that cause transaction
+failure before entering the kernel (which may produce different failure codes).
+Examples are glibc's getpid() and lazy symbol resolution.
 
 
 Signals
diff --git a/arch/powerpc/include/uapi/asm/tm.h b/arch/powerpc/include/uapi/asm/tm.h
index 5d836b7..5047659 100644
--- a/arch/powerpc/include/uapi/asm/tm.h
+++ b/arch/powerpc/include/uapi/asm/tm.h
@@ -11,7 +11,7 @@
 #define TM_CAUSE_RESCHED	0xde
 #define TM_CAUSE_TLBI		0xdc
 #define TM_CAUSE_FAC_UNAV	0xda
-#define TM_CAUSE_SYSCALL	0xd8  /* future use */
+#define TM_CAUSE_SYSCALL	0xd8
 #define TM_CAUSE_MISC		0xd6  /* future use */
 #define TM_CAUSE_SIGNAL		0xd4
 #define TM_CAUSE_ALIGNMENT	0xd2
diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_64.S
index d180caf2..85bf81d 100644
--- a/arch/powerpc/kernel/entry_64.S
+++ b/arch/powerpc/kernel/entry_64.S
@@ -34,6 +34,7 @@
 #include <asm/ftrace.h>
 #include <asm/hw_irq.h>
 #include <asm/context_tracking.h>
+#include <asm/tm.h>
 
 /*
  * System calls.
@@ -145,6 +146,24 @@ END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
 	andi.	r11,r10,_TIF_SYSCALL_DOTRACE
 	bne	syscall_dotrace
 .Lsyscall_dotrace_cont:
+#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
+BEGIN_FTR_SECTION
+	b	1f
+END_FTR_SECTION_IFCLR(CPU_FTR_TM)
+	extrdi.	r11, r12, 1, (63-MSR_TS_T_LG) /* transaction active? */
+	beq+	1f
+
+	/* Doom the transaction and don't perform the syscall: */
+	mfmsr	r11
+	li	r12, 1
+	rldimi	r11, r12, MSR_TM_LG, 63-MSR_TM_LG
+	mtmsrd	r11, 0
+	li	r11, (TM_CAUSE_SYSCALL|TM_CAUSE_PERSISTENT)
+	tabort. r11
+
+	b	.Lsyscall_exit
+1:
+#endif
 	cmpldi	0,r0,NR_syscalls
 	bge-	syscall_enosys
 
-- 
1.7.10.4

[PATCH 2/3] selftests/powerpc: Move get_auxv_entry() to harness.c

[Sam Bobroff]

Move get_auxv_entry() from pmu/lib.c up to harness.c in order to make
it available to other tests.

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
 tools/testing/selftests/powerpc/harness.c |   47 +++++++++++++++++++++++++++++
 tools/testing/selftests/powerpc/pmu/lib.c |   47 -----------------------------
 tools/testing/selftests/powerpc/pmu/lib.h |    1 -
 tools/testing/selftests/powerpc/utils.h   |    2 +-
 4 files changed, 48 insertions(+), 49 deletions(-)

diff --git a/tools/testing/selftests/powerpc/harness.c b/tools/testing/selftests/powerpc/harness.c
index 8ebc58a..f7997af 100644
--- a/tools/testing/selftests/powerpc/harness.c
+++ b/tools/testing/selftests/powerpc/harness.c
@@ -11,6 +11,10 @@
 #include <sys/types.h>
 #include <sys/wait.h>
 #include <unistd.h>
+#include <elf.h>
+#include <fcntl.h>
+#include <link.h>
+#include <sys/stat.h>
 
 #include "subunit.h"
 #include "utils.h"
@@ -112,3 +116,46 @@ int test_harness(int (test_function)(void), char *name)
 
 	return rc;
 }
+
+static char auxv[4096];
+
+void *get_auxv_entry(int type)
+{
+	ElfW(auxv_t) *p;
+	void *result;
+	ssize_t num;
+	int fd;
+
+	fd = open("/proc/self/auxv", O_RDONLY);
+	if (fd == -1) {
+		perror("open");
+		return NULL;
+	}
+
+	result = NULL;
+
+	num = read(fd, auxv, sizeof(auxv));
+	if (num < 0) {
+		perror("read");
+		goto out;
+	}
+
+	if (num > sizeof(auxv)) {
+		printf("Overflowed auxv buffer\n");
+		goto out;
+	}
+
+	p = (ElfW(auxv_t) *)auxv;
+
+	while (p->a_type != AT_NULL) {
+		if (p->a_type == type) {
+			result = (void *)p->a_un.a_val;
+			break;
+		}
+
+		p++;
+	}
+out:
+	close(fd);
+	return result;
+}
diff --git a/tools/testing/selftests/powerpc/pmu/lib.c b/tools/testing/selftests/powerpc/pmu/lib.c
index 9768dea..a07104c 100644
--- a/tools/testing/selftests/powerpc/pmu/lib.c
+++ b/tools/testing/selftests/powerpc/pmu/lib.c
@@ -5,15 +5,10 @@
 
 #define _GNU_SOURCE	/* For CPU_ZERO etc. */
 
-#include <elf.h>
 #include <errno.h>
-#include <fcntl.h>
-#include <link.h>
 #include <sched.h>
 #include <setjmp.h>
 #include <stdlib.h>
-#include <sys/stat.h>
-#include <sys/types.h>
 #include <sys/wait.h>
 
 #include "utils.h"
@@ -256,45 +251,3 @@ out:
 	return rc;
 }
 
-static char auxv[4096];
-
-void *get_auxv_entry(int type)
-{
-	ElfW(auxv_t) *p;
-	void *result;
-	ssize_t num;
-	int fd;
-
-	fd = open("/proc/self/auxv", O_RDONLY);
-	if (fd == -1) {
-		perror("open");
-		return NULL;
-	}
-
-	result = NULL;
-
-	num = read(fd, auxv, sizeof(auxv));
-	if (num < 0) {
-		perror("read");
-		goto out;
-	}
-
-	if (num > sizeof(auxv)) {
-		printf("Overflowed auxv buffer\n");
-		goto out;
-	}
-
-	p = (ElfW(auxv_t) *)auxv;
-
-	while (p->a_type != AT_NULL) {
-		if (p->a_type == type) {
-			result = (void *)p->a_un.a_val;
-			break;
-		}
-
-		p++;
-	}
-out:
-	close(fd);
-	return result;
-}
diff --git a/tools/testing/selftests/powerpc/pmu/lib.h b/tools/testing/selftests/powerpc/pmu/lib.h
index 0f0339c..ca5d72a 100644
--- a/tools/testing/selftests/powerpc/pmu/lib.h
+++ b/tools/testing/selftests/powerpc/pmu/lib.h
@@ -29,7 +29,6 @@ extern int notify_parent(union pipe write_pipe);
 extern int notify_parent_of_error(union pipe write_pipe);
 extern pid_t eat_cpu(int (test_function)(void));
 extern bool require_paranoia_below(int level);
-extern void *get_auxv_entry(int type);
 
 struct addr_range {
 	uint64_t first, last;
diff --git a/tools/testing/selftests/powerpc/utils.h b/tools/testing/selftests/powerpc/utils.h
index a93777a..64f53cd 100644
--- a/tools/testing/selftests/powerpc/utils.h
+++ b/tools/testing/selftests/powerpc/utils.h
@@ -19,7 +19,7 @@ typedef uint8_t u8;
 
 
 int test_harness(int (test_function)(void), char *name);
-
+extern void *get_auxv_entry(int type);
 
 /* Yes, this is evil */
 #define FAIL_IF(x)						\
-- 
1.7.10.4

[PATCH 3/3] selftests/powerpc: Add transactional syscall test

[Sam Bobroff]

Check that a syscall made during an active transaction will fail with
the correct failure code and that one made during a suspended
transaction will succeed.

Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
 tools/testing/selftests/powerpc/tm/Makefile     |    3 +-
 tools/testing/selftests/powerpc/tm/tm-syscall.c |  113 +++++++++++++++++++++++
 2 files changed, 115 insertions(+), 1 deletion(-)
 create mode 100644 tools/testing/selftests/powerpc/tm/tm-syscall.c

diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile
index 2cede23..d8dab0d 100644
--- a/tools/testing/selftests/powerpc/tm/Makefile
+++ b/tools/testing/selftests/powerpc/tm/Makefile
@@ -1,4 +1,5 @@
-PROGS := tm-resched-dscr
+PROGS := tm-resched-dscr tm-syscall
+CFLAGS:=$(CFLAGS) -mhtm -Wl,-z,now
 
 all: $(PROGS)
 
diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall.c b/tools/testing/selftests/powerpc/tm/tm-syscall.c
new file mode 100644
index 0000000..7c60e53
--- /dev/null
+++ b/tools/testing/selftests/powerpc/tm/tm-syscall.c
@@ -0,0 +1,113 @@
+/* Test the kernel's system call code to ensure that a system call
+ * made from within an active HTM transaction is aborted with the
+ * correct failure code.
+ * Conversely, ensure that a system call made from within a
+ * suspended transaction can succeed.
+ *
+ * It is important to compile with -Wl,-z,now to prevent
+ * lazy symbol resolution from affecting the results.
+ */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <asm/tm.h>
+#include <asm/cputable.h>
+#include <linux/auxvec.h>
+
+#include "utils.h"
+
+#define TM_RETRIES 10
+#define TM_TEST_RUNS 1000
+
+int t_failure_persistent(void)
+{
+	long texasr = __builtin_get_texasr();
+	long failure_code = (texasr >> 56) & 0xff;
+
+	return failure_code & TM_CAUSE_PERSISTENT;
+}
+
+int t_failure_code_syscall(void)
+{
+	long texasr = __builtin_get_texasr();
+	long failure_code = (texasr >> 56) & 0xff;
+
+	return (failure_code & TM_CAUSE_SYSCALL) == TM_CAUSE_SYSCALL;
+}
+
+int t_active_getppid(void)
+{
+	int i;
+
+	for (i = 0; i < TM_RETRIES; i++) {
+		if (__builtin_tbegin(0)) {
+			getppid();
+			__builtin_tend(0);
+			return 1;
+		}
+		if (t_failure_persistent())
+			return 0;
+	}
+	return 0;
+}
+
+int t_active_getppid_test(void)
+{
+	int i;
+
+	for (i = 0; i < TM_TEST_RUNS; i++) {
+		if (t_active_getppid())
+			return 0;
+		if (!t_failure_persistent())
+			return 0;
+		if (!t_failure_code_syscall())
+			return 0;
+	}
+	return 1;
+}
+
+int t_suspended_getppid(void)
+{
+	int i;
+
+	for (i = 0; i < TM_RETRIES; i++) {
+		if (__builtin_tbegin(0)) {
+			__builtin_tsuspend();
+			getppid();
+			__builtin_tresume();
+			__builtin_tend(0);
+			return 1;
+		}
+		if (t_failure_persistent())
+			return 0;
+	}
+	return 0;
+}
+
+int t_suspended_getppid_test(void)
+{
+	int i;
+
+	for (i = 0; i < TM_TEST_RUNS; i++) {
+		if (!t_suspended_getppid())
+			return 0;
+	}
+	return 1;
+}
+
+int tm_syscall(void)
+{
+	SKIP_IF(!((long)get_auxv_entry(AT_HWCAP2) & PPC_FEATURE2_HTM));
+	setbuf(stdout, 0);
+	FAIL_IF(!t_active_getppid_test());
+	printf("%d active transactions correctly aborted.\n", TM_TEST_RUNS);
+	FAIL_IF(!t_suspended_getppid_test());
+	printf("%d suspended transactions succeeded.\n", TM_TEST_RUNS);
+	return 0;
+}
+
+int main(void)
+{
+	return test_harness(tm_syscall, "tm_syscall");
+}
+
-- 
1.7.10.4

Re: [PATCH 1/3] powerpc/tm: Abort syscalls in active transactions

[Michael Neuling]

On Thu, 2015-03-19 at 15:43 +1100, Sam Bobroff wrote:
> This patch changes the syscall handler to doom (tabort) active
> transactions when a syscall is made and return immediately without
> performing the syscall.
>=20
> Currently, the system call instruction automatically suspends an
> active transaction which causes side effects to persist when an active
> transaction fails.
>=20
> This does change the kernel's behaviour, but in a way that was
> documented as unsupported. It doesn't reduce functionality because
> syscalls will still be performed after tsuspend. It also provides a
> consistent interface and makes the behaviour of user code
> substantially the same across powerpc and platforms that do not
> support suspended transactions (e.g. x86 and s390).
>=20
> Performance measurements using
> http://ozlabs.org/~anton/junkcode/null_syscall.c
> indicate the cost of a system call increases by about 0.5%.
>=20
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

Thanks Sam!

Acked-By: Michael Neuling <mikey@neuling.org>

> ---
>  Documentation/powerpc/transactional_memory.txt |   33 ++++++++++++------=
------
>  arch/powerpc/include/uapi/asm/tm.h             |    2 +-
>  arch/powerpc/kernel/entry_64.S                 |   19 ++++++++++++++
>  3 files changed, 37 insertions(+), 17 deletions(-)
>=20
> diff --git a/Documentation/powerpc/transactional_memory.txt b/Documentati=
on/powerpc/transactional_memory.txt
> index 9791e98..4167bc2 100644
> --- a/Documentation/powerpc/transactional_memory.txt
> +++ b/Documentation/powerpc/transactional_memory.txt
> @@ -74,22 +74,23 @@ Causes of transaction aborts
>  Syscalls
>  =3D=3D=3D=3D=3D=3D=3D=3D
> =20
> -Performing syscalls from within transaction is not recommended, and can =
lead
> -to unpredictable results.
> -
> -Syscalls do not by design abort transactions, but beware: The kernel cod=
e will
> -not be running in transactional state.  The effect of syscalls will alwa=
ys
> -remain visible, but depending on the call they may abort your transactio=
n as a
> -side-effect, read soon-to-be-aborted transactional data that should not =
remain
> -invisible, etc.  If you constantly retry a transaction that constantly a=
borts
> -itself by calling a syscall, you'll have a livelock & make no progress.
> -
> -Simple syscalls (e.g. sigprocmask()) "could" be OK.  Even things like wr=
ite()
> -from, say, printf() should be OK as long as the kernel does not access a=
ny
> -memory that was accessed transactionally.
> -
> -Consider any syscalls that happen to work as debug-only -- not recommend=
ed for
> -production use.  Best to queue them up till after the transaction is ove=
r.
> +Syscalls made from within an active transaction will not be performed an=
d the
> +transaction will be doomed by the kernel with the failure code TM_CAUSE_=
SYSCALL
> +| TM_CAUSE_PERSISTENT.
> +
> +Syscalls made from within a suspended transaction are performed as norma=
l and
> +the transaction is not explicitly doomed by the kernel.  However, what t=
he
> +kernel does to perform the syscall may result in the transaction being d=
oomed
> +by the hardware.  The syscall is performed in suspended mode so any side
> +effects will be persistent, independent of transaction success or failur=
e.  No
> +guarantees are provided by the kernel about which syscalls will affect
> +transaction success.
> +
> +Care must be taken when relying on syscalls to abort during active trans=
actions
> +if the calls are made via a library.  Libraries may cache values (which =
may
> +give the appearence of success) or perform operations that cause transac=
tion
> +failure before entering the kernel (which may produce different failure =
codes).
> +Examples are glibc's getpid() and lazy symbol resolution.
> =20
>=20
>  Signals
> diff --git a/arch/powerpc/include/uapi/asm/tm.h b/arch/powerpc/include/ua=
pi/asm/tm.h
> index 5d836b7..5047659 100644
> --- a/arch/powerpc/include/uapi/asm/tm.h
> +++ b/arch/powerpc/include/uapi/asm/tm.h
> @@ -11,7 +11,7 @@
>  #define TM_CAUSE_RESCHED	0xde
>  #define TM_CAUSE_TLBI		0xdc
>  #define TM_CAUSE_FAC_UNAV	0xda
> -#define TM_CAUSE_SYSCALL	0xd8  /* future use */
> +#define TM_CAUSE_SYSCALL	0xd8
>  #define TM_CAUSE_MISC		0xd6  /* future use */
>  #define TM_CAUSE_SIGNAL		0xd4
>  #define TM_CAUSE_ALIGNMENT	0xd2
> diff --git a/arch/powerpc/kernel/entry_64.S b/arch/powerpc/kernel/entry_6=
4.S
> index d180caf2..85bf81d 100644
> --- a/arch/powerpc/kernel/entry_64.S
> +++ b/arch/powerpc/kernel/entry_64.S
> @@ -34,6 +34,7 @@
>  #include <asm/ftrace.h>
>  #include <asm/hw_irq.h>
>  #include <asm/context_tracking.h>
> +#include <asm/tm.h>
> =20
>  /*
>   * System calls.
> @@ -145,6 +146,24 @@ END_FW_FTR_SECTION_IFSET(FW_FEATURE_SPLPAR)
>  	andi.	r11,r10,_TIF_SYSCALL_DOTRACE
>  	bne	syscall_dotrace
>  .Lsyscall_dotrace_cont:
> +#ifdef CONFIG_PPC_TRANSACTIONAL_MEM
> +BEGIN_FTR_SECTION
> +	b	1f
> +END_FTR_SECTION_IFCLR(CPU_FTR_TM)
> +	extrdi.	r11, r12, 1, (63-MSR_TS_T_LG) /* transaction active? */
> +	beq+	1f
> +
> +	/* Doom the transaction and don't perform the syscall: */
> +	mfmsr	r11
> +	li	r12, 1
> +	rldimi	r11, r12, MSR_TM_LG, 63-MSR_TM_LG
> +	mtmsrd	r11, 0
> +	li	r11, (TM_CAUSE_SYSCALL|TM_CAUSE_PERSISTENT)
> +	tabort. r11
> +
> +	b	.Lsyscall_exit
> +1:
> +#endif
>  	cmpldi	0,r0,NR_syscalls
>  	bge-	syscall_enosys
> =20

Re: [PATCH 1/3] powerpc/tm: Abort syscalls in active transactions

[Anshuman Khandual]

On 03/19/2015 10:13 AM, Sam Bobroff wrote:
> This patch changes the syscall handler to doom (tabort) active
> transactions when a syscall is made and return immediately without
> performing the syscall.
> 
> Currently, the system call instruction automatically suspends an
> active transaction which causes side effects to persist when an active
> transaction fails.
> 
> This does change the kernel's behaviour, but in a way that was
> documented as unsupported. It doesn't reduce functionality because
> syscalls will still be performed after tsuspend. It also provides a
> consistent interface and makes the behaviour of user code
> substantially the same across powerpc and platforms that do not
> support suspended transactions (e.g. x86 and s390).
> 
> Performance measurements using
> http://ozlabs.org/~anton/junkcode/null_syscall.c
> indicate the cost of a system call increases by about 0.5%.

Performance test results verified which shows an improvement of
around ~0.52% with the patch compared to without it.

[With patch]		null_syscall:     757.59 cycles     100.00%
[Without patch]]	null_syscall:     753.66 cycles     100.00%

> 
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

Tested-by: Anshuman Khandual <khandual@linux.vnet.ibm.com>

Re: [PATCH 3/3] selftests/powerpc: Add transactional syscall test

[Anshuman Khandual]

On 03/19/2015 10:13 AM, Sam Bobroff wrote:
> Check that a syscall made during an active transaction will fail with
> the correct failure code and that one made during a suspended
> transaction will succeed.
> 
> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>

The test works.

> +
> +int tm_syscall(void)
> +{
> +	SKIP_IF(!((long)get_auxv_entry(AT_HWCAP2) & PPC_FEATURE2_HTM));
> +	setbuf(stdout, 0);
> +	FAIL_IF(!t_active_getppid_test());
> +	printf("%d active transactions correctly aborted.\n", TM_TEST_RUNS);
> +	FAIL_IF(!t_suspended_getppid_test());
> +	printf("%d suspended transactions succeeded.\n", TM_TEST_RUNS);
> +	return 0;
> +}
> +
> +int main(void)
> +{
> +	return test_harness(tm_syscall, "tm_syscall");
> +}
> +

There is an extra blank line at the end of this file. Interchanging return
codes of 0 and 1 for various functions make it very confusing along with
negative FAIL_IF checks in the primary test function. Control flow structures
like these can use some in-code documentation for readability.

+	for (i = 0; i < TM_RETRIES; i++) {
+		if (__builtin_tbegin(0)) {
+			getppid();
+			__builtin_tend(0);
+			return 1;
+		}
+		if (t_failure_persistent())
+			return 0;

or

+		if (__builtin_tbegin(0)) {
+			__builtin_tsuspend();
+			getppid();
+			__builtin_tresume();
+			__builtin_tend(0);
+			return 1;
+		}
+		if (t_failure_persistent())
+			return 0;

Re: [PATCH 3/3] selftests/powerpc: Add transactional syscall test

[Sam Bobroff]

On 20/03/15 20:25, Anshuman Khandual wrote:
> On 03/19/2015 10:13 AM, Sam Bobroff wrote:
>> Check that a syscall made during an active transaction will fail with
>> the correct failure code and that one made during a suspended
>> transaction will succeed.
>>
>> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> 
> The test works.

Great :-)

>> +
>> +int tm_syscall(void)
>> +{
>> +	SKIP_IF(!((long)get_auxv_entry(AT_HWCAP2) & PPC_FEATURE2_HTM));
>> +	setbuf(stdout, 0);
>> +	FAIL_IF(!t_active_getppid_test());
>> +	printf("%d active transactions correctly aborted.\n", TM_TEST_RUNS);
>> +	FAIL_IF(!t_suspended_getppid_test());
>> +	printf("%d suspended transactions succeeded.\n", TM_TEST_RUNS);
>> +	return 0;
>> +}
>> +
>> +int main(void)
>> +{
>> +	return test_harness(tm_syscall, "tm_syscall");
>> +}
>> +
> 
> There is an extra blank line at the end of this file. Interchanging return
> codes of 0 and 1 for various functions make it very confusing along with
> negative FAIL_IF checks in the primary test function. Control flow structures
> like these can use some in-code documentation for readability.
> 
> +	for (i = 0; i < TM_RETRIES; i++) {
> +		if (__builtin_tbegin(0)) {
> +			getppid();
> +			__builtin_tend(0);
> +			return 1;
> +		}
> +		if (t_failure_persistent())
> +			return 0;
> 
> or
> 
> +		if (__builtin_tbegin(0)) {
> +			__builtin_tsuspend();
> +			getppid();
> +			__builtin_tresume();
> +			__builtin_tend(0);
> +			return 1;
> +		}
> +		if (t_failure_persistent())
> +			return 0;
> 

Good points. I'll remove the blank line and comment the code.

I'm not sure I can do any better with the FAIL_IF() macro: I wanted it
to read "fail if the test failed", but I can see what you mean about a
double negative. Maybe it would be better to introduce a different
macro, more like a standard assert: TEST(XXX) which fails if XXX is
false. However, I think "TEST" would be too generic a name and I'm not
should what would be better. Any comments/suggestions?

Thanks for the review!

Cheers,
Sam.

Re: [PATCH 3/3] selftests/powerpc: Add transactional syscall test

[Michael Ellerman]

On Tue, 2015-03-24 at 12:52 +1100, Sam Bobroff wrote:
> On 20/03/15 20:25, Anshuman Khandual wrote:
> > On 03/19/2015 10:13 AM, Sam Bobroff wrote:
> >> Check that a syscall made during an active transaction will fail with
> >> the correct failure code and that one made during a suspended
> >> transaction will succeed.
> >>
> >> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
> > 
> > The test works.
> 
> Great :-)
> 
> >> +
> >> +int tm_syscall(void)
> >> +{
> >> +	SKIP_IF(!((long)get_auxv_entry(AT_HWCAP2) & PPC_FEATURE2_HTM));
> >> +	setbuf(stdout, 0);
> >> +	FAIL_IF(!t_active_getppid_test());
> >> +	printf("%d active transactions correctly aborted.\n", TM_TEST_RUNS);
> >> +	FAIL_IF(!t_suspended_getppid_test());
> >> +	printf("%d suspended transactions succeeded.\n", TM_TEST_RUNS);
> >> +	return 0;
> >> +}
> >> +
> >> +int main(void)
> >> +{
> >> +	return test_harness(tm_syscall, "tm_syscall");
> >> +}
> >> +
> > 
> > There is an extra blank line at the end of this file. Interchanging return
> > codes of 0 and 1 for various functions make it very confusing along with
> > negative FAIL_IF checks in the primary test function. Control flow structures
> > like these can use some in-code documentation for readability.
> > 
> > +	for (i = 0; i < TM_RETRIES; i++) {
> > +		if (__builtin_tbegin(0)) {
> > +			getppid();
> > +			__builtin_tend(0);
> > +			return 1;
> > +		}
> > +		if (t_failure_persistent())
> > +			return 0;
> > 
> > or
> > 
> > +		if (__builtin_tbegin(0)) {
> > +			__builtin_tsuspend();
> > +			getppid();
> > +			__builtin_tresume();
> > +			__builtin_tend(0);
> > +			return 1;
> > +		}
> > +		if (t_failure_persistent())
> > +			return 0;
> > 
> 
> Good points. I'll remove the blank line and comment the code.
> 
> I'm not sure I can do any better with the FAIL_IF() macro: I wanted it
> to read "fail if the test failed", but I can see what you mean about a
> double negative. Maybe it would be better to introduce a different
> macro, more like a standard assert: TEST(XXX) which fails if XXX is
> false. However, I think "TEST" would be too generic a name and I'm not
> should what would be better. Any comments/suggestions?

FAIL_IF() is designed for things that return 0 for OK and !0 for failure. Like
most things in C.

So I think it would be improved if you inverted your return codes in your test
routines.

Even better to return ESOMETHING in the error cases, and zero otherwise.

cheers

Re: [PATCH 1/3] powerpc/tm: Abort syscalls in active transactions

[Michael Ellerman]

On Fri, 2015-03-20 at 14:34 +0530, Anshuman Khandual wrote:
> On 03/19/2015 10:13 AM, Sam Bobroff wrote:
> > This patch changes the syscall handler to doom (tabort) active
> > transactions when a syscall is made and return immediately without
> > performing the syscall.
> > 
> > Currently, the system call instruction automatically suspends an
> > active transaction which causes side effects to persist when an active
> > transaction fails.
> > 
> > This does change the kernel's behaviour, but in a way that was
> > documented as unsupported. It doesn't reduce functionality because
> > syscalls will still be performed after tsuspend. It also provides a
> > consistent interface and makes the behaviour of user code
> > substantially the same across powerpc and platforms that do not
> > support suspended transactions (e.g. x86 and s390).
> > 
> > Performance measurements using
> > http://ozlabs.org/~anton/junkcode/null_syscall.c
> > indicate the cost of a system call increases by about 0.5%.
> 
> Performance test results verified which shows an improvement of
> around ~0.52% with the patch compared to without it.
> 
> [With patch]		null_syscall:     757.59 cycles     100.00%
> [Without patch]]	null_syscall:     753.66 cycles     100.00%

No that's a performance *decrease* with the patch applied. ?

cheers

Re: [PATCH 1/3] powerpc/tm: Abort syscalls in active transactions

[Anshuman Khandual]

On 03/24/2015 07:34 AM, Michael Ellerman wrote:
> On Fri, 2015-03-20 at 14:34 +0530, Anshuman Khandual wrote:
>> On 03/19/2015 10:13 AM, Sam Bobroff wrote:
>>> This patch changes the syscall handler to doom (tabort) active
>>> transactions when a syscall is made and return immediately without
>>> performing the syscall.
>>>
>>> Currently, the system call instruction automatically suspends an
>>> active transaction which causes side effects to persist when an active
>>> transaction fails.
>>>
>>> This does change the kernel's behaviour, but in a way that was
>>> documented as unsupported. It doesn't reduce functionality because
>>> syscalls will still be performed after tsuspend. It also provides a
>>> consistent interface and makes the behaviour of user code
>>> substantially the same across powerpc and platforms that do not
>>> support suspended transactions (e.g. x86 and s390).
>>>
>>> Performance measurements using
>>> http://ozlabs.org/~anton/junkcode/null_syscall.c
>>> indicate the cost of a system call increases by about 0.5%.
>>
>> Performance test results verified which shows an improvement of
>> around ~0.52% with the patch compared to without it.
>>
>> [With patch]		null_syscall:     757.59 cycles     100.00%
>> [Without patch]]	null_syscall:     753.66 cycles     100.00%
> 
> No that's a performance *decrease* with the patch applied. ?

Micheal, thats true. The cycle counts are more with patch being applied.
Here are some more results on mainline.

Without the patch:

        null_syscall:     754.14 cycles     100.00%
        null_syscall:     754.50 cycles     100.00%
        null_syscall:     754.17 cycles     100.00%
        null_syscall:     754.43 cycles     100.00%
        null_syscall:     754.17 cycles     100.00%
        null_syscall:     754.20 cycles     100.00%
        null_syscall:     754.47 cycles     100.00%
        null_syscall:     754.70 cycles     100.00%
        null_syscall:     754.43 cycles     100.00%
        null_syscall:     753.72 cycles     100.00%

With the patch:

        null_syscall:     756.76 cycles     100.00%
        null_syscall:     756.40 cycles     100.00%
        null_syscall:     756.46 cycles     100.00%
        null_syscall:     756.86 cycles     100.00%
        null_syscall:     756.56 cycles     100.00%
        null_syscall:     756.66 cycles     100.00%
        null_syscall:     756.86 cycles     100.00%
        null_syscall:     756.86 cycles     100.00%
        null_syscall:     757.02 cycles     100.00%
        null_syscall:     756.93 cycles     100.00%

Re: [PATCH 1/3] powerpc/tm: Abort syscalls in active transactions

[Michael Ellerman]

On Tue, 2015-03-24 at 09:56 +0530, Anshuman Khandual wrote:
> On 03/24/2015 07:34 AM, Michael Ellerman wrote:
> > On Fri, 2015-03-20 at 14:34 +0530, Anshuman Khandual wrote:
> >> On 03/19/2015 10:13 AM, Sam Bobroff wrote:
> >>> This patch changes the syscall handler to doom (tabort) active
> >>> transactions when a syscall is made and return immediately without
> >>> performing the syscall.
> >>>
> >>> Currently, the system call instruction automatically suspends an
> >>> active transaction which causes side effects to persist when an active
> >>> transaction fails.
> >>>
> >>> This does change the kernel's behaviour, but in a way that was
> >>> documented as unsupported. It doesn't reduce functionality because
> >>> syscalls will still be performed after tsuspend. It also provides a
> >>> consistent interface and makes the behaviour of user code
> >>> substantially the same across powerpc and platforms that do not
> >>> support suspended transactions (e.g. x86 and s390).
> >>>
> >>> Performance measurements using
> >>> http://ozlabs.org/~anton/junkcode/null_syscall.c
> >>> indicate the cost of a system call increases by about 0.5%.
> >>
> >> Performance test results verified which shows an improvement of
> >> around ~0.52% with the patch compared to without it.
> >>
> >> [With patch]		null_syscall:     757.59 cycles     100.00%
> >> [Without patch]]	null_syscall:     753.66 cycles     100.00%
> > 
> > No that's a performance *decrease* with the patch applied. ?
> 
> Micheal, thats true. The cycle counts are more with patch being applied.
> Here are some more results on mainline.

Right, more cycles == worse :)

But it's not much more so we'll merge it.

cheers

Re: [PATCH 3/3] selftests/powerpc: Add transactional syscall test

[Sam Bobroff]

On 24/03/15 13:02, Michael Ellerman wrote:
> On Tue, 2015-03-24 at 12:52 +1100, Sam Bobroff wrote:
>> On 20/03/15 20:25, Anshuman Khandual wrote:
>>> On 03/19/2015 10:13 AM, Sam Bobroff wrote:
>>>> Check that a syscall made during an active transaction will fail with
>>>> the correct failure code and that one made during a suspended
>>>> transaction will succeed.
>>>>
>>>> Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
>>>
>>> The test works.
>>
>> Great :-)
>>
>>>> +
>>>> +int tm_syscall(void)
>>>> +{
>>>> +	SKIP_IF(!((long)get_auxv_entry(AT_HWCAP2) & PPC_FEATURE2_HTM));
>>>> +	setbuf(stdout, 0);
>>>> +	FAIL_IF(!t_active_getppid_test());
>>>> +	printf("%d active transactions correctly aborted.\n", TM_TEST_RUNS);
>>>> +	FAIL_IF(!t_suspended_getppid_test());
>>>> +	printf("%d suspended transactions succeeded.\n", TM_TEST_RUNS);
>>>> +	return 0;
>>>> +}
>>>> +
>>>> +int main(void)
>>>> +{
>>>> +	return test_harness(tm_syscall, "tm_syscall");
>>>> +}
>>>> +
>>>
>>> There is an extra blank line at the end of this file. Interchanging return
>>> codes of 0 and 1 for various functions make it very confusing along with
>>> negative FAIL_IF checks in the primary test function. Control flow structures
>>> like these can use some in-code documentation for readability.
>>>
>>> +	for (i = 0; i < TM_RETRIES; i++) {
>>> +		if (__builtin_tbegin(0)) {
>>> +			getppid();
>>> +			__builtin_tend(0);
>>> +			return 1;
>>> +		}
>>> +		if (t_failure_persistent())
>>> +			return 0;
>>>
>>> or
>>>
>>> +		if (__builtin_tbegin(0)) {
>>> +			__builtin_tsuspend();
>>> +			getppid();
>>> +			__builtin_tresume();
>>> +			__builtin_tend(0);
>>> +			return 1;
>>> +		}
>>> +		if (t_failure_persistent())
>>> +			return 0;
>>>
>>
>> Good points. I'll remove the blank line and comment the code.
>>
>> I'm not sure I can do any better with the FAIL_IF() macro: I wanted it
>> to read "fail if the test failed", but I can see what you mean about a
>> double negative. Maybe it would be better to introduce a different
>> macro, more like a standard assert: TEST(XXX) which fails if XXX is
>> false. However, I think "TEST" would be too generic a name and I'm not
>> should what would be better. Any comments/suggestions?
> 
> FAIL_IF() is designed for things that return 0 for OK and !0 for failure. Like
> most things in C.
> 
> So I think it would be improved if you inverted your return codes in your test
> routines.
> 
> Even better to return ESOMETHING in the error cases, and zero otherwise.
> 
> cheers

Fair enough. I think the *_test() functions I added for "clarity" were
just making it more confusing, so I've dropped them.

Moving the code around, even a little, has also exposed the fact that
transactions are very sensitive to how the code is compiled so I'm going
to move the transaction blocks out into a separate assembly file where I
can control exactly what instructions get used. This will also mean that
it's no longer dependent on using linker magic (or some other trick) to
avoid lazy symbol loading.

I'll repost the series.

Thanks for the review!

Cheers,
Sam.