From: <Tim.Bird@sony.com>
To: <shuah@kernel.org>, <alexandre.belloni@bootlin.com>,
<keescook@chromium.org>
Cc: <anders.roxell@linaro.org>, <linux-kernel@vger.kernel.org>,
<linux-kselftest@vger.kernel.org>
Subject: RE: [PATCH] selftests/kselftest/runner.sh: Add 45 second timeout per test
Date: Thu, 19 Sep 2019 20:47:51 +0000 [thread overview]
Message-ID: <ECADFF3FD767C149AD96A924E7EA6EAF977BA651@USCULXMSG01.am.sony.com> (raw)
In-Reply-To: <4844c68f-603d-14f2-f976-5bd255268c0d@kernel.org>
> -----Original Message-----
> From: shuah
>
> On 9/19/19 12:55 PM, Alexandre Belloni wrote:
> > On 19/09/2019 11:06:44-0700, Kees Cook wrote:
> >> Commit a745f7af3cbd ("selftests/harness: Add 30 second timeout per
> >> test") solves the problem of kselftest_harness.h-using binary tests
> >> possibly hanging forever. However, scripts and other binaries can still
> >> hang forever. This adds a global timeout to each test script run.
> >>
>
> Timeout is good, but really tests should not hang. So we have to somehow
> indicate that the test needs to be fixed.
Well, a test hanging is something that might indicate either a failing
in the test, or the detection of an actual problem.
>
> This timeout is a band-aid and not real solution for the problem. This
> arbitrary value doesn't take into account that the test(s) in that
> particular directory (TARGET) could be running normally and working
> through all the tests.
Yes. Also, having a single timeout value doesn't reflect the need
to test on different hardware with potentially huge differences in
CPU speed (or other resource performance differences).
Since kselftest has a policy of having each test run quickly, this is somewhat
mitigated. But still, some embedded boards are running magnitudes
slower than your "average" enterprise Linux machine. Finding a timeout
that can handle all hardware cases is difficult.
-- Tim
>
> We need some way to differentiate the two cases.
Agreed.
>
> >> To make this configurable (e.g. as needed in the "rtc" test case),
> >> include a new per-test-directory "settings" file (similar to "config")
> >> that can contain kselftest-specific settings. The first recognized field
> >> is "timeout".
> >>
> >
> > Seems good to me. I was also wondering whether this is actually
> > reasonable to have tests running for so long. I wanted to discuss that
> > at LPC but I missed the session.
> >
>
> There is the individual test times and overall kselftest run time. We
> have lots of tests now and it does take long.
>
> >> Additionally, this splits the reporting for timeouts into a specific
> >> "TIMEOUT" not-ok (and adds exit code reporting in the remaining case).
> >>
> >> Signed-off-by: Kees Cook <keescook@chromium.org>
> >> ---
> >> tools/testing/selftests/kselftest/runner.sh | 36
> +++++++++++++++++++--
> >> tools/testing/selftests/rtc/settings | 1 +
> >> 2 files changed, 34 insertions(+), 3 deletions(-)
> >> create mode 100644 tools/testing/selftests/rtc/settings
> >>
> >> diff --git a/tools/testing/selftests/kselftest/runner.sh
> b/tools/testing/selftests/kselftest/runner.sh
> >> index 00c9020bdda8..84de7bc74f2c 100644
> >> --- a/tools/testing/selftests/kselftest/runner.sh
> >> +++ b/tools/testing/selftests/kselftest/runner.sh
> >> @@ -3,9 +3,14 @@
> >> #
> >> # Runs a set of tests in a given subdirectory.
> >> export skip_rc=4
> >> +export timeout_rc=124
> >> export logfile=/dev/stdout
> >> export per_test_logging=
> >>
> >> +# Defaults for "settings" file fields:
> >> +# "timeout" how many seconds to let each test run before failing.
> >> +export kselftest_default_timeout=45
> >> +
> >> # There isn't a shell-agnostic way to find the path of a sourced file,
> >> # so we must rely on BASE_DIR being set to find other tools.
> >> if [ -z "$BASE_DIR" ]; then
> >> @@ -24,6 +29,16 @@ tap_prefix()
> >> fi
> >> }
> >>
> >> +tap_timeout()
> >> +{
> >> + # Make sure tests will time out if utility is available.
> >> + if [ -x /usr/bin/timeout ] ; then
> >> + /usr/bin/timeout "$kselftest_timeout" "$1"
> >> + else
> >> + "$1"
> >> + fi
> >> +}
> >> +
> >> run_one()
> >> {
> >> DIR="$1"
> >> @@ -32,6 +47,18 @@ run_one()
> >>
> >> BASENAME_TEST=$(basename $TEST)
> >>
> >> + # Reset any "settings"-file variables.
> >> + export kselftest_timeout="$kselftest_default_timeout"
> >> + # Load per-test-directory kselftest "settings" file.
> >> + settings="$BASE_DIR/$DIR/settings"
> >> + if [ -r "$settings" ] ; then
> >> + while read line ; do
> >> + field=$(echo "$line" | cut -d= -f1)
> >> + value=$(echo "$line" | cut -d= -f2-)
> >> + eval "kselftest_$field"="$value"
> >> + done < "$settings"
> >> + fi
> >> +
> >> TEST_HDR_MSG="selftests: $DIR: $BASENAME_TEST"
> >> echo "# $TEST_HDR_MSG"
> >> if [ ! -x "$TEST" ]; then
> >> @@ -44,14 +71,17 @@ run_one()
> >> echo "not ok $test_num $TEST_HDR_MSG"
> >> else
> >> cd `dirname $TEST` > /dev/null
> >> - (((((./$BASENAME_TEST 2>&1; echo $? >&3) |
> >> + ((((( tap_timeout ./$BASENAME_TEST 2>&1; echo $? >&3) |
> >> tap_prefix >&4) 3>&1) |
> >> (read xs; exit $xs)) 4>>"$logfile" &&
> >> echo "ok $test_num $TEST_HDR_MSG") ||
> >> - (if [ $? -eq $skip_rc ]; then \
> >> + (rc=$?; \
> >> + if [ $rc -eq $skip_rc ]; then \
> >> echo "not ok $test_num $TEST_HDR_MSG # SKIP"
> >> + elif [ $rc -eq $timeout_rc ]; then \
> >> + echo "not ok $test_num $TEST_HDR_MSG #
> TIMEOUT"
> >> else
> >> - echo "not ok $test_num $TEST_HDR_MSG"
> >> + echo "not ok $test_num $TEST_HDR_MSG #
> exit=$rc"
> >> fi)
> >> cd - >/dev/null
> >> fi
> >> diff --git a/tools/testing/selftests/rtc/settings
> b/tools/testing/selftests/rtc/settings
> >> new file mode 100644
> >> index 000000000000..ba4d85f74cd6
> >> --- /dev/null
> >> +++ b/tools/testing/selftests/rtc/settings
> >> @@ -0,0 +1 @@
> >> +timeout=90
> >> --
> >> 2.17.1
> >>
> >>
> >> --
> >> Kees Cook
> >
>
> thanks,
> -- Shuah
next prev parent reply other threads:[~2019-09-19 20:48 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-09-19 18:06 [PATCH] selftests/kselftest/runner.sh: Add 45 second timeout per test Kees Cook
2019-09-19 18:55 ` Alexandre Belloni
2019-09-19 20:09 ` shuah
2019-09-19 20:47 ` Tim.Bird [this message]
2019-09-19 21:17 ` Kees Cook
2019-09-20 20:24 ` shuah
2019-09-19 20:41 ` Tim.Bird
2019-09-19 20:49 ` Tim.Bird
2019-09-19 20:58 ` Kees Cook
2019-09-19 21:11 ` Kees Cook
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=ECADFF3FD767C149AD96A924E7EA6EAF977BA651@USCULXMSG01.am.sony.com \
--to=tim.bird@sony.com \
--cc=alexandre.belloni@bootlin.com \
--cc=anders.roxell@linaro.org \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=shuah@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).