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: <20220817211356.273019-1-axelrasmussen@google.com> <190edda8-1f37-0fa5-1cc1-ada97518698a@nvidia.com>
In-Reply-To: <190edda8-1f37-0fa5-1cc1-ada97518698a@nvidia.com>
From:   Axel Rasmussen <axelrasmussen@google.com>
Date:   Thu, 18 Aug 2022 10:58:43 -0700
Message-ID: <CAJHvVcgktSjo5CncC25+2j1amXvn3TjnsfOCV6CxNDp5joey9Q@mail.gmail.com>
Subject: Re: [PATCH] selftests/vm: fix inability to build any vm tests
To:     John Hubbard <jhubbard@nvidia.com>
Cc:     Andrew Morton <akpm@linux-foundation.org>,
        Shuah Khan <shuah@kernel.org>,
        Guillaume Tucker <guillaume.tucker@collabora.com>,
        Linux MM <linux-mm@kvack.org>,
        Linuxkselftest <linux-kselftest@vger.kernel.org>,
        LKML <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

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