Re: [PATCH] selftests/vm: fix va_128TBswitch.sh permissions

[David Vernet <void@manifault.com>]

On Fri, Jul 08, 2022 at 01:08:01PM -0700, Andrew Morton wrote:
> On Fri,  8 Jul 2022 11:06:46 +0200 Adam Sindelar <adam@wowsignal.io> wrote:
> 
> > Restores the +x bit to va_128TBswitch.sh, which got dropped from the
> > previous patch, somehow.
> > 
> > Fixes: 1afd01d43efc3 ("selftests/vm: Only run 128TBswitch with 5-level
> > paging")
> > 
> > Signed-off-by: Adam Sindelar <adam@wowsignal.io>
> > ---
> >  tools/testing/selftests/vm/va_128TBswitch.sh | 0
> >  1 file changed, 0 insertions(+), 0 deletions(-)
> >  mode change 100644 => 100755 tools/testing/selftests/vm/va_128TBswitch.sh
> > 
> > diff --git a/tools/testing/selftests/vm/va_128TBswitch.sh b/tools/testing/selftests/vm/va_128TBswitch.sh
> > old mode 100644
> > new mode 100755
> 
> Half of tools/testing/selftests/vm/*.sh don't have the x bit set. 
> They're invoked via `/bin/sh foo.sh', which is more robust.
> 
> Can we hunt down and fix the invoking code?  Might be as simple as
> 
> --- a/tools/testing/selftests/vm/run_vmtests.sh~a
> +++ a/tools/testing/selftests/vm/run_vmtests.sh
> @@ -144,7 +144,7 @@ run_test() {
>  		local sep=$(echo -n "$title" | tr "[:graph:][:space:]" -)
>  		printf "%s\n%s\n%s\n" "$sep" "$title" "$sep"
>  
> -		"$@"
> +		/bin/sh "$@"
>  		local ret=$?

So, I think tools/testing/selftests/vm/ is a bit interesting in that
run_vmtests.sh kind of circumvents the "standard" model for running
kselftests.  In the "standard" model (which I'm putting in quotes because
there's no formal, prescriptive way to use kselftests), testcases are
specified as Makefile targets that are put into one of several possible
Make variables to inform how they should be built and run.  This is
described in more details in [0].

[0]: https://docs.kernel.org/dev-tools/kselftest.html?highlight=kselftest#contributing-new-tests-details

As it relates to vmtests, we have the following relevant variables:

TEST_GEN_FILES
--------------

Contains targets that generate executable ELF binaries that are _not_
automatically run by the kselftests runner, but which are built. These are
the ELF executables that we run in run_vmtests.sh, sometimes with multiple
invocations using different parameters (for e.g. ksm_tests).

TEST_GEN_PROGS
--------------

Contains targets that generate executable ELF binaries that _are_
automatically run by the kselftest runner. Note that some of the executable
targets in the vm suite are not included in run_vmtests.sh, e.g.
soft-dirty, and split_huge_page_test. madv_populate actually is, which
looks like a bug to me (which was introduced in 749d999c06c2 ("selftests:
vm: bring common functions to a new file") and which I'll send out a fix
for).

TEST_PROGS
----------

Contains test shell scripts. These are automatically invoked by th
kselftest runner as well, and (as far as I understand) from the perspective
of kselftest, are the exact same as TEST_GEN_PROGS with the exception that
of course they're not compiled. In the kselftest runner, if the executable
bit is not set, the runner will echo out a warning and then try to parse
the interpreter from the first line of the file to figure out how to invoke
it.

My two cents are that the correct way to structure vm selftests is
something like the following:

1. For any testcase that's compiled directly into an ELF binary, and which
   doesn't need to be invoked with any arguments, convert it to a
   TEST_GEN_PROGS target in tools/testing/selftests/vm/Makefile, and remove
   it from being invoked from run_vmtests.sh.
2. For any testcase that's compiled directly into an ELF binary, but which
   _does_ need to be invoked with multiple arguments and/or needs to be
   invoked in some other special way such as va_128TBswitch, leave it in
   TEST_GEN_FILES, and instead add a separate executable shell script wraps
   it, and which lives in TEST_PROGS. This would be a nice cleanup, because
   it would let us remove some of the custom logic at the top of
   run_vmtests.sh that only applies to some of the testcases.
3. Once (1) and (2) are done for all relevant targets, remove
   run_vmtests.sh and instead rely on the kselftest runner.

This would allow us to remove all custom runner logic from
tools/testing/selftests/vm, and to instead just rely on the kselftest
runner to do the heavy lifting for us.

As it relates specifically to Adam's patch, IMO setting that script include
the executable bit does make sense, as it's in-line with the guidance for
kselftests in general, and I don't think we can assume that all of the
shell scripts will be bin/sh.

FWIW, I'm happy to do the above cleanup if nobody objects (after Adam's
patch lands).

Thanks,
David