* [PATCH v2 5/7] kunit: test: add test plan to KUnit TAP format
From: Brendan Higgins @ 2020-01-30 23:08 UTC (permalink / raw)
To: jdike, richard, anton.ivanov, arnd, keescook, skhan, alan.maguire,
yzaikin, davidgow, akpm, rppt, frowand.list
Cc: gregkh, sboyd, logang, mcgrof, knut.omang, linux-um, linux-arch,
linux-kselftest, kunit-dev, linux-kernel, linux-doc,
Brendan Higgins
In-Reply-To: <20200130230812.142642-1-brendanhiggins@google.com>
TAP 14 allows an optional test plan to be emitted before the start of
the start of testing[1]; this is valuable because it makes it possible
for a test harness to detect whether the number of tests run matches the
number of tests expected to be run, ensuring that no tests silently
failed.
Link[1]: https://github.com/isaacs/testanything.github.io/blob/tap14/tap-version-14-specification.md#the-plan
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
lib/kunit/executor.c | 17 +++++
lib/kunit/test.c | 11 ---
tools/testing/kunit/kunit_parser.py | 74 ++++++++++++++++---
.../test_is_test_passed-all_passed.log | 1 +
.../test_data/test_is_test_passed-crash.log | 1 +
.../test_data/test_is_test_passed-failure.log | 1 +
6 files changed, 82 insertions(+), 23 deletions(-)
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index b75a46c560847..7fd16feff157e 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -11,11 +11,28 @@ extern struct kunit_suite * const * const __kunit_suites_end[];
#if IS_BUILTIN(CONFIG_KUNIT)
+static void kunit_print_tap_header(void)
+{
+ struct kunit_suite * const * const *suites, * const *subsuite;
+ int num_of_suites = 0;
+
+ for (suites = __kunit_suites_start;
+ suites < __kunit_suites_end;
+ suites++)
+ for (subsuite = *suites; *subsuite != NULL; subsuite++)
+ num_of_suites++;
+
+ pr_info("TAP version 14\n");
+ pr_info("1..%d\n", num_of_suites);
+}
+
int kunit_run_all_tests(void)
{
struct kunit_suite * const * const *suites, * const *subsuite;
bool has_test_failed = false;
+ kunit_print_tap_header();
+
for (suites = __kunit_suites_start;
suites < __kunit_suites_end;
suites++) {
diff --git a/lib/kunit/test.c b/lib/kunit/test.c
index 9242f932896c7..da56b94261b43 100644
--- a/lib/kunit/test.c
+++ b/lib/kunit/test.c
@@ -18,16 +18,6 @@ static void kunit_set_failure(struct kunit *test)
WRITE_ONCE(test->success, false);
}
-static void kunit_print_tap_version(void)
-{
- static bool kunit_has_printed_tap_version;
-
- if (!kunit_has_printed_tap_version) {
- pr_info("TAP version 14\n");
- kunit_has_printed_tap_version = true;
- }
-}
-
static size_t kunit_test_cases_len(struct kunit_case *test_cases)
{
struct kunit_case *test_case;
@@ -41,7 +31,6 @@ static size_t kunit_test_cases_len(struct kunit_case *test_cases)
static void kunit_print_subtest_start(struct kunit_suite *suite)
{
- kunit_print_tap_version();
pr_info("\t# Subtest: %s\n", suite->name);
pr_info("\t1..%zd\n", kunit_test_cases_len(suite->test_cases));
}
diff --git a/tools/testing/kunit/kunit_parser.py b/tools/testing/kunit/kunit_parser.py
index 4ffbae0f67325..78b3bdd03b1e4 100644
--- a/tools/testing/kunit/kunit_parser.py
+++ b/tools/testing/kunit/kunit_parser.py
@@ -45,6 +45,7 @@ class TestStatus(Enum):
FAILURE = auto()
TEST_CRASHED = auto()
NO_TESTS = auto()
+ FAILURE_TO_PARSE_TESTS = auto()
kunit_start_re = re.compile(r'^TAP version [0-9]+$')
kunit_end_re = re.compile('List of all partitions:')
@@ -106,7 +107,7 @@ OkNotOkResult = namedtuple('OkNotOkResult', ['is_ok','description', 'text'])
OK_NOT_OK_SUBTEST = re.compile(r'^\t(ok|not ok) [0-9]+ - (.*)$')
-OK_NOT_OK_MODULE = re.compile(r'^(ok|not ok) [0-9]+ - (.*)$')
+OK_NOT_OK_MODULE = re.compile(r'^(ok|not ok) ([0-9]+) - (.*)$')
def parse_ok_not_ok_test_case(lines: List[str],
test_case: TestCase,
@@ -196,7 +197,9 @@ def max_status(left: TestStatus, right: TestStatus) -> TestStatus:
else:
return TestStatus.SUCCESS
-def parse_ok_not_ok_test_suite(lines: List[str], test_suite: TestSuite) -> bool:
+def parse_ok_not_ok_test_suite(lines: List[str],
+ test_suite: TestSuite,
+ expected_suite_index: int) -> bool:
consume_non_diagnositic(lines)
if not lines:
test_suite.status = TestStatus.TEST_CRASHED
@@ -209,6 +212,12 @@ def parse_ok_not_ok_test_suite(lines: List[str], test_suite: TestSuite) -> bool:
test_suite.status = TestStatus.SUCCESS
else:
test_suite.status = TestStatus.FAILURE
+ suite_index = int(match.group(2))
+ if suite_index != expected_suite_index:
+ print_with_timestamp(
+ red('[ERROR] ') + 'expected_suite_index ' +
+ str(expected_suite_index) + ', but got ' +
+ str(suite_index))
return True
else:
return False
@@ -221,7 +230,7 @@ def bubble_up_test_case_errors(test_suite: TestSuite) -> TestStatus:
max_test_case_status = bubble_up_errors(lambda x: x.status, test_suite.cases)
return max_status(max_test_case_status, test_suite.status)
-def parse_test_suite(lines: List[str]) -> TestSuite:
+def parse_test_suite(lines: List[str], expected_suite_index: int) -> TestSuite:
if not lines:
return None
consume_non_diagnositic(lines)
@@ -240,7 +249,7 @@ def parse_test_suite(lines: List[str]) -> TestSuite:
test_suite.cases.append(test_case)
test_case = parse_test_case(lines, expected_test_case_num > 0)
expected_test_case_num -= 1
- if parse_ok_not_ok_test_suite(lines, test_suite):
+ if parse_ok_not_ok_test_suite(lines, test_suite, expected_suite_index):
test_suite.status = bubble_up_test_case_errors(test_suite)
return test_suite
elif not lines:
@@ -260,6 +269,17 @@ def parse_tap_header(lines: List[str]) -> bool:
else:
return False
+TEST_PLAN = re.compile(r'[0-9]+\.\.([0-9]+)')
+
+def parse_test_plan(lines: List[str]) -> int:
+ consume_non_diagnositic(lines)
+ match = TEST_PLAN.match(lines[0])
+ if match:
+ lines.pop(0)
+ return int(match.group(1))
+ else:
+ return None
+
def bubble_up_suite_errors(test_suite_list: List[TestSuite]) -> TestStatus:
return bubble_up_errors(lambda x: x.status, test_suite_list)
@@ -268,19 +288,34 @@ def parse_test_result(lines: List[str]) -> TestResult:
return TestResult(TestStatus.NO_TESTS, [], lines)
consume_non_diagnositic(lines)
if not parse_tap_header(lines):
- return None
+ return TestResult(TestStatus.NO_TESTS, [], lines)
+ expected_test_suite_num = parse_test_plan(lines)
+ if not expected_test_suite_num:
+ return TestResult(TestStatus.FAILURE_TO_PARSE_TESTS, [], lines)
test_suites = []
- test_suite = parse_test_suite(lines)
- while test_suite:
- test_suites.append(test_suite)
- test_suite = parse_test_suite(lines)
- return TestResult(bubble_up_suite_errors(test_suites), test_suites, lines)
+ for i in range(1, expected_test_suite_num + 1):
+ test_suite = parse_test_suite(lines, i)
+ if test_suite:
+ test_suites.append(test_suite)
+ else:
+ print_with_timestamp(
+ red('[ERROR] ') + ' expected ' +
+ str(expected_test_suite_num) +
+ ' test suites, but got ' + str(i - 2))
+ break
+ test_suite = parse_test_suite(lines, -1)
+ if test_suite:
+ print_with_timestamp(red('[ERROR] ') +
+ 'got unexpected test suite: ' + test_suite.name)
+ if test_suites:
+ return TestResult(bubble_up_suite_errors(test_suites), test_suites, lines)
+ else:
+ return TestResult(TestStatus.NO_TESTS, [], lines)
-def parse_run_tests(kernel_output) -> TestResult:
+def print_and_count_results(test_result: TestResult) -> None:
total_tests = 0
failed_tests = 0
crashed_tests = 0
- test_result = parse_test_result(list(isolate_kunit_output(kernel_output)))
for test_suite in test_result.suites:
if test_suite.status == TestStatus.SUCCESS:
print_suite_divider(green('[PASSED] ') + test_suite.name)
@@ -302,6 +337,21 @@ def parse_run_tests(kernel_output) -> TestResult:
print_with_timestamp(red('[FAILED] ') + test_case.name)
print_log(map(yellow, test_case.log))
print_with_timestamp('')
+ return total_tests, failed_tests, crashed_tests
+
+def parse_run_tests(kernel_output) -> TestResult:
+ total_tests = 0
+ failed_tests = 0
+ crashed_tests = 0
+ test_result = parse_test_result(list(isolate_kunit_output(kernel_output)))
+ if test_result.status == TestStatus.NO_TESTS:
+ print(red('[ERROR] ') + yellow('no tests run!'))
+ elif test_result.status == TestStatus.FAILURE_TO_PARSE_TESTS:
+ print(red('[ERROR] ') + yellow('could not parse test results!'))
+ else:
+ (total_tests,
+ failed_tests,
+ crashed_tests) = print_and_count_results(test_result)
print_with_timestamp(DIVIDER)
fmt = green if test_result.status == TestStatus.SUCCESS else red
print_with_timestamp(
diff --git a/tools/testing/kunit/test_data/test_is_test_passed-all_passed.log b/tools/testing/kunit/test_data/test_is_test_passed-all_passed.log
index 62ebc0288355c..bc0dc8fe35b76 100644
--- a/tools/testing/kunit/test_data/test_is_test_passed-all_passed.log
+++ b/tools/testing/kunit/test_data/test_is_test_passed-all_passed.log
@@ -1,4 +1,5 @@
TAP version 14
+1..2
# Subtest: sysctl_test
1..8
# sysctl_test_dointvec_null_tbl_data: sysctl_test_dointvec_null_tbl_data passed
diff --git a/tools/testing/kunit/test_data/test_is_test_passed-crash.log b/tools/testing/kunit/test_data/test_is_test_passed-crash.log
index 0b249870c8be4..4d97f6708c4a5 100644
--- a/tools/testing/kunit/test_data/test_is_test_passed-crash.log
+++ b/tools/testing/kunit/test_data/test_is_test_passed-crash.log
@@ -1,6 +1,7 @@
printk: console [tty0] enabled
printk: console [mc-1] enabled
TAP version 14
+1..2
# Subtest: sysctl_test
1..8
# sysctl_test_dointvec_null_tbl_data: sysctl_test_dointvec_null_tbl_data passed
diff --git a/tools/testing/kunit/test_data/test_is_test_passed-failure.log b/tools/testing/kunit/test_data/test_is_test_passed-failure.log
index 9e89d32d5667a..7a416497e3bec 100644
--- a/tools/testing/kunit/test_data/test_is_test_passed-failure.log
+++ b/tools/testing/kunit/test_data/test_is_test_passed-failure.log
@@ -1,4 +1,5 @@
TAP version 14
+1..2
# Subtest: sysctl_test
1..8
# sysctl_test_dointvec_null_tbl_data: sysctl_test_dointvec_null_tbl_data passed
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply related
* [PATCH v2 4/7] init: main: add KUnit to kernel init
From: Brendan Higgins @ 2020-01-30 23:08 UTC (permalink / raw)
To: jdike, richard, anton.ivanov, arnd, keescook, skhan, alan.maguire,
yzaikin, davidgow, akpm, rppt, frowand.list
Cc: gregkh, sboyd, logang, mcgrof, knut.omang, linux-um, linux-arch,
linux-kselftest, kunit-dev, linux-kernel, linux-doc,
Brendan Higgins
In-Reply-To: <20200130230812.142642-1-brendanhiggins@google.com>
Remove KUnit from init calls entirely, instead call directly from
kernel_init().
Co-developed-by: Alan Maguire <alan.maguire@oracle.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
include/kunit/test.h | 9 +++++++++
init/main.c | 4 ++++
lib/kunit/executor.c | 4 +---
3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 8a02f93a6b505..8689dd1459844 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -197,6 +197,15 @@ void kunit_init_test(struct kunit *test, const char *name);
int kunit_run_tests(struct kunit_suite *suite);
+#if IS_BUILTIN(CONFIG_KUNIT)
+int kunit_run_all_tests(void);
+#else
+static inline int kunit_run_all_tests(void)
+{
+ return 0;
+}
+#endif /* IS_BUILTIN(CONFIG_KUNIT) */
+
/*
* If a test suite is built-in, module_init() gets translated into
* an initcall which we don't want as the idea is that for builtins
diff --git a/init/main.c b/init/main.c
index 2cd736059416f..90301d4fbd1bb 100644
--- a/init/main.c
+++ b/init/main.c
@@ -103,6 +103,8 @@
#define CREATE_TRACE_POINTS
#include <trace/events/initcall.h>
+#include <kunit/test.h>
+
static int kernel_init(void *);
extern void init_IRQ(void);
@@ -1201,6 +1203,8 @@ static noinline void __init kernel_init_freeable(void)
do_basic_setup();
+ kunit_run_all_tests();
+
console_on_rootfs();
/*
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 6429927d598a5..b75a46c560847 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -11,7 +11,7 @@ extern struct kunit_suite * const * const __kunit_suites_end[];
#if IS_BUILTIN(CONFIG_KUNIT)
-static int kunit_run_all_tests(void)
+int kunit_run_all_tests(void)
{
struct kunit_suite * const * const *suites, * const *subsuite;
bool has_test_failed = false;
@@ -31,6 +31,4 @@ static int kunit_run_all_tests(void)
return 0;
}
-late_initcall(kunit_run_all_tests);
-
#endif /* IS_BUILTIN(CONFIG_KUNIT) */
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply related
* [PATCH v2 4/7] init: main: add KUnit to kernel init
From: Brendan Higgins @ 2020-01-30 23:08 UTC (permalink / raw)
To: jdike, richard, anton.ivanov, arnd, keescook, skhan, alan.maguire,
yzaikin, davidgow, akpm, rppt, frowand.list
Cc: gregkh, sboyd, logang, mcgrof, knut.omang, linux-um, linux-arch,
linux-kselftest, kunit-dev, linux-kernel, linux-doc,
Brendan Higgins
In-Reply-To: <20200130230812.142642-1-brendanhiggins@google.com>
Remove KUnit from init calls entirely, instead call directly from
kernel_init().
Co-developed-by: Alan Maguire <alan.maguire@oracle.com>
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
include/kunit/test.h | 9 +++++++++
init/main.c | 4 ++++
lib/kunit/executor.c | 4 +---
3 files changed, 14 insertions(+), 3 deletions(-)
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 8a02f93a6b505..8689dd1459844 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -197,6 +197,15 @@ void kunit_init_test(struct kunit *test, const char *name);
int kunit_run_tests(struct kunit_suite *suite);
+#if IS_BUILTIN(CONFIG_KUNIT)
+int kunit_run_all_tests(void);
+#else
+static inline int kunit_run_all_tests(void)
+{
+ return 0;
+}
+#endif /* IS_BUILTIN(CONFIG_KUNIT) */
+
/*
* If a test suite is built-in, module_init() gets translated into
* an initcall which we don't want as the idea is that for builtins
diff --git a/init/main.c b/init/main.c
index 2cd736059416f..90301d4fbd1bb 100644
--- a/init/main.c
+++ b/init/main.c
@@ -103,6 +103,8 @@
#define CREATE_TRACE_POINTS
#include <trace/events/initcall.h>
+#include <kunit/test.h>
+
static int kernel_init(void *);
extern void init_IRQ(void);
@@ -1201,6 +1203,8 @@ static noinline void __init kernel_init_freeable(void)
do_basic_setup();
+ kunit_run_all_tests();
+
console_on_rootfs();
/*
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
index 6429927d598a5..b75a46c560847 100644
--- a/lib/kunit/executor.c
+++ b/lib/kunit/executor.c
@@ -11,7 +11,7 @@ extern struct kunit_suite * const * const __kunit_suites_end[];
#if IS_BUILTIN(CONFIG_KUNIT)
-static int kunit_run_all_tests(void)
+int kunit_run_all_tests(void)
{
struct kunit_suite * const * const *suites, * const *subsuite;
bool has_test_failed = false;
@@ -31,6 +31,4 @@ static int kunit_run_all_tests(void)
return 0;
}
-late_initcall(kunit_run_all_tests);
-
#endif /* IS_BUILTIN(CONFIG_KUNIT) */
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply related
* [PATCH v2 3/7] kunit: test: create a single centralized executor for all tests
From: Brendan Higgins @ 2020-01-30 23:08 UTC (permalink / raw)
To: jdike, richard, anton.ivanov, arnd, keescook, skhan, alan.maguire,
yzaikin, davidgow, akpm, rppt, frowand.list
Cc: gregkh, sboyd, logang, mcgrof, knut.omang, linux-um, linux-arch,
linux-kselftest, kunit-dev, linux-kernel, linux-doc,
Brendan Higgins
In-Reply-To: <20200130230812.142642-1-brendanhiggins@google.com>
From: Alan Maguire <alan.maguire@oracle.com>
Add a centralized executor to dispatch tests rather than relying on
late_initcall to schedule each test suite separately. Centralized
execution is for built-in tests only; modules will execute tests
when loaded.
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Co-developed-by: Iurii Zaikin <yzaikin@google.com>
Signed-off-by: Iurii Zaikin <yzaikin@google.com>
Co-developed-by: Brendan Higgins <brendanhiggins@google.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
include/kunit/test.h | 73 +++++++++++++++++++++++++++-----------------
lib/kunit/Makefile | 3 +-
lib/kunit/executor.c | 36 ++++++++++++++++++++++
3 files changed, 83 insertions(+), 29 deletions(-)
create mode 100644 lib/kunit/executor.c
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 2dfb550c6723a..8a02f93a6b505 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -197,46 +197,63 @@ void kunit_init_test(struct kunit *test, const char *name);
int kunit_run_tests(struct kunit_suite *suite);
-/**
- * kunit_test_suites() - used to register one or more &struct kunit_suite
- * with KUnit.
- *
- * @suites: a statically allocated list of &struct kunit_suite.
- *
- * Registers @suites with the test framework. See &struct kunit_suite for
- * more information.
- *
- * When builtin, KUnit tests are all run as late_initcalls; this means
- * that they cannot test anything where tests must run at a different init
- * phase. One significant restriction resulting from this is that KUnit
- * cannot reliably test anything that is initialize in the late_init phase;
- * another is that KUnit is useless to test things that need to be run in
- * an earlier init phase.
- *
- * An alternative is to build the tests as a module. Because modules
- * do not support multiple late_initcall()s, we need to initialize an
- * array of suites for a module.
- *
- * TODO(brendanhiggins@google.com): Don't run all KUnit tests as
- * late_initcalls. I have some future work planned to dispatch all KUnit
- * tests from the same place, and at the very least to do so after
- * everything else is definitely initialized.
+/*
+ * If a test suite is built-in, module_init() gets translated into
+ * an initcall which we don't want as the idea is that for builtins
+ * the executor will manage execution. So ensure we do not define
+ * module_{init|exit} functions for the builtin case when registering
+ * suites via kunit_test_suites() below.
*/
-#define kunit_test_suites(...) \
- static struct kunit_suite *suites[] = { __VA_ARGS__, NULL}; \
- static int kunit_test_suites_init(void) \
+#ifdef MODULE
+#define kunit_test_suites_for_module(__suites) \
+ static int __init kunit_test_suites_init(void) \
{ \
+ struct kunit_suite *suites[] = (__suites); \
unsigned int i; \
+ \
for (i = 0; suites[i] != NULL; i++) \
kunit_run_tests(suites[i]); \
return 0; \
} \
- late_initcall(kunit_test_suites_init); \
+ module_init(kunit_test_suites_init); \
+ \
static void __exit kunit_test_suites_exit(void) \
{ \
return; \
} \
module_exit(kunit_test_suites_exit)
+#else
+#define kunit_test_suites_for_module(__suites)
+#endif /* MODULE */
+
+#define __kunit_test_suites(unique_array, unique_suites, ...) \
+ static struct kunit_suite *unique_array[] = { __VA_ARGS__, NULL }; \
+ kunit_test_suites_for_module(unique_array); \
+ static struct kunit_suite **unique_suites \
+ __used __aligned(8) __section(.kunit_test_suites) = unique_array
+
+/**
+ * kunit_test_suites() - used to register one or more &struct kunit_suite
+ * with KUnit.
+ *
+ * @suites: a statically allocated list of &struct kunit_suite.
+ *
+ * Registers @suites with the test framework. See &struct kunit_suite for
+ * more information.
+ *
+ * When builtin, KUnit tests are all run via executor; this is done
+ * by placing the array of struct kunit_suite * in the .kunit_test_suites
+ * ELF section.
+ *
+ * An alternative is to build the tests as a module. Because modules do not
+ * support multiple initcall()s, we need to initialize an array of suites for a
+ * module.
+ *
+ */
+#define kunit_test_suites(...) \
+ __kunit_test_suites(__UNIQUE_ID(array), \
+ __UNIQUE_ID(suites), \
+ __VA_ARGS__)
#define kunit_test_suite(suite) kunit_test_suites(&suite)
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index fab55649b69a5..c282f02ca066b 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -3,7 +3,8 @@ obj-$(CONFIG_KUNIT) += kunit.o
kunit-objs += test.o \
string-stream.o \
assert.o \
- try-catch.o
+ try-catch.o \
+ executor.o
obj-$(CONFIG_KUNIT_TEST) += kunit-test.o
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
new file mode 100644
index 0000000000000..6429927d598a5
--- /dev/null
+++ b/lib/kunit/executor.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <kunit/test.h>
+
+/*
+ * These symbols point to the .kunit_test_suites section and are defined in
+ * include/asm-generic/vmlinux.lds.h, and consequently must be extern.
+ */
+extern struct kunit_suite * const * const __kunit_suites_start[];
+extern struct kunit_suite * const * const __kunit_suites_end[];
+
+#if IS_BUILTIN(CONFIG_KUNIT)
+
+static int kunit_run_all_tests(void)
+{
+ struct kunit_suite * const * const *suites, * const *subsuite;
+ bool has_test_failed = false;
+
+ for (suites = __kunit_suites_start;
+ suites < __kunit_suites_end;
+ suites++) {
+ for (subsuite = *suites; *subsuite != NULL; subsuite++) {
+ if (kunit_run_tests(*subsuite))
+ has_test_failed = true;
+ }
+ }
+
+ if (has_test_failed)
+ return -EFAULT;
+
+ return 0;
+}
+
+late_initcall(kunit_run_all_tests);
+
+#endif /* IS_BUILTIN(CONFIG_KUNIT) */
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply related
* [PATCH v2 3/7] kunit: test: create a single centralized executor for all tests
From: Brendan Higgins @ 2020-01-30 23:08 UTC (permalink / raw)
To: jdike, richard, anton.ivanov, arnd, keescook, skhan, alan.maguire,
yzaikin, davidgow, akpm, rppt, frowand.list
Cc: gregkh, sboyd, logang, mcgrof, knut.omang, linux-um, linux-arch,
linux-kselftest, kunit-dev, linux-kernel, linux-doc,
Brendan Higgins
In-Reply-To: <20200130230812.142642-1-brendanhiggins@google.com>
From: Alan Maguire <alan.maguire@oracle.com>
Add a centralized executor to dispatch tests rather than relying on
late_initcall to schedule each test suite separately. Centralized
execution is for built-in tests only; modules will execute tests
when loaded.
Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
Co-developed-by: Iurii Zaikin <yzaikin@google.com>
Signed-off-by: Iurii Zaikin <yzaikin@google.com>
Co-developed-by: Brendan Higgins <brendanhiggins@google.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
include/kunit/test.h | 73 +++++++++++++++++++++++++++-----------------
lib/kunit/Makefile | 3 +-
lib/kunit/executor.c | 36 ++++++++++++++++++++++
3 files changed, 83 insertions(+), 29 deletions(-)
create mode 100644 lib/kunit/executor.c
diff --git a/include/kunit/test.h b/include/kunit/test.h
index 2dfb550c6723a..8a02f93a6b505 100644
--- a/include/kunit/test.h
+++ b/include/kunit/test.h
@@ -197,46 +197,63 @@ void kunit_init_test(struct kunit *test, const char *name);
int kunit_run_tests(struct kunit_suite *suite);
-/**
- * kunit_test_suites() - used to register one or more &struct kunit_suite
- * with KUnit.
- *
- * @suites: a statically allocated list of &struct kunit_suite.
- *
- * Registers @suites with the test framework. See &struct kunit_suite for
- * more information.
- *
- * When builtin, KUnit tests are all run as late_initcalls; this means
- * that they cannot test anything where tests must run at a different init
- * phase. One significant restriction resulting from this is that KUnit
- * cannot reliably test anything that is initialize in the late_init phase;
- * another is that KUnit is useless to test things that need to be run in
- * an earlier init phase.
- *
- * An alternative is to build the tests as a module. Because modules
- * do not support multiple late_initcall()s, we need to initialize an
- * array of suites for a module.
- *
- * TODO(brendanhiggins@google.com): Don't run all KUnit tests as
- * late_initcalls. I have some future work planned to dispatch all KUnit
- * tests from the same place, and at the very least to do so after
- * everything else is definitely initialized.
+/*
+ * If a test suite is built-in, module_init() gets translated into
+ * an initcall which we don't want as the idea is that for builtins
+ * the executor will manage execution. So ensure we do not define
+ * module_{init|exit} functions for the builtin case when registering
+ * suites via kunit_test_suites() below.
*/
-#define kunit_test_suites(...) \
- static struct kunit_suite *suites[] = { __VA_ARGS__, NULL}; \
- static int kunit_test_suites_init(void) \
+#ifdef MODULE
+#define kunit_test_suites_for_module(__suites) \
+ static int __init kunit_test_suites_init(void) \
{ \
+ struct kunit_suite *suites[] = (__suites); \
unsigned int i; \
+ \
for (i = 0; suites[i] != NULL; i++) \
kunit_run_tests(suites[i]); \
return 0; \
} \
- late_initcall(kunit_test_suites_init); \
+ module_init(kunit_test_suites_init); \
+ \
static void __exit kunit_test_suites_exit(void) \
{ \
return; \
} \
module_exit(kunit_test_suites_exit)
+#else
+#define kunit_test_suites_for_module(__suites)
+#endif /* MODULE */
+
+#define __kunit_test_suites(unique_array, unique_suites, ...) \
+ static struct kunit_suite *unique_array[] = { __VA_ARGS__, NULL }; \
+ kunit_test_suites_for_module(unique_array); \
+ static struct kunit_suite **unique_suites \
+ __used __aligned(8) __section(.kunit_test_suites) = unique_array
+
+/**
+ * kunit_test_suites() - used to register one or more &struct kunit_suite
+ * with KUnit.
+ *
+ * @suites: a statically allocated list of &struct kunit_suite.
+ *
+ * Registers @suites with the test framework. See &struct kunit_suite for
+ * more information.
+ *
+ * When builtin, KUnit tests are all run via executor; this is done
+ * by placing the array of struct kunit_suite * in the .kunit_test_suites
+ * ELF section.
+ *
+ * An alternative is to build the tests as a module. Because modules do not
+ * support multiple initcall()s, we need to initialize an array of suites for a
+ * module.
+ *
+ */
+#define kunit_test_suites(...) \
+ __kunit_test_suites(__UNIQUE_ID(array), \
+ __UNIQUE_ID(suites), \
+ __VA_ARGS__)
#define kunit_test_suite(suite) kunit_test_suites(&suite)
diff --git a/lib/kunit/Makefile b/lib/kunit/Makefile
index fab55649b69a5..c282f02ca066b 100644
--- a/lib/kunit/Makefile
+++ b/lib/kunit/Makefile
@@ -3,7 +3,8 @@ obj-$(CONFIG_KUNIT) += kunit.o
kunit-objs += test.o \
string-stream.o \
assert.o \
- try-catch.o
+ try-catch.o \
+ executor.o
obj-$(CONFIG_KUNIT_TEST) += kunit-test.o
diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
new file mode 100644
index 0000000000000..6429927d598a5
--- /dev/null
+++ b/lib/kunit/executor.c
@@ -0,0 +1,36 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <kunit/test.h>
+
+/*
+ * These symbols point to the .kunit_test_suites section and are defined in
+ * include/asm-generic/vmlinux.lds.h, and consequently must be extern.
+ */
+extern struct kunit_suite * const * const __kunit_suites_start[];
+extern struct kunit_suite * const * const __kunit_suites_end[];
+
+#if IS_BUILTIN(CONFIG_KUNIT)
+
+static int kunit_run_all_tests(void)
+{
+ struct kunit_suite * const * const *suites, * const *subsuite;
+ bool has_test_failed = false;
+
+ for (suites = __kunit_suites_start;
+ suites < __kunit_suites_end;
+ suites++) {
+ for (subsuite = *suites; *subsuite != NULL; subsuite++) {
+ if (kunit_run_tests(*subsuite))
+ has_test_failed = true;
+ }
+ }
+
+ if (has_test_failed)
+ return -EFAULT;
+
+ return 0;
+}
+
+late_initcall(kunit_run_all_tests);
+
+#endif /* IS_BUILTIN(CONFIG_KUNIT) */
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply related
* [PATCH v2 2/7] arch: um: add linker section for KUnit test suites
From: Brendan Higgins @ 2020-01-30 23:08 UTC (permalink / raw)
To: jdike, richard, anton.ivanov, arnd, keescook, skhan, alan.maguire,
yzaikin, davidgow, akpm, rppt, frowand.list
Cc: gregkh, sboyd, logang, mcgrof, knut.omang, linux-um, linux-arch,
linux-kselftest, kunit-dev, linux-kernel, linux-doc,
Brendan Higgins
In-Reply-To: <20200130230812.142642-1-brendanhiggins@google.com>
Add a linker section to UML where KUnit can put references to its test
suites. This patch is an early step in transitioning to dispatching all
KUnit tests from a centralized executor rather than having each as its
own separate late_initcall.
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
arch/um/include/asm/common.lds.S | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/arch/um/include/asm/common.lds.S b/arch/um/include/asm/common.lds.S
index 7145ce6999822..eab9ceb450efd 100644
--- a/arch/um/include/asm/common.lds.S
+++ b/arch/um/include/asm/common.lds.S
@@ -52,6 +52,10 @@
CON_INITCALL
}
+ .kunit_test_suites : {
+ KUNIT_TEST_SUITES
+ }
+
.exitcall : {
__exitcall_begin = .;
*(.exitcall.exit)
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply related
* [PATCH v2 1/7] vmlinux.lds.h: add linker section for KUnit test suites
From: Brendan Higgins @ 2020-01-30 23:08 UTC (permalink / raw)
To: jdike, richard, anton.ivanov, arnd, keescook, skhan, alan.maguire,
yzaikin, davidgow, akpm, rppt, frowand.list
Cc: gregkh, sboyd, logang, mcgrof, knut.omang, linux-um, linux-arch,
linux-kselftest, kunit-dev, linux-kernel, linux-doc,
Brendan Higgins
In-Reply-To: <20200130230812.142642-1-brendanhiggins@google.com>
Add a linker section where KUnit can put references to its test suites.
This patch is the first step in transitioning to dispatching all KUnit
tests from a centralized executor rather than having each as its own
separate late_initcall.
Co-developed-by: Iurii Zaikin <yzaikin@google.com>
Signed-off-by: Iurii Zaikin <yzaikin@google.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
include/asm-generic/vmlinux.lds.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index e00f41aa8ec4f..99a866f49cb3d 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -856,6 +856,13 @@
KEEP(*(.con_initcall.init)) \
__con_initcall_end = .;
+/* Alignment must be consistent with (kunit_suite *) in include/kunit/test.h */
+#define KUNIT_TEST_SUITES \
+ . = ALIGN(8); \
+ __kunit_suites_start = .; \
+ KEEP(*(.kunit_test_suites)) \
+ __kunit_suites_end = .;
+
#ifdef CONFIG_BLK_DEV_INITRD
#define INIT_RAM_FS \
. = ALIGN(4); \
@@ -1024,6 +1031,7 @@
INIT_CALLS \
CON_INITCALL \
INIT_RAM_FS \
+ KUNIT_TEST_SUITES \
}
#define BSS_SECTION(sbss_align, bss_align, stop_align) \
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply related
* [PATCH v2 1/7] vmlinux.lds.h: add linker section for KUnit test suites
From: Brendan Higgins @ 2020-01-30 23:08 UTC (permalink / raw)
To: jdike, richard, anton.ivanov, arnd, keescook, skhan, alan.maguire,
yzaikin, davidgow, akpm, rppt, frowand.list
Cc: gregkh, sboyd, logang, mcgrof, knut.omang, linux-um, linux-arch,
linux-kselftest, kunit-dev, linux-kernel, linux-doc,
Brendan Higgins
In-Reply-To: <20200130230812.142642-1-brendanhiggins@google.com>
Add a linker section where KUnit can put references to its test suites.
This patch is the first step in transitioning to dispatching all KUnit
tests from a centralized executor rather than having each as its own
separate late_initcall.
Co-developed-by: Iurii Zaikin <yzaikin@google.com>
Signed-off-by: Iurii Zaikin <yzaikin@google.com>
Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
Reviewed-by: Stephen Boyd <sboyd@kernel.org>
---
include/asm-generic/vmlinux.lds.h | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/include/asm-generic/vmlinux.lds.h b/include/asm-generic/vmlinux.lds.h
index e00f41aa8ec4f..99a866f49cb3d 100644
--- a/include/asm-generic/vmlinux.lds.h
+++ b/include/asm-generic/vmlinux.lds.h
@@ -856,6 +856,13 @@
KEEP(*(.con_initcall.init)) \
__con_initcall_end = .;
+/* Alignment must be consistent with (kunit_suite *) in include/kunit/test.h */
+#define KUNIT_TEST_SUITES \
+ . = ALIGN(8); \
+ __kunit_suites_start = .; \
+ KEEP(*(.kunit_test_suites)) \
+ __kunit_suites_end = .;
+
#ifdef CONFIG_BLK_DEV_INITRD
#define INIT_RAM_FS \
. = ALIGN(4); \
@@ -1024,6 +1031,7 @@
INIT_CALLS \
CON_INITCALL \
INIT_RAM_FS \
+ KUNIT_TEST_SUITES \
}
#define BSS_SECTION(sbss_align, bss_align, stop_align) \
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply related
* [PATCH v2 0/7] kunit: create a centralized executor to dispatch all KUnit tests
From: Brendan Higgins @ 2020-01-30 23:08 UTC (permalink / raw)
To: jdike, richard, anton.ivanov, arnd, keescook, skhan, alan.maguire,
yzaikin, davidgow, akpm, rppt, frowand.list
Cc: gregkh, sboyd, logang, mcgrof, knut.omang, linux-um, linux-arch,
linux-kselftest, kunit-dev, linux-kernel, linux-doc,
Brendan Higgins
## TL;DR
This patchset adds a centralized executor to dispatch tests rather than
relying on late_initcall to schedule each test suite separately along
with a couple of new features that depend on it.
## What am I trying to do?
Conceptually, I am trying to provide a mechanism by which test suites
can be grouped together so that they can be reasoned about collectively.
The last two of three patches in this series add features which depend
on this:
PATCH 5/7 Prints out a test plan right before KUnit tests are run[1];
this is valuable because it makes it possible for a test
harness to detect whether the number of tests run matches the
number of tests expected to be run, ensuring that no tests
silently failed.
PATCH 6/7 Add a new kernel command-line option which allows the user to
specify that the kernel poweroff, halt, or reboot after
completing all KUnit tests; this is very handy for running
KUnit tests on UML or a VM so that the UML/VM process exits
cleanly immediately after running all tests without needing a
special initramfs.
In addition, by dispatching tests from a single location, we can
guarantee that all KUnit tests run after late_init is complete, which
was a concern during the initial KUnit patchset review (this has not
been a problem in practice, but resolving with certainty is nevertheless
desirable).
Other use cases for this exist, but the above features should provide an
idea of the value that this could provide.
Alan Maguire (1):
kunit: test: create a single centralized executor for all tests
Brendan Higgins (5):
vmlinux.lds.h: add linker section for KUnit test suites
arch: um: add linker section for KUnit test suites
init: main: add KUnit to kernel init
kunit: test: add test plan to KUnit TAP format
Documentation: Add kunit_shutdown to kernel-parameters.txt
David Gow (1):
kunit: Add 'kunit_shutdown' option
.../admin-guide/kernel-parameters.txt | 7 ++
arch/um/include/asm/common.lds.S | 4 +
include/asm-generic/vmlinux.lds.h | 8 ++
include/kunit/test.h | 82 ++++++++++++-------
init/main.c | 4 +
lib/kunit/Makefile | 3 +-
lib/kunit/executor.c | 71 ++++++++++++++++
lib/kunit/test.c | 11 ---
tools/testing/kunit/kunit_kernel.py | 2 +-
tools/testing/kunit/kunit_parser.py | 76 ++++++++++++++---
.../test_is_test_passed-all_passed.log | 1 +
.../test_data/test_is_test_passed-crash.log | 1 +
.../test_data/test_is_test_passed-failure.log | 1 +
13 files changed, 217 insertions(+), 54 deletions(-)
create mode 100644 lib/kunit/executor.c
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply
* [PATCH v2 0/7] kunit: create a centralized executor to dispatch all KUnit tests
From: Brendan Higgins @ 2020-01-30 23:08 UTC (permalink / raw)
To: jdike, richard, anton.ivanov, arnd, keescook, skhan, alan.maguire,
yzaikin, davidgow, akpm, rppt, frowand.list
Cc: gregkh, sboyd, logang, mcgrof, knut.omang, linux-um, linux-arch,
linux-kselftest, kunit-dev, linux-kernel, linux-doc,
Brendan Higgins
## TL;DR
This patchset adds a centralized executor to dispatch tests rather than
relying on late_initcall to schedule each test suite separately along
with a couple of new features that depend on it.
## What am I trying to do?
Conceptually, I am trying to provide a mechanism by which test suites
can be grouped together so that they can be reasoned about collectively.
The last two of three patches in this series add features which depend
on this:
PATCH 5/7 Prints out a test plan right before KUnit tests are run[1];
this is valuable because it makes it possible for a test
harness to detect whether the number of tests run matches the
number of tests expected to be run, ensuring that no tests
silently failed.
PATCH 6/7 Add a new kernel command-line option which allows the user to
specify that the kernel poweroff, halt, or reboot after
completing all KUnit tests; this is very handy for running
KUnit tests on UML or a VM so that the UML/VM process exits
cleanly immediately after running all tests without needing a
special initramfs.
In addition, by dispatching tests from a single location, we can
guarantee that all KUnit tests run after late_init is complete, which
was a concern during the initial KUnit patchset review (this has not
been a problem in practice, but resolving with certainty is nevertheless
desirable).
Other use cases for this exist, but the above features should provide an
idea of the value that this could provide.
Alan Maguire (1):
kunit: test: create a single centralized executor for all tests
Brendan Higgins (5):
vmlinux.lds.h: add linker section for KUnit test suites
arch: um: add linker section for KUnit test suites
init: main: add KUnit to kernel init
kunit: test: add test plan to KUnit TAP format
Documentation: Add kunit_shutdown to kernel-parameters.txt
David Gow (1):
kunit: Add 'kunit_shutdown' option
.../admin-guide/kernel-parameters.txt | 7 ++
arch/um/include/asm/common.lds.S | 4 +
include/asm-generic/vmlinux.lds.h | 8 ++
include/kunit/test.h | 82 ++++++++++++-------
init/main.c | 4 +
lib/kunit/Makefile | 3 +-
lib/kunit/executor.c | 71 ++++++++++++++++
lib/kunit/test.c | 11 ---
tools/testing/kunit/kunit_kernel.py | 2 +-
tools/testing/kunit/kunit_parser.py | 76 ++++++++++++++---
.../test_is_test_passed-all_passed.log | 1 +
.../test_data/test_is_test_passed-crash.log | 1 +
.../test_data/test_is_test_passed-failure.log | 1 +
13 files changed, 217 insertions(+), 54 deletions(-)
create mode 100644 lib/kunit/executor.c
--
2.25.0.341.g760bfbb309-goog
^ permalink raw reply
* Re: [PATCH v1 7/7] Documentation: Add kunit_shutdown to kernel-parameters.txt
From: Brendan Higgins @ 2020-01-30 23:04 UTC (permalink / raw)
To: Stephen Boyd
Cc: Andrew Morton, Alan Maguire, Anton Ivanov, Arnd Bergmann,
David Gow, Frank Rowand, Jeff Dike, Kees Cook, Richard Weinberger,
rppt, Shuah Khan, Iurii Zaikin, Greg KH, Logan Gunthorpe,
Luis Chamberlain, Knut Omang, linux-um, linux-arch,
open list:KERNEL SELFTEST FRAMEWORK,
KUnit Development <kunit-d>
In-Reply-To: <20200129062709.5B2A22067C@mail.kernel.org>
On Tue, Jan 28, 2020 at 10:27 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2020-01-27 23:20:02)
> > Add kunit_shutdown, an option to specify that the kernel shutsdown after
> > running KUnit tests, to the kernel-parameters.txt documentation.
> >
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > ---
> > Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index ade4e6ec23e03..0472b02ce16bb 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -2054,6 +2054,13 @@
> > 0: force disabled
> > 1: force enabled
> >
> > + kunit_shutdown [KERNEL UNIT TESTING FRAMEWORK] Shutdown kernel after
> > + running tests.
> > + Default: (flag not present) don't shutdown
> > + poweroff: poweroff the kernel after running tests.
> > + halt: halt the kernel after running tests.
> > + reboot: reboot the kernel after running tests.
>
> Maybe drop the full stops on the short descriptions.
Will fix.
> Otherwise,
>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>
Thanks!
^ permalink raw reply
* Re: [PATCH v1 7/7] Documentation: Add kunit_shutdown to kernel-parameters.txt
From: Brendan Higgins @ 2020-01-30 23:04 UTC (permalink / raw)
To: Stephen Boyd
Cc: Andrew Morton, Alan Maguire, Anton Ivanov, Arnd Bergmann,
David Gow, Frank Rowand, Jeff Dike, Kees Cook, Richard Weinberger,
rppt, Shuah Khan, Iurii Zaikin, Greg KH, Logan Gunthorpe,
Luis Chamberlain, Knut Omang, linux-um, linux-arch,
open list:KERNEL SELFTEST FRAMEWORK, KUnit Development,
Linux Kernel Mailing List, open list:DOCUMENTATION
In-Reply-To: <20200129062709.5B2A22067C@mail.kernel.org>
On Tue, Jan 28, 2020 at 10:27 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2020-01-27 23:20:02)
> > Add kunit_shutdown, an option to specify that the kernel shutsdown after
> > running KUnit tests, to the kernel-parameters.txt documentation.
> >
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > ---
> > Documentation/admin-guide/kernel-parameters.txt | 7 +++++++
> > 1 file changed, 7 insertions(+)
> >
> > diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> > index ade4e6ec23e03..0472b02ce16bb 100644
> > --- a/Documentation/admin-guide/kernel-parameters.txt
> > +++ b/Documentation/admin-guide/kernel-parameters.txt
> > @@ -2054,6 +2054,13 @@
> > 0: force disabled
> > 1: force enabled
> >
> > + kunit_shutdown [KERNEL UNIT TESTING FRAMEWORK] Shutdown kernel after
> > + running tests.
> > + Default: (flag not present) don't shutdown
> > + poweroff: poweroff the kernel after running tests.
> > + halt: halt the kernel after running tests.
> > + reboot: reboot the kernel after running tests.
>
> Maybe drop the full stops on the short descriptions.
Will fix.
> Otherwise,
>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>
Thanks!
^ permalink raw reply
* Re: [PATCH v1 6/7] kunit: Add 'kunit_shutdown' option
From: Brendan Higgins @ 2020-01-30 22:56 UTC (permalink / raw)
To: Stephen Boyd
Cc: Andrew Morton, Alan Maguire, Anton Ivanov, Arnd Bergmann,
David Gow, Frank Rowand, Jeff Dike, Kees Cook, Richard Weinberger,
rppt, Shuah Khan, Iurii Zaikin, Greg KH, Logan Gunthorpe,
Luis Chamberlain, Knut Omang, linux-um, linux-arch,
open list:KERNEL SELFTEST FRAMEWORK, KUnit Development,
Linux Kernel Mailing List, open list:DOCUMENTATION
In-Reply-To: <20200129063307.19CB4206F0@mail.kernel.org>
On Tue, Jan 28, 2020 at 10:33 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2020-01-27 23:20:01)
> > From: David Gow <davidgow@google.com>
> >
> > Add a new kernel command-line option, 'kunit_shutdown', which allows the
> > user to specify that the kernel poweroff, halt, or reboot after
> > completing all KUnit tests; this is very handy for running KUnit tests
> > on UML or a VM so that the UML/VM process exits cleanly immediately
> > after running all tests without needing a special initramfs.
> >
> > Signed-off-by: David Gow <davidgow@google.com>
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > ---
>
> Two nitpicks below
>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>
> > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > index 7fd16feff157e..d3ec1265a72fd 100644
> > --- a/lib/kunit/executor.c
> > +++ b/lib/kunit/executor.c
> > @@ -1,6 +1,7 @@
> > // SPDX-License-Identifier: GPL-2.0
> >
> > #include <kunit/test.h>
> > +#include <linux/reboot.h>
>
> Should this include come before kunit/test.h? I imagine the order of
> includes would be linux, kunit, local?
I think some reviewers/maintainers want them all to be alphabetical.
So I have probably done it both ways in the past. Will fix.
> >
> > /*
> > * These symbols point to the .kunit_test_suites section and are defined in
> > @@ -11,6 +12,23 @@ extern struct kunit_suite * const * const __kunit_suites_end[];
> >
> > #if IS_BUILTIN(CONFIG_KUNIT)
> >
> > +static char *kunit_shutdown;
> > +core_param(kunit_shutdown, kunit_shutdown, charp, 0644);
> > +
> > +static void kunit_handle_shutdown(void)
> > +{
> > + if (!kunit_shutdown)
> > + return;
> > +
> > + if (!strcmp(kunit_shutdown, "poweroff")) {
> > + kernel_power_off();
> > + } else if (!strcmp(kunit_shutdown, "halt")) {
> > + kernel_halt();
> > + } else if (!strcmp(kunit_shutdown, "reboot")) {
> > + kernel_restart(NULL);
> > + }
>
> Kernel style would be to not have braces on single line if statements.
Whoops. Sometimes I forget :-)
> > +}
> > +
> > static void kunit_print_tap_header(void)
> > {
> > struct kunit_suite * const * const *suites, * const *subsuite;
Thanks!
^ permalink raw reply
* Re: [PATCH v1 6/7] kunit: Add 'kunit_shutdown' option
From: Brendan Higgins @ 2020-01-30 22:56 UTC (permalink / raw)
To: Stephen Boyd
Cc: Andrew Morton, Alan Maguire, Anton Ivanov, Arnd Bergmann,
David Gow, Frank Rowand, Jeff Dike, Kees Cook, Richard Weinberger,
rppt, Shuah Khan, Iurii Zaikin, Greg KH, Logan Gunthorpe,
Luis Chamberlain, Knut Omang, linux-um, linux-arch,
open list:KERNEL SELFTEST FRAMEWORK,
KUnit Development <kunit-d>
In-Reply-To: <20200129063307.19CB4206F0@mail.kernel.org>
On Tue, Jan 28, 2020 at 10:33 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2020-01-27 23:20:01)
> > From: David Gow <davidgow@google.com>
> >
> > Add a new kernel command-line option, 'kunit_shutdown', which allows the
> > user to specify that the kernel poweroff, halt, or reboot after
> > completing all KUnit tests; this is very handy for running KUnit tests
> > on UML or a VM so that the UML/VM process exits cleanly immediately
> > after running all tests without needing a special initramfs.
> >
> > Signed-off-by: David Gow <davidgow@google.com>
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > ---
>
> Two nitpicks below
>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>
> > diff --git a/lib/kunit/executor.c b/lib/kunit/executor.c
> > index 7fd16feff157e..d3ec1265a72fd 100644
> > --- a/lib/kunit/executor.c
> > +++ b/lib/kunit/executor.c
> > @@ -1,6 +1,7 @@
> > // SPDX-License-Identifier: GPL-2.0
> >
> > #include <kunit/test.h>
> > +#include <linux/reboot.h>
>
> Should this include come before kunit/test.h? I imagine the order of
> includes would be linux, kunit, local?
I think some reviewers/maintainers want them all to be alphabetical.
So I have probably done it both ways in the past. Will fix.
> >
> > /*
> > * These symbols point to the .kunit_test_suites section and are defined in
> > @@ -11,6 +12,23 @@ extern struct kunit_suite * const * const __kunit_suites_end[];
> >
> > #if IS_BUILTIN(CONFIG_KUNIT)
> >
> > +static char *kunit_shutdown;
> > +core_param(kunit_shutdown, kunit_shutdown, charp, 0644);
> > +
> > +static void kunit_handle_shutdown(void)
> > +{
> > + if (!kunit_shutdown)
> > + return;
> > +
> > + if (!strcmp(kunit_shutdown, "poweroff")) {
> > + kernel_power_off();
> > + } else if (!strcmp(kunit_shutdown, "halt")) {
> > + kernel_halt();
> > + } else if (!strcmp(kunit_shutdown, "reboot")) {
> > + kernel_restart(NULL);
> > + }
>
> Kernel style would be to not have braces on single line if statements.
Whoops. Sometimes I forget :-)
> > +}
> > +
> > static void kunit_print_tap_header(void)
> > {
> > struct kunit_suite * const * const *suites, * const *subsuite;
Thanks!
^ permalink raw reply
* Re: [PATCH v1 4/7] init: main: add KUnit to kernel init
From: Brendan Higgins @ 2020-01-30 22:50 UTC (permalink / raw)
To: Stephen Boyd
Cc: Andrew Morton, Alan Maguire, Anton Ivanov, Arnd Bergmann,
David Gow, Frank Rowand, Jeff Dike, Kees Cook, Richard Weinberger,
rppt, Shuah Khan, Iurii Zaikin, Greg KH, Logan Gunthorpe,
Luis Chamberlain, Knut Omang, linux-um, linux-arch,
open list:KERNEL SELFTEST FRAMEWORK, KUnit Development,
Linux Kernel Mailing List, open list:DOCUMENTATION
In-Reply-To: <20200129063836.6C2A62064C@mail.kernel.org>
On Tue, Jan 28, 2020 at 10:38 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2020-01-27 23:19:59)
> > Remove KUnit from init calls entirely, instead call directly from
> > kernel_init().
> >
> > Co-developed-by: Alan Maguire <alan.maguire@oracle.com>
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > ---
>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>
> Although, why can't it be squashed with the previous patch?
I think that this is pretty much the smallest logical change that
doesn't touch just KUnit. I figured it might make it easier for people
not interested in KUnit what changes I am making to init. I assume
that people don't touch init willy-nilly, right?
^ permalink raw reply
* Re: [PATCH v1 4/7] init: main: add KUnit to kernel init
From: Brendan Higgins @ 2020-01-30 22:50 UTC (permalink / raw)
To: Stephen Boyd
Cc: Andrew Morton, Alan Maguire, Anton Ivanov, Arnd Bergmann,
David Gow, Frank Rowand, Jeff Dike, Kees Cook, Richard Weinberger,
rppt, Shuah Khan, Iurii Zaikin, Greg KH, Logan Gunthorpe,
Luis Chamberlain, Knut Omang, linux-um, linux-arch,
open list:KERNEL SELFTEST FRAMEWORK,
KUnit Development <kunit-d>
In-Reply-To: <20200129063836.6C2A62064C@mail.kernel.org>
On Tue, Jan 28, 2020 at 10:38 PM Stephen Boyd <sboyd@kernel.org> wrote:
>
> Quoting Brendan Higgins (2020-01-27 23:19:59)
> > Remove KUnit from init calls entirely, instead call directly from
> > kernel_init().
> >
> > Co-developed-by: Alan Maguire <alan.maguire@oracle.com>
> > Signed-off-by: Alan Maguire <alan.maguire@oracle.com>
> > Signed-off-by: Brendan Higgins <brendanhiggins@google.com>
> > ---
>
> Reviewed-by: Stephen Boyd <sboyd@kernel.org>
>
> Although, why can't it be squashed with the previous patch?
I think that this is pretty much the smallest logical change that
doesn't touch just KUnit. I figured it might make it easier for people
not interested in KUnit what changes I am making to init. I assume
that people don't touch init willy-nilly, right?
^ permalink raw reply
* Re: [PATCH v8 4/5] locking/qspinlock: Introduce starvation avoidance into CNA
From: Alex Kogan @ 2020-01-30 22:05 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Waiman Long, linux, Ingo Molnar, Will Deacon, Arnd Bergmann,
linux-arch, linux-arm-kernel, linux-kernel, Thomas Gleixner,
Borislav Petkov, hpa, x86, Hanjun Guo, Jan Glauber,
Steven Sistare, Daniel Jordan, dave.dice
In-Reply-To: <20200125111931.GW11457@worktop.programming.kicks-ass.net>
> On Jan 25, 2020, at 6:19 AM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Fri, Jan 24, 2020 at 01:19:05PM -0500, Alex Kogan wrote:
>
>> Is there a lightweight way to identify such a “prioritized” thread?
>
> No; people might for instance care about tail latencies between their
> identically spec'ed worker tasks.
I would argue that those users need to tune/reduce the intra-node handoff
threshold for their needs. Or disable CNA altogether.
In general, Peter, seems like you are not on board with the way Longman
suggested to handle prioritized threads. Am I right?
Thanks,
— Alex
^ permalink raw reply
* Re: [PATCH v7 3/5] locking/qspinlock: Introduce CNA into the slow path of qspinlock
From: Alex Kogan @ 2020-01-30 22:01 UTC (permalink / raw)
To: Peter Zijlstra
Cc: linux, Ingo Molnar, Will Deacon, Arnd Bergmann, Waiman Long,
linux-arch, linux-arm-kernel, linux-kernel, Thomas Gleixner,
Borislav Petkov, hpa, x86, Hanjun Guo, Jan Glauber,
Steven Sistare, Daniel Jordan, dave.dice
In-Reply-To: <20200122170456.GY14879@hirez.programming.kicks-ass.net>
Hi, Peter.
It looks good — thanks for your review!
I have a couple of comments and suggestions.
Please, see below.
> On Jan 22, 2020, at 12:04 PM, Peter Zijlstra <peterz@infradead.org> wrote:
>
> On Wed, Jan 22, 2020 at 10:51:27AM +0100, Peter Zijlstra wrote:
>> On Tue, Jan 21, 2020 at 09:29:19PM +0100, Peter Zijlstra wrote:
>>>
>>> various notes and changes in the below.
>>
>> Also, sorry for replying to v7 and v8, I forgot to refresh email on the
>> laptop and had spotty cell service last night and only found v7 in that
>> mailbox.
>>
>> Afaict none of the things I commented on were fundamentally changed
>> though.
Nothing fundamental, but some things you may find objectionable, like
the names of new enum elements :)
>
> But since I was editing, here is the latest version..
>
> ---
>
> Index: linux-2.6/kernel/locking/qspinlock_cna.h
> ===================================================================
> --- /dev/null
> +++ linux-2.6/kernel/locking/qspinlock_cna.h
> @@ -0,0 +1,261 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _GEN_CNA_LOCK_SLOWPATH
> +#error "do not include this file"
> +#endif
> +
> +#include <linux/topology.h>
> +
> +/*
> + * Implement a NUMA-aware version of MCS (aka CNA, or compact NUMA-aware lock).
> + *
> + * In CNA, spinning threads are organized in two queues, a primary queue for
> + * threads running on the same NUMA node as the current lock holder, and a
> + * secondary queue for threads running on other nodes. Schematically, it looks
> + * like this:
> + *
> + * cna_node
> + * +----------+ +--------+ +--------+
> + * |mcs:next | --> |mcs:next| --> ... |mcs:next| --> NULL [Primary queue]
> + * |mcs:locked| -. +--------+ +--------+
> + * +----------+ |
> + * `----------------------.
> + * v
> + * +--------+ +--------+
> + * |mcs:next| --> ... |mcs:next| [Secondary queue]
> + * +--------+ +--------+
> + * ^ |
> + * `--------------------'
> + *
> + * N.B. locked := 1 if secondary queue is absent. Otherwise, it contains the
> + * encoded pointer to the tail of the secondary queue, which is organized as a
> + * circular list.
> + *
> + * After acquiring the MCS lock and before acquiring the spinlock, the lock
> + * holder scans the primary queue looking for a thread running on the same node
> + * (pre-scan). If found (call it thread T), all threads in the primary queue
> + * between the current lock holder and T are moved to the end of the secondary
> + * queue. If such T is not found, we make another scan of the primary queue
> + * when unlocking the MCS lock (post-scan), starting at the node where pre-scan
> + * stopped. If both scans fail to find such T, the MCS lock is passed to the
> + * first thread in the secondary queue. If the secondary queue is empty, the
> + * lock is passed to the next thread in the primary queue.
> + *
> + * For more details, see https://urldefense.proofpoint.com/v2/url?u=https-3A__arxiv.org_abs_1810.05600&d=DwIBAg&c=RoP1YumCXCgaWHvlZYR8PZh8Bv7qIrMUB65eapI_JnE&r=Hvhk3F4omdCk-GE1PTOm3Kn0A7ApWOZ2aZLTuVxFK4k&m=49NdSlRXQxlPifNqUk_E7p7Q-nJ0_HP9fkF_F_GsC_Y&s=0c25gE6r6cKpOC3J0KaPnnkUd4wFXwPNilBflDnNOSQ&e= .
> + *
> + * Authors: Alex Kogan <alex.kogan@oracle.com>
> + * Dave Dice <dave.dice@oracle.com>
> + */
> +
> +struct cna_node {
> + struct mcs_spinlock mcs;
> + int numa_node;
> + u32 encoded_tail; /* self */
> + u32 partial_order;
I will not argue about names, just point out that I think pre_scan_result
is more self-explanatory.
> +};
> +
> +static void __init cna_init_nodes_per_cpu(unsigned int cpu)
> +{
> + struct mcs_spinlock *base = per_cpu_ptr(&qnodes[0].mcs, cpu);
> + int numa_node = cpu_to_node(cpu);
> + int i;
> +
> + for (i = 0; i < MAX_NODES; i++) {
> + struct cna_node *cn = (struct cna_node *)grab_mcs_node(base, i);
> +
> + cn->numa_node = numa_node;
> + cn->encoded_tail = encode_tail(cpu, i);
> + /*
> + * @encoded_tail has to be larger than 1, so we do not confuse
> + * it with other valid values for @locked or @partial_order
> + * (0 or 1)
> + */
> + WARN_ON(cn->encoded_tail <= 1);
> + }
> +}
> +
> +static int __init cna_init_nodes(void)
> +{
> + unsigned int cpu;
> +
> + /*
> + * this will break on 32bit architectures, so we restrict
> + * the use of CNA to 64bit only (see arch/x86/Kconfig)
> + */
> + BUILD_BUG_ON(sizeof(struct cna_node) > sizeof(struct qnode));
> + /* we store an ecoded tail word in the node's @locked field */
> + BUILD_BUG_ON(sizeof(u32) > sizeof(unsigned int));
> +
> + for_each_possible_cpu(cpu)
> + cna_init_nodes_per_cpu(cpu);
> +
> + return 0;
> +}
> +early_initcall(cna_init_nodes);
> +
> +/*
> + * cna_splice_tail -- splice nodes in the primary queue between [first, last]
> + * onto the secondary queue.
> + */
> +static void cna_splice_tail(struct mcs_spinlock *node,
> + struct mcs_spinlock *first,
> + struct mcs_spinlock *last)
> +{
> + /* remove [first,last] */
> + node->next = last->next;
> +
> + /* stick [first,last] on the secondary queue tail */
> + if (node->locked <= 1) { /* if secondary queue is empty */
> + /* create secondary queue */
> + last->next = first;
> + } else {
> + /* add to the tail of the secondary queue */
> + struct mcs_spinlock *tail_2nd = decode_tail(node->locked);
> + struct mcs_spinlock *head_2nd = tail_2nd->next;
> +
> + tail_2nd->next = first;
> + last->next = head_2nd;
> + }
> +
> + node->locked = ((struct cna_node *)last)->encoded_tail;
> +}
> +
> +/*
> + * cna_order_queue - scan the primary queue looking for the first lock node on
> + * the same NUMA node as the lock holder and move any skipped nodes onto the
> + * secondary queue.
Oh man, you took out the ascii figure I was working so hard to put in. Oh well.
> + *
> + * Returns 0 if a matching node is found; otherwise return the encoded pointer
> + * to the last element inspected (such that a subsequent scan can continue there).
> + *
> + * The worst case complexity of the scan is O(n), where n is the number
> + * of current waiters. However, the amortized complexity is close to O(1),
> + * as the immediate successor is likely to be running on the same node once
> + * threads from other nodes are moved to the secondary queue.
> + *
> + * XXX does not compute; given equal contention it should average to O(nr_nodes).
Let me try to convince you. Under contention, the immediate waiter would be
a local one. So the scan would typically take O(1) steps. We need to consider
the extra work/steps we take to move nodes to the secondary queue. The
number of such nodes is O(n) (to be more precise, it is O(n-m), where m
is nr_cpus_per_node), and we spend a constant amount of work, per node,
to scan those nodes and move them to the secondary queue. So in 2^thresholds
lock handovers, we scan 2^thresholds x 1 + n-m nodes. Assuming
2^thresholds > n, the amortized complexity of this function then is O(1).
> + */
> +static u32 cna_order_queue(struct mcs_spinlock *node,
> + struct mcs_spinlock *iter)
> +{
> + struct cna_node *cni = (struct cna_node *)READ_ONCE(iter->next);
> + struct cna_node *cn = (struct cna_node *)node;
> + int nid = cn->numa_node;
> + struct cna_node *last;
> +
> + /* find any next waiter on 'our' NUMA node */
> + for (last = cn;
> + cni && cni->numa_node != nid;
> + last = cni, cni = (struct cna_node *)READ_ONCE(cni->mcs.next))
> + ;
> +
> + if (!cna)
Should be ‘cni’
> + return last->encoded_tail; /* continue from here */
> +
> + if (last != cn) /* did we skip any waiters? */
> + cna_splice_tail(node, node->next, (struct mcs_spinlock *)last);
> +
> + return 0;
> +}
> +
> +/*
> + * cna_splice_head -- splice the entire secondary queue onto the head of the
> + * primary queue.
> + */
> +static cna_splice_head(struct qspinlock *lock, u32 val,
> + struct mcs_spinlock *node, struct mcs_spinlock *next)
Missing return value type (struct mcs_spinlock *).
> +{
> + struct mcs_spinlock *head_2nd, *tail_2nd;
> +
> + tail_2nd = decode_tail(node->locked);
> + head_2nd = tail_2nd->next;
I understand that you are trying to avoid repeating those two lines
in both places this function is called from, but you do it at the cost
of adding the following unnecessary if-statement, and in general this function
looks clunky.
Maybe move those two lines into a separate function, e.g.,
static struct mcs_spinlock *cna_extract_2dary_head_tail(unsigned int locked,
struct mcs_spinlock **tail_2nd)
and then call this function from cna_pass_lock(), while here you can do:
struct mcs_spinlock *head_2nd, *tail_2nd;
head_2nd = cna_extract_2dary_head_tail(lock, &tail_2nd);
u32 new = ((struct cna_node *)tail_2nd)->encoded_tail | _Q_LOCKED_VAL;
…
> +
> + if (lock) {
> + u32 new = ((struct cna_node *)tail_2nd)->encoded_tail | _Q_LOCKED_VAL;
> + if (!atomic_try_cmpxchg_relaxed(&lock->val, &val, new))
> + return NULL;
> +
> + /*
> + * The moment we've linked the primary tail we can race with
> + * the WRITE_ONCE(prev->next, node) store from new waiters.
> + * That store must win.
> + */
> + cmpxchg_relaxed(&tail_2nd->next, head_2nd, next);
> + } else {
> + tail_2nd->next = next;
> + }
> +
> + return head_2nd;
> +}
> +
> +/* Abuse the pv_wait_head_or_lock() hook to get some work done */
> +static __always_inline u32 cna_wait_head_or_lock(struct qspinlock *lock,
> + struct mcs_spinlock *node)
> +{
> + struct cna_node *cn = (struct cna_node *)node;
> +
> + /*
> + * Try and put the time otherwise spend spin waiting on
> + * _Q_LOCKED_PENDING_MASK to use by sorting our lists.
> + */
> + cn->partial_order = cna_order_queue(node, node);
> +
> + return 0; /* we lied; we didn't wait, go do so now */
> +}
> +
> +static inline bool cna_try_clear_tail(struct qspinlock *lock, u32 val,
> + struct mcs_spinlock *node)
> +{
> + struct mcs_spinlock *next;
> +
> + /*
> + * We're here because the primary queue is empty; check the secondary
> + * queue for remote waiters.
> + */
> + if (node->locked > 1) {
> + /*
> + * When there are waiters on the secondary queue move them back
> + * onto the primary queue and let them rip.
> + */
> + next = cna_splice_head(lock, val, node, NULL);
> + if (next) {
And, again, this if-statement is here only because you moved the rest of the code
into cna_splice_head(). Perhaps, with cna_extract_2dary_head_tail() you can
bring that code back?
> + arch_mcs_pass_lock(&head_2nd->locked, 1);
Should be next->locked. Better yet, ‘next' should be called ‘head_2nd’.
> + return true;
> + }
> +
> + return false;
> + }
> +
> + /* Both queues empty. */
> + return __try_clear_tail(lock, val, node);
> +}
> +
> +static inline void cna_pass_lock(struct mcs_spinlock *node,
> + struct mcs_spinlock *next)
> +{
> + struct cna_node *cn = (struct cna_node *)node;
> + u32 partial_order = cn->partial_order;
> + u32 val = _Q_LOCKED_VAL;
> +
> + /* cna_wait_head_or_lock() left work for us. */
> + if (partial_order) {
> + partial_order = cna_order_queue(node, decode_tail(partial_order));
> +
> + if (!partial_order) {
> + /*
> + * We found a local waiter; reload @next in case we called
> + * cna_order_queue() above.
> + */
> + next = node->next;
> + val |= node->locked; /* preseve secondary queue */
This is wrong. node->locked can be 0, 1 or an encoded tail at this point, and
the latter case will be handled incorrectly.
Should be
if (node->locked) val = node->locked;
instead.
> +
> + } else if (node->locked > 1) {
> + /*
> + * When there are no local waiters on the primary queue, splice
> + * the secondary queue onto the primary queue and pass the lock
> + * to the longest waiting remote waiter.
> + */
> + next = cna_splice_head(NULL, 0, node, next);
> + }
> +
> + arch_mcs_pass_lock(&next->locked, val);
> +}
Regards,
— Alex
^ permalink raw reply
* Re: [PATCH v18 00/24] selftests, powerpc, x86: Memory Protection Keys
From: Dave Hansen @ 2020-01-30 21:51 UTC (permalink / raw)
To: Sandipan Das, shuah, skhan, linux-kselftest
Cc: linux-arch, linuxppc-dev, x86, linux-mm, fweimer, linuxram,
mhocko, mingo, aneesh.kumar, bauerman, msuchanek, mpe
In-Reply-To: <cover.1580365432.git.sandipan@linux.ibm.com>
On 1/29/20 10:36 PM, Sandipan Das wrote:
> v18:
> (1) Fixed issues with x86 multilib builds based on
> feedback from Dave.
> (2) Moved patch 2 to the end of the series.
These (finally) build and run successfully for me on an x86 system with
protection keys. Feel free to add my Tested-by, and Acked-by.
FWIW, I don't think look perfect, but my standards are lower for
selftests/ than normal kernel code. :)
^ permalink raw reply
* Re: [PATCH v18 00/24] selftests, powerpc, x86: Memory Protection Keys
From: Dave Hansen @ 2020-01-30 21:51 UTC (permalink / raw)
To: Sandipan Das, shuah, skhan, linux-kselftest
Cc: linux-arch, linuxppc-dev, x86, linux-mm, fweimer, linuxram,
mhocko, mingo, aneesh.kumar, bauerman, msuchanek, mpe
In-Reply-To: <cover.1580365432.git.sandipan@linux.ibm.com>
On 1/29/20 10:36 PM, Sandipan Das wrote:
> v18:
> (1) Fixed issues with x86 multilib builds based on
> feedback from Dave.
> (2) Moved patch 2 to the end of the series.
These (finally) build and run successfully for me on an x86 system with
protection keys. Feel free to add my Tested-by, and Acked-by.
FWIW, I don't think look perfect, but my standards are lower for
selftests/ than normal kernel code. :)
^ permalink raw reply
* Re: [kernel-hardening] [PATCH 09/38] usercopy: Mark kmalloc caches as usercopy caches
From: Kees Cook @ 2020-01-30 19:23 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Christoph Hellwig, Christopher Lameter, Jiri Slaby,
Julian Wiedmann, Ursula Braun, Alexander Viro, linux-kernel,
David Windsor, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, linux-mm, linux-xfs, Linus Torvalds,
Andy Lutomirski, David S. Miller, Laura Abbott, Mark Rutland,
Martin K. Petersen, Paolo Bonzini, Christoffer Dall,
Dave Kleikamp, Jan Kara, Luis de Bethencourt, Marc Zyngier,
Rik van Riel, Matthew Garrett, linux-fsdevel, linux-arch, netdev,
kernel-hardening, Vlastimil Babka, Michal Kubecek
In-Reply-To: <771c5511-c5ab-3dd1-d938-5dbc40396daa@de.ibm.com>
On Wed, Jan 29, 2020 at 06:19:56PM +0100, Christian Borntraeger wrote:
> On 29.01.20 18:09, Christoph Hellwig wrote:
> > On Wed, Jan 29, 2020 at 06:07:14PM +0100, Christian Borntraeger wrote:
> >>> DMA can be done to NORMAL memory as well.
> >>
> >> Exactly.
> >> I think iucv uses GFP_DMA because z/VM needs those buffers to reside below 2GB (which is ZONA_DMA for s390).
> >
> > The normal way to allocate memory with addressing limits would be to
> > use dma_alloc_coherent and friends. Any chance to switch iucv over to
> > that? Or is there no device associated with it?
>
> There is not necessarily a device for that. It is a hypervisor interface (an
> instruction that is interpreted by z/VM). We do have the netiucv driver that
> creates a virtual nic, but there is also AF_IUCV which works without a device.
>
> But back to the original question: If we mark kmalloc caches as usercopy caches,
> we should do the same for DMA kmalloc caches. As outlined by Christoph, this has
> nothing to do with device DMA.
Hm, looks like it's allocated from the low 16MB. Seems like poor naming!
:) There seems to be a LOT of stuff using GFP_DMA, and it seems unlikely
those are all expecting low addresses?
Since this has only been a problem on s390, should just s390 gain the
weakening of the usercopy restriction? Something like:
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 1907cb2903c7..c5bbc141f20b 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -1303,7 +1303,9 @@ void __init create_kmalloc_caches(slab_flags_t flags)
kmalloc_caches[KMALLOC_DMA][i] = create_kmalloc_cache(
kmalloc_info[i].name[KMALLOC_DMA],
kmalloc_info[i].size,
- SLAB_CACHE_DMA | flags, 0, 0);
+ SLAB_CACHE_DMA | flags, 0,
+ IS_ENABLED(CONFIG_S390) ?
+ kmalloc_info[i].size : 0);
}
}
#endif
--
Kees Cook
^ permalink raw reply related
* Re: [kernel-hardening] [PATCH 09/38] usercopy: Mark kmalloc caches as usercopy caches
From: Kees Cook @ 2020-01-30 19:23 UTC (permalink / raw)
To: Christian Borntraeger
Cc: Christoph Hellwig, Christopher Lameter, Jiri Slaby,
Julian Wiedmann, Ursula Braun, Alexander Viro, linux-kernel,
David Windsor, Pekka Enberg, David Rientjes, Joonsoo Kim,
Andrew Morton, linux-mm, linux-xfs, Linus Torvalds,
Andy Lutomirski, David S. Miller, Laura Abbott, Mark Rutland,
Martin K. Petersen, Paolo Bonzini <pbon>
In-Reply-To: <771c5511-c5ab-3dd1-d938-5dbc40396daa@de.ibm.com>
On Wed, Jan 29, 2020 at 06:19:56PM +0100, Christian Borntraeger wrote:
> On 29.01.20 18:09, Christoph Hellwig wrote:
> > On Wed, Jan 29, 2020 at 06:07:14PM +0100, Christian Borntraeger wrote:
> >>> DMA can be done to NORMAL memory as well.
> >>
> >> Exactly.
> >> I think iucv uses GFP_DMA because z/VM needs those buffers to reside below 2GB (which is ZONA_DMA for s390).
> >
> > The normal way to allocate memory with addressing limits would be to
> > use dma_alloc_coherent and friends. Any chance to switch iucv over to
> > that? Or is there no device associated with it?
>
> There is not necessarily a device for that. It is a hypervisor interface (an
> instruction that is interpreted by z/VM). We do have the netiucv driver that
> creates a virtual nic, but there is also AF_IUCV which works without a device.
>
> But back to the original question: If we mark kmalloc caches as usercopy caches,
> we should do the same for DMA kmalloc caches. As outlined by Christoph, this has
> nothing to do with device DMA.
Hm, looks like it's allocated from the low 16MB. Seems like poor naming!
:) There seems to be a LOT of stuff using GFP_DMA, and it seems unlikely
those are all expecting low addresses?
Since this has only been a problem on s390, should just s390 gain the
weakening of the usercopy restriction? Something like:
diff --git a/mm/slab_common.c b/mm/slab_common.c
index 1907cb2903c7..c5bbc141f20b 100644
--- a/mm/slab_common.c
+++ b/mm/slab_common.c
@@ -1303,7 +1303,9 @@ void __init create_kmalloc_caches(slab_flags_t flags)
kmalloc_caches[KMALLOC_DMA][i] = create_kmalloc_cache(
kmalloc_info[i].name[KMALLOC_DMA],
kmalloc_info[i].size,
- SLAB_CACHE_DMA | flags, 0, 0);
+ SLAB_CACHE_DMA | flags, 0,
+ IS_ENABLED(CONFIG_S390) ?
+ kmalloc_info[i].size : 0);
}
}
#endif
--
Kees Cook
^ permalink raw reply related
* Re: [PATCH] riscv: Use flush_icache_mm for flush_icache_user_range
From: Palmer Dabbelt @ 2020-01-30 17:40 UTC (permalink / raw)
Cc: linux-riscv, linux-arch, linux-kernel, linux-csky, guoren,
Andrew Waterman, palmer
In-Reply-To: <20200124161810.24322-1-guoren@linux.alibaba.com>
On Fri, 24 Jan 2020 16:18:10 GMT (+0000), guoren@linux.alibaba.com wrote:
> The only call path is:
>
> __access_remote_vm -> copy_to_user_page -> flush_icache_user_range
>
> Seems it's ok to use flush_icache_mm instead of flush_icache_all and
> it could reduce flush_icache_all called on other harts.
>
> I think the patch is the fixup for the commit 08f051eda33b.
>
> Signed-off-by: Guo Ren <guoren@linux.alibaba.com>
> Cc: Andrew Waterman <andrew@sifive.com>
> Cc: Palmer Dabbelt <palmer@sifive.com>
> ---
> arch/riscv/include/asm/cacheflush.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/arch/riscv/include/asm/cacheflush.h b/arch/riscv/include/asm/cacheflush.h
> index b69aecbb36d3..26589623fd57 100644
> --- a/arch/riscv/include/asm/cacheflush.h
> +++ b/arch/riscv/include/asm/cacheflush.h
> @@ -85,7 +85,7 @@ static inline void flush_dcache_page(struct page *page)
> * so instead we just flush the whole thing.
> */
> #define flush_icache_range(start, end) flush_icache_all()
> -#define flush_icache_user_range(vma, pg, addr, len) flush_icache_all()
> +#define flush_icache_user_range(vma, pg, addr, len) flush_icache_mm(vma->vm_mm, 0)
>
> void dma_wbinv_range(unsigned long start, unsigned long end);
> void dma_wb_range(unsigned long start, unsigned long end);
Reviewed-by: Palmer Dabbelt <palmerdabbelt@google.com>
I've added this to for-next with some minor modifications as
4d99abce8ce80e866020ffa5b2bd790269235f37. It missed the PR I'm sending
now-ish, but I'll include it as part of the next one even if it's during an
early RC.
Thanks!
^ permalink raw reply
* Re: [PATCH v16 00/23] selftests, powerpc, x86: Memory Protection Keys
From: Sandipan Das @ 2020-01-30 9:20 UTC (permalink / raw)
To: Dave Hansen
Cc: shuah, linux-kselftest, linux-arch, fweimer, x86, linuxram,
mhocko, linux-mm, mingo, aneesh.kumar, bauerman, msuchanek, mpe,
linuxppc-dev
In-Reply-To: <26f630e5-1f70-888c-4b43-30e73c9f270c@linux.ibm.com>
On 30/01/20 11:49 am, Sandipan Das wrote:
> Hi Dave,
>
> On 30/01/20 12:29 am, Dave Hansen wrote:
>> On 1/28/20 1:38 AM, Sandipan Das wrote:
>>> On 27/01/20 9:12 pm, Dave Hansen wrote:
>>>> How have you tested this patch (and the whole series for that matter)?
>>>>
>>> I replaced the second patch with this one and did a build test.
>>> Till v16, I had tested the whole series (build + run) on both a POWER8
>>> system (with 4K and 64K page sizes) and a Skylake SP system but for
>>> x86_64 only.
>>
>> Do you have any idea why I was seeing x86 build errors and you were not?
>>
>
> There were problems with patch 2 from v17. The fixed patch is what I replied
> with previously in this thread. The test results that I posted were with that
> patch included. Will post out v18 today with the fix.
>
In patch 2 of v17, the issue was with the target names. Upon adding something
to TEST_GEN_FILES, rules for targets like the following are expected to be
defined.
<path-to-linux-source>/tools/testing/selftests/vm/protection_keys_32
<path-to-linux-source>/tools/testing/selftests/vm/protection_keys_64
<path-to-linux-source>/tools/testing/selftests/vm/protection_keys
But instead, I only defined rules for these.
protection_keys_32
protection_keys_64
protection_keys
Hence the build was failing in these cases:
$ make -C tools/testing/selftests
$ make -C tools/testing/selftests/vm
$ cd tools/testing/selftests/vm
$ make
But worked in these cases:
$ make -C tools/testing/selftests/vm protection_keys
$ cd tools/testing/selftests/vm
$ make protection_keys
This has been addressed in v18.
- Sandipan
^ permalink raw reply
* Re: [PATCH v16 00/23] selftests, powerpc, x86: Memory Protection Keys
From: Sandipan Das @ 2020-01-30 9:20 UTC (permalink / raw)
To: Dave Hansen
Cc: shuah, linux-kselftest, linux-arch, fweimer, x86, linuxram,
mhocko, linux-mm, mingo, aneesh.kumar, bauerman, msuchanek, mpe,
linuxppc-dev
In-Reply-To: <26f630e5-1f70-888c-4b43-30e73c9f270c@linux.ibm.com>
On 30/01/20 11:49 am, Sandipan Das wrote:
> Hi Dave,
>
> On 30/01/20 12:29 am, Dave Hansen wrote:
>> On 1/28/20 1:38 AM, Sandipan Das wrote:
>>> On 27/01/20 9:12 pm, Dave Hansen wrote:
>>>> How have you tested this patch (and the whole series for that matter)?
>>>>
>>> I replaced the second patch with this one and did a build test.
>>> Till v16, I had tested the whole series (build + run) on both a POWER8
>>> system (with 4K and 64K page sizes) and a Skylake SP system but for
>>> x86_64 only.
>>
>> Do you have any idea why I was seeing x86 build errors and you were not?
>>
>
> There were problems with patch 2 from v17. The fixed patch is what I replied
> with previously in this thread. The test results that I posted were with that
> patch included. Will post out v18 today with the fix.
>
In patch 2 of v17, the issue was with the target names. Upon adding something
to TEST_GEN_FILES, rules for targets like the following are expected to be
defined.
<path-to-linux-source>/tools/testing/selftests/vm/protection_keys_32
<path-to-linux-source>/tools/testing/selftests/vm/protection_keys_64
<path-to-linux-source>/tools/testing/selftests/vm/protection_keys
But instead, I only defined rules for these.
protection_keys_32
protection_keys_64
protection_keys
Hence the build was failing in these cases:
$ make -C tools/testing/selftests
$ make -C tools/testing/selftests/vm
$ cd tools/testing/selftests/vm
$ make
But worked in these cases:
$ make -C tools/testing/selftests/vm protection_keys
$ cd tools/testing/selftests/vm
$ make protection_keys
This has been addressed in v18.
- Sandipan
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox