| Message ID | 20180809202157.21148-1-rafael.tinoco@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [v2] selftests: membarrier: fix test by checking supported commands | expand |
Hi Rafael, Thanks for the ping. On 08/09/2018 02:21 PM, Rafael David Tinoco wrote: > Makes membarrier_test compatible with older kernels (LTS) by checking if > the membarrier features exist before running the tests. > > Link: https://bugs.linaro.org/show_bug.cgi?id=3771 > Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org> > Cc: <stable@vger.kernel.org> #v4.17 > --- > .../selftests/membarrier/membarrier_test.c | 71 +++++++++++-------- > 1 file changed, 40 insertions(+), 31 deletions(-) > > diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c > index 6793f8ecc8e7..4dc263824bda 100644 > --- a/tools/testing/selftests/membarrier/membarrier_test.c > +++ b/tools/testing/selftests/membarrier/membarrier_test.c > @@ -223,7 +223,7 @@ static int test_membarrier_global_expedited_success(void) > return 0; > } > > -static int test_membarrier(void) > +static int test_membarrier(int supported) > { > int status; > > @@ -236,21 +236,22 @@ static int test_membarrier(void) > status = test_membarrier_global_success(); > if (status) > return status; > - status = test_membarrier_private_expedited_fail(); > - if (status) > - return status; > - status = test_membarrier_register_private_expedited_success(); > - if (status) > - return status; > - status = test_membarrier_private_expedited_success(); > - if (status) > - return status; > - status = sys_membarrier(MEMBARRIER_CMD_QUERY, 0); > - if (status < 0) { > - ksft_test_result_fail("sys_membarrier() failed\n"); > - return status; > + > + /* commit 22e4ebb975822833b083533035233d128b30e98f added this feature */ Get rid of this comment. > + if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED) { > + status = test_membarrier_private_expedited_fail(); > + if (status) > + return status; > + status = test_membarrier_register_private_expedited_success(); > + if (status) > + return status; > + status = test_membarrier_private_expedited_success(); > + if (status) > + return status; > } This change moves several tests under this check. These should run to test the case when MEMBARRIER_CMD_PRIVATE_EXPEDITED isn't supported. This change reduces coverage. > - if (status & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) { > + > + /* commit 70216e18e519a54a2f13569e8caff99a092a92d6 added this feature */ Get rid of the above comment. > + if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) { > status = test_membarrier_private_expedited_sync_core_fail(); > if (status) > return status; > @@ -261,23 +262,28 @@ static int test_membarrier(void) > if (status) > return status; > } > - /* > - * It is valid to send a global membarrier from a non-registered > - * process. > - */ > - status = test_membarrier_global_expedited_success(); > - if (status) > - return status; > - status = test_membarrier_register_global_expedited_success(); > - if (status) > - return status; > - status = test_membarrier_global_expedited_success(); > - if (status) > - return status; > + > + /* commit c5f58bd58f432be5d92df33c5458e0bcbee3aadf added this feature */ Get rid of the above comment. > + if (supported & MEMBARRIER_CMD_GLOBAL_EXPEDITED) { > + /* > + * It is valid to send a global membarrier from a non-registered > + * process. > + */ > + status = test_membarrier_global_expedited_success(); > + if (status) > + return status; > + status = test_membarrier_register_global_expedited_success(); > + if (status) > + return status; > + status = test_membarrier_global_expedited_success(); > + if (status) > + return status; > + } > + There skip handling missing here. Without this the test result reports pass which is incorrect. If feature isn't supported, test should report that the feature test is skipped not passed. What I would like to see here is a skip for each individual test not one skip for all 3 tests. This applies to the if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) case above. When I run this test on 4.19 I see, TAP version 13 selftests: membarrier: membarrier_test ======================================== ok 1 sys_membarrier available ok 2 sys membarrier invalid command test: command = -1, flags = 0, errno = 22. Failed as expected ok 3 sys membarrier MEMBARRIER_CMD_QUERY invalid flags test: flags = 1, errno = 22. Failed as expected ok 4 sys membarrier MEMBARRIER_CMD_GLOBAL test: flags = 0 ok 5 sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED not registered failure test: flags = 0, errno = 1 ok 6 sys membarrier MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED test: flags = 0 ok 7 sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED test: flags = 0 ok 8 sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE not registered failure test: flags = 0, errno = 1 ok 9 sys membarrier MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE test: flags = 0 ok 10 sys membarrier MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE test: flags = 0 ok 11 sys membarrier MEMBARRIER_CMD_GLOBAL_EXPEDITED test: flags = 0 ok 12 sys membarrier MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED test: flags = 0 ok 13 sys membarrier MEMBARRIER_CMD_GLOBAL_EXPEDITED test: flags = 0 Pass 13 Fail 0 Xfail 0 Xpass 0 Skip 0 Error 0 1..13 ok 1..1 selftests: membarrier: membarrier_test [PASS] A total of 13 tests out of which, maybe 6 should be skips on 4.14. For example, if MEMBARRIER_CMD_GLOBAL_EXPEDITED isn't supported, it should report that test as a skip. > return 0; > } > > -static int test_membarrier_query(void) > +static int test_membarrier_query(int *supported) > { > int flags = 0, ret; > > @@ -297,16 +303,19 @@ static int test_membarrier_query(void) > ksft_exit_skip( > "sys_membarrier unsupported: CMD_GLOBAL not found.\n"); > > + *supported = ret; > ksft_test_result_pass("sys_membarrier available\n"); > return 0; > } > > int main(int argc, char **argv) > { > + int supported; > + > ksft_print_header(); > > - test_membarrier_query(); > - test_membarrier(); > + test_membarrier_query(&supported); > + test_membarrier(supported); > > return ksft_exit_pass(); > } > thanks, -- Shuah
Hi Rafael, Thanks for the patch. Comments below. On 09/02/2018 08:12 PM, Rafael David Tinoco wrote: > Shuah, > > This is a draft only. I will remove summary from the top, run checkers, > etc. Im thinking in replacing membarrier_test entirely with this code > (compatible to existing one). Right now, this code: > > - allows each test to succeed, fail or be skipped independently > - allows each test to be tested even when not supported (force option) > - considers false negatives and false positives on every case > - can be extended easily > > Right now, just to show as an example, it gives us: > > TAP version 13 > ok 1 sys_membarrier(): cmd_query succeeded. > ok 2 sys_membarrier(): bad_cmd failed as expected. > ok 3 sys_membarrier(): cmd_with_flags_set failed as expected. > ok 4 sys_membarrier(): cmd_global succeeded. > Pass 4 Fail 0 Xfail 0 Xpass 0 Skip 0 Error 0 > 1..4 > > Are you okay with such move ? Only big TODO here is adding all covered > tests in the test array (easy move), testing all combinations with all > supported kernel versions (lab already ready) and suggesting it to you, > replacing membarrier_test.c. > > PS: This is pretty close to how a LTP test would be, using their new > API, but, since it addresses your concerns and seems like a > simple/clean, code, I decided to suggest it as a replacement here (and > it also fixes the issue with this test and LTS kernels). > --- > tools/testing/selftests/membarrier/Makefile | 2 +- > .../selftests/membarrier/membarrier_test2.c | 180 ++++++++++++++++++ > 2 files changed, 181 insertions(+), 1 deletion(-) > create mode 100644 tools/testing/selftests/membarrier/membarrier_test2.c > > diff --git a/tools/testing/selftests/membarrier/Makefile b/tools/testing/selftests/membarrier/Makefile > index 02845532b059..3d44d4cd3a9d 100644 > --- a/tools/testing/selftests/membarrier/Makefile > +++ b/tools/testing/selftests/membarrier/Makefile > @@ -1,6 +1,6 @@ > CFLAGS += -g -I../../../../usr/include/ > > -TEST_GEN_PROGS := membarrier_test > +TEST_GEN_PROGS := membarrier_test membarrier_test2 > > include ../lib.mk > > diff --git a/tools/testing/selftests/membarrier/membarrier_test2.c b/tools/testing/selftests/membarrier/membarrier_test2.c > new file mode 100644 > index 000000000000..8fa1be6156fb > --- /dev/null > +++ b/tools/testing/selftests/membarrier/membarrier_test2.c > @@ -0,0 +1,180 @@ > +// SPDX-License-Identifier: GPL-2.0 > +#define _GNU_SOURCE > +#include <linux/membarrier.h> > +#include <syscall.h> > +#include <stdio.h> > +#include <errno.h> > +#include <string.h> > + > +#include "../kselftest.h" > +/* > + MEMBARRIER_CMD_QUERY > + returns membarrier_cmd with supported features > + MEMBARRIER_CMD_GLOBAL > + returns 0 > + EINVAL = if nohz_full is enabled > + MEMBARRIER_CMD_GLOBAL_EXPEDITED > + returns 0 > + MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED > + returns 0 > + MEMBARRIER_CMD_PRIVATE_EXPEDITED > + returns 0 > + EINVAL = if CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE is not enabled > + EPERM = if process did not register for PRIVATE_EXPEDITED > + MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED > + returns 0 > + EINVAL = if CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE is not enabled > + MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE > + returns 0 > + EINVAL = if CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE is not enabled > + EPERM = if process did not register for PRIVATE_EXPEDITED > + MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE > + returns 0 > + EINVAL = if CONFIG_ARCH_HAS_MEMBARRIER_SYNC_CORE is not enabled > +*/ > + > +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])) > + > +struct memb_tests { > + char testname[80]; > + int command; > + int flags; > + int exp_ret; > + int exp_errno; > + int supported; > + int force; > +}; > + > +struct memb_tests mbt[] = { > + { > + .testname = "bad_cmd\0", > + .command = -1, > + .exp_ret = -1, > + .exp_errno = EINVAL, > + .supported = 1, > + }, > + { > + .testname = "cmd_with_flags_set\0", > + .command = MEMBARRIER_CMD_QUERY, > + .flags = 1, > + .exp_ret = -1, > + .exp_errno = EINVAL, > + .supported = 1, > + }, > + { > + .testname = "cmd_global\0", > + .command = MEMBARRIER_CMD_GLOBAL, > + .flags = 0, > + .exp_ret = 0, > + }, > +}; > + > +static void info_passed_ok(struct memb_tests test) > +{ > + ksft_test_result_pass("sys_membarrier(): %s succeeded.\n", > + test.testname); > +} > + Why do we need to add new routines for these conditions. Why can't handle these strings in array. For example you can define an array of strings for passed unexpectedly etc. and the pass the string to appropriate ksft_* interface instead of adding of these routines. Also it is hard to review the code this way. I would like to see the changes made to membarrier_test.c instead of adding a new file. I do like the direction though. thanks, -- Shuah
On 09/03/2018 01:04 PM, Rafael David Tinoco wrote: > This commit re-organizes membarrier test, solving issues when testing > LTS kernels. Now, the code: > > - always run the same amount of tests (even on older kernels). > - allows each test to succeed, fail or be skipped independently. > - allows testing features even when explicitly unsupported (force=1). > - able to consider different return codes for diff kernel versions. > - checks false positive/negative by checking ret code and errno. > - can be extended easily: to expand an array with commands. > > Link: https://bugs.linaro.org/show_bug.cgi?id=3771 > Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org> > --- > .../selftests/membarrier/membarrier_test.c | 482 +++++++++--------- > 1 file changed, 241 insertions(+), 241 deletions(-) > > diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c > index 6793f8ecc8e7..151bc8a944a3 100644 > --- a/tools/testing/selftests/membarrier/membarrier_test.c > +++ b/tools/testing/selftests/membarrier/membarrier_test.c > @@ -1,6 +1,7 @@ > // SPDX-License-Identifier: GPL-2.0 > #define _GNU_SOURCE > #include <linux/membarrier.h> > +#include <sys/utsname.h> > #include <syscall.h> > #include <stdio.h> > #include <errno.h> > @@ -8,305 +9,304 @@ > > #include "../kselftest.h" > > -static int sys_membarrier(int cmd, int flags) > +#define ARRAY_SIZE(arr) (sizeof(arr) / sizeof((arr)[0])) > +#define KERNEL_VERSION(a, b, c) (((a) << 16) + ((b) << 8) + (c)) > + > +struct memb_tests { > + char testname[80]; > + int command; > + int flags; > + int exp_ret; > + int exp_errno; > + int enabled; > + int force; > + int force_exp_errno; > + int above; > + int bellow; > +}; > + > +struct memb_tests mbt[] = { > + { > + .testname = "cmd_fail\0", > + .command = -1, > + .exp_ret = -1, > + .exp_errno = EINVAL, > + .enabled = 1, > + }, > + { > + .testname = "cmd_flags_fail\0", > + .command = MEMBARRIER_CMD_QUERY, > + .flags = 1, > + .exp_ret = -1, > + .exp_errno = EINVAL, > + .enabled = 1, > + }, > + { > + .testname = "cmd_global_success\0", > + .command = MEMBARRIER_CMD_GLOBAL, > + .flags = 0, > + .exp_ret = 0, > + }, > + /* > + * PRIVATE EXPEDITED (forced) > + */ > + { > + .testname = "cmd_private_expedited_fail\0", > + .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED, > + .flags = 0, > + .exp_ret = -1, > + .exp_errno = EPERM, > + .force = 1, > + .force_exp_errno = EINVAL, > + .bellow = KERNEL_VERSION(4, 10, 0), > + }, > + { > + .testname = "cmd_private_expedited_fail\0", > + .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED, > + .flags = 0, > + .exp_ret = -1, > + .exp_errno = EPERM, > + .force = 1, > + .force_exp_errno = EPERM, > + .above = KERNEL_VERSION(4, 10, 0), > + }, > + { > + .testname = "cmd_register_private_expedited_success\0", > + .command = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED, > + .flags = 0, > + .exp_ret = 0, > + .force = 1, > + .force_exp_errno = EINVAL, > + }, > + { > + .testname = "cmd_private_expedited_success\0", > + .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED, > + .flags = 0, > + .exp_ret = 0, > + .force = 1, > + .force_exp_errno = EINVAL, > + }, > + /* > + * PRIVATE EXPEDITED SYNC CORE > + */ > + { > + .testname = "cmd_private_expedited_sync_core_fail\0", > + .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE, > + .flags = 0, > + .exp_ret = -1, > + .exp_errno = EPERM, > + }, > + { > + .testname = "cmd_register_private_expedited_sync_core_success\0", > + .command = MEMBARRIER_CMD_REGISTER_PRIVATE_EXPEDITED_SYNC_CORE, > + .flags = 0, > + .exp_ret = 0, > + }, > + { > + .testname = "cmd_private_expedited_sync_core_success\0", > + .command = MEMBARRIER_CMD_PRIVATE_EXPEDITED, > + .flags = 0, > + .exp_ret = 0, > + }, > + /* > + * GLOBAL EXPEDITED > + * global membarrier from a non-registered process is valid > + */ > + { > + .testname = "cmd_global_expedited_success\0", > + .command = MEMBARRIER_CMD_GLOBAL_EXPEDITED, > + .flags = 0, > + .exp_ret = 0, > + }, > + { > + .testname = "cmd_register_global_expedited_success\0", > + .command = MEMBARRIER_CMD_REGISTER_GLOBAL_EXPEDITED, > + .flags = 0, > + .exp_ret = 0, > + }, > + { > + .testname = "cmd_global_expedited_success\0", > + .command = MEMBARRIER_CMD_GLOBAL_EXPEDITED, > + .flags = 0, > + .exp_ret = 0, > + }, > +}; > + > +static void > +info_passed_ok(struct memb_tests test) > { > - return syscall(__NR_membarrier, cmd, flags); > + ksft_test_result_pass("sys_membarrier(): %s succeeded.\n", > + test.testname); > } > Why do we need to add new routines for these conditions. Why can't handle these strings in array. For example you can define an array of strings for passed unexpectedly etc. and the pass the string to appropriate ksft_* interface instead of adding of these routines. Also it is hard to review the code this way. I do like the direction though. Also please run get_maintainer.pl and cc everybody it suggests. thanks, -- Shuah
diff --git a/tools/testing/selftests/membarrier/membarrier_test.c b/tools/testing/selftests/membarrier/membarrier_test.c index 6793f8ecc8e7..4dc263824bda 100644 --- a/tools/testing/selftests/membarrier/membarrier_test.c +++ b/tools/testing/selftests/membarrier/membarrier_test.c @@ -223,7 +223,7 @@ static int test_membarrier_global_expedited_success(void) return 0; } -static int test_membarrier(void) +static int test_membarrier(int supported) { int status; @@ -236,21 +236,22 @@ static int test_membarrier(void) status = test_membarrier_global_success(); if (status) return status; - status = test_membarrier_private_expedited_fail(); - if (status) - return status; - status = test_membarrier_register_private_expedited_success(); - if (status) - return status; - status = test_membarrier_private_expedited_success(); - if (status) - return status; - status = sys_membarrier(MEMBARRIER_CMD_QUERY, 0); - if (status < 0) { - ksft_test_result_fail("sys_membarrier() failed\n"); - return status; + + /* commit 22e4ebb975822833b083533035233d128b30e98f added this feature */ + if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED) { + status = test_membarrier_private_expedited_fail(); + if (status) + return status; + status = test_membarrier_register_private_expedited_success(); + if (status) + return status; + status = test_membarrier_private_expedited_success(); + if (status) + return status; } - if (status & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) { + + /* commit 70216e18e519a54a2f13569e8caff99a092a92d6 added this feature */ + if (supported & MEMBARRIER_CMD_PRIVATE_EXPEDITED_SYNC_CORE) { status = test_membarrier_private_expedited_sync_core_fail(); if (status) return status; @@ -261,23 +262,28 @@ static int test_membarrier(void) if (status) return status; } - /* - * It is valid to send a global membarrier from a non-registered - * process. - */ - status = test_membarrier_global_expedited_success(); - if (status) - return status; - status = test_membarrier_register_global_expedited_success(); - if (status) - return status; - status = test_membarrier_global_expedited_success(); - if (status) - return status; + + /* commit c5f58bd58f432be5d92df33c5458e0bcbee3aadf added this feature */ + if (supported & MEMBARRIER_CMD_GLOBAL_EXPEDITED) { + /* + * It is valid to send a global membarrier from a non-registered + * process. + */ + status = test_membarrier_global_expedited_success(); + if (status) + return status; + status = test_membarrier_register_global_expedited_success(); + if (status) + return status; + status = test_membarrier_global_expedited_success(); + if (status) + return status; + } + return 0; } -static int test_membarrier_query(void) +static int test_membarrier_query(int *supported) { int flags = 0, ret; @@ -297,16 +303,19 @@ static int test_membarrier_query(void) ksft_exit_skip( "sys_membarrier unsupported: CMD_GLOBAL not found.\n"); + *supported = ret; ksft_test_result_pass("sys_membarrier available\n"); return 0; } int main(int argc, char **argv) { + int supported; + ksft_print_header(); - test_membarrier_query(); - test_membarrier(); + test_membarrier_query(&supported); + test_membarrier(supported); return ksft_exit_pass(); }
Makes membarrier_test compatible with older kernels (LTS) by checking if the membarrier features exist before running the tests. Link: https://bugs.linaro.org/show_bug.cgi?id=3771 Signed-off-by: Rafael David Tinoco <rafael.tinoco@linaro.org> Cc: <stable@vger.kernel.org> #v4.17 --- .../selftests/membarrier/membarrier_test.c | 71 +++++++++++-------- 1 file changed, 40 insertions(+), 31 deletions(-)