| Message ID | 20201017231425.0OZYgQdDr%akpm@linux-foundation.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/40] ia64: fix build error with !COREDUMP | expand |
On Sat, Oct 17, 2020 at 4:14 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > --- a/mm/gup_test.c~selftests-vm-use-a-common-gup_testh > +++ a/mm/gup_test.c > @@ -4,22 +4,7 @@ > +#include "../../../../mm/gup_test.h" > +++ a/mm/gup_test.h There is no way I'm applying a crazy patch like this. That can't be right. And even if it works, it *still* isn't right. Linus
On Sun, Oct 18, 2020 at 9:18 AM Linus Torvalds <torvalds@linux-foundation.org> wrote: > > On Sat, Oct 17, 2020 at 4:14 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > --- a/mm/gup_test.c~selftests-vm-use-a-common-gup_testh > > +++ a/mm/gup_test.c > > @@ -4,22 +4,7 @@ > > +#include "../../../../mm/gup_test.h" > > +++ a/mm/gup_test.h > > There is no way I'm applying a crazy patch like this. > > That can't be right. > > And even if it works, it *still* isn't right. .. and it looks like I'll have to drop all the other gup-test patches too because this one is broken. Linus
On 10/18/20 9:18 AM, Linus Torvalds wrote: > On Sat, Oct 17, 2020 at 4:14 PM Andrew Morton <akpm@linux-foundation.org> wrote: >> >> --- a/mm/gup_test.c~selftests-vm-use-a-common-gup_testh >> +++ a/mm/gup_test.c >> @@ -4,22 +4,7 @@ >> +#include "../../../../mm/gup_test.h" >> +++ a/mm/gup_test.h > > There is no way I'm applying a crazy patch like this. > > That can't be right. > > And even if it works, it *still* isn't right. > > Linus > I feel the same way about the ridiculous ../..'s in the include path. But that seemed to be the accepted way (I even have an email that I'm trying to find, in which someone confirmed that...ah here, it is [1], and I'm adding Jason to CC) for the odd situation of sharing a header file between kernel and user space, for the kselftest world. If there's a different acceptable way, I'd be delighted to use that. [1] https://lore.kernel.org/r/20200929163507.GV9916@ziepe.ca thanks,
On Sun, Oct 18, 2020 at 11:50:10AM -0700, John Hubbard wrote: > On 10/18/20 9:18 AM, Linus Torvalds wrote: > > On Sat, Oct 17, 2020 at 4:14 PM Andrew Morton <akpm@linux-foundation.org> wrote: > > > > > > --- a/mm/gup_test.c~selftests-vm-use-a-common-gup_testh > > > +++ a/mm/gup_test.c > > > @@ -4,22 +4,7 @@ > > > +#include "../../../../mm/gup_test.h" > > > +++ a/mm/gup_test.h > > > > There is no way I'm applying a crazy patch like this. > > > > That can't be right. > > > > And even if it works, it *still* isn't right. > > > > Linus > > > > I feel the same way about the ridiculous ../..'s in the include path. But that > seemed to be the accepted way (I even have an email that I'm trying to find, in > which someone confirmed that...ah here, it is [1], and I'm adding Jason to CC) > for the odd situation of sharing a header file between kernel and user space, > for the kselftest world. > > If there's a different acceptable way, I'd be delighted to use that. See where it says: CFLAGS = -Wall -I ../../../../usr/include $(EXTRA_CFLAGS) Does adding an extra -I../../../../mm to that line not do the trick?
On 10/18/20 12:03 PM, Matthew Wilcox wrote: > On Sun, Oct 18, 2020 at 11:50:10AM -0700, John Hubbard wrote: >> On 10/18/20 9:18 AM, Linus Torvalds wrote: >>> On Sat, Oct 17, 2020 at 4:14 PM Andrew Morton <akpm@linux-foundation.org> wrote: >>>> >>>> --- a/mm/gup_test.c~selftests-vm-use-a-common-gup_testh >>>> +++ a/mm/gup_test.c >>>> @@ -4,22 +4,7 @@ >>>> +#include "../../../../mm/gup_test.h" >>>> +++ a/mm/gup_test.h >>> >>> There is no way I'm applying a crazy patch like this. >>> >>> That can't be right. >>> >>> And even if it works, it *still* isn't right. >>> >>> Linus >>> >> >> I feel the same way about the ridiculous ../..'s in the include path. But that >> seemed to be the accepted way (I even have an email that I'm trying to find, in >> which someone confirmed that...ah here, it is [1], and I'm adding Jason to CC) >> for the odd situation of sharing a header file between kernel and user space, >> for the kselftest world. >> >> If there's a different acceptable way, I'd be delighted to use that. > > See where it says: > > CFLAGS = -Wall -I ../../../../usr/include $(EXTRA_CFLAGS) > > Does adding an extra -I../../../../mm to that line not do the trick? > Yes, that works. Here's a differential patch on top of the series to change it over: diff --git a/tools/testing/selftests/vm/Makefile b/tools/testing/selftests/vm/Makefile index a9332a7cf33f..350d37c21671 100644 --- a/tools/testing/selftests/vm/Makefile +++ b/tools/testing/selftests/vm/Makefile @@ -23,7 +23,7 @@ MACHINE ?= $(shell echo $(uname_M) | sed -e 's/aarch64.*/arm64/') # LDLIBS. MAKEFLAGS += --no-builtin-rules -CFLAGS = -Wall -I ../../../../usr/include $(EXTRA_CFLAGS) +CFLAGS = -Wall -I ../../../../usr/include -I ../../../../mm $(EXTRA_CFLAGS) LDLIBS = -lrt -lpthread TEST_GEN_FILES = compaction_test TEST_GEN_FILES += gup_test diff --git a/tools/testing/selftests/vm/gup_test.c b/tools/testing/selftests/vm/gup_test.c index 1bef28174f3f..a8adda6edb07 100644 --- a/tools/testing/selftests/vm/gup_test.c +++ b/tools/testing/selftests/vm/gup_test.c @@ -6,7 +6,7 @@ #include <sys/mman.h> #include <sys/stat.h> #include <sys/types.h> -#include "../../../../mm/gup_test.h" +#include "gup_test.h" #define MB (1UL << 20) #define PAGE_SIZE sysconf(_SC_PAGESIZE) thanks,
On Sun, Oct 18, 2020 at 12:13 PM John Hubbard <jhubbard@nvidia.com> wrote: > > > > > Does adding an extra -I../../../../mm to that line not do the trick? > > > > Yes, that works. Here's a differential patch on top of the series to change it > over: Guys, you're looking at the wrong thing. Inside the *tools* subdirectory, that "../../.." thing makes sense. You're in tools/testing/selftests/vm, and those crazy ".." paths are ok. Ugly, but ok. But the thing I objected to wasn't that tools directory ugliness. Let me repeat what I found completely and utterly unacceptable, and what you don't seem to have noticed, and what your patch didn't fix or touch: +++ a/mm/gup_test.c +#include "../../../../mm/gup_test.h" Notice what directory it is in? Notice how "../../.." etc is COMPLETELY UNACCEPTABLE GARBAGE. Linus
On 10/18/20 12:33 PM, Linus Torvalds wrote: > On Sun, Oct 18, 2020 at 12:13 PM John Hubbard <jhubbard@nvidia.com> wrote: ... > Guys, you're looking at the wrong thing. > > Inside the *tools* subdirectory, that "../../.." thing makes sense. > You're in tools/testing/selftests/vm, and those crazy ".." paths are > ok. Ugly, but ok. > > But the thing I objected to wasn't that tools directory ugliness. > > Let me repeat what I found completely and utterly unacceptable, and > what you don't seem to have noticed, and what your patch didn't fix or > touch: > > +++ a/mm/gup_test.c > +#include "../../../../mm/gup_test.h" > > Notice what directory it is in? Notice how "../../.." etc is > COMPLETELY UNACCEPTABLE GARBAGE. > > Linus > aha, yes. yuck. So can we please just fix it up like this? A quick compile test shows that it does build as expected: diff --git a/mm/gup_test.c b/mm/gup_test.c index 32770656cc32..e4c396146e90 100644 --- a/mm/gup_test.c +++ b/mm/gup_test.c @@ -4,7 +4,7 @@ #include <linux/uaccess.h> #include <linux/ktime.h> #include <linux/debugfs.h> -#include "../../../../mm/gup_test.h" +#include "gup_test.h" static void put_back_pages(unsigned int cmd, struct page **pages, unsigned long nr_pages, unsigned int gup_test_flags) Meanwhile, I'll work on coming up with a rationalization for how I let that mistake slip past me. I've got a teenager living under the same roof with me here, so I'm routinely exposed to some truly inspirational ideas for excuses. :) thanks,
On 10/18/20 12:48 PM, John Hubbard wrote: > On 10/18/20 12:33 PM, Linus Torvalds wrote: >> On Sun, Oct 18, 2020 at 12:13 PM John Hubbard <jhubbard@nvidia.com> wrote: > ... > >> Guys, you're looking at the wrong thing. >> >> Inside the *tools* subdirectory, that "../../.." thing makes sense. >> You're in tools/testing/selftests/vm, and those crazy ".." paths are >> ok. Ugly, but ok. >> >> But the thing I objected to wasn't that tools directory ugliness. >> >> Let me repeat what I found completely and utterly unacceptable, and >> what you don't seem to have noticed, and what your patch didn't fix or >> touch: >> >> +++ a/mm/gup_test.c >> +#include "../../../../mm/gup_test.h" >> >> Notice what directory it is in? Notice how "../../.." etc is >> COMPLETELY UNACCEPTABLE GARBAGE. >> >> Linus >> > > aha, yes. yuck. So can we please just fix it up like this? A quick compile > test shows that it does build as expected: > > diff --git a/mm/gup_test.c b/mm/gup_test.c > index 32770656cc32..e4c396146e90 100644 > --- a/mm/gup_test.c > +++ b/mm/gup_test.c > @@ -4,7 +4,7 @@ > #include <linux/uaccess.h> > #include <linux/ktime.h> > #include <linux/debugfs.h> > -#include "../../../../mm/gup_test.h" > +#include "gup_test.h" > > static void put_back_pages(unsigned int cmd, struct page **pages, > unsigned long nr_pages, unsigned int gup_test_flags) > > Also, if there is anything I can do to unblock this, such as sending a formal patch, or an updated patch or patchset, please let me know. I realize that this is a low-priority area, but it would still be nice to get it merged and behind us. thanks,
On Mon, Oct 19, 2020 at 9:13 PM John Hubbard <jhubbard@nvidia.com> wrote: > > Also, if there is anything I can do to unblock this, such as sending a formal > patch, or an updated patch or patchset, please let me know. I realize that > this is a low-priority area, but it would still be nice to get it merged > and behind us. So what happens when I notice something like this, I just drop it from my queue, the policy being "this was clearly not ready to be sent to Linus". That doesn't mean that it's blacklisted, but it does mean that I don't want to see it returned immediately with the superficial thing fixed that I noticed. That's partly because I feel that people need to take another look and see if there was perhaps something else going on (if I found one small thing, maybe there are others lurking). But admittedly it's probably mostly because I really don't want to feel like I'm the wall that people throw spaghetti at to see if it sticks. By the time it came to me, it was supposed to be ready (unless I'm actively part of the development of said feature of course - which still happens occasionally, but on the whole is a vanishingly small part of the stuff I merge). So I do expect to see it again later in some patch submission, just not right now. Linus
On 10/20/20 9:45 AM, Linus Torvalds wrote: > On Mon, Oct 19, 2020 at 9:13 PM John Hubbard <jhubbard@nvidia.com> wrote: >> >> Also, if there is anything I can do to unblock this, such as sending a formal >> patch, or an updated patch or patchset, please let me know. I realize that >> this is a low-priority area, but it would still be nice to get it merged >> and behind us. > > So what happens when I notice something like this, I just drop it from > my queue, the policy being "this was clearly not ready to be sent to > Linus". > > That doesn't mean that it's blacklisted, but it does mean that I don't > want to see it returned immediately with the superficial thing fixed > that I noticed. > > That's partly because I feel that people need to take another look and > see if there was perhaps something else going on (if I found one small > thing, maybe there are others lurking). > > But admittedly it's probably mostly because I really don't want to > feel like I'm the wall that people throw spaghetti at to see if it > sticks. By the time it came to me, it was supposed to be ready (unless > I'm actively part of the development of said feature of course - which > still happens occasionally, but on the whole is a vanishingly small > part of the stuff I merge). > > So I do expect to see it again later in some patch submission, just > not right now. > > Linus Makes perfect sense. I was wondering if it was blacklisted or in purgatory or what, but now I get it. Thanks for explaining that part of the flow. I'll see if I can pick up some more reviews, during this next cycle. thanks,
--- a/mm/gup_test.c~selftests-vm-use-a-common-gup_testh +++ a/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 "../../../../mm/gup_test.h" static void put_back_pages(unsigned int cmd, struct page **pages, unsigned long nr_pages) --- /dev/null +++ a/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 */ --- a/tools/testing/selftests/vm/gup_test.c~selftests-vm-use-a-common-gup_testh +++ a/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_benchmark) -#define GUP_BENCHMARK _IOWR('g', 2, struct gup_benchmark) - -/* Similar to above, but use FOLL_PIN instead of FOLL_GET. */ -#define PIN_FAST_BENCHMARK _IOWR('g', 3, struct gup_benchmark) -#define PIN_BENCHMARK _IOWR('g', 4, struct gup_benchmark) -#define PIN_LONGTERM_BENCHMARK _IOWR('g', 5, struct gup_benchmark) - /* Just the flags we need, copied from mm.h: */ #define FOLL_WRITE 0x01 /* check pte is writable */ -struct gup_benchmark { - __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_benchmark gup; --- a/tools/testing/selftests/vm/Makefile~selftests-vm-use-a-common-gup_testh +++ a/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