| Message ID | 20191013221155.382378-3-jhubbard@nvidia.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | gup.c, gup_benchmark.c trivial fixes before the storm | expand |
On 10/13/19 11:12 PM, kbuild test robot wrote: > Hi John, > > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on linus/master] > [cannot apply to v5.4-rc3 next-20191011] > [if your patch is applied to the wrong git tree, please drop us a note to help > improve the system. BTW, we also suggest to use '--base' option to specify the > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > url: https://github.com/0day-ci/linux/commits/John-Hubbard/gup-c-gup_benchmark-c-trivial-fixes-before-the-storm/20191014-114158 > config: powerpc-defconfig (attached as .config) > compiler: powerpc64-linux-gcc (GCC) 7.4.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # save the attached .config to linux build tree > GCC_VERSION=7.4.0 make.cross ARCH=powerpc > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot <lkp@intel.com> > > All errors (new ones prefixed by >>): > > mm/gup.c: In function 'gup_hugepte': >>> mm/gup.c:1990:33: error: 'write' undeclared (first use in this function); did you mean 'writeq'? > if (!pte_access_permitted(pte, write)) > ^~~~~ > writeq > mm/gup.c:1990:33: note: each undeclared identifier is reported only once for each function it appears in > OK, so this shows that my cross-compiler test scripts are faulty lately, sorry I missed this. But more importantly, the above missed case is an example of when "write" really means "write", as opposed to meaning flags. Please put this patch on hold or drop it, until we hear from the authors as to how they would like to resolve this. I suspect it will end up as something like: bool write = (flags & FOLL_WRITE); ...perhaps? thanks,
On Sun, Oct 13, 2019 at 11:43:10PM -0700, John Hubbard wrote: > On 10/13/19 11:12 PM, kbuild test robot wrote: > > Hi John, > > > > Thank you for the patch! Yet something to improve: > > > > [auto build test ERROR on linus/master] > > [cannot apply to v5.4-rc3 next-20191011] > > [if your patch is applied to the wrong git tree, please drop us a note to help > > improve the system. BTW, we also suggest to use '--base' option to specify the > > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > > > url: https://github.com/0day-ci/linux/commits/John-Hubbard/gup-c-gup_benchmark-c-trivial-fixes-before-the-storm/20191014-114158 > > config: powerpc-defconfig (attached as .config) > > compiler: powerpc64-linux-gcc (GCC) 7.4.0 > > reproduce: > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > chmod +x ~/bin/make.cross > > # save the attached .config to linux build tree > > GCC_VERSION=7.4.0 make.cross ARCH=powerpc > > > > If you fix the issue, kindly add following tag > > Reported-by: kbuild test robot <lkp@intel.com> > > > > All errors (new ones prefixed by >>): > > > > mm/gup.c: In function 'gup_hugepte': > > > > mm/gup.c:1990:33: error: 'write' undeclared (first use in this function); did you mean 'writeq'? > > if (!pte_access_permitted(pte, write)) > > ^~~~~ > > writeq > > mm/gup.c:1990:33: note: each undeclared identifier is reported only once for each function it appears in > > > > OK, so this shows that my cross-compiler test scripts are faulty lately, > sorry I missed this. > > But more importantly, the above missed case is an example of when "write" really > means "write", as opposed to meaning flags. > > Please put this patch on hold or drop it, until we hear from the authors as to how > they would like to resolve this. I suspect it will end up as something like: > > bool write = (flags & FOLL_WRITE); > > ...perhaps? Just use if (!pte_access_permitted(pte, flags & FOLL_WRITE)) as we have in gup_pte_range(). And add: Fixes: cbd34da7dc9a ("mm: move the powerpc hugepd code to mm/gup.c")
On 10/14/19 7:22 PM, Kirill A. Shutemov wrote: > On Sun, Oct 13, 2019 at 11:43:10PM -0700, John Hubbard wrote: >> On 10/13/19 11:12 PM, kbuild test robot wrote: >>> Hi John, >>> >>> Thank you for the patch! Yet something to improve: >>> >>> [auto build test ERROR on linus/master] >>> [cannot apply to v5.4-rc3 next-20191011] >>> [if your patch is applied to the wrong git tree, please drop us a note to help >>> improve the system. BTW, we also suggest to use '--base' option to specify the >>> base tree in git format-patch, please see https://stackoverflow.com/a/37406982] >>> >>> url: https://github.com/0day-ci/linux/commits/John-Hubbard/gup-c-gup_benchmark-c-trivial-fixes-before-the-storm/20191014-114158 >>> config: powerpc-defconfig (attached as .config) >>> compiler: powerpc64-linux-gcc (GCC) 7.4.0 >>> reproduce: >>> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >>> chmod +x ~/bin/make.cross >>> # save the attached .config to linux build tree >>> GCC_VERSION=7.4.0 make.cross ARCH=powerpc >>> >>> If you fix the issue, kindly add following tag >>> Reported-by: kbuild test robot <lkp@intel.com> >>> >>> All errors (new ones prefixed by >>): >>> >>> mm/gup.c: In function 'gup_hugepte': >>>>> mm/gup.c:1990:33: error: 'write' undeclared (first use in this function); did you mean 'writeq'? >>> if (!pte_access_permitted(pte, write)) >>> ^~~~~ >>> writeq >>> mm/gup.c:1990:33: note: each undeclared identifier is reported only once for each function it appears in >>> >> >> OK, so this shows that my cross-compiler test scripts are faulty lately, >> sorry I missed this. >> >> But more importantly, the above missed case is an example of when "write" really >> means "write", as opposed to meaning flags. >> >> Please put this patch on hold or drop it, until we hear from the authors as to how >> they would like to resolve this. I suspect it will end up as something like: >> >> bool write = (flags & FOLL_WRITE); >> >> ...perhaps? > > Just use > > if (!pte_access_permitted(pte, flags & FOLL_WRITE)) > > as we have in gup_pte_range(). > > And add: > > Fixes: cbd34da7dc9a ("mm: move the powerpc hugepd code to mm/gup.c") > b798bec4741bdd80224214fdd004c8e52698e42 isn't this the commit that need to be mentioned in the Fixes: tag? -aneesh
On Mon, Oct 14, 2019 at 08:14:04PM +0530, Aneesh Kumar K.V wrote: > On 10/14/19 7:22 PM, Kirill A. Shutemov wrote: > > On Sun, Oct 13, 2019 at 11:43:10PM -0700, John Hubbard wrote: > > > On 10/13/19 11:12 PM, kbuild test robot wrote: > > > > Hi John, > > > > > > > > Thank you for the patch! Yet something to improve: > > > > > > > > [auto build test ERROR on linus/master] > > > > [cannot apply to v5.4-rc3 next-20191011] > > > > [if your patch is applied to the wrong git tree, please drop us a note to help > > > > improve the system. BTW, we also suggest to use '--base' option to specify the > > > > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > > > > > > > url: https://github.com/0day-ci/linux/commits/John-Hubbard/gup-c-gup_benchmark-c-trivial-fixes-before-the-storm/20191014-114158 > > > > config: powerpc-defconfig (attached as .config) > > > > compiler: powerpc64-linux-gcc (GCC) 7.4.0 > > > > reproduce: > > > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > > > chmod +x ~/bin/make.cross > > > > # save the attached .config to linux build tree > > > > GCC_VERSION=7.4.0 make.cross ARCH=powerpc > > > > > > > > If you fix the issue, kindly add following tag > > > > Reported-by: kbuild test robot <lkp@intel.com> > > > > > > > > All errors (new ones prefixed by >>): > > > > > > > > mm/gup.c: In function 'gup_hugepte': > > > > > > mm/gup.c:1990:33: error: 'write' undeclared (first use in this function); did you mean 'writeq'? > > > > if (!pte_access_permitted(pte, write)) > > > > ^~~~~ > > > > writeq > > > > mm/gup.c:1990:33: note: each undeclared identifier is reported only once for each function it appears in > > > > > > > > > > OK, so this shows that my cross-compiler test scripts are faulty lately, > > > sorry I missed this. > > > > > > But more importantly, the above missed case is an example of when "write" really > > > means "write", as opposed to meaning flags. > > > > > > Please put this patch on hold or drop it, until we hear from the authors as to how > > > they would like to resolve this. I suspect it will end up as something like: > > > > > > bool write = (flags & FOLL_WRITE); > > > > > > ...perhaps? > > > > Just use > > > > if (!pte_access_permitted(pte, flags & FOLL_WRITE)) > > > > as we have in gup_pte_range(). > > > > And add: > > > > Fixes: cbd34da7dc9a ("mm: move the powerpc hugepd code to mm/gup.c") > > > > b798bec4741bdd80224214fdd004c8e52698e42 isn't this the commit that need to > be mentioned in the Fixes: tag? Ah. Okay, I see you point. Yes, this is the sourch of the breakage.
On Mon, Oct 14, 2019 at 08:14:04PM +0530, Aneesh Kumar K.V wrote: > On 10/14/19 7:22 PM, Kirill A. Shutemov wrote: > > On Sun, Oct 13, 2019 at 11:43:10PM -0700, John Hubbard wrote: > > > On 10/13/19 11:12 PM, kbuild test robot wrote: > > > > Hi John, > > > > > > > > Thank you for the patch! Yet something to improve: > > > > > > > > [auto build test ERROR on linus/master] > > > > [cannot apply to v5.4-rc3 next-20191011] > > > > [if your patch is applied to the wrong git tree, please drop us a note to help > > > > improve the system. BTW, we also suggest to use '--base' option to specify the > > > > base tree in git format-patch, please see https://stackoverflow.com/a/37406982] > > > > > > > > url: https://github.com/0day-ci/linux/commits/John-Hubbard/gup-c-gup_benchmark-c-trivial-fixes-before-the-storm/20191014-114158 > > > > config: powerpc-defconfig (attached as .config) > > > > compiler: powerpc64-linux-gcc (GCC) 7.4.0 > > > > reproduce: > > > > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > > > > chmod +x ~/bin/make.cross > > > > # save the attached .config to linux build tree > > > > GCC_VERSION=7.4.0 make.cross ARCH=powerpc > > > > > > > > If you fix the issue, kindly add following tag > > > > Reported-by: kbuild test robot <lkp@intel.com> > > > > > > > > All errors (new ones prefixed by >>): > > > > > > > > mm/gup.c: In function 'gup_hugepte': > > > > > > mm/gup.c:1990:33: error: 'write' undeclared (first use in this function); did you mean 'writeq'? > > > > if (!pte_access_permitted(pte, write)) > > > > ^~~~~ > > > > writeq > > > > mm/gup.c:1990:33: note: each undeclared identifier is reported only once for each function it appears in > > > > > > > > > > OK, so this shows that my cross-compiler test scripts are faulty lately, > > > sorry I missed this. > > > > > > But more importantly, the above missed case is an example of when "write" really > > > means "write", as opposed to meaning flags. > > > > > > Please put this patch on hold or drop it, until we hear from the authors as to how > > > they would like to resolve this. I suspect it will end up as something like: > > > > > > bool write = (flags & FOLL_WRITE); > > > > > > ...perhaps? > > > > Just use > > > > if (!pte_access_permitted(pte, flags & FOLL_WRITE)) > > > > as we have in gup_pte_range(). > > > > And add: > > > > Fixes: cbd34da7dc9a ("mm: move the powerpc hugepd code to mm/gup.c") > > > > b798bec4741bdd80224214fdd004c8e52698e42 isn't this the commit that need to > be mentioned in the Fixes: tag? Yes, and while we are at it the type should probably be changed to unsigned int. Ira > > -aneesh
On 10/14/19 9:45 AM, Ira Weiny wrote: > On Mon, Oct 14, 2019 at 08:14:04PM +0530, Aneesh Kumar K.V wrote: >> On 10/14/19 7:22 PM, Kirill A. Shutemov wrote: >>> On Sun, Oct 13, 2019 at 11:43:10PM -0700, John Hubbard wrote: >>>> On 10/13/19 11:12 PM, kbuild test robot wrote: >>>>> Hi John, >>>>> >>>>> Thank you for the patch! Yet something to improve: >>>>> >>>>> [auto build test ERROR on linus/master] >>>>> [cannot apply to v5.4-rc3 next-20191011] >>>>> [if your patch is applied to the wrong git tree, please drop us a note to help >>>>> improve the system. BTW, we also suggest to use '--base' option to specify the >>>>> base tree in git format-patch, please see https://stackoverflow.com/a/37406982] >>>>> >>>>> url: https://github.com/0day-ci/linux/commits/John-Hubbard/gup-c-gup_benchmark-c-trivial-fixes-before-the-storm/20191014-114158 >>>>> config: powerpc-defconfig (attached as .config) >>>>> compiler: powerpc64-linux-gcc (GCC) 7.4.0 >>>>> reproduce: >>>>> wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross >>>>> chmod +x ~/bin/make.cross >>>>> # save the attached .config to linux build tree >>>>> GCC_VERSION=7.4.0 make.cross ARCH=powerpc >>>>> >>>>> If you fix the issue, kindly add following tag >>>>> Reported-by: kbuild test robot <lkp@intel.com> >>>>> >>>>> All errors (new ones prefixed by >>): >>>>> >>>>> mm/gup.c: In function 'gup_hugepte': >>>>>>> mm/gup.c:1990:33: error: 'write' undeclared (first use in this function); did you mean 'writeq'? >>>>> if (!pte_access_permitted(pte, write)) >>>>> ^~~~~ >>>>> writeq >>>>> mm/gup.c:1990:33: note: each undeclared identifier is reported only once for each function it appears in >>>>> >>>> >>>> OK, so this shows that my cross-compiler test scripts are faulty lately, >>>> sorry I missed this. >>>> >>>> But more importantly, the above missed case is an example of when "write" really >>>> means "write", as opposed to meaning flags. >>>> >>>> Please put this patch on hold or drop it, until we hear from the authors as to how >>>> they would like to resolve this. I suspect it will end up as something like: >>>> >>>> bool write = (flags & FOLL_WRITE); >>>> >>>> ...perhaps? >>> >>> Just use >>> >>> if (!pte_access_permitted(pte, flags & FOLL_WRITE)) >>> >>> as we have in gup_pte_range(). >>> >>> And add: >>> >>> Fixes: cbd34da7dc9a ("mm: move the powerpc hugepd code to mm/gup.c") >>> >> >> b798bec4741bdd80224214fdd004c8e52698e42 isn't this the commit that need to >> be mentioned in the Fixes: tag? > > Yes, and while we are at it the type should probably be changed to unsigned > int. > OK, I'm posting a v2 with all the above, thanks for these reviews! thanks, John Hubbard NVIDIA
diff --git a/mm/gup.c b/mm/gup.c index 23a9f9c9d377..0438221d8c53 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -1973,7 +1973,8 @@ static unsigned long hugepte_addr_end(unsigned long addr, unsigned long end, } static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr, - unsigned long end, int write, struct page **pages, int *nr) + unsigned long end, int flags, struct page **pages, + int *nr) { unsigned long pte_end; struct page *head, *page; @@ -2023,7 +2024,7 @@ static int gup_hugepte(pte_t *ptep, unsigned long sz, unsigned long addr, } static int gup_huge_pd(hugepd_t hugepd, unsigned long addr, - unsigned int pdshift, unsigned long end, int write, + unsigned int pdshift, unsigned long end, int flags, struct page **pages, int *nr) { pte_t *ptep; @@ -2033,7 +2034,7 @@ static int gup_huge_pd(hugepd_t hugepd, unsigned long addr, ptep = hugepte_offset(hugepd, addr, pdshift); do { next = hugepte_addr_end(addr, end, sz); - if (!gup_hugepte(ptep, sz, addr, end, write, pages, nr)) + if (!gup_hugepte(ptep, sz, addr, end, flags, pages, nr)) return 0; } while (ptep++, addr = next, addr != end); @@ -2041,7 +2042,7 @@ static int gup_huge_pd(hugepd_t hugepd, unsigned long addr, } #else static inline int gup_huge_pd(hugepd_t hugepd, unsigned long addr, - unsigned pdshift, unsigned long end, int write, + unsigned int pdshift, unsigned long end, int flags, struct page **pages, int *nr) { return 0; @@ -2049,7 +2050,8 @@ static inline int gup_huge_pd(hugepd_t hugepd, unsigned long addr, #endif /* CONFIG_ARCH_HAS_HUGEPD */ static int gup_huge_pmd(pmd_t orig, pmd_t *pmdp, unsigned long addr, - unsigned long end, unsigned int flags, struct page **pages, int *nr) + unsigned long end, unsigned int flags, + struct page **pages, int *nr) { struct page *head, *page; int refs;
In several routines, the "flags" argument is incorrectly named "write". Change it to "flags". You can see that this was a simple oversight, because the calling code passes "flags" to the fifth argument: gup_pgd_range(): ... if (!gup_huge_pd(__hugepd(pgd_val(pgd)), addr, PGDIR_SHIFT, next, flags, pages, nr)) ...which, until this patch, the callees referred to as "write". Also, change two lines to avoid checkpatch line length complaints, and another line to fix another oversight that checkpatch called out: missing "int" on pdshift. Cc: Christoph Hellwig <hch@lst.de> Cc: Aneesh Kumar K.V <aneesh.kumar@linux.ibm.com> Signed-off-by: John Hubbard <jhubbard@nvidia.com> --- mm/gup.c | 12 +++++++----- 1 file changed, 7 insertions(+), 5 deletions(-)