| Message ID | 20200928062159.923212-3-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | selftests/vm: gup_test, hmm-tests, assorted improvements | expand |
On Sun, Sep 27, 2020 at 11:21:53PM -0700, John Hubbard wrote: > diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile > index d1ae706d9927..9cc6bc087461 100644 > +++ b/tools/testing/selftests/vm/Makefile > @@ -130,3 +130,5 @@ endif > $(OUTPUT)/userfaultfd: LDLIBS += -lpthread > > $(OUTPUT)/mlock-random-test: LDLIBS += -lcap > + > +$(OUTPUT)/gup_test: ../../../../mm/gup_test.h There is no reason to do this, the auto depends will pick up header files, and gup_test.h isn't a generated file Jason
On 9/28/20 5:57 AM, Jason Gunthorpe wrote: > On Sun, Sep 27, 2020 at 11:21:53PM -0700, John Hubbard wrote: >> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile >> index d1ae706d9927..9cc6bc087461 100644 >> +++ b/tools/testing/selftests/vm/Makefile >> @@ -130,3 +130,5 @@ endif >> $(OUTPUT)/userfaultfd: LDLIBS += -lpthread >> >> $(OUTPUT)/mlock-random-test: LDLIBS += -lcap >> + >> +$(OUTPUT)/gup_test: ../../../../mm/gup_test.h > > There is no reason to do this, the auto depends will pick up header > files, and gup_test.h isn't a generated file > It is less capable than you might think. Without the admittedly ugly technique above, it fails to build, and as you can see, the include paths that are fed to gcc are just a single one: usr/include: $ make make --no-builtin-rules ARCH=x86 -C ../../../.. headers_install gcc -Wall -I ../../../../usr/include gup_test.c /kernel_work/linux-next-github/tools/testing/selftests/kselftest_harness.h /kernel_work/linux-next-github/tools/testing/selftests/kselftest.h ../../../../mm/gup_test.h -lrt -o /kernel_work/linux-next-github/tools/testing/selftests/vm/gup_test make[1]: Entering directory '/kernel_work/linux-next-github' gup_test.c:10:10: fatal error: gup_test.h: No such file or directory 10 | #include "gup_test.h" | ^~~~~~~~~~~~ thanks,
On Mon, Sep 28, 2020 at 01:10:24PM -0700, John Hubbard wrote: > On 9/28/20 5:57 AM, Jason Gunthorpe wrote: > > On Sun, Sep 27, 2020 at 11:21:53PM -0700, John Hubbard wrote: > > > diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile > > > index d1ae706d9927..9cc6bc087461 100644 > > > +++ b/tools/testing/selftests/vm/Makefile > > > @@ -130,3 +130,5 @@ endif > > > $(OUTPUT)/userfaultfd: LDLIBS += -lpthread > > > $(OUTPUT)/mlock-random-test: LDLIBS += -lcap > > > + > > > +$(OUTPUT)/gup_test: ../../../../mm/gup_test.h > > > > There is no reason to do this, the auto depends will pick up header > > files, and gup_test.h isn't a generated file > > > > It is less capable than you might think. Without the admittedly ugly technique > above, it fails to build, and as you can see, the include paths that are fed to > gcc are just a single one: usr/include: > > $ make > make --no-builtin-rules ARCH=x86 -C ../../../.. headers_install > gcc -Wall -I ../../../../usr/include gup_test.c > /kernel_work/linux-next-github/tools/testing/selftests/kselftest_harness.h > /kernel_work/linux-next-github/tools/testing/selftests/kselftest.h > ../../../../mm/gup_test.h -lrt -o > /kernel_work/linux-next-github/tools/testing/selftests/vm/gup_test > make[1]: Entering directory '/kernel_work/linux-next-github' > gup_test.c:10:10: fatal error: gup_test.h: No such file or directory > 10 | #include "gup_test.h" > | ^~~~~~~~~~~~ You are supposed to use #include "../../../../mm/gup_test.h" I have no idea what weird behavior the makefile is triggering that the above include works Jason
On 9/29/20 9:35 AM, Jason Gunthorpe wrote: > On Mon, Sep 28, 2020 at 01:10:24PM -0700, John Hubbard wrote: >> On 9/28/20 5:57 AM, Jason Gunthorpe wrote: >>> On Sun, Sep 27, 2020 at 11:21:53PM -0700, John Hubbard wrote: >>>> diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile >>>> index d1ae706d9927..9cc6bc087461 100644 >>>> +++ b/tools/testing/selftests/vm/Makefile >>>> @@ -130,3 +130,5 @@ endif >>>> $(OUTPUT)/userfaultfd: LDLIBS += -lpthread >>>> $(OUTPUT)/mlock-random-test: LDLIBS += -lcap >>>> + >>>> +$(OUTPUT)/gup_test: ../../../../mm/gup_test.h >>> >>> There is no reason to do this, the auto depends will pick up header >>> files, and gup_test.h isn't a generated file >>> Oh, I misread your comment! You were talking about this Makefile dependency that I'm adding, rather than the ../'s in the path. Well, for that though, it also has to stay as shown in this patch, because of this: I don't see any "gcc -m" type of dependency generation pass happening in this relatively simple Make system. And so, without including an explicit header file dependency (at least, that's the simplest way), changes to gup_test.h are not detected. Both the Makefile code and the observed behavior back this up. (I expect that this is because there is less use of header files in this area, because most unit tests are self-contained within a single .c file.) >> >> It is less capable than you might think. Without the admittedly ugly technique >> above, it fails to build, and as you can see, the include paths that are fed to >> gcc are just a single one: usr/include: >> >> $ make >> make --no-builtin-rules ARCH=x86 -C ../../../.. headers_install >> gcc -Wall -I ../../../../usr/include gup_test.c >> /kernel_work/linux-next-github/tools/testing/selftests/kselftest_harness.h >> /kernel_work/linux-next-github/tools/testing/selftests/kselftest.h >> ../../../../mm/gup_test.h -lrt -o >> /kernel_work/linux-next-github/tools/testing/selftests/vm/gup_test >> make[1]: Entering directory '/kernel_work/linux-next-github' >> gup_test.c:10:10: fatal error: gup_test.h: No such file or directory >> 10 | #include "gup_test.h" >> | ^~~~~~~~~~~~ > > You are supposed to use > > #include "../../../../mm/gup_test.h" Good, I'll leave it as I had it. > I have no idea what weird behavior the makefile is triggering that the > above include works > See the commit description for yet another Makefile weird behavior point. :) thanks,
On Tue, Sep 29, 2020 at 10:44:31AM -0700, John Hubbard wrote: > On 9/29/20 9:35 AM, Jason Gunthorpe wrote: > > On Mon, Sep 28, 2020 at 01:10:24PM -0700, John Hubbard wrote: > > > On 9/28/20 5:57 AM, Jason Gunthorpe wrote: > > > > On Sun, Sep 27, 2020 at 11:21:53PM -0700, John Hubbard wrote: > > > > > diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile > > > > > index d1ae706d9927..9cc6bc087461 100644 > > > > > +++ b/tools/testing/selftests/vm/Makefile > > > > > @@ -130,3 +130,5 @@ endif > > > > > $(OUTPUT)/userfaultfd: LDLIBS += -lpthread > > > > > $(OUTPUT)/mlock-random-test: LDLIBS += -lcap > > > > > + > > > > > +$(OUTPUT)/gup_test: ../../../../mm/gup_test.h > > > > > > > > There is no reason to do this, the auto depends will pick up header > > > > files, and gup_test.h isn't a generated file > > > > > > Oh, I misread your comment! You were talking about this Makefile > dependency that I'm adding, rather than the ../'s in the path. > > Well, for that though, it also has to stay as shown in this patch, > because of this: > > I don't see any "gcc -m" type of dependency generation pass happening > in this relatively simple Make system. It happens with -MD, all the deps are stored in files like mm/.init-mm.o.cmd and sucked into the build. > And so, without including an explicit header file dependency (at > least, that's the simplest way), changes to gup_test.h are not > detected. Shouldn't be > Both the Makefile code and the observed behavior back this up. (I > expect that this is because there is less use of header files in > this area, because most unit tests are self-contained within a > single .c file.) Something else is very wrong then. Jason
On 9/29/20 10:55 AM, Jason Gunthorpe wrote: > On Tue, Sep 29, 2020 at 10:44:31AM -0700, John Hubbard wrote: >> On 9/29/20 9:35 AM, Jason Gunthorpe wrote: >>> On Mon, Sep 28, 2020 at 01:10:24PM -0700, John Hubbard wrote: >>>> On 9/28/20 5:57 AM, Jason Gunthorpe wrote: >>>>> On Sun, Sep 27, 2020 at 11:21:53PM -0700, John Hubbard wrote: ... >> I don't see any "gcc -m" type of dependency generation pass happening >> in this relatively simple Make system. > > It happens with -MD, all the deps are stored in files like mm/.init-mm.o.cmd > and sucked into the build. You are thinking of kbuild. This is not kbuild. There are no such artifacts being generated. >> And so, without including an explicit header file dependency (at >> least, that's the simplest way), changes to gup_test.h are not >> detected. > > Shouldn't be > >> Both the Makefile code and the observed behavior back this up. (I >> expect that this is because there is less use of header files in >> this area, because most unit tests are self-contained within a >> single .c file.) > > Something else is very wrong then. > Not really, it's just a less-cabable system than kbuild. thanks,
On Tue, Sep 29, 2020 at 11:59:55AM -0700, John Hubbard wrote: > On 9/29/20 10:55 AM, Jason Gunthorpe wrote: > > On Tue, Sep 29, 2020 at 10:44:31AM -0700, John Hubbard wrote: > > > On 9/29/20 9:35 AM, Jason Gunthorpe wrote: > > > > On Mon, Sep 28, 2020 at 01:10:24PM -0700, John Hubbard wrote: > > > > > On 9/28/20 5:57 AM, Jason Gunthorpe wrote: > > > > > > On Sun, Sep 27, 2020 at 11:21:53PM -0700, John Hubbard wrote: > ... > > > I don't see any "gcc -m" type of dependency generation pass happening > > > in this relatively simple Make system. > > > > It happens with -MD, all the deps are stored in files like mm/.init-mm.o.cmd > > and sucked into the build. > > You are thinking of kbuild. This is not kbuild. There are no such artifacts > being generated. Oh. Really? That's horrible. Jason
On 9/29/20 12:08 PM, Jason Gunthorpe wrote: > On Tue, Sep 29, 2020 at 11:59:55AM -0700, John Hubbard wrote: >> On 9/29/20 10:55 AM, Jason Gunthorpe wrote: >>> On Tue, Sep 29, 2020 at 10:44:31AM -0700, John Hubbard wrote: >>>> On 9/29/20 9:35 AM, Jason Gunthorpe wrote: >>>>> On Mon, Sep 28, 2020 at 01:10:24PM -0700, John Hubbard wrote: >>>>>> On 9/28/20 5:57 AM, Jason Gunthorpe wrote: >>>>>>> On Sun, Sep 27, 2020 at 11:21:53PM -0700, John Hubbard wrote: >> ... >>>> I don't see any "gcc -m" type of dependency generation pass happening >>>> in this relatively simple Make system. >>> >>> It happens with -MD, all the deps are stored in files like mm/.init-mm.o.cmd >>> and sucked into the build. >> >> You are thinking of kbuild. This is not kbuild. There are no such artifacts >> being generated. > > Oh. Really? That's horrible. > Well, yes, it's not a perfect build system down here in selftests/. Are you saying that it is worth upgrading? I'm open to suggestions and ideas for improvements, and at the moment, I have the miniature build system here mostly loaded into my head. So for a brief shining moment I can probably understand it well enough to work on it. :) thanks,
On Tue, Sep 29, 2020 at 12:48:43PM -0700, John Hubbard wrote: > On 9/29/20 12:08 PM, Jason Gunthorpe wrote: > > On Tue, Sep 29, 2020 at 11:59:55AM -0700, John Hubbard wrote: > > > On 9/29/20 10:55 AM, Jason Gunthorpe wrote: > > > > On Tue, Sep 29, 2020 at 10:44:31AM -0700, John Hubbard wrote: > > > > > On 9/29/20 9:35 AM, Jason Gunthorpe wrote: > > > > > > On Mon, Sep 28, 2020 at 01:10:24PM -0700, John Hubbard wrote: > > > > > > > On 9/28/20 5:57 AM, Jason Gunthorpe wrote: > > > > > > > > On Sun, Sep 27, 2020 at 11:21:53PM -0700, John Hubbard wrote: > > > ... > > > > > I don't see any "gcc -m" type of dependency generation pass happening > > > > > in this relatively simple Make system. > > > > > > > > It happens with -MD, all the deps are stored in files like mm/.init-mm.o.cmd > > > > and sucked into the build. > > > > > > You are thinking of kbuild. This is not kbuild. There are no such artifacts > > > being generated. > > > > Oh. Really? That's horrible. > > > > Well, yes, it's not a perfect build system down here in selftests/. Are you saying > that it is worth upgrading? I'm open to suggestions and ideas for improvements, > and at the moment, I have the miniature build system here mostly loaded into my > head. So for a brief shining moment I can probably understand it well enough to > work on it. :) I only remarked because I didn't know it wasn't using kbuild. I thought it would have used the existing HOSTCC stuff, not sure why it is special. The only investment that seems worthwhile would be to switch it to use the normal kbuild stuff?? Jason
On 9/29/20 1:53 PM, Jason Gunthorpe wrote: > On Tue, Sep 29, 2020 at 12:48:43PM -0700, John Hubbard wrote: >> On 9/29/20 12:08 PM, Jason Gunthorpe wrote: >>> On Tue, Sep 29, 2020 at 11:59:55AM -0700, John Hubbard wrote: >>>> On 9/29/20 10:55 AM, Jason Gunthorpe wrote: >>>>> On Tue, Sep 29, 2020 at 10:44:31AM -0700, John Hubbard wrote: >>>>>> On 9/29/20 9:35 AM, Jason Gunthorpe wrote: >>>>>>> On Mon, Sep 28, 2020 at 01:10:24PM -0700, John Hubbard wrote: >>>>>>>> On 9/28/20 5:57 AM, Jason Gunthorpe wrote: >>>>>>>>> On Sun, Sep 27, 2020 at 11:21:53PM -0700, John Hubbard wrote: >>>> ... >>>>>> I don't see any "gcc -m" type of dependency generation pass happening >>>>>> in this relatively simple Make system. >>>>> >>>>> It happens with -MD, all the deps are stored in files like mm/.init-mm.o.cmd >>>>> and sucked into the build. >>>> >>>> You are thinking of kbuild. This is not kbuild. There are no such artifacts >>>> being generated. >>> >>> Oh. Really? That's horrible. >>> >> >> Well, yes, it's not a perfect build system down here in selftests/. Are you saying >> that it is worth upgrading? I'm open to suggestions and ideas for improvements, >> and at the moment, I have the miniature build system here mostly loaded into my >> head. So for a brief shining moment I can probably understand it well enough to >> work on it. :) > > I only remarked because I didn't know it wasn't using kbuild. I > thought it would have used the existing HOSTCC stuff, not sure why it > is special. > > The only investment that seems worthwhile would be to switch it to use > the normal kbuild stuff?? > I explored switching to kbuild at the kernel summit last year during my kselftest where are we talk. There was push back from several developers. We can definitely explore it as long as we can still support being able to build and run individual subsystem tests and doesn't break workflow for developers. If you are up for it, propose a patch and we can discuss it. thanks, -- Shuah
On 9/29/20 1:00 PM, Shuah Khan wrote: > On 9/29/20 1:53 PM, Jason Gunthorpe wrote: >> I only remarked because I didn't know it wasn't using kbuild. I >> thought it would have used the existing HOSTCC stuff, not sure why it >> is special. >> >> The only investment that seems worthwhile would be to switch it to use >> the normal kbuild stuff?? >> > > I explored switching to kbuild at the kernel summit last year during > my kselftest where are we talk. > > There was push back from several developers. We can definitely explore > it as long as we can still support being able to build and run > individual subsystem tests and doesn't break workflow for developers. > Do you have a link or two for that? Especially about the pushback, and conclusions reached, if any. thanks,
On 9/29/20 2:11 PM, John Hubbard wrote: > On 9/29/20 1:00 PM, Shuah Khan wrote: >> On 9/29/20 1:53 PM, Jason Gunthorpe wrote: >>> I only remarked because I didn't know it wasn't using kbuild. I >>> thought it would have used the existing HOSTCC stuff, not sure why it >>> is special. >>> >>> The only investment that seems worthwhile would be to switch it to use >>> the normal kbuild stuff?? >>> >> >> I explored switching to kbuild at the kernel summit last year during >> my kselftest where are we talk. >> >> There was push back from several developers. We can definitely explore >> it as long as we can still support being able to build and run >> individual subsystem tests and doesn't break workflow for developers. >> > > Do you have a link or two for that? Especially about the pushback, and > conclusions reached, if any. > Unfortunately no. A I recall it was workflow related issues and ease of running individual subsystem tests and backwards compatibility with stables. Let's start a new discussion and see where we land. thanks, -- Shuah
On 9/29/20 1:20 PM, Shuah Khan wrote: > On 9/29/20 2:11 PM, John Hubbard wrote: ... >> Do you have a link or two for that? Especially about the pushback, and >> conclusions reached, if any. >> > > Unfortunately no. A I recall it was workflow related issues and ease of > running individual subsystem tests and backwards compatibility with > stables. > > Let's start a new discussion and see where we land. > OK, sure. I can take a quick pass at converting just the selftests/vm directory to kbuild, and post that as an RFC to start the discussion. thanks,
diff --git a/mm/gup_test.c b/mm/gup_test.c index 10f41c0528de..a3c86d0fdff7 100644 --- a/mm/gup_test.c +++ b/mm/gup_test.c @@ -4,22 +4,7 @@ #include <linux/uaccess.h> #include <linux/ktime.h> #include <linux/debugfs.h> - -#define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_test) -#define GUP_BENCHMARK _IOWR('g', 2, struct gup_test) -#define PIN_FAST_BENCHMARK _IOWR('g', 3, struct gup_test) -#define PIN_BENCHMARK _IOWR('g', 4, struct gup_test) -#define PIN_LONGTERM_BENCHMARK _IOWR('g', 5, struct gup_test) - -struct gup_test { - __u64 get_delta_usec; - __u64 put_delta_usec; - __u64 addr; - __u64 size; - __u32 nr_pages_per_call; - __u32 flags; - __u64 expansion[10]; /* For future use */ -}; +#include "gup_test.h" static void put_back_pages(unsigned int cmd, struct page **pages, unsigned long nr_pages) diff --git a/mm/gup_test.h b/mm/gup_test.h new file mode 100644 index 000000000000..931c2f3f477a --- /dev/null +++ b/mm/gup_test.h @@ -0,0 +1,22 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +#ifndef __GUP_TEST_H +#define __GUP_TEST_H + +#include <linux/types.h> + +#define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_test) +#define GUP_BENCHMARK _IOWR('g', 2, struct gup_test) +#define PIN_FAST_BENCHMARK _IOWR('g', 3, struct gup_test) +#define PIN_BENCHMARK _IOWR('g', 4, struct gup_test) +#define PIN_LONGTERM_BENCHMARK _IOWR('g', 5, struct gup_test) + +struct gup_test { + __u64 get_delta_usec; + __u64 put_delta_usec; + __u64 addr; + __u64 size; + __u32 nr_pages_per_call; + __u32 flags; +}; + +#endif /* __GUP_TEST_H */ diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile index d1ae706d9927..9cc6bc087461 100644 --- a/tools/testing/selftests/vm/Makefile +++ b/tools/testing/selftests/vm/Makefile @@ -130,3 +130,5 @@ endif $(OUTPUT)/userfaultfd: LDLIBS += -lpthread $(OUTPUT)/mlock-random-test: LDLIBS += -lcap + +$(OUTPUT)/gup_test: ../../../../mm/gup_test.h diff --git a/tools/testing/selftests/vm/gup_test.c b/tools/testing/selftests/vm/gup_test.c index e930135727a2..70db259582c3 100644 --- a/tools/testing/selftests/vm/gup_test.c +++ b/tools/testing/selftests/vm/gup_test.c @@ -2,39 +2,19 @@ #include <stdio.h> #include <stdlib.h> #include <unistd.h> - #include <sys/ioctl.h> #include <sys/mman.h> #include <sys/prctl.h> #include <sys/stat.h> #include <sys/types.h> - -#include <linux/types.h> +#include "../../../../mm/gup_test.h" #define MB (1UL << 20) #define PAGE_SIZE sysconf(_SC_PAGESIZE) -#define GUP_FAST_BENCHMARK _IOWR('g', 1, struct gup_test) -#define GUP_BENCHMARK _IOWR('g', 2, struct gup_test) - -/* Similar to above, but use FOLL_PIN instead of FOLL_GET. */ -#define PIN_FAST_BENCHMARK _IOWR('g', 3, struct gup_test) -#define PIN_BENCHMARK _IOWR('g', 4, struct gup_test) -#define PIN_LONGTERM_BENCHMARK _IOWR('g', 5, struct gup_test) - /* Just the flags we need, copied from mm.h: */ #define FOLL_WRITE 0x01 /* check pte is writable */ -struct gup_test { - __u64 get_delta_usec; - __u64 put_delta_usec; - __u64 addr; - __u64 size; - __u32 nr_pages_per_call; - __u32 flags; - __u64 expansion[10]; /* For future use */ -}; - int main(int argc, char **argv) { struct gup_test gup;
Avoid the need to copy-paste the gup_test ioctl commands and the struct gup_test definition, between the kernel and the user space application, by providing a new header file for these. This allows easier and safer adding of new ioctl calls, as well as reducing the overall line count. Details: The header file has to be able to compile independently, because of the arguably unfortunate way that the Makefile is written: the Makefile tries to build all of its prerequisites, when really it should be only building the .c files, and leaving the other prerequisites (LOCAL_HDRS) as pure dependencies. That Makefile limitation is probably not worth fixing, but it explains why one of the includes had to be moved into the new header file. Also: simplify the ioctl struct (struct gup_test), by deleting the unused __expansion[10] field. This sort of thing is what you might see in a stable ABI, but this low-level, kernel-developer-oriented selftests/vm system is very much not subject to ABI stability. So "expansion" and "reserved" fields are unnecessary here. Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- mm/gup_test.c | 17 +---------------- mm/gup_test.h | 22 ++++++++++++++++++++++ tools/testing/selftests/vm/Makefile | 2 ++ tools/testing/selftests/vm/gup_test.c | 22 +--------------------- 4 files changed, 26 insertions(+), 37 deletions(-) create mode 100644 mm/gup_test.h