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@xxxxxxxxxx>
---
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.
--
Cheers,
David / dhildenb
