| Message ID | 20230619231044.112894-4-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/gup: Unify hugetlb, speed up thp | expand |
On 20.06.23 01:10, Peter Xu wrote: > follow_page() doesn't need it, but we'll start to need it when unifying gup > for hugetlb. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > include/linux/hugetlb.h | 8 +++++--- > mm/gup.c | 3 ++- > mm/hugetlb.c | 5 ++++- > 3 files changed, 11 insertions(+), 5 deletions(-) > > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h > index beb7c63d2871..2e2d89e79d6c 100644 > --- a/include/linux/hugetlb.h > +++ b/include/linux/hugetlb.h > @@ -131,7 +131,8 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma, > int copy_hugetlb_page_range(struct mm_struct *, struct mm_struct *, > struct vm_area_struct *, struct vm_area_struct *); > struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, > - unsigned long address, unsigned int flags); > + unsigned long address, unsigned int flags, > + unsigned int *page_mask); > long follow_hugetlb_page(struct mm_struct *, struct vm_area_struct *, > struct page **, unsigned long *, unsigned long *, > long, unsigned int, int *); > @@ -297,8 +298,9 @@ static inline void adjust_range_if_pmd_sharing_possible( > { > } > > -static inline struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, > - unsigned long address, unsigned int flags) > +static inline struct page *hugetlb_follow_page_mask( > + struct vm_area_struct *vma, unsigned long address, unsigned int flags, > + unsigned int *page_mask) > { > BUILD_BUG(); /* should never be compiled in if !CONFIG_HUGETLB_PAGE*/ > } > diff --git a/mm/gup.c b/mm/gup.c > index abcd841d94b7..9fc9271cba8d 100644 > --- a/mm/gup.c > +++ b/mm/gup.c > @@ -780,7 +780,8 @@ static struct page *follow_page_mask(struct vm_area_struct *vma, > * Ordinary GUP uses follow_hugetlb_page for hugetlb processing. > */ > if (is_vm_hugetlb_page(vma)) > - return hugetlb_follow_page_mask(vma, address, flags); > + return hugetlb_follow_page_mask(vma, address, flags, > + &ctx->page_mask); > > pgd = pgd_offset(mm, address); > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 9a6918c4250a..fbf6a09c0ec4 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -6454,7 +6454,8 @@ static inline bool __follow_hugetlb_must_fault(struct vm_area_struct *vma, > } > > struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, > - unsigned long address, unsigned int flags) > + unsigned long address, unsigned int flags, > + unsigned int *page_mask) > { > struct hstate *h = hstate_vma(vma); > struct mm_struct *mm = vma->vm_mm; > @@ -6499,6 +6500,8 @@ struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, > page = NULL; > goto out; > } > + > + *page_mask = ~huge_page_mask(h) >> PAGE_SHIFT; As discussed, can be simplified. But can be done on top (or not at all, but it is confusing code). Reviewed-by: David Hildenbrand <david@redhat.com>
On Tue, Jun 20, 2023 at 05:23:09PM +0200, David Hildenbrand wrote: > On 20.06.23 01:10, Peter Xu wrote: > > follow_page() doesn't need it, but we'll start to need it when unifying gup > > for hugetlb. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > include/linux/hugetlb.h | 8 +++++--- > > mm/gup.c | 3 ++- > > mm/hugetlb.c | 5 ++++- > > 3 files changed, 11 insertions(+), 5 deletions(-) > > > > diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h > > index beb7c63d2871..2e2d89e79d6c 100644 > > --- a/include/linux/hugetlb.h > > +++ b/include/linux/hugetlb.h > > @@ -131,7 +131,8 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma, > > int copy_hugetlb_page_range(struct mm_struct *, struct mm_struct *, > > struct vm_area_struct *, struct vm_area_struct *); > > struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, > > - unsigned long address, unsigned int flags); > > + unsigned long address, unsigned int flags, > > + unsigned int *page_mask); > > long follow_hugetlb_page(struct mm_struct *, struct vm_area_struct *, > > struct page **, unsigned long *, unsigned long *, > > long, unsigned int, int *); > > @@ -297,8 +298,9 @@ static inline void adjust_range_if_pmd_sharing_possible( > > { > > } > > -static inline struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, > > - unsigned long address, unsigned int flags) > > +static inline struct page *hugetlb_follow_page_mask( > > + struct vm_area_struct *vma, unsigned long address, unsigned int flags, > > + unsigned int *page_mask) > > { > > BUILD_BUG(); /* should never be compiled in if !CONFIG_HUGETLB_PAGE*/ > > } > > diff --git a/mm/gup.c b/mm/gup.c > > index abcd841d94b7..9fc9271cba8d 100644 > > --- a/mm/gup.c > > +++ b/mm/gup.c > > @@ -780,7 +780,8 @@ static struct page *follow_page_mask(struct vm_area_struct *vma, > > * Ordinary GUP uses follow_hugetlb_page for hugetlb processing. > > */ > > if (is_vm_hugetlb_page(vma)) > > - return hugetlb_follow_page_mask(vma, address, flags); > > + return hugetlb_follow_page_mask(vma, address, flags, > > + &ctx->page_mask); > > pgd = pgd_offset(mm, address); > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index 9a6918c4250a..fbf6a09c0ec4 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -6454,7 +6454,8 @@ static inline bool __follow_hugetlb_must_fault(struct vm_area_struct *vma, > > } > > struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, > > - unsigned long address, unsigned int flags) > > + unsigned long address, unsigned int flags, > > + unsigned int *page_mask) > > { > > struct hstate *h = hstate_vma(vma); > > struct mm_struct *mm = vma->vm_mm; > > @@ -6499,6 +6500,8 @@ struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, > > page = NULL; > > goto out; > > } > > + > > + *page_mask = ~huge_page_mask(h) >> PAGE_SHIFT; > > As discussed, can be simplified. But can be done on top (or not at all, but > it is confusing code). Since we decided to make this prettier.. At last I decided to go with this: *page_mask = (1U << huge_page_order(h)) - 1; The previous suggestion of PHYS_PFN() will do two shifts over PAGE_SIZE (the other one in huge_page_size()) which might be unnecessary, also, PHYS_ can be slightly misleading too as prefix. > > Reviewed-by: David Hildenbrand <david@redhat.com> I'll take this with above change, please shoot if not applicable. Thanks,
On 20.06.23 18:28, Peter Xu wrote: > On Tue, Jun 20, 2023 at 05:23:09PM +0200, David Hildenbrand wrote: >> On 20.06.23 01:10, Peter Xu wrote: >>> follow_page() doesn't need it, but we'll start to need it when unifying gup >>> for hugetlb. >>> >>> Signed-off-by: Peter Xu <peterx@redhat.com> >>> --- >>> include/linux/hugetlb.h | 8 +++++--- >>> mm/gup.c | 3 ++- >>> mm/hugetlb.c | 5 ++++- >>> 3 files changed, 11 insertions(+), 5 deletions(-) >>> >>> diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h >>> index beb7c63d2871..2e2d89e79d6c 100644 >>> --- a/include/linux/hugetlb.h >>> +++ b/include/linux/hugetlb.h >>> @@ -131,7 +131,8 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma, >>> int copy_hugetlb_page_range(struct mm_struct *, struct mm_struct *, >>> struct vm_area_struct *, struct vm_area_struct *); >>> struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, >>> - unsigned long address, unsigned int flags); >>> + unsigned long address, unsigned int flags, >>> + unsigned int *page_mask); >>> long follow_hugetlb_page(struct mm_struct *, struct vm_area_struct *, >>> struct page **, unsigned long *, unsigned long *, >>> long, unsigned int, int *); >>> @@ -297,8 +298,9 @@ static inline void adjust_range_if_pmd_sharing_possible( >>> { >>> } >>> -static inline struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, >>> - unsigned long address, unsigned int flags) >>> +static inline struct page *hugetlb_follow_page_mask( >>> + struct vm_area_struct *vma, unsigned long address, unsigned int flags, >>> + unsigned int *page_mask) >>> { >>> BUILD_BUG(); /* should never be compiled in if !CONFIG_HUGETLB_PAGE*/ >>> } >>> diff --git a/mm/gup.c b/mm/gup.c >>> index abcd841d94b7..9fc9271cba8d 100644 >>> --- a/mm/gup.c >>> +++ b/mm/gup.c >>> @@ -780,7 +780,8 @@ static struct page *follow_page_mask(struct vm_area_struct *vma, >>> * Ordinary GUP uses follow_hugetlb_page for hugetlb processing. >>> */ >>> if (is_vm_hugetlb_page(vma)) >>> - return hugetlb_follow_page_mask(vma, address, flags); >>> + return hugetlb_follow_page_mask(vma, address, flags, >>> + &ctx->page_mask); >>> pgd = pgd_offset(mm, address); >>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>> index 9a6918c4250a..fbf6a09c0ec4 100644 >>> --- a/mm/hugetlb.c >>> +++ b/mm/hugetlb.c >>> @@ -6454,7 +6454,8 @@ static inline bool __follow_hugetlb_must_fault(struct vm_area_struct *vma, >>> } >>> struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, >>> - unsigned long address, unsigned int flags) >>> + unsigned long address, unsigned int flags, >>> + unsigned int *page_mask) >>> { >>> struct hstate *h = hstate_vma(vma); >>> struct mm_struct *mm = vma->vm_mm; >>> @@ -6499,6 +6500,8 @@ struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, >>> page = NULL; >>> goto out; >>> } >>> + >>> + *page_mask = ~huge_page_mask(h) >> PAGE_SHIFT; >> >> As discussed, can be simplified. But can be done on top (or not at all, but >> it is confusing code). > > Since we decided to make this prettier.. At last I decided to go with this: > > *page_mask = (1U << huge_page_order(h)) - 1; > > The previous suggestion of PHYS_PFN() will do two shifts over PAGE_SIZE > (the other one in huge_page_size()) which might be unnecessary, also, PHYS_ > can be slightly misleading too as prefix. > >> >> Reviewed-by: David Hildenbrand <david@redhat.com> > > I'll take this with above change, please shoot if not applicable. Thanks, > Perfectly fine :)
diff --git a/include/linux/hugetlb.h b/include/linux/hugetlb.h index beb7c63d2871..2e2d89e79d6c 100644 --- a/include/linux/hugetlb.h +++ b/include/linux/hugetlb.h @@ -131,7 +131,8 @@ int move_hugetlb_page_tables(struct vm_area_struct *vma, int copy_hugetlb_page_range(struct mm_struct *, struct mm_struct *, struct vm_area_struct *, struct vm_area_struct *); struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, - unsigned long address, unsigned int flags); + unsigned long address, unsigned int flags, + unsigned int *page_mask); long follow_hugetlb_page(struct mm_struct *, struct vm_area_struct *, struct page **, unsigned long *, unsigned long *, long, unsigned int, int *); @@ -297,8 +298,9 @@ static inline void adjust_range_if_pmd_sharing_possible( { } -static inline struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, - unsigned long address, unsigned int flags) +static inline struct page *hugetlb_follow_page_mask( + struct vm_area_struct *vma, unsigned long address, unsigned int flags, + unsigned int *page_mask) { BUILD_BUG(); /* should never be compiled in if !CONFIG_HUGETLB_PAGE*/ } diff --git a/mm/gup.c b/mm/gup.c index abcd841d94b7..9fc9271cba8d 100644 --- a/mm/gup.c +++ b/mm/gup.c @@ -780,7 +780,8 @@ static struct page *follow_page_mask(struct vm_area_struct *vma, * Ordinary GUP uses follow_hugetlb_page for hugetlb processing. */ if (is_vm_hugetlb_page(vma)) - return hugetlb_follow_page_mask(vma, address, flags); + return hugetlb_follow_page_mask(vma, address, flags, + &ctx->page_mask); pgd = pgd_offset(mm, address); diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 9a6918c4250a..fbf6a09c0ec4 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -6454,7 +6454,8 @@ static inline bool __follow_hugetlb_must_fault(struct vm_area_struct *vma, } struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, - unsigned long address, unsigned int flags) + unsigned long address, unsigned int flags, + unsigned int *page_mask) { struct hstate *h = hstate_vma(vma); struct mm_struct *mm = vma->vm_mm; @@ -6499,6 +6500,8 @@ struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, page = NULL; goto out; } + + *page_mask = ~huge_page_mask(h) >> PAGE_SHIFT; } out: spin_unlock(ptl);
follow_page() doesn't need it, but we'll start to need it when unifying gup for hugetlb. Signed-off-by: Peter Xu <peterx@redhat.com> --- include/linux/hugetlb.h | 8 +++++--- mm/gup.c | 3 ++- mm/hugetlb.c | 5 ++++- 3 files changed, 11 insertions(+), 5 deletions(-)