Hi Jens,
Can you consider taking this through the block tree?
These patches make some changes to the kunit tests previously added for
iov_iter testing, in particular adding testing of UBUF/IOVEC iterators and
some benchmarking:
(1) Clean up a couple of checkpatch style complaints.
(2) Consolidate some repeated bits of code into helper functions and use
the same struct to represent straight offset/address ranges and
partial page lists.
(3) Add a function to set up a userspace VM, attach the VM to the kunit
testing thread, create an anonymous file, stuff some pages into the
file and map the file into the VM to act as a buffer that can be used
with UBUF/IOVEC iterators.
I map an anonymous file with pages attached rather than using MAP_ANON
so that I can check the pages obtained from iov_iter_extract_pages()
without worrying about them changing due to swap, migrate, etc..
[?] Is this the best way to do things? Mirroring execve, it requires
a number of extra core symbols to be exported. Should this be done in
the core code?
(4) Add tests for copying into and out of UBUF and IOVEC iterators.
(5) Add tests for extracting pages from UBUF and IOVEC iterators.
(6) Add tests to benchmark copying 256MiB to UBUF, IOVEC, KVEC, BVEC and
XARRAY iterators.
(7) Add a test to bencmark copying 256MiB from an xarray that gets decanted
into 256-page BVEC iterators to model batching from the pagecache.
(8) Add a test to benchmark copying 256MiB through dynamically allocated
256-page bvecs to simulate bio construction.
Example benchmarks output:
iov_kunit_benchmark_ubuf: avg 4474 uS, stddev 1340 uS
iov_kunit_benchmark_iovec: avg 6619 uS, stddev 23 uS
iov_kunit_benchmark_kvec: avg 2672 uS, stddev 14 uS
iov_kunit_benchmark_bvec: avg 3189 uS, stddev 19 uS
iov_kunit_benchmark_bvec_split: avg 3403 uS, stddev 8 uS
iov_kunit_benchmark_xarray: avg 3709 uS, stddev 7 uS
I've pushed the patches here also:
https://git.kernel.org/pub/scm/linux/kernel/git/dhowells/linux-fs.git/log/?…
David
Changes
=======
ver #3)
- #include <linux/personality.h> to get READ_IMPLIES_EXEC.
- Add a test to benchmark decanting an xarray into bio_vecs.
ver #2)
- Use MAP_ANON to make the user buffer if we don't want a list of pages.
- KUNIT_ASSERT_NOT_ERR_OR_NULL() doesn't like __user pointers as the
condition, so cast.
- Make the UBUF benchmark loop, doing an iterator per page so that the
overhead from the iterator code is not negligible.
- Make the KVEC benchmark use an iovec per page so that the iteration is
not not negligible.
- Switch the benchmarking to use copy_from_iter() so that only a single
page is needed in the userspace buffer (as it can be shared R/O), not
256MiB's worth.
Link: https://lore.kernel.org/r/20230914221526.3153402-1-dhowells@redhat.com/ # v1
Link: https://lore.kernel.org/r/20230920130400.203330-1-dhowells@redhat.com/ # v2
David Howells (10):
iov_iter: Fix some checkpatch complaints in kunit tests
iov_iter: Consolidate some of the repeated code into helpers
iov_iter: Consolidate the test vector struct in the kunit tests
iov_iter: Consolidate bvec pattern checking
iov_iter: Create a function to prepare userspace VM for UBUF/IOVEC
tests
iov_iter: Add copy kunit tests for ITER_UBUF and ITER_IOVEC
iov_iter: Add extract kunit tests for ITER_UBUF and ITER_IOVEC
iov_iter: Add benchmarking kunit tests
iov_iter: Add kunit to benchmark decanting of xarray to bvec
iov_iter: Add benchmarking kunit tests for UBUF/IOVEC
arch/s390/kernel/vdso.c | 1 +
fs/anon_inodes.c | 1 +
kernel/fork.c | 2 +
lib/kunit_iov_iter.c | 1317 +++++++++++++++++++++++++++++++++------
mm/mmap.c | 1 +
mm/util.c | 3 +
6 files changed, 1139 insertions(+), 186 deletions(-)
The test_cases is not freed in kunit_free_suite_set().
And the copy pointer may be moved in kunit_filter_suites().
The filtered_suite and filtered_suite->test_cases allocated in the last
kunit_filter_attr_tests() in last inner for loop may be leaked if
kunit_filter_suites() fails.
If kunit_filter_suites() succeeds, not only copy but also filtered_suite
and filtered_suite->test_cases should be freed.
Changes in v2:
- Add Reviewed-by.
- Add the memory leak backtrace for the 4th patch.
- Remove the unused func kernel test robot noticed for the 4th patch.
- Update the commit message for the 4th patch.
Jinjie Ruan (4):
kunit: Fix missed memory release in kunit_free_suite_set()
kunit: Fix the wrong kfree of copy for kunit_filter_suites()
kunit: Fix possible memory leak in kunit_filter_suites()
kunit: test: Fix the possible memory leak in executor_test
lib/kunit/executor.c | 23 ++++++++++++++++------
lib/kunit/executor_test.c | 40 ++++++++++++++++++---------------------
2 files changed, 35 insertions(+), 28 deletions(-)
--
2.34.1
On Thu, 21 Sept 2023 at 16:18, Ma Ke <make_ruc2021(a)163.com> wrote:
>
> To avoid the failure of alloc, we could check the return value of
> kmalloc() and kzalloc().
>
> Signed-off-by: Ma Ke <make_ruc2021(a)163.com>
> ---
Fair enough, though I'd want the test to fail in this case (or, at the
very least, be skipped).
Could we use KUNIT_ASSERT_NOT_NULL() here?
Furthermore, there are a few bugs in the patch, see below.
Cheers,
-- David
> lib/list-test.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/lib/list-test.c b/lib/list-test.c
> index 0cc27de9cec8..9f82cac3a822 100644
> --- a/lib/list-test.c
> +++ b/lib/list-test.c
> @@ -27,9 +27,14 @@ static void list_test_list_init(struct kunit *test)
> INIT_LIST_HEAD(&list2);
>
> list4 = kzalloc(sizeof(*list4), GFP_KERNEL | __GFP_NOFAIL);
> + if (!list4)
> + return;
Instead, let's use:
KUNIT_ASSERT_NOT_NULL(test, list4)
> INIT_LIST_HEAD(list4);
>
> list5 = kmalloc(sizeof(*list5), GFP_KERNEL | __GFP_NOFAIL);
> + if (!list5)
Shouldn't this be in {}s? We don't want to return unconditionally.
> + kfree(list5);
We shouldn't free a NULL pointer. Should this be kfree(list4)?
Either way, maybe we should swap the allocations out for
kunit_kzalloc(), which will automatically free everything on test
exit.
> + return;
Again, let's use KUNIT_ASSERT_NOT_NULL() here. Or at the very least,
call KUNIT_FAIL() to make sure we're noting the test has failed.
> memset(list5, 0xFF, sizeof(*list5));
> INIT_LIST_HEAD(list5);
>
> --
> 2.37.2
>