* [PATCH v2 1/4] powerpc/tm: Abort syscalls in active transactions
2015-03-30 3:20 [PATCH v2 0/4] powerpc/tm: Abort syscalls in active transactions Sam Bobroff
@ 2015-03-30 3:20 ` Sam Bobroff
2015-04-04 9:14 ` Michael Ellerman
2015-03-30 3:20 ` [PATCH v2 2/4] selftests/powerpc: Move get_auxv_entry() to harness.c Sam Bobroff
` (2 subsequent siblings)
3 siblings, 1 reply; 6+ messages in thread
From: Sam Bobroff @ 2015-03-30 3:20 UTC (permalink / raw)
To: mpe, benh; +Cc: mikey, azanella, linuxppc-dev, matt
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>
---
v2:
Also update the failure code table.
Documentation/powerpc/transactional_memory.txt | 32 ++++++++++++------------
arch/powerpc/include/uapi/asm/tm.h | 2 +-
arch/powerpc/kernel/entry_64.S | 19 ++++++++++++++
3 files changed, 36 insertions(+), 17 deletions(-)
diff --git a/Documentation/powerpc/transactional_memory.txt b/Documentation/powerpc/transactional_memory.txt
index 9791e98..98b39af 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 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 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.
+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.
-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.
+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 appearance 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
@@ -176,8 +177,7 @@ kernel aborted a transaction:
TM_CAUSE_RESCHED Thread was rescheduled.
TM_CAUSE_TLBI Software TLB invalide.
TM_CAUSE_FAC_UNAV FP/VEC/VSX unavailable trap.
- TM_CAUSE_SYSCALL Currently unused; future syscalls that must abort
- transactions for consistency will use this.
+ TM_CAUSE_SYSCALL Syscall from active transaction.
TM_CAUSE_SIGNAL Signal delivered.
TM_CAUSE_MISC Currently unused.
TM_CAUSE_ALIGNMENT Alignment fault.
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* Re: [PATCH v2 1/4] powerpc/tm: Abort syscalls in active transactions
2015-03-30 3:20 ` [PATCH v2 1/4] " Sam Bobroff
@ 2015-04-04 9:14 ` Michael Ellerman
0 siblings, 0 replies; 6+ messages in thread
From: Michael Ellerman @ 2015-04-04 9:14 UTC (permalink / raw)
To: Sam Bobroff; +Cc: mikey, azanella, linuxppc-dev, matt
On Mon, 2015-03-30 at 14:20 +1100, Sam Bobroff wrote:
> @@ -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
TABORT please, otherwise old toolchains can't build it.
cheers
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH v2 2/4] selftests/powerpc: Move get_auxv_entry() to harness.c
2015-03-30 3:20 [PATCH v2 0/4] powerpc/tm: Abort syscalls in active transactions Sam Bobroff
2015-03-30 3:20 ` [PATCH v2 1/4] " Sam Bobroff
@ 2015-03-30 3:20 ` Sam Bobroff
2015-03-30 3:20 ` [PATCH v2 3/4] selftests/powerpc: Add transactional syscall test Sam Bobroff
2015-03-30 3:20 ` [PATCH v2 4/4] powerpc/tm: Correct minor documentation typos Sam Bobroff
3 siblings, 0 replies; 6+ messages in thread
From: Sam Bobroff @ 2015-03-30 3:20 UTC (permalink / raw)
To: mpe, benh; +Cc: mikey, azanella, linuxppc-dev, matt
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
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 3/4] selftests/powerpc: Add transactional syscall test
2015-03-30 3:20 [PATCH v2 0/4] powerpc/tm: Abort syscalls in active transactions Sam Bobroff
2015-03-30 3:20 ` [PATCH v2 1/4] " Sam Bobroff
2015-03-30 3:20 ` [PATCH v2 2/4] selftests/powerpc: Move get_auxv_entry() to harness.c Sam Bobroff
@ 2015-03-30 3:20 ` Sam Bobroff
2015-03-30 3:20 ` [PATCH v2 4/4] powerpc/tm: Correct minor documentation typos Sam Bobroff
3 siblings, 0 replies; 6+ messages in thread
From: Sam Bobroff @ 2015-03-30 3:20 UTC (permalink / raw)
To: mpe, benh; +Cc: mikey, azanella, linuxppc-dev, matt
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>
---
v2:
Further testing has shown that the success or failure of the transactions was
affected by minor changes to the code, compiler optimisation and linker
settings.
To address this, I've moved the transactional part of the test to a separate
function, written in assembly. I've also extended the test to as many
transactions as it can fit into ten seconds, to better test for failures that
occur more rarely. This has stabilised the results, and it's no longer
necessary to use special compiler or linker flags.
tools/testing/selftests/powerpc/tm/.gitignore | 1 +
tools/testing/selftests/powerpc/tm/Makefile | 4 +-
.../testing/selftests/powerpc/tm/tm-syscall-asm.S | 27 +++++
.../testing/selftests/powerpc/tm/tm-syscall-asm.h | 2 +
tools/testing/selftests/powerpc/tm/tm-syscall.c | 109 ++++++++++++++++++++
5 files changed, 142 insertions(+), 1 deletion(-)
create mode 100644 tools/testing/selftests/powerpc/tm/tm-syscall-asm.S
create mode 100644 tools/testing/selftests/powerpc/tm/tm-syscall-asm.h
create mode 100644 tools/testing/selftests/powerpc/tm/tm-syscall.c
diff --git a/tools/testing/selftests/powerpc/tm/.gitignore b/tools/testing/selftests/powerpc/tm/.gitignore
index 33d02cc..2699635d 100644
--- a/tools/testing/selftests/powerpc/tm/.gitignore
+++ b/tools/testing/selftests/powerpc/tm/.gitignore
@@ -1 +1,2 @@
tm-resched-dscr
+tm-syscall
diff --git a/tools/testing/selftests/powerpc/tm/Makefile b/tools/testing/selftests/powerpc/tm/Makefile
index 2cede23..93bbff3 100644
--- a/tools/testing/selftests/powerpc/tm/Makefile
+++ b/tools/testing/selftests/powerpc/tm/Makefile
@@ -1,8 +1,10 @@
-PROGS := tm-resched-dscr
+PROGS := tm-resched-dscr tm-syscall
+CFLAGS:=$(CFLAGS) -mhtm
all: $(PROGS)
$(PROGS): ../harness.c
+tm-syscall: tm-syscall-asm.S
run_tests: all
@-for PROG in $(PROGS); do \
diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S
new file mode 100644
index 0000000..2b2daa7
--- /dev/null
+++ b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.S
@@ -0,0 +1,27 @@
+#include <ppc-asm.h>
+#include <asm/unistd.h>
+
+ .text
+FUNC_START(getppid_tm_active_impl)
+ tbegin.
+ beq 1f
+ li r0, __NR_getppid
+ sc
+ tend.
+ blr
+1:
+ li r3, -1
+ blr
+
+FUNC_START(getppid_tm_suspended_impl)
+ tbegin.
+ beq 1f
+ li r0, __NR_getppid
+ tsuspend.
+ sc
+ tresume.
+ tend.
+ blr
+1:
+ li r3, -1
+ blr
diff --git a/tools/testing/selftests/powerpc/tm/tm-syscall-asm.h b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.h
new file mode 100644
index 0000000..6136328
--- /dev/null
+++ b/tools/testing/selftests/powerpc/tm/tm-syscall-asm.h
@@ -0,0 +1,2 @@
+extern int getppid_tm_active_impl(void);
+extern int getppid_tm_suspended_impl(void);
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..ff3b15c
--- /dev/null
+++ b/tools/testing/selftests/powerpc/tm/tm-syscall.c
@@ -0,0 +1,109 @@
+/* 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.
+ */
+
+#include <stdio.h>
+#include <unistd.h>
+#include <sys/syscall.h>
+#include <asm/tm.h>
+#include <asm/cputable.h>
+#include <linux/auxvec.h>
+#include <sys/time.h>
+#include <stdlib.h>
+
+#include "utils.h"
+#include "tm-syscall-asm.h"
+
+unsigned retries = 0;
+
+#define TEST_DURATION 10 /* seconds */
+#define TM_RETRIES 100
+
+long failure_code(void)
+{
+ return __builtin_get_texasr() >> 56;
+}
+
+bool failure_is_persistent(void)
+{
+ return (failure_code() & TM_CAUSE_PERSISTENT) == TM_CAUSE_PERSISTENT;
+}
+
+bool failure_is_syscall(void)
+{
+ return (failure_code() & TM_CAUSE_SYSCALL) == TM_CAUSE_SYSCALL;
+}
+
+pid_t getppid_tm(bool suspend)
+{
+ int i;
+ pid_t pid;
+
+ for (i = 0; i < TM_RETRIES; i++) {
+ if (suspend)
+ pid = getppid_tm_suspended_impl();
+ else
+ pid = getppid_tm_active_impl();
+ if (pid >= 0)
+ return pid;
+ if (failure_is_persistent()) {
+ if (failure_is_syscall())
+ return -1;
+ printf("Unexpected persistent transaction failure.\n");
+ printf("TEXASR 0x%016lx, TFIAR 0x%016lx.\n",
+ __builtin_get_texasr(), __builtin_get_tfiar());
+ exit(-1);
+ }
+ retries++;
+ }
+ printf("Exceeded limit of %d temporary transaction failures.\n",
+ TM_RETRIES);
+ printf("TEXASR 0x%016lx, TFIAR 0x%016lx.\n",
+ __builtin_get_texasr(), __builtin_get_tfiar());
+ exit(-1);
+}
+
+int tm_syscall(void)
+{
+ unsigned count = 0;
+ struct timeval end, now;
+
+ SKIP_IF(!((long)get_auxv_entry(AT_HWCAP2) & PPC_FEATURE2_HTM));
+ setbuf(stdout, 0);
+
+ printf("Testing transactional syscalls for %d seconds...\n",
+ TEST_DURATION);
+
+ gettimeofday(&end, 0);
+ now.tv_sec = TEST_DURATION;
+ now.tv_usec = 0;
+ timeradd(&end, &now, &end);
+
+ for (count = 0; timercmp(&now, &end, <); count++) {
+ /* Test a syscall within a suspended transaction and verify
+ * that it succeeds.
+ */
+ FAIL_IF(getppid_tm(true) == -1); /* Should succeed. */
+
+ /* Test a syscall within an active transaction and verify that
+ * it fails with the correct failure code.
+ */
+ FAIL_IF(getppid_tm(false) != -1); /* Should fail... */
+ FAIL_IF(!failure_is_persistent()); /* ...persistently... */
+ FAIL_IF(!failure_is_syscall()); /* ...with code syscall. */
+ gettimeofday(&now, 0);
+ }
+ printf("%d active and suspended transactions behaved correctly.\n",
+ count);
+ printf("(There were %d transaction retries.)\n", retries);
+
+ return 0;
+}
+
+int main(void)
+{
+ return test_harness(tm_syscall, "tm_syscall");
+}
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread* [PATCH v2 4/4] powerpc/tm: Correct minor documentation typos
2015-03-30 3:20 [PATCH v2 0/4] powerpc/tm: Abort syscalls in active transactions Sam Bobroff
` (2 preceding siblings ...)
2015-03-30 3:20 ` [PATCH v2 3/4] selftests/powerpc: Add transactional syscall test Sam Bobroff
@ 2015-03-30 3:20 ` Sam Bobroff
3 siblings, 0 replies; 6+ messages in thread
From: Sam Bobroff @ 2015-03-30 3:20 UTC (permalink / raw)
To: mpe, benh; +Cc: mikey, azanella, linuxppc-dev, matt
Signed-off-by: Sam Bobroff <sam.bobroff@au1.ibm.com>
---
v2:
Discovered some typos while updating the documentation.
Documentation/powerpc/transactional_memory.txt | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/powerpc/transactional_memory.txt b/Documentation/powerpc/transactional_memory.txt
index 98b39af..ba0a2a4 100644
--- a/Documentation/powerpc/transactional_memory.txt
+++ b/Documentation/powerpc/transactional_memory.txt
@@ -175,7 +175,7 @@ These are defined in <asm/reg.h>, and distinguish different reasons why the
kernel aborted a transaction:
TM_CAUSE_RESCHED Thread was rescheduled.
- TM_CAUSE_TLBI Software TLB invalide.
+ TM_CAUSE_TLBI Software TLB invalid.
TM_CAUSE_FAC_UNAV FP/VEC/VSX unavailable trap.
TM_CAUSE_SYSCALL Syscall from active transaction.
TM_CAUSE_SIGNAL Signal delivered.
@@ -185,7 +185,7 @@ kernel aborted a transaction:
These can be checked by the user program's abort handler as TEXASR[0:7]. If
bit 7 is set, it indicates that the error is consider persistent. For example
-a TM_CAUSE_ALIGNMENT will be persistent while a TM_CAUSE_RESCHED will not.q
+a TM_CAUSE_ALIGNMENT will be persistent while a TM_CAUSE_RESCHED will not.
GDB
===
--
1.7.10.4
^ permalink raw reply related [flat|nested] 6+ messages in thread