diff mbox series

[v2,08/10] mm/hugetlb: Make walk_hugetlb_range() safe to pmd unshare

Message ID 20221207203034.650899-9-peterx@redhat.com
State New
Headers
Series [v2,01/10] mm/hugetlb: Let vma_offset_start() to return start |

Commit Message

Peter Xu Dec. 7, 2022, 8:30 p.m. UTC 
  Since walk_hugetlb_range() walks the pgtable, it needs the vma lock
to make sure the pgtable page will not be freed concurrently.

Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
Signed-off-by: Peter Xu <peterx@redhat.com>
---
 arch/s390/mm/gmap.c      |  2 ++
 fs/proc/task_mmu.c       |  2 ++
 include/linux/pagewalk.h | 11 ++++++++++-
 mm/hmm.c                 | 15 ++++++++++++++-
 mm/pagewalk.c            |  2 ++
 5 files changed, 30 insertions(+), 2 deletions(-)

Comments

John Hubbard Dec. 7, 2022, 8:34 p.m. UTC | #1 
On 12/7/22 12:30, Peter Xu wrote:
> Since walk_hugetlb_range() walks the pgtable, it needs the vma lock
> to make sure the pgtable page will not be freed concurrently.
> 
> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   arch/s390/mm/gmap.c      |  2 ++
>   fs/proc/task_mmu.c       |  2 ++
>   include/linux/pagewalk.h | 11 ++++++++++-
>   mm/hmm.c                 | 15 ++++++++++++++-
>   mm/pagewalk.c            |  2 ++
>   5 files changed, 30 insertions(+), 2 deletions(-)

Reviewed-by: John Hubbard <jhubbard@nvidia.com>


thanks,
David Hildenbrand Dec. 8, 2022, 1:14 p.m. UTC | #2 
On 07.12.22 21:30, Peter Xu wrote:
> Since walk_hugetlb_range() walks the pgtable, it needs the vma lock
> to make sure the pgtable page will not be freed concurrently.
> 
> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
> Signed-off-by: Peter Xu <peterx@redhat.com>
> ---
>   arch/s390/mm/gmap.c      |  2 ++
>   fs/proc/task_mmu.c       |  2 ++
>   include/linux/pagewalk.h | 11 ++++++++++-
>   mm/hmm.c                 | 15 ++++++++++++++-
>   mm/pagewalk.c            |  2 ++
>   5 files changed, 30 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> index 8947451ae021..292a54c490d4 100644
> --- a/arch/s390/mm/gmap.c
> +++ b/arch/s390/mm/gmap.c
> @@ -2643,7 +2643,9 @@ static int __s390_enable_skey_hugetlb(pte_t *pte, unsigned long addr,
>   	end = start + HPAGE_SIZE - 1;
>   	__storage_key_init_range(start, end);
>   	set_bit(PG_arch_1, &page->flags);
> +	hugetlb_vma_unlock_read(walk->vma);
>   	cond_resched();
> +	hugetlb_vma_lock_read(walk->vma);
>   	return 0;
>   }
>   
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index e35a0398db63..cf3887fb2905 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1613,7 +1613,9 @@ static int pagemap_hugetlb_range(pte_t *ptep, unsigned long hmask,
>   			frame++;
>   	}
>   
> +	hugetlb_vma_unlock_read(walk->vma);
>   	cond_resched();
> +	hugetlb_vma_lock_read(walk->vma);

We already hold the mmap_lock and reschedule. Even without the 
cond_resched() we might happily reschedule on a preemptive kernel. So 
I'm not sure if this optimization is strictly required or even helpful 
in practice here.

In the worst case, concurrent unsharing would have to wait.
For example, s390_enable_skey() is called at most once for a VM, for 
most VMs it gets never even called.

Or am I missing something important?
Peter Xu Dec. 8, 2022, 8:47 p.m. UTC | #3 
On Thu, Dec 08, 2022 at 02:14:42PM +0100, David Hildenbrand wrote:
> On 07.12.22 21:30, Peter Xu wrote:
> > Since walk_hugetlb_range() walks the pgtable, it needs the vma lock
> > to make sure the pgtable page will not be freed concurrently.
> > 
> > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> > ---
> >   arch/s390/mm/gmap.c      |  2 ++
> >   fs/proc/task_mmu.c       |  2 ++
> >   include/linux/pagewalk.h | 11 ++++++++++-
> >   mm/hmm.c                 | 15 ++++++++++++++-
> >   mm/pagewalk.c            |  2 ++
> >   5 files changed, 30 insertions(+), 2 deletions(-)
> > 
> > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> > index 8947451ae021..292a54c490d4 100644
> > --- a/arch/s390/mm/gmap.c
> > +++ b/arch/s390/mm/gmap.c
> > @@ -2643,7 +2643,9 @@ static int __s390_enable_skey_hugetlb(pte_t *pte, unsigned long addr,
> >   	end = start + HPAGE_SIZE - 1;
> >   	__storage_key_init_range(start, end);
> >   	set_bit(PG_arch_1, &page->flags);
> > +	hugetlb_vma_unlock_read(walk->vma);
> >   	cond_resched();
> > +	hugetlb_vma_lock_read(walk->vma);
> >   	return 0;
> >   }
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index e35a0398db63..cf3887fb2905 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -1613,7 +1613,9 @@ static int pagemap_hugetlb_range(pte_t *ptep, unsigned long hmask,
> >   			frame++;
> >   	}
> > +	hugetlb_vma_unlock_read(walk->vma);
> >   	cond_resched();
> > +	hugetlb_vma_lock_read(walk->vma);
> 
> We already hold the mmap_lock and reschedule. Even without the
> cond_resched() we might happily reschedule on a preemptive kernel. So I'm
> not sure if this optimization is strictly required or even helpful in
> practice here.

It's just low hanging fruit if we need that complexity anyway.

That's also why I didn't do that for v1 (where I missed hmm special case,
though..), but I think since we'll need that anyway, we'd better release
the vma lock if we can easily do so.

mmap_lock is just more special because it needs more work in the caller to
release (e.g. vma invalidations).  Otherwise I'm happy dropping that too.

> 
> In the worst case, concurrent unsharing would have to wait.
> For example, s390_enable_skey() is called at most once for a VM, for most
> VMs it gets never even called.
> 
> Or am I missing something important?

Nothing important.  I just don't see why we need to strictly follow the
same release rule of mmap_lock here when talking about vma lock.

In short - if we can drop a lock earlier before sleep, why not?

I tend to just keep it as-is, but let me know if you have further thoughts
or concerns.
Peter Xu Dec. 8, 2022, 9:20 p.m. UTC | #4 
On Thu, Dec 08, 2022 at 03:47:26PM -0500, Peter Xu wrote:
> On Thu, Dec 08, 2022 at 02:14:42PM +0100, David Hildenbrand wrote:
> > On 07.12.22 21:30, Peter Xu wrote:
> > > Since walk_hugetlb_range() walks the pgtable, it needs the vma lock
> > > to make sure the pgtable page will not be freed concurrently.
> > > 
> > > Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
> > > Signed-off-by: Peter Xu <peterx@redhat.com>
> > > ---
> > >   arch/s390/mm/gmap.c      |  2 ++
> > >   fs/proc/task_mmu.c       |  2 ++
> > >   include/linux/pagewalk.h | 11 ++++++++++-
> > >   mm/hmm.c                 | 15 ++++++++++++++-
> > >   mm/pagewalk.c            |  2 ++
> > >   5 files changed, 30 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
> > > index 8947451ae021..292a54c490d4 100644
> > > --- a/arch/s390/mm/gmap.c
> > > +++ b/arch/s390/mm/gmap.c
> > > @@ -2643,7 +2643,9 @@ static int __s390_enable_skey_hugetlb(pte_t *pte, unsigned long addr,
> > >   	end = start + HPAGE_SIZE - 1;
> > >   	__storage_key_init_range(start, end);
> > >   	set_bit(PG_arch_1, &page->flags);
> > > +	hugetlb_vma_unlock_read(walk->vma);
> > >   	cond_resched();
> > > +	hugetlb_vma_lock_read(walk->vma);
> > >   	return 0;
> > >   }
> > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > index e35a0398db63..cf3887fb2905 100644
> > > --- a/fs/proc/task_mmu.c
> > > +++ b/fs/proc/task_mmu.c
> > > @@ -1613,7 +1613,9 @@ static int pagemap_hugetlb_range(pte_t *ptep, unsigned long hmask,
> > >   			frame++;
> > >   	}
> > > +	hugetlb_vma_unlock_read(walk->vma);
> > >   	cond_resched();
> > > +	hugetlb_vma_lock_read(walk->vma);
> > 
> > We already hold the mmap_lock and reschedule. Even without the
> > cond_resched() we might happily reschedule on a preemptive kernel. So I'm
> > not sure if this optimization is strictly required or even helpful in
> > practice here.
> 
> It's just low hanging fruit if we need that complexity anyway.
> 
> That's also why I didn't do that for v1 (where I missed hmm special case,
> though..), but I think since we'll need that anyway, we'd better release
> the vma lock if we can easily do so.
> 
> mmap_lock is just more special because it needs more work in the caller to
> release (e.g. vma invalidations).  Otherwise I'm happy dropping that too.
> 
> > 
> > In the worst case, concurrent unsharing would have to wait.
> > For example, s390_enable_skey() is called at most once for a VM, for most
> > VMs it gets never even called.
> > 
> > Or am I missing something important?
> 
> Nothing important.  I just don't see why we need to strictly follow the
> same release rule of mmap_lock here when talking about vma lock.
> 
> In short - if we can drop a lock earlier before sleep, why not?
> 
> I tend to just keep it as-is, but let me know if you have further thoughts
> or concerns.

One thing I can do better here is:

-       cond_resched();
+
+       if (need_resched()) {
+               hugetlb_vma_unlock_read(walk->vma);
+               cond_resched();
+               hugetlb_vma_lock_read(walk->vma);
+       }
+

It's a pity we don't have rwsem version of cond_resched_rwlock_read(), or
it'll be even cleaner.
David Hildenbrand Dec. 9, 2022, 10:24 a.m. UTC | #5 
On 08.12.22 21:47, Peter Xu wrote:
> On Thu, Dec 08, 2022 at 02:14:42PM +0100, David Hildenbrand wrote:
>> On 07.12.22 21:30, Peter Xu wrote:
>>> Since walk_hugetlb_range() walks the pgtable, it needs the vma lock
>>> to make sure the pgtable page will not be freed concurrently.
>>>
>>> Reviewed-by: Mike Kravetz <mike.kravetz@oracle.com>
>>> Signed-off-by: Peter Xu <peterx@redhat.com>
>>> ---
>>>    arch/s390/mm/gmap.c      |  2 ++
>>>    fs/proc/task_mmu.c       |  2 ++
>>>    include/linux/pagewalk.h | 11 ++++++++++-
>>>    mm/hmm.c                 | 15 ++++++++++++++-
>>>    mm/pagewalk.c            |  2 ++
>>>    5 files changed, 30 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
>>> index 8947451ae021..292a54c490d4 100644
>>> --- a/arch/s390/mm/gmap.c
>>> +++ b/arch/s390/mm/gmap.c
>>> @@ -2643,7 +2643,9 @@ static int __s390_enable_skey_hugetlb(pte_t *pte, unsigned long addr,
>>>    	end = start + HPAGE_SIZE - 1;
>>>    	__storage_key_init_range(start, end);
>>>    	set_bit(PG_arch_1, &page->flags);
>>> +	hugetlb_vma_unlock_read(walk->vma);
>>>    	cond_resched();
>>> +	hugetlb_vma_lock_read(walk->vma);
>>>    	return 0;
>>>    }
>>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>>> index e35a0398db63..cf3887fb2905 100644
>>> --- a/fs/proc/task_mmu.c
>>> +++ b/fs/proc/task_mmu.c
>>> @@ -1613,7 +1613,9 @@ static int pagemap_hugetlb_range(pte_t *ptep, unsigned long hmask,
>>>    			frame++;
>>>    	}
>>> +	hugetlb_vma_unlock_read(walk->vma);
>>>    	cond_resched();
>>> +	hugetlb_vma_lock_read(walk->vma);
>>
>> We already hold the mmap_lock and reschedule. Even without the
>> cond_resched() we might happily reschedule on a preemptive kernel. So I'm
>> not sure if this optimization is strictly required or even helpful in
>> practice here.
> 
> It's just low hanging fruit if we need that complexity anyway.
> 
> That's also why I didn't do that for v1 (where I missed hmm special case,
> though..), but I think since we'll need that anyway, we'd better release
> the vma lock if we can easily do so.
> 
> mmap_lock is just more special because it needs more work in the caller to
> release (e.g. vma invalidations).  Otherwise I'm happy dropping that too.
> 
>>
>> In the worst case, concurrent unsharing would have to wait.
>> For example, s390_enable_skey() is called at most once for a VM, for most
>> VMs it gets never even called.
>>
>> Or am I missing something important?
> 
> Nothing important.  I just don't see why we need to strictly follow the
> same release rule of mmap_lock here when talking about vma lock.
> 
> In short - if we can drop a lock earlier before sleep, why not?
> 
> I tend to just keep it as-is, but let me know if you have further thoughts
> or concerns.

To me this looks like a possibly unnecessary optimization, requiring 
additional code. IMHO, possibly unnecessary unlock+relock makes thatthat 
code harder to get.

For such cases, it would be good to have any evidence that it really helps.
Peter Xu Dec. 9, 2022, 2:39 p.m. UTC | #6 
On Fri, Dec 09, 2022 at 11:24:55AM +0100, David Hildenbrand wrote:
> For such cases, it would be good to have any evidence that it really helps.

I don't know much on the s390 path, but if a process has a large hugetlb
vma, even MADV_DONTNEED will be blocked for whatever long time if there's
another process or thread scanning pagemap for this vma.

Would this justify a bit?

It's just that the vma lock is taken write far more than mmap_lock taken
write I think, meanwhile what we need here is only release the lock and
retake, nothing else.  It didn't make things over complicated, IMO.

No strong ipinion from my side, as I said to me it's really low hanging
fruit.  If you still think that doesn't justify and if Mike doesn't have a
preference either I can just drop it for later.
David Hildenbrand Dec. 9, 2022, 3:18 p.m. UTC | #7 
On 09.12.22 15:39, Peter Xu wrote:
> On Fri, Dec 09, 2022 at 11:24:55AM +0100, David Hildenbrand wrote:
>> For such cases, it would be good to have any evidence that it really helps.
> 
> I don't know much on the s390 path, but if a process has a large hugetlb
> vma, even MADV_DONTNEED will be blocked for whatever long time if there's
> another process or thread scanning pagemap for this vma.
> 
> Would this justify a bit?

I get your point. But that raises the question if we should voluntarily 
drop the VMA lock already in the caller every now and then on such large 
VMAs and maybe move even the cond_resched() into the common page walker, 
if you get what I mean?

On a preemtible kernel you could reschedule just before you drop the 
lock and call cond_resched() ... hmm

No strong opinion here, it just looked a bit weird to optimize for a 
cond_resched() if we might just reschedule either way even without the 
cond_resched().
Peter Xu Dec. 9, 2022, 4:48 p.m. UTC | #8 
On Fri, Dec 09, 2022 at 04:18:11PM +0100, David Hildenbrand wrote:
> On 09.12.22 15:39, Peter Xu wrote:
> > On Fri, Dec 09, 2022 at 11:24:55AM +0100, David Hildenbrand wrote:
> > > For such cases, it would be good to have any evidence that it really helps.
> > 
> > I don't know much on the s390 path, but if a process has a large hugetlb
> > vma, even MADV_DONTNEED will be blocked for whatever long time if there's
> > another process or thread scanning pagemap for this vma.
> > 
> > Would this justify a bit?
> 
> I get your point. But that raises the question if we should voluntarily drop
> the VMA lock already in the caller every now and then on such large VMAs and
> maybe move even the cond_resched() into the common page walker, if you get
> what I mean?
> 
> On a preemtible kernel you could reschedule just before you drop the lock
> and call cond_resched() ... hmm

On full preempt the old cond_resched() doesn't work anyway.  IIUC most
distros now should be using dynamic preempt which makes voluntary mode by
default, which this change should help.

> 
> No strong opinion here, it just looked a bit weird to optimize for a
> cond_resched() if we might just reschedule either way even without the
> cond_resched().

It's really not the core of the patchset.  Let me drop it for now.
diff mbox series

Patch

diff --git a/arch/s390/mm/gmap.c b/arch/s390/mm/gmap.c
index 8947451ae021..292a54c490d4 100644
--- a/arch/s390/mm/gmap.c
+++ b/arch/s390/mm/gmap.c
@@ -2643,7 +2643,9 @@  static int __s390_enable_skey_hugetlb(pte_t *pte, unsigned long addr,
 	end = start + HPAGE_SIZE - 1;
 	__storage_key_init_range(start, end);
 	set_bit(PG_arch_1, &page->flags);
+	hugetlb_vma_unlock_read(walk->vma);
 	cond_resched();
+	hugetlb_vma_lock_read(walk->vma);
 	return 0;
 }
 
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index e35a0398db63..cf3887fb2905 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1613,7 +1613,9 @@  static int pagemap_hugetlb_range(pte_t *ptep, unsigned long hmask,
 			frame++;
 	}
 
+	hugetlb_vma_unlock_read(walk->vma);
 	cond_resched();
+	hugetlb_vma_lock_read(walk->vma);
 
 	return err;
 }
diff --git a/include/linux/pagewalk.h b/include/linux/pagewalk.h
index 959f52e5867d..27a6df448ee5 100644
--- a/include/linux/pagewalk.h
+++ b/include/linux/pagewalk.h
@@ -21,7 +21,16 @@  struct mm_walk;
  *			depth is -1 if not known, 0:PGD, 1:P4D, 2:PUD, 3:PMD.
  *			Any folded depths (where PTRS_PER_P?D is equal to 1)
  *			are skipped.
- * @hugetlb_entry:	if set, called for each hugetlb entry
+ * @hugetlb_entry:	if set, called for each hugetlb entry. This hook
+ *			function is called with the vma lock held, in order to
+ *			protect against a concurrent freeing of the pte_t* or
+ *			the ptl. In some cases, the hook function needs to drop
+ *			and retake the vma lock in order to avoid deadlocks
+ *			while calling other functions. In such cases the hook
+ *			function must either refrain from accessing the pte or
+ *			ptl after dropping the vma lock, or else revalidate
+ *			those items after re-acquiring the vma lock and before
+ *			accessing them.
  * @test_walk:		caller specific callback function to determine whether
  *			we walk over the current vma or not. Returning 0 means
  *			"do page table walk over the current vma", returning
diff --git a/mm/hmm.c b/mm/hmm.c
index 3850fb625dda..796de6866089 100644
--- a/mm/hmm.c
+++ b/mm/hmm.c
@@ -493,8 +493,21 @@  static int hmm_vma_walk_hugetlb_entry(pte_t *pte, unsigned long hmask,
 	required_fault =
 		hmm_pte_need_fault(hmm_vma_walk, pfn_req_flags, cpu_flags);
 	if (required_fault) {
+		int ret;
+
 		spin_unlock(ptl);
-		return hmm_vma_fault(addr, end, required_fault, walk);
+		hugetlb_vma_unlock_read(vma);
+		/*
+		 * Avoid deadlock: drop the vma lock before calling
+		 * hmm_vma_fault(), which will itself potentially take and
+		 * drop the vma lock. This is also correct from a
+		 * protection point of view, because there is no further
+		 * use here of either pte or ptl after dropping the vma
+		 * lock.
+		 */
+		ret = hmm_vma_fault(addr, end, required_fault, walk);
+		hugetlb_vma_lock_read(vma);
+		return ret;
 	}
 
 	pfn = pte_pfn(entry) + ((start & ~hmask) >> PAGE_SHIFT);
diff --git a/mm/pagewalk.c b/mm/pagewalk.c
index 7f1c9b274906..d98564a7be57 100644
--- a/mm/pagewalk.c
+++ b/mm/pagewalk.c
@@ -302,6 +302,7 @@  static int walk_hugetlb_range(unsigned long addr, unsigned long end,
 	const struct mm_walk_ops *ops = walk->ops;
 	int err = 0;
 
+	hugetlb_vma_lock_read(vma);
 	do {
 		next = hugetlb_entry_end(h, addr, end);
 		pte = huge_pte_offset(walk->mm, addr & hmask, sz);
@@ -314,6 +315,7 @@  static int walk_hugetlb_range(unsigned long addr, unsigned long end,
 		if (err)
 			break;
 	} while (addr = next, addr != end);
+	hugetlb_vma_unlock_read(vma);
 
 	return err;
 }