Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   Maximilian Heyne <mheyne@amazon.de>
To:     SeongJae Park <sj38.park@gmail.com>
CC:     <akpm@linux-foundation.org>, SeongJae Park <sjpark@amazon.de>,
        <Jonathan.Cameron@Huawei.com>, <acme@kernel.org>,
        <alexander.shishkin@linux.intel.com>, <amit@kernel.org>,
        <benh@kernel.crashing.org>, <brendanhiggins@google.com>,
        <corbet@lwn.net>, <david@redhat.com>, <dwmw@amazon.com>,
        <elver@google.com>, <fan.du@intel.com>, <foersleo@amazon.de>,
        <greg@kroah.com>, <gthelen@google.com>,
        <guoju.fgj@alibaba-inc.com>, <mgorman@suse.de>,
        <minchan@kernel.org>, <mingo@redhat.com>, <namhyung@kernel.org>,
        <peterz@infradead.org>, <riel@surriel.com>, <rientjes@google.com>,
        <rostedt@goodmis.org>, <rppt@kernel.org>, <shakeelb@google.com>,
        <shuah@kernel.org>, <snu@amazon.de>, <vbabka@suse.cz>,
        <vdavydov.dev@gmail.com>, <zgf574564920@gmail.com>,
        <linux-damon@amazon.com>, <linux-mm@kvack.org>,
        <linux-doc@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH v29 12/13] mm/damon: Add user space selftests
Date:   Fri, 11 Jun 2021 13:57:37 +0000
Message-ID: <20210611135737.104838-1-mheyne@amazon.de>
In-Reply-To: <20210520075629.4332-13-sj38.park@gmail.com>
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

On Thu, 20 May 2021 07:56:28 +0000 SeongJae Park <sj38.park@gmail.com> wrote:

> From: SeongJae Park <sjpark@amazon.de>
> 
> This commit adds a simple user space tests for DAMON.  The tests are
> using kselftest framework.
> 
> Signed-off-by: SeongJae Park <sjpark@amazon.de>
> ---
>  tools/testing/selftests/damon/Makefile        |  7 ++
>  .../selftests/damon/_chk_dependency.sh        | 28 ++++++
>  .../testing/selftests/damon/debugfs_attrs.sh  | 98 +++++++++++++++++++
>  3 files changed, 133 insertions(+)
>  create mode 100644 tools/testing/selftests/damon/Makefile
>  create mode 100644 tools/testing/selftests/damon/_chk_dependency.sh
>  create mode 100755 tools/testing/selftests/damon/debugfs_attrs.sh
> 
> diff --git a/tools/testing/selftests/damon/Makefile b/tools/testing/selftests/damon/Makefile
> new file mode 100644
> index 000000000000..8a3f2cd9fec0
> --- /dev/null
> +++ b/tools/testing/selftests/damon/Makefile
> @@ -0,0 +1,7 @@
> +# SPDX-License-Identifier: GPL-2.0
> +# Makefile for damon selftests
> +
> +TEST_FILES = _chk_dependency.sh
> +TEST_PROGS = debugfs_attrs.sh
> +
> +include ../lib.mk
> diff --git a/tools/testing/selftests/damon/_chk_dependency.sh b/tools/testing/selftests/damon/_chk_dependency.sh
> new file mode 100644
> index 000000000000..e090836c2bf7
> --- /dev/null
> +++ b/tools/testing/selftests/damon/_chk_dependency.sh
> @@ -0,0 +1,28 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +# Kselftest framework requirement - SKIP code is 4.
> +ksft_skip=4
> +
> +DBGFS=/sys/kernel/debug/damon
> +
> +if [ $EUID -ne 0 ];
> +then
> +	echo "Run as root"
> +	exit $ksft_skip
> +fi
> +
> +if [ ! -d $DBGFS ]
> +then
> +	echo "$DBGFS not found"
> +	exit $ksft_skip
> +fi
> +
> +for f in attrs target_ids monitor_on
> +do
> +	if [ ! -f "$DBGFS/$f" ]
> +	then
> +		echo "$f not found"
> +		exit 1
> +	fi
> +done
> diff --git a/tools/testing/selftests/damon/debugfs_attrs.sh b/tools/testing/selftests/damon/debugfs_attrs.sh
> new file mode 100755
> index 000000000000..4a8ab4910ee4
> --- /dev/null
> +++ b/tools/testing/selftests/damon/debugfs_attrs.sh
> @@ -0,0 +1,98 @@
> +#!/bin/bash
> +# SPDX-License-Identifier: GPL-2.0
> +
> +source ./_chk_dependency.sh
> +
> +# Test attrs file
> +file="$DBGFS/attrs"
> +
> +ORIG_CONTENT=$(cat $file)

Missing quotes around $file. Can you run shellcheck on this code and fix all
reportings, please?

> +
> +echo 1 2 3 4 5 > $file
> +if [ $? -ne 0 ]
> +then
> +	echo "$file write failed"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +echo 1 2 3 4 > $file
> +if [ $? -eq 0 ]
> +then
> +	echo "$file write success (should failed)"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +CONTENT=$(cat $file)
> +if [ "$CONTENT" != "1 2 3 4 5" ]
> +then
> +	echo "$file not written"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi

I'd add test cases for the contents written to the attrs, like checking that
input min_nr_regions is actually smaller than the input max_nr_regions values.

> +
> +echo $ORIG_CONTENT > $file
> +
> +# Test target_ids file
> +file="$DBGFS/target_ids"
> +
> +ORIG_CONTENT=$(cat $file)
> +
> +echo "1 2 3 4" > $file
> +if [ $? -ne 0 ]
> +then
> +	echo "$file write fail"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +echo "1 2 abc 4" > $file
> +if [ $? -ne 0 ]
> +then
> +	echo "$file write fail"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi

I've seen this construct more than once. Any chance to refactor this code? Or is
this selftest not expected to grow in the future?

> +
> +CONTENT=$(cat $file)
> +if [ "$CONTENT" != "1 2" ]
> +then
> +	echo "$file not written"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +echo abc 2 3 > $file
> +if [ $? -ne 0 ]
> +then
> +	echo "$file wrong value write fail"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +if [ ! -z "$(cat $file)" ]
> +then
> +	echo "$file not cleared"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +echo > $file
> +if [ $? -ne 0 ]
> +then
> +	echo "$file init fail"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +if [ ! -z "$(cat $file)" ]
> +then
> +	echo "$file not initialized"
> +	echo $ORIG_CONTENT > $file
> +	exit 1
> +fi
> +
> +echo $ORIG_CONTENT > $file
> +
> +echo "PASS"
> -- 
> 2.17.1
> 
> 
> 
> 


Amazon Development Center Germany GmbH
Krausenstr. 38
10117 Berlin
Geschaeftsfuehrung: Christian Schlaeger, Jonathan Weiss
Eingetragen am Amtsgericht Charlottenburg unter HRB 149173 B
Sitz: Berlin
Ust-ID: DE 289 237 879