[PATCH] selftests/vm: fix inability to build any vm tests

[PATCH] selftests/vm: fix inability to build any vm tests

[Axel Rasmussen]

When we stopped using KSFT_KHDR_INSTALL, a side effect is we also
changed the value of `top_srcdir`. This can be seen by looking at the
code removed by:

    49de12ba06ef ("selftests: drop KSFT_KHDR_INSTALL make target"):

(Note though that this commit didn't break this, technically the one
before it did since that's the one that stopped KSFT_KHDR_INSTALL from
being used, even though the code was still there.)

Previously lib.mk reconfigured `top_srcdir` when KSFT_KHDR_INSTALL was
being used. Now, that's no longer the case.

As a result, the path to gup_test.h in vm/Makefile was wrong, and
since it's a dependency of all of the vm binaries none of them could
be built. Instead, we'd get an "error" like:

    make[1]: *** No rule to make target '/[...]/tools/testing/selftests/vm/compaction_test', needed by 'all'.  Stop.

If we specify the path of gup_test.h relatively using selfdir instead,
now it is found correctly, and things work again.

Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
---
 tools/testing/selftests/vm/Makefile | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
index d9fa6a9ea584..f2a12494f2d8 100644
--- a/tools/testing/selftests/vm/Makefile
+++ b/tools/testing/selftests/vm/Makefile
@@ -1,7 +1,7 @@
 # SPDX-License-Identifier: GPL-2.0
 # Makefile for vm selftests
 
-LOCAL_HDRS += $(selfdir)/vm/local_config.h $(top_srcdir)/mm/gup_test.h
+LOCAL_HDRS += $(selfdir)/vm/local_config.h $(selfdir)/../../../mm/gup_test.h
 
 include local_config.mk
 
-- 
2.37.1.595.g718a3a8f04-goog

Re: [PATCH] selftests/vm: fix inability to build any vm tests

[John Hubbard]

On 8/17/22 14:13, Axel Rasmussen wrote:
> Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
> Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
> ---
>   tools/testing/selftests/vm/Makefile | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
> index d9fa6a9ea584..f2a12494f2d8 100644
> --- a/tools/testing/selftests/vm/Makefile
> +++ b/tools/testing/selftests/vm/Makefile
> @@ -1,7 +1,7 @@
>   # SPDX-License-Identifier: GPL-2.0
>   # Makefile for vm selftests
>   
> -LOCAL_HDRS += $(selfdir)/vm/local_config.h $(top_srcdir)/mm/gup_test.h
> +LOCAL_HDRS += $(selfdir)/vm/local_config.h $(selfdir)/../../../mm/gup_test.h

Hi Alex,

Thanks for fixing up this build, it's always frustrating to finally
finish working on something, only to find that the selftests build is
broken!

This looks correct, and also I've tested it locally, and it works. So
please feel free to add:

Reviewed-by: John Hubbard <jhubbard@nvidia.com>


A couple of follow-up thoughts:

1) I recalled that hmm-tests.c in the same directory also needs
gup_test.h, and wondered if that was covered. And it turns out the the
relative "up and over" include path is done in hmm-tests.c itself,
instead of in the Makefile, like this:

/*
  * This is a private UAPI to the kernel test module so it isn't exported
  * in the usual include/uapi/... directory.
  */
#include "../../../../lib/test_hmm_uapi.h"
#include "../../../../mm/gup_test.h"

It would be nice (maybe follow-up polishing for someone) if this were
done once, instead of twice (or more?) via different (source code vs.
Makefile) mechanisms.

2) Commit f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
claims that it is now required to "make headers_install" before building
the selftests. However, after applying your fix (not to imply that there
is anything wrong with the fix; it's fine), I am still able to build
vm/selftests, directly after a top-level "make clean".

I believe that this is because the selftests are directly including
gup_test.h, via relative up-and-over paths. And I recall (and also did a
quick dry run, to be sure) that this internal gup_test.h header is not
part of the headers_install list. So that seems to be all working as
intended. But I wanted to say all of this out loud, in order to be sure
I fully understand these build steps.


thanks,
-- 
John Hubbard
NVIDIA

Re: [PATCH] selftests/vm: fix inability to build any vm tests

[Axel Rasmussen]

On Wed, Aug 17, 2022 at 3:15 PM John Hubbard <jhubbard@nvidia.com> wrote:
>
> On 8/17/22 14:13, Axel Rasmussen wrote:
> > Fixes: f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
> > Signed-off-by: Axel Rasmussen <axelrasmussen@google.com>
> > ---
> >   tools/testing/selftests/vm/Makefile | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile
> > index d9fa6a9ea584..f2a12494f2d8 100644
> > --- a/tools/testing/selftests/vm/Makefile
> > +++ b/tools/testing/selftests/vm/Makefile
> > @@ -1,7 +1,7 @@
> >   # SPDX-License-Identifier: GPL-2.0
> >   # Makefile for vm selftests
> >
> > -LOCAL_HDRS += $(selfdir)/vm/local_config.h $(top_srcdir)/mm/gup_test.h
> > +LOCAL_HDRS += $(selfdir)/vm/local_config.h $(selfdir)/../../../mm/gup_test.h
>
> Hi Alex,
>
> Thanks for fixing up this build, it's always frustrating to finally
> finish working on something, only to find that the selftests build is
> broken!
>
> This looks correct, and also I've tested it locally, and it works. So
> please feel free to add:
>
> Reviewed-by: John Hubbard <jhubbard@nvidia.com>
>
>
> A couple of follow-up thoughts:
>
> 1) I recalled that hmm-tests.c in the same directory also needs
> gup_test.h, and wondered if that was covered. And it turns out the the
> relative "up and over" include path is done in hmm-tests.c itself,
> instead of in the Makefile, like this:
>
> /*
>   * This is a private UAPI to the kernel test module so it isn't exported
>   * in the usual include/uapi/... directory.
>   */
> #include "../../../../lib/test_hmm_uapi.h"
> #include "../../../../mm/gup_test.h"
>
> It would be nice (maybe follow-up polishing for someone) if this were
> done once, instead of twice (or more?) via different (source code vs.
> Makefile) mechanisms.

Hmm, I suppose the way to clean this up would be to have the Makefile
compute this once, and pass in "-I $(selfdir)/../../.." to the
compiler so we could just "#include <mm/gup_test.h>" directly?

If there aren't objections to something like that being too weird, I
can write a follow-up patch which does that.

>
> 2) Commit f2745dc0ba3d ("selftests: stop using KSFT_KHDR_INSTALL")
> claims that it is now required to "make headers_install" before building
> the selftests. However, after applying your fix (not to imply that there
> is anything wrong with the fix; it's fine), I am still able to build
> vm/selftests, directly after a top-level "make clean".
>
> I believe that this is because the selftests are directly including
> gup_test.h, via relative up-and-over paths. And I recall (and also did a
> quick dry run, to be sure) that this internal gup_test.h header is not
> part of the headers_install list. So that seems to be all working as
> intended. But I wanted to say all of this out loud, in order to be sure
> I fully understand these build steps.

I agree this is working as intended, my understanding is that "make
headers_install" is really for the stuff under include/uapi/*, whereas
headers under mm/ or lib/ aren't really meant to be "exposed to
userspace" except for these particular selftests. So including them
directly instead of looking for them under usr/include/ is
intentional.

>
>
> thanks,
> --
> John Hubbard
> NVIDIA

Re: [PATCH] selftests/vm: fix inability to build any vm tests

[John Hubbard]

On 8/18/22 10:58, Axel Rasmussen wrote:
>> 1) I recalled that hmm-tests.c in the same directory also needs
>> gup_test.h, and wondered if that was covered. And it turns out the the
>> relative "up and over" include path is done in hmm-tests.c itself,
>> instead of in the Makefile, like this:
>>
>> /*
>>    * This is a private UAPI to the kernel test module so it isn't exported
>>    * in the usual include/uapi/... directory.
>>    */
>> #include "../../../../lib/test_hmm_uapi.h"
>> #include "../../../../mm/gup_test.h"
>>
>> It would be nice (maybe follow-up polishing for someone) if this were
>> done once, instead of twice (or more?) via different (source code vs.
>> Makefile) mechanisms.
> 
> Hmm, I suppose the way to clean this up would be to have the Makefile
> compute this once, and pass in "-I $(selfdir)/../../.." to the
> compiler so we could just "#include <mm/gup_test.h>" directly?

Yes, it's better to have the Makefile know where the include paths are,
rather than each source file, so that looks better.

But hold on, now I see that selftests/vm/Makefile already uses what is
effectively a src_topdir, anyway! Here:

CFLAGS = -Wall -I ../../../../usr/include $(EXTRA_CFLAGS) $(KHDR_INCLUDES)

...which makes me think that real fix for the original problem should be
simply this:

diff --git a/tools/testing/selftests/lib.mk b/tools/testing/selftests/lib.mk
index 947fc72413e9..d44c72b3abe3 100644
--- a/tools/testing/selftests/lib.mk
+++ b/tools/testing/selftests/lib.mk
@@ -40,6 +40,7 @@ ifeq (0,$(MAKELEVEL))
      endif
  endif
  selfdir = $(realpath $(dir $(filter %/lib.mk,$(MAKEFILE_LIST))))
+top_srcdir = $(selfdir)/../../..

  # The following are built by lib.mk common compile rules.
  # TEST_CUSTOM_PROGS should be used by tests that require


...and then the follow up cleanup can use top_srcdir to cleanup CFLAGS and
also pull the relative paths out of the source files and into the Makefile.


thanks,
-- 
John Hubbard
NVIDIA