| Message ID | 20190721104612.19120-3-willy@infradead.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make working with compound pages easier | expand |
On Sun, Jul 21, 2019 at 03:46:11AM -0700, Matthew Wilcox wrote: > From: "Matthew Wilcox (Oracle)" <willy@infradead.org> > > Replace PAGE_SHIFT + compound_order(page) with the new page_shift() > function. Minor improvements in readability. > > Signed-off-by: Matthew Wilcox (Oracle) <willy@infradead.org> Reviewed-by: Ira Weiny <ira.weiny@intel.com> > --- > arch/powerpc/mm/book3s64/iommu_api.c | 7 ++----- > drivers/vfio/vfio_iommu_spapr_tce.c | 2 +- > include/linux/mm.h | 6 ++++++ > 3 files changed, 9 insertions(+), 6 deletions(-) > > diff --git a/arch/powerpc/mm/book3s64/iommu_api.c b/arch/powerpc/mm/book3s64/iommu_api.c > index b056cae3388b..56cc84520577 100644 > --- a/arch/powerpc/mm/book3s64/iommu_api.c > +++ b/arch/powerpc/mm/book3s64/iommu_api.c > @@ -129,11 +129,8 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, > * Allow to use larger than 64k IOMMU pages. Only do that > * if we are backed by hugetlb. > */ > - if ((mem->pageshift > PAGE_SHIFT) && PageHuge(page)) { > - struct page *head = compound_head(page); > - > - pageshift = compound_order(head) + PAGE_SHIFT; > - } > + if ((mem->pageshift > PAGE_SHIFT) && PageHuge(page)) > + pageshift = page_shift(compound_head(page)); > mem->pageshift = min(mem->pageshift, pageshift); > /* > * We don't need struct page reference any more, switch > diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c > index 8ce9ad21129f..1883fd2901b2 100644 > --- a/drivers/vfio/vfio_iommu_spapr_tce.c > +++ b/drivers/vfio/vfio_iommu_spapr_tce.c > @@ -190,7 +190,7 @@ static bool tce_page_is_contained(struct mm_struct *mm, unsigned long hpa, > * a page we just found. Otherwise the hardware can get access to > * a bigger memory chunk that it should. > */ > - return (PAGE_SHIFT + compound_order(compound_head(page))) >= page_shift; > + return page_shift(compound_head(page)) >= page_shift; > } > > static inline bool tce_groups_attached(struct tce_container *container) > diff --git a/include/linux/mm.h b/include/linux/mm.h > index 899dfcf7c23d..64762559885f 100644 > --- a/include/linux/mm.h > +++ b/include/linux/mm.h > @@ -811,6 +811,12 @@ static inline unsigned long page_size(struct page *page) > return PAGE_SIZE << compound_order(page); > } > > +/* Returns the number of bits needed for the number of bytes in a page */ > +static inline unsigned int page_shift(struct page *page) > +{ > + return PAGE_SHIFT + compound_order(page); > +} > + > void free_compound_page(struct page *page); > > #ifdef CONFIG_MMU > -- > 2.20.1 >
On Wed, 24 Jul 2019 18:40:25 +0800 kbuild test robot <lkp@intel.com> wrote: > Thank you for the patch! Yet something to improve: > > [auto build test ERROR on linus/master] > [cannot apply to v5.3-rc1 next-20190724] > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > url: https://github.com/0day-ci/linux/commits/Matthew-Wilcox/Make-working-with-compound-pages-easier/20190722-030555 > config: powerpc64-allyesconfig (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=powerpc64 > > If you fix the issue, kindly add following tag > Reported-by: kbuild test robot <lkp@intel.com> > > Note: the linux-review/Matthew-Wilcox/Make-working-with-compound-pages-easier/20190722-030555 HEAD e1bb8b04ba8cf861b2610b0ae646ee49cb069568 builds fine. > It only hurts bisectibility. > > All error/warnings (new ones prefixed by >>): > > drivers/vfio/vfio_iommu_spapr_tce.c: In function 'tce_page_is_contained': > >> drivers/vfio/vfio_iommu_spapr_tce.c:193:9: error: called object 'page_shift' is not a function or function pointer > return page_shift(compound_head(page)) >= page_shift; > ^~~~~~~~~~ > drivers/vfio/vfio_iommu_spapr_tce.c:179:16: note: declared here > unsigned int page_shift) > ^~~~~~~~~~ This? --- a/drivers/vfio/vfio_iommu_spapr_tce.c~mm-introduce-page_shift-fix +++ a/drivers/vfio/vfio_iommu_spapr_tce.c @@ -176,13 +176,13 @@ put_exit: } static bool tce_page_is_contained(struct mm_struct *mm, unsigned long hpa, - unsigned int page_shift) + unsigned int it_page_shift) { struct page *page; unsigned long size = 0; - if (mm_iommu_is_devmem(mm, hpa, page_shift, &size)) - return size == (1UL << page_shift); + if (mm_iommu_is_devmem(mm, hpa, it_page_shift, &size)) + return size == (1UL << it_page_shift); page = pfn_to_page(hpa >> PAGE_SHIFT); /* @@ -190,7 +190,7 @@ static bool tce_page_is_contained(struct * a page we just found. Otherwise the hardware can get access to * a bigger memory chunk that it should. */ - return page_shift(compound_head(page)) >= page_shift; + return page_shift(compound_head(page)) >= it_page_shift; } static inline bool tce_groups_attached(struct tce_container *container)
On Wed, Jul 24, 2019 at 05:30:55PM -0700, Andrew Morton wrote: > On Wed, 24 Jul 2019 18:40:25 +0800 kbuild test robot <lkp@intel.com> wrote: > > > Thank you for the patch! Yet something to improve: > > > > [auto build test ERROR on linus/master] > > [cannot apply to v5.3-rc1 next-20190724] > > [if your patch is applied to the wrong git tree, please drop us a note to help improve the system] > > > > url: https://github.com/0day-ci/linux/commits/Matthew-Wilcox/Make-working-with-compound-pages-easier/20190722-030555 > > config: powerpc64-allyesconfig (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=powerpc64 > > > > If you fix the issue, kindly add following tag > > Reported-by: kbuild test robot <lkp@intel.com> > > > > Note: the linux-review/Matthew-Wilcox/Make-working-with-compound-pages-easier/20190722-030555 HEAD e1bb8b04ba8cf861b2610b0ae646ee49cb069568 builds fine. > > It only hurts bisectibility. > > > > All error/warnings (new ones prefixed by >>): > > > > drivers/vfio/vfio_iommu_spapr_tce.c: In function 'tce_page_is_contained': > > >> drivers/vfio/vfio_iommu_spapr_tce.c:193:9: error: called object 'page_shift' is not a function or function pointer > > return page_shift(compound_head(page)) >= page_shift; > > ^~~~~~~~~~ > > drivers/vfio/vfio_iommu_spapr_tce.c:179:16: note: declared here > > unsigned int page_shift) > > ^~~~~~~~~~ > > This? Looks reasonable to me. But checking around it does seem like "page_shift" is used as a parameter or variable in quite a few other places. Is this something to be concerned with? Reviewed-by: Ira Weiny <ira.weiny@intel.com> > > --- a/drivers/vfio/vfio_iommu_spapr_tce.c~mm-introduce-page_shift-fix > +++ a/drivers/vfio/vfio_iommu_spapr_tce.c > @@ -176,13 +176,13 @@ put_exit: > } > > static bool tce_page_is_contained(struct mm_struct *mm, unsigned long hpa, > - unsigned int page_shift) > + unsigned int it_page_shift) > { > struct page *page; > unsigned long size = 0; > > - if (mm_iommu_is_devmem(mm, hpa, page_shift, &size)) > - return size == (1UL << page_shift); > + if (mm_iommu_is_devmem(mm, hpa, it_page_shift, &size)) > + return size == (1UL << it_page_shift); > > page = pfn_to_page(hpa >> PAGE_SHIFT); > /* > @@ -190,7 +190,7 @@ static bool tce_page_is_contained(struct > * a page we just found. Otherwise the hardware can get access to > * a bigger memory chunk that it should. > */ > - return page_shift(compound_head(page)) >= page_shift; > + return page_shift(compound_head(page)) >= it_page_shift; > } > > static inline bool tce_groups_attached(struct tce_container *container) > _ >
On Thu, Jul 25, 2019 at 01:30:12PM -0700, Ira Weiny wrote: > On Wed, Jul 24, 2019 at 05:30:55PM -0700, Andrew Morton wrote: > > On Wed, 24 Jul 2019 18:40:25 +0800 kbuild test robot <lkp@intel.com> wrote: > > > drivers/vfio/vfio_iommu_spapr_tce.c: In function 'tce_page_is_contained': > > > >> drivers/vfio/vfio_iommu_spapr_tce.c:193:9: error: called object 'page_shift' is not a function or function pointer > > > return page_shift(compound_head(page)) >= page_shift; > > > ^~~~~~~~~~ > > > drivers/vfio/vfio_iommu_spapr_tce.c:179:16: note: declared here > > > unsigned int page_shift) > > > ^~~~~~~~~~ > > > > This? > > Looks reasonable to me. But checking around it does seem like "page_shift" is > used as a parameter or variable in quite a few other places. > > Is this something to be concerned with? Sorry, I missed this earlier. It's not currently a warning because we don't enable -Wshadow. For functions which have a parameter or variable named page_shift, the local definition overrides the global function. They continue to work "as expected". The only reason this particular function has an issue is that it also wants to call the function page_shift(). The compiler resolves the symbol 'page_shift' to be the parameter, and says "This is an int, not a function, you're clearly mistaken". We don't need to mass-rename all the local variables called page_shift, unless we want to enable -Wshadow. Which I would actually like to do, but I don't have time.
diff --git a/arch/powerpc/mm/book3s64/iommu_api.c b/arch/powerpc/mm/book3s64/iommu_api.c index b056cae3388b..56cc84520577 100644 --- a/arch/powerpc/mm/book3s64/iommu_api.c +++ b/arch/powerpc/mm/book3s64/iommu_api.c @@ -129,11 +129,8 @@ static long mm_iommu_do_alloc(struct mm_struct *mm, unsigned long ua, * Allow to use larger than 64k IOMMU pages. Only do that * if we are backed by hugetlb. */ - if ((mem->pageshift > PAGE_SHIFT) && PageHuge(page)) { - struct page *head = compound_head(page); - - pageshift = compound_order(head) + PAGE_SHIFT; - } + if ((mem->pageshift > PAGE_SHIFT) && PageHuge(page)) + pageshift = page_shift(compound_head(page)); mem->pageshift = min(mem->pageshift, pageshift); /* * We don't need struct page reference any more, switch diff --git a/drivers/vfio/vfio_iommu_spapr_tce.c b/drivers/vfio/vfio_iommu_spapr_tce.c index 8ce9ad21129f..1883fd2901b2 100644 --- a/drivers/vfio/vfio_iommu_spapr_tce.c +++ b/drivers/vfio/vfio_iommu_spapr_tce.c @@ -190,7 +190,7 @@ static bool tce_page_is_contained(struct mm_struct *mm, unsigned long hpa, * a page we just found. Otherwise the hardware can get access to * a bigger memory chunk that it should. */ - return (PAGE_SHIFT + compound_order(compound_head(page))) >= page_shift; + return page_shift(compound_head(page)) >= page_shift; } static inline bool tce_groups_attached(struct tce_container *container) diff --git a/include/linux/mm.h b/include/linux/mm.h index 899dfcf7c23d..64762559885f 100644 --- a/include/linux/mm.h +++ b/include/linux/mm.h @@ -811,6 +811,12 @@ static inline unsigned long page_size(struct page *page) return PAGE_SIZE << compound_order(page); } +/* Returns the number of bits needed for the number of bytes in a page */ +static inline unsigned int page_shift(struct page *page) +{ + return PAGE_SHIFT + compound_order(page); +} + void free_compound_page(struct page *page); #ifdef CONFIG_MMU