On Wed, Sep 21, 2022 at 5:06 AM Muhammad Usama Anjum
<usama.anjum@xxxxxxxxxxxxx> 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@xxxxxxxxxxxxx>
> >> ---
> >> 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
>
