From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from kanga.kvack.org (kanga.kvack.org [205.233.56.17]) by smtp.lore.kernel.org (Postfix) with ESMTP id 90CB4C43334 for ; Tue, 12 Jul 2022 16:17:50 +0000 (UTC) Received: by kanga.kvack.org (Postfix) id B7FF09400A2; Tue, 12 Jul 2022 12:17:49 -0400 (EDT) Received: by kanga.kvack.org (Postfix, from userid 40) id B319A940063; Tue, 12 Jul 2022 12:17:49 -0400 (EDT) X-Delivered-To: int-list-linux-mm@kvack.org Received: by kanga.kvack.org (Postfix, from userid 63042) id 9F7A89400A2; Tue, 12 Jul 2022 12:17:49 -0400 (EDT) X-Delivered-To: linux-mm@kvack.org Received: from relay.hostedemail.com (smtprelay0012.hostedemail.com [216.40.44.12]) by kanga.kvack.org (Postfix) with ESMTP id 8EDD1940063 for ; Tue, 12 Jul 2022 12:17:49 -0400 (EDT) Received: from smtpin21.hostedemail.com (a10.router.float.18 [10.200.18.1]) by unirelay10.hostedemail.com (Postfix) with ESMTP id 5F0A62EA for ; Tue, 12 Jul 2022 16:17:49 +0000 (UTC) X-FDA: 79678953858.21.1E7100E Received: from mail-qt1-f178.google.com (mail-qt1-f178.google.com [209.85.160.178]) by imf20.hostedemail.com (Postfix) with ESMTP id 969A51C00A0 for ; Tue, 12 Jul 2022 16:17:48 +0000 (UTC) Received: by mail-qt1-f178.google.com with SMTP id 23so9710628qtt.3 for ; Tue, 12 Jul 2022 09:17:48 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=+wfkrjv7dKT50y8qNm6L3gLU8tf9TON6rd+MNfozwLw=; b=wFLIrhAJlZZBUwBTH+i/yIBlRIFA8GLZ8IyumOhsZTNbEoy+4LBfu9Zr62T7vDQCLg 64OQlfx9/3V8O2DVKAQdLDPGbSfDHwxsqMNm40OqbU4Pzl5EKa/Rx7c3Bu+dx/yB9Mpy wvsbRIaFT520/m57QGAWRkkj57n6hflMfsPXWIxgIQdDF4Cd6YI+NCs7KcWy+7o+B8jW /Bg/l6dduPLNiJiFq8BlRiWfWziu5V9LJi61A+zXDBwW2e5NI/FR1m3COzWDR7/9yp3k CA/Nlt5hq0CxDP1mXnDlPD5jgFRaMCL6WIY90E2+gtA5bkE5q3yJWNe+DSV/4rhlAL/Q u+Ng== X-Gm-Message-State: AJIora/Lroh8jl7kI+1oiybfmnS4k7xEhbkeMnQBLvbVzqGKLlWN9exA 1KcpAggfhGj4YoZMd7B2P+E= X-Google-Smtp-Source: AGRyM1vSE/bj1g1/GZb1qx9O/Vjra9j4Bww4F12mrnOxjUmjjXxoQrWsRcwMOvoAUuLrW9iXqO51/w== X-Received: by 2002:a05:622a:1a10:b0:31e:bb0d:3d9a with SMTP id f16-20020a05622a1a1000b0031ebb0d3d9amr5469842qtb.345.1657642667587; Tue, 12 Jul 2022 09:17:47 -0700 (PDT) Received: from dev0025.ash9.facebook.com (fwdproxy-ash-012.fbsv.net. [2a03:2880:20ff:c::face:b00c]) by smtp.gmail.com with ESMTPSA id cm23-20020a05622a251700b0031bed25394csm7819906qtb.3.2022.07.12.09.17.46 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Tue, 12 Jul 2022 09:17:47 -0700 (PDT) Date: Tue, 12 Jul 2022 09:17:45 -0700 From: David Vernet To: Andrew Morton Cc: Adam Sindelar , linux-mm@kvack.org, Adam Sindelar , kernel-team@fb.com Subject: Re: [PATCH] selftests/vm: fix va_128TBswitch.sh permissions Message-ID: <20220712161745.ujkkgsxt2xmeop6r@dev0025.ash9.facebook.com> References: <20220708090646.34927-1-adam@wowsignal.io> <20220708130801.411ace64e0244a75f483a9f0@linux-foundation.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20220708130801.411ace64e0244a75f483a9f0@linux-foundation.org> User-Agent: NeoMutt/20211029 ARC-Seal: i=1; s=arc-20220608; d=hostedemail.com; t=1657642668; a=rsa-sha256; cv=none; b=OhhESg7HaI9kMmMZFyNqnYPDyz+kQvNDxiCF0z+CAFsMkYaaWGP87Dm3uwxdUc66dhVkF2 9nZGp1pwBpcJpJ0QwaH8WX2A8lUgzk69qdbABxZh3SUqg1SzRqManFl8mj6SvvOmfbruvL AirGrTTZaVcBWs8GuTq19qwQqpWHyIo= ARC-Authentication-Results: i=1; imf20.hostedemail.com; dkim=none; spf=pass (imf20.hostedemail.com: domain of dcvernet@gmail.com designates 209.85.160.178 as permitted sender) smtp.mailfrom=dcvernet@gmail.com; dmarc=none ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=hostedemail.com; s=arc-20220608; t=1657642668; h=from:from:sender:reply-to:subject:subject:date:date: message-id:message-id:to:to:cc:cc:mime-version:mime-version: content-type:content-type:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=+wfkrjv7dKT50y8qNm6L3gLU8tf9TON6rd+MNfozwLw=; b=XBJXclFLMrnMpE6ciZvo6CSZdiCu6WD3YwrcatWHxIAbjkgHNV6cNNC+dqrItILJw444aF KuuDa1XX3LqlRT6dHIyPcIcQALr+NuOhYphr5FNboBgLfSTZ1JmIGJHbEj+6AFovTfHlei 17x6pgF8Jd5UFCkCDBozrBTx6Pv5ibA= X-Rspamd-Queue-Id: 969A51C00A0 Authentication-Results: imf20.hostedemail.com; dkim=none; spf=pass (imf20.hostedemail.com: domain of dcvernet@gmail.com designates 209.85.160.178 as permitted sender) smtp.mailfrom=dcvernet@gmail.com; dmarc=none X-Rspamd-Server: rspam02 X-Rspam-User: X-Stat-Signature: 3n4hc5xrxfbkxg5gcgs8mpyumtiu539w X-HE-Tag: 1657642668-2192 X-Bogosity: Ham, tests=bogofilter, spamicity=0.000000, version=1.2.4 Sender: owner-linux-mm@kvack.org Precedence: bulk X-Loop: owner-majordomo@kvack.org List-ID: On Fri, Jul 08, 2022 at 01:08:01PM -0700, Andrew Morton wrote: > On Fri, 8 Jul 2022 11:06:46 +0200 Adam Sindelar 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 > > --- > > 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