Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
MIME-Version: 1.0
References: <20220420084036.4101604-1-usama.anjum@collabora.com>
 <01f64e01-580e-abca-2b86-aa586d987bf4@redhat.com> <c3df7c71-79c1-7876-e39a-15cffc444da3@collabora.com>
In-Reply-To: <c3df7c71-79c1-7876-e39a-15cffc444da3@collabora.com>
From:   Nico Pache <npache@redhat.com>
Date:   Fri, 23 Sep 2022 16:35:19 -0600
Message-ID: <CAA1CXcAMeoaSTc3Oa-ZppX9GqpDRH=cJ+aLnTgWU1-=OEcwTsw@mail.gmail.com>
Subject: Re: [PATCH v6 1/2] selftests: vm: bring common functions to a new file
To:     Muhammad Usama Anjum <usama.anjum@collabora.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Shuah Khan <shuah@kernel.org>, kernel@collabora.com,
        krisman@collabora.com, linux-kernel@vger.kernel.org,
        linux-mm@kvack.org, linux-kselftest@vger.kernel.org,
        Joel Savitz <jsavitz@redhat.com>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Wed, Sep 21, 2022 at 5:06 AM Muhammad Usama Anjum
<usama.anjum@collabora.com> wrote:
>
> On 9/9/22 8:06 AM, Nico Pache wrote:
> >
> >
> > On 4/20/22 04:40, Muhammad Usama Anjum wrote:
> >> Bring common functions to a new file while keeping code as much same as
> >> possible. These functions can be used in the new tests. This helps in
> >> avoiding code duplication.
> >
> > This commit breaks a pattern in the way tests are run in the current scheme.
> > Before this commit the only executable (or TEST_PROGS) that was executed was
> > run_vmselftests.sh. Now both madv_populate and split_huge_page_test are being
> > run as well.
> >>
> >> Signed-off-by: Muhammad Usama Anjum <usama.anjum@collabora.com>
> >> ---
> >> Changes in V6:
> >> - Correct header files inclusion
> >>
> >> Changes in V5:
> >> Keep moved code as same as possible
> >> - Updated macros names
> >> - Removed macro used to show bit number of dirty bit, added a comment
> >>   instead
> >> - Corrected indentation
> >> ---
> >>  tools/testing/selftests/vm/Makefile           |   7 +-
> >>  tools/testing/selftests/vm/madv_populate.c    |  34 +-----
> >>  .../selftests/vm/split_huge_page_test.c       |  79 +------------
> >>  tools/testing/selftests/vm/vm_util.c          | 108 ++++++++++++++++++
> >>  tools/testing/selftests/vm/vm_util.h          |   9 ++
> >>  5 files changed, 124 insertions(+), 113 deletions(-)
> >>  create mode 100644 tools/testing/selftests/vm/vm_util.c
> >>  create mode 100644 tools/testing/selftests/vm/vm_util.h
> >>
> >> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
> >> index 5e43f072f5b76..4e68edb26d6b6 100644
> >> --- a/tools/testing/selftests/vm/Makefile
> >> +++ b/tools/testing/selftests/vm/Makefile
> >> @@ -34,7 +34,7 @@ TEST_GEN_FILES += hugepage-mremap
> >>  TEST_GEN_FILES += hugepage-shm
> >>  TEST_GEN_FILES += hugepage-vmemmap
> >>  TEST_GEN_FILES += khugepaged
> >> -TEST_GEN_FILES += madv_populate
> >> +TEST_GEN_PROGS = madv_populate
> > madv_populate is already being run in run_vmselftests.sh
> >>  TEST_GEN_FILES += map_fixed_noreplace
> >>  TEST_GEN_FILES += map_hugetlb
> >>  TEST_GEN_FILES += map_populate
> >> @@ -47,7 +47,7 @@ TEST_GEN_FILES += on-fault-limit
> >>  TEST_GEN_FILES += thuge-gen
> >>  TEST_GEN_FILES += transhuge-stress
> >>  TEST_GEN_FILES += userfaultfd
> >> -TEST_GEN_FILES += split_huge_page_test
> >> +TEST_GEN_PROGS += split_huge_page_test
> >>  TEST_GEN_FILES += ksm_tests
> >>
> >>  ifeq ($(MACHINE),x86_64)
> >> @@ -91,6 +91,9 @@ TEST_FILES := test_vmalloc.sh
> >>  KSFT_KHDR_INSTALL := 1
> >>  include ../lib.mk
> >>
> >> +$(OUTPUT)/madv_populate: vm_util.c
> >> +$(OUTPUT)/split_huge_page_test: vm_util.c
> > Not sure what this does but if we add a run entry for split_huge_page_test in
> > run_vmselftests.sh we wont really need this patch.
> >
> > I'm not sure the reduction in code size is worth the change in run behavior.
> >
> > Unless I'm missing something I suggest we revert this patch and add a run entry
> > for split_huge_page_test in run_vmselftests.sh. I can do this if no one objects.
> The run behavior isn't being changed here. Only the build behavior is
> being changed as we want to keep the common code in one file. You can
> add the run entry as required. I don't know why do you think the
> Makefile has changed the run behavior.

Before your commit running
    `make TARGETS=vm; make TARGETS=vm run_tests`
had the following run behavior:
    - The only thing executed via the run_tests wrapper is run_vmtests.sh
    - TAP output is 1/1:
        TAP version 13
        1..1
        # selftests: vm: run_vmtests.sh
        # arm64 ia64 mips64 parisc64 ppc64 ppc64le riscv64 s390x sh64
sparc64 x86_64
        # -----------------------
        # running ./hugepage-mmap
        # -----------------------
        ....

After your commit:
    - Multiple executables (madv_populate, soft-dirty,
split_huge_page_test, run_vmtests.sh) are defined and ran:
         # selftests: vm: madv_populate
         not ok 1 selftests: vm: madv_populate # exit=1
         # selftests: vm: soft-dirty
         ok 2 selftests: vm: soft-dirty
         # selftests: vm: split_huge_page_test
         ok 3 selftests: vm: split_huge_page_test
         # selftests: vm: run_vmtests.sh
         ok 4 selftests: vm: run_vmtests.sh # SKIP

I'm not saying utilizing the TEST_GEN_PROG variable is incorrect, I'm
just pointing out that we have a sudden change in run behavior via the
run_test wrapper. I personally think the TEST_GEN_PROG output is
cleaner, but having two different ways of outputting results may be
harder/confusing to parser. Additionally there is still the issue that
madv_populate is being run twice for no reason.

Cheers,
-- Nico

>
> >
> > Cheers,
> > -- Nico
> >
>
> --
> Muhammad Usama Anjum
>