| Message ID | 20230613215346.1022773-5-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/gup: Unify hugetlb, speed up thp | expand |
On 13.06.23 23:53, Peter Xu wrote: > It's coming, not yet, but soon. Loose the restriction. > > Signed-off-by: Peter Xu <peterx@redhat.com> > --- > mm/hugetlb.c | 7 ------- > 1 file changed, 7 deletions(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index f037eaf9d819..31d8f18bc2e4 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -6467,13 +6467,6 @@ struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, > spinlock_t *ptl; > pte_t *pte, entry; > > - /* > - * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via > - * follow_hugetlb_page(). > - */ > - if (WARN_ON_ONCE(flags & FOLL_PIN)) > - return NULL; > - > hugetlb_vma_lock_read(vma); > pte = hugetlb_walk(vma, haddr, huge_page_size(h)); > if (!pte) Did you fix why the warning was placed there in the first place? (IIRC, at least unsharing support needs to be added, maybe more)
On Wed, Jun 14, 2023 at 04:57:37PM +0200, David Hildenbrand wrote: > On 13.06.23 23:53, Peter Xu wrote: > > It's coming, not yet, but soon. Loose the restriction. > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > --- > > mm/hugetlb.c | 7 ------- > > 1 file changed, 7 deletions(-) > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > index f037eaf9d819..31d8f18bc2e4 100644 > > --- a/mm/hugetlb.c > > +++ b/mm/hugetlb.c > > @@ -6467,13 +6467,6 @@ struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, > > spinlock_t *ptl; > > pte_t *pte, entry; > > - /* > > - * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via > > - * follow_hugetlb_page(). > > - */ > > - if (WARN_ON_ONCE(flags & FOLL_PIN)) > > - return NULL; > > - > > hugetlb_vma_lock_read(vma); > > pte = hugetlb_walk(vma, haddr, huge_page_size(h)); > > if (!pte) > Did you fix why the warning was placed there in the first place? (IIRC, at > least unsharing support needs to be added, maybe more) Feel free to have a look at patch 2 - it should be done there, hopefully in the right way. And IIUC it could be a bug to not do that before (besides CoR there was also the pgtable permission checks that was missing). More details in patch 2's commit message. Thanks,
On 14.06.23 17:11, Peter Xu wrote: > On Wed, Jun 14, 2023 at 04:57:37PM +0200, David Hildenbrand wrote: >> On 13.06.23 23:53, Peter Xu wrote: >>> It's coming, not yet, but soon. Loose the restriction. >>> >>> Signed-off-by: Peter Xu <peterx@redhat.com> >>> --- >>> mm/hugetlb.c | 7 ------- >>> 1 file changed, 7 deletions(-) >>> >>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>> index f037eaf9d819..31d8f18bc2e4 100644 >>> --- a/mm/hugetlb.c >>> +++ b/mm/hugetlb.c >>> @@ -6467,13 +6467,6 @@ struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, >>> spinlock_t *ptl; >>> pte_t *pte, entry; >>> - /* >>> - * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via >>> - * follow_hugetlb_page(). >>> - */ >>> - if (WARN_ON_ONCE(flags & FOLL_PIN)) >>> - return NULL; >>> - >>> hugetlb_vma_lock_read(vma); >>> pte = hugetlb_walk(vma, haddr, huge_page_size(h)); >>> if (!pte) >> Did you fix why the warning was placed there in the first place? (IIRC, at >> least unsharing support needs to be added, maybe more) > > Feel free to have a look at patch 2 - it should be done there, hopefully in > the right way. And IIUC it could be a bug to not do that before (besides > CoR there was also the pgtable permission checks that was missing). More > details in patch 2's commit message. Thanks, Oh, that slipped my eyes (unsharing is not really a permission check) -- and the patch description could have been more explicit about why we can now lift the restrictions. For the records: we don't use CoR terminology upstream. As suggested by John, we use "GUP-triggered unsharing". As unsharing only applies to FOLL_PIN, it doesn't quite fit into patch #2. Either move that to this patch or squash both.
On Wed, Jun 14, 2023 at 05:17:13PM +0200, David Hildenbrand wrote: > On 14.06.23 17:11, Peter Xu wrote: > > On Wed, Jun 14, 2023 at 04:57:37PM +0200, David Hildenbrand wrote: > > > On 13.06.23 23:53, Peter Xu wrote: > > > > It's coming, not yet, but soon. Loose the restriction. > > > > > > > > Signed-off-by: Peter Xu <peterx@redhat.com> > > > > --- > > > > mm/hugetlb.c | 7 ------- > > > > 1 file changed, 7 deletions(-) > > > > > > > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > > > > index f037eaf9d819..31d8f18bc2e4 100644 > > > > --- a/mm/hugetlb.c > > > > +++ b/mm/hugetlb.c > > > > @@ -6467,13 +6467,6 @@ struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, > > > > spinlock_t *ptl; > > > > pte_t *pte, entry; > > > > - /* > > > > - * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via > > > > - * follow_hugetlb_page(). > > > > - */ > > > > - if (WARN_ON_ONCE(flags & FOLL_PIN)) > > > > - return NULL; > > > > - > > > > hugetlb_vma_lock_read(vma); > > > > pte = hugetlb_walk(vma, haddr, huge_page_size(h)); > > > > if (!pte) > > > Did you fix why the warning was placed there in the first place? (IIRC, at > > > least unsharing support needs to be added, maybe more) > > > > Feel free to have a look at patch 2 - it should be done there, hopefully in > > the right way. And IIUC it could be a bug to not do that before (besides > > CoR there was also the pgtable permission checks that was missing). More > > details in patch 2's commit message. Thanks, > > Oh, that slipped my eyes (unsharing is not really a permission check) -- and I think it is still a "permission check"? It means, we forbid anyone R/O taking the page if it's not exclusively owned, just like we forbid anyone RW taking the page if it's not writable? It's just that the permission check only applies to PIN which follow_page() doesn't yet care, so it won't ever trigger. > the patch description could have been more explicit about why we can now > lift the restrictions. > > For the records: we don't use CoR terminology upstream. As suggested by > John, we use "GUP-triggered unsharing". Sure. > > As unsharing only applies to FOLL_PIN, it doesn't quite fit into patch #2. > Either move that to this patch or squash both. Sure, no strong opinions here. The plan is _if_ someone wants to backport patch 2, this patch should not be part of it. But then maybe it makes more sense to move the CoR change there into this one, not because "it's not permission check", but because CoR is not relevant in follow_page(), so not relevant to a backport.
On 14.06.23 17:31, Peter Xu wrote: > On Wed, Jun 14, 2023 at 05:17:13PM +0200, David Hildenbrand wrote: >> On 14.06.23 17:11, Peter Xu wrote: >>> On Wed, Jun 14, 2023 at 04:57:37PM +0200, David Hildenbrand wrote: >>>> On 13.06.23 23:53, Peter Xu wrote: >>>>> It's coming, not yet, but soon. Loose the restriction. >>>>> >>>>> Signed-off-by: Peter Xu <peterx@redhat.com> >>>>> --- >>>>> mm/hugetlb.c | 7 ------- >>>>> 1 file changed, 7 deletions(-) >>>>> >>>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>>>> index f037eaf9d819..31d8f18bc2e4 100644 >>>>> --- a/mm/hugetlb.c >>>>> +++ b/mm/hugetlb.c >>>>> @@ -6467,13 +6467,6 @@ struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, >>>>> spinlock_t *ptl; >>>>> pte_t *pte, entry; >>>>> - /* >>>>> - * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via >>>>> - * follow_hugetlb_page(). >>>>> - */ >>>>> - if (WARN_ON_ONCE(flags & FOLL_PIN)) >>>>> - return NULL; >>>>> - >>>>> hugetlb_vma_lock_read(vma); >>>>> pte = hugetlb_walk(vma, haddr, huge_page_size(h)); >>>>> if (!pte) >>>> Did you fix why the warning was placed there in the first place? (IIRC, at >>>> least unsharing support needs to be added, maybe more) >>> >>> Feel free to have a look at patch 2 - it should be done there, hopefully in >>> the right way. And IIUC it could be a bug to not do that before (besides >>> CoR there was also the pgtable permission checks that was missing). More >>> details in patch 2's commit message. Thanks, >> >> Oh, that slipped my eyes (unsharing is not really a permission check) -- and > > I think it is still a "permission check"? It means, we forbid anyone R/O > taking the page if it's not exclusively owned, just like we forbid anyone > RW taking the page if it's not writable? Agreed, just not in the traditional PTE-protection case. > > It's just that the permission check only applies to PIN which follow_page() > doesn't yet care, so it won't ever trigger. > >> the patch description could have been more explicit about why we can now >> lift the restrictions. >> >> For the records: we don't use CoR terminology upstream. As suggested by >> John, we use "GUP-triggered unsharing". > > Sure. > >> >> As unsharing only applies to FOLL_PIN, it doesn't quite fit into patch #2. >> Either move that to this patch or squash both. > > Sure, no strong opinions here. > > The plan is _if_ someone wants to backport patch 2, this patch should not > be part of it. But then maybe it makes more sense to move the CoR change > there into this one, not because "it's not permission check", but because > CoR is not relevant in follow_page(), so not relevant to a backport. Right. Then just call patch #2 "Add missing write-permission check" and this patch "Support FOLL_PIN in hugetlb_follow_page_mask()" or sth. like that. Regarding the backport, I really wonder if patch #2 is required at all, because I didn't sport any applicable FOLL_WRITE users. Maybe there were some? Hm. If it's not applicable, a single "Support FOLL_PIN in hugetlb_follow_page_mask()" patch might be cleanest.
On Wed, Jun 14, 2023 at 05:47:31PM +0200, David Hildenbrand wrote: > Right. Then just call patch #2 "Add missing write-permission check" and this > patch "Support FOLL_PIN in hugetlb_follow_page_mask()" or sth. like that. > > Regarding the backport, I really wonder if patch #2 is required at all, > because I didn't sport any applicable FOLL_WRITE users. Maybe there were > some? Hm. If it's not applicable, a single "Support FOLL_PIN in > hugetlb_follow_page_mask()" patch might be cleanest. Yeah, I agree. The code is definitely needed, not the split of patches if no need for a backport. Let me merge then.
On 06/14/23 11:51, Peter Xu wrote: > On Wed, Jun 14, 2023 at 05:47:31PM +0200, David Hildenbrand wrote: > > Right. Then just call patch #2 "Add missing write-permission check" and this > > patch "Support FOLL_PIN in hugetlb_follow_page_mask()" or sth. like that. > > > > Regarding the backport, I really wonder if patch #2 is required at all, > > because I didn't sport any applicable FOLL_WRITE users. Maybe there were > > some? Hm. If it's not applicable, a single "Support FOLL_PIN in > > hugetlb_follow_page_mask()" patch might be cleanest. > > Yeah, I agree. The code is definitely needed, not the split of patches if > no need for a backport. Let me merge then. > Should have read this before adding my RB to patch 2. I assumed no backport. Agree, than merging the gup_must_unshare here makes more sense.
On Wed, Jun 14, 2023 at 05:25:25PM -0700, Mike Kravetz wrote: > On 06/14/23 11:51, Peter Xu wrote: > > On Wed, Jun 14, 2023 at 05:47:31PM +0200, David Hildenbrand wrote: > > > Right. Then just call patch #2 "Add missing write-permission check" and this > > > patch "Support FOLL_PIN in hugetlb_follow_page_mask()" or sth. like that. > > > > > > Regarding the backport, I really wonder if patch #2 is required at all, > > > because I didn't sport any applicable FOLL_WRITE users. Maybe there were > > > some? Hm. If it's not applicable, a single "Support FOLL_PIN in > > > hugetlb_follow_page_mask()" patch might be cleanest. > > > > Yeah, I agree. The code is definitely needed, not the split of patches if > > no need for a backport. Let me merge then. > > > > Should have read this before adding my RB to patch 2. I assumed no > backport. Agree, than merging the gup_must_unshare here makes more sense. Thanks for taking a look! No worries, I'll make bold to just take your R-b over the merged patch when I repost, according to your R-b in patch 2 and the comment here. I hope it's fine to you.
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index f037eaf9d819..31d8f18bc2e4 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -6467,13 +6467,6 @@ struct page *hugetlb_follow_page_mask(struct vm_area_struct *vma, spinlock_t *ptl; pte_t *pte, entry; - /* - * FOLL_PIN is not supported for follow_page(). Ordinary GUP goes via - * follow_hugetlb_page(). - */ - if (WARN_ON_ONCE(flags & FOLL_PIN)) - return NULL; - hugetlb_vma_lock_read(vma); pte = hugetlb_walk(vma, haddr, huge_page_size(h)); if (!pte)
It's coming, not yet, but soon. Loose the restriction. Signed-off-by: Peter Xu <peterx@redhat.com> --- mm/hugetlb.c | 7 ------- 1 file changed, 7 deletions(-)