| Message ID | b4a4ea23b20098512cf350b97ce98e0d1ea58531.1426740212.git.sam.bobroff@au1.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
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;
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.
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
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.
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"); +} +
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