On Jun 07, 2023 / 17:18, Nitesh Shetty wrote:
> This tests memory leak, by loading/unloading nullblk driver.
> Steps:
> 1. Load nullblk driver with memory_backed=1
> 2. "dd" of 50M
> 3. Unload null-blk driver
> We do it for 5 iterations to avoid any noise.
>
> Commit 8cfb98196cceec35416041c6b91212d2b99392e4 fixes issue in kernel
Hi Nitesh, thanks for the patch. Good to have this test case. I ran it
and confirmed it detects the issue in stable manner. Please find some
comments in line for improvements, and see if they are reasonable.
>
> Signed-off-by: Nitesh Shetty <nj.shetty@xxxxxxxxxxx>
> ---
> tests/block/034 | 60 +++++++++++++++++++++++++++++++++++++++++++++
> tests/block/034.out | 2 ++
> 2 files changed, 62 insertions(+)
> create mode 100644 tests/block/034
> create mode 100644 tests/block/034.out
>
> diff --git a/tests/block/034 b/tests/block/034
> new file mode 100644
> index 0000000..dc4f447
> --- /dev/null
> +++ b/tests/block/034
> @@ -0,0 +1,60 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-3.0+
> +# Copyright (C) 2023 Nitesh Shetty
> +#
> +# Check memory leak when null_blk driver is loaded with memory_backed=1
> +
> +. tests/block/rc
> +. common/null_blk
> +
> +DESCRIPTION="load/unload null_blk memory_backed=1 and run dd to check memleak"
Nit: the line is a bit long. I suggest to remove the part "and run dd".
> +TIMED=1
The line above is not appropriate since this test case does not refer $TIMEOUT.
Instead, I suggest to add "QUICK=1", since it takes only a few seconds to run.
> +
> +requires() {
> + _have_null_blk
Instead of the line above, I suggest "_have_module" null_blk. It will explicitly
tell that this test case requires loadable null_blk. It will avoid execution of
the test() function when null_blk is built-in.
> + _have_module_param null_blk memory_backed
> + _have_program dd
> +}
> +
> +run_nullblk_dd() {
> + if ! _init_null_blk memory_backed=1; then
> + echo "Loading null_blk failed"
> + return 1
> + fi
> + dd if=/dev/urandom of=/dev/nullb0 oflag=direct bs=1M count="$1" > \
> + /dev/null 2>&1
> + _exit_null_blk
> +}
> +
> +free_memory() {
> + mem=$(sed -n 's/^MemFree:[[:blank:]]*\([0-9]*\)[[:blank:]]*kB$/\1/p' \
> + /proc/meminfo)
> + echo "$mem"
Nit: I think the mem variable assignment and echo command are not required. Just
executing the sed command is enough.
> +}
> +
> +test() {
> + echo "Running ${TEST_NAME}"
> +
> + mem_leak=0
> + size=50
Nit: local variable declarations will help to read code a bit, like:
local mem_leak=0 size=50
local i mem_start mem_end mem_used
> + for ((i = 0; i < 5; i++)); do
> + mem_start=$(free_memory)
> + run_nullblk_dd $size
> + mem_end=$(free_memory)
> +
> + mem_used=$((((mem_start - mem_end)) / 1024))
Number of parenetheses can be reduced:
mem_used=$(((mem_start - mem_end) / 1024))
> + # -10MB to account for some randomness in freeing by some
> + # simultaneous process
> + if [ $mem_used -ge $((size - 10)) ]; then
Nit: Bash arithmetic could be easier to read:
if ((mem_used >= size - 10)); then
> + mem_leak=$((mem_leak + 1))
> + fi
> + done
> +
> + # There might be possibilty of some random process freeing up memory at
> + # same time nullblk is unloaded.
> + # we consider 3/5 times to be positive.
> + if [ $mem_leak -gt 3 ]; then
Nit: same here:
if ((mem_leak > 3)); then
> + echo "Memleak: Memory is not freed by null-blk driver"
> + fi
> + echo "Test complete"
> +}
> diff --git a/tests/block/034.out b/tests/block/034.out
> new file mode 100644
> index 0000000..a916dd8
> --- /dev/null
> +++ b/tests/block/034.out
> @@ -0,0 +1,2 @@
> +Running block/034
> +Test complete
> --
> 2.25.1
>
