diff mbox series

[v3,5/8] mm: make folio_lock_fault indicate the state of mmap_lock upon return

Message ID 20230627042321.1763765-6-surenb@google.com (mailing list archive)
State New, archived
Headers show
Series Per-VMA lock support for swap and userfaults | expand

Commit Message

Suren Baghdasaryan June 27, 2023, 4:23 a.m. UTC 
folio_lock_fault might drop mmap_lock before returning and to extend it
to work with per-VMA locks, the callers will need to know whether the
lock was dropped or is still held. Introduce new fault_flag to indicate
whether the lock got dropped and store it inside vm_fault flags.

Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
 include/linux/mm_types.h | 1 +
 mm/filemap.c             | 2 ++
 2 files changed, 3 insertions(+)

Comments

Alistair Popple June 27, 2023, 8:06 a.m. UTC | #1 
Suren Baghdasaryan <surenb@google.com> writes:

> folio_lock_fault might drop mmap_lock before returning and to extend it
> to work with per-VMA locks, the callers will need to know whether the
> lock was dropped or is still held. Introduce new fault_flag to indicate
> whether the lock got dropped and store it inside vm_fault flags.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
>  include/linux/mm_types.h | 1 +
>  mm/filemap.c             | 2 ++
>  2 files changed, 3 insertions(+)
>
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 79765e3dd8f3..6f0dbef7aa1f 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -1169,6 +1169,7 @@ enum fault_flag {
>  	FAULT_FLAG_UNSHARE =		1 << 10,
>  	FAULT_FLAG_ORIG_PTE_VALID =	1 << 11,
>  	FAULT_FLAG_VMA_LOCK =		1 << 12,
> +	FAULT_FLAG_LOCK_DROPPED =	1 << 13,

Minor nit but this should also be added to the enum documentation
comment above this.

>  };
>  
>  typedef unsigned int __bitwise zap_flags_t;
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 87b335a93530..8ad06d69895b 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1723,6 +1723,7 @@ vm_fault_t __folio_lock_fault(struct folio *folio, struct vm_fault *vmf)
>  			return VM_FAULT_RETRY;
>  
>  		mmap_read_unlock(mm);
> +		vmf->flags |= FAULT_FLAG_LOCK_DROPPED;
>  		if (vmf->flags & FAULT_FLAG_KILLABLE)
>  			folio_wait_locked_killable(folio);
>  		else
> @@ -1735,6 +1736,7 @@ vm_fault_t __folio_lock_fault(struct folio *folio, struct vm_fault *vmf)
>  		ret = __folio_lock_killable(folio);
>  		if (ret) {
>  			mmap_read_unlock(mm);
> +			vmf->flags |= FAULT_FLAG_LOCK_DROPPED;
>  			return VM_FAULT_RETRY;
>  		}
>  	} else {
Peter Xu June 27, 2023, 3:32 p.m. UTC | #2 
On Mon, Jun 26, 2023 at 09:23:18PM -0700, Suren Baghdasaryan wrote:
> folio_lock_fault might drop mmap_lock before returning and to extend it
> to work with per-VMA locks, the callers will need to know whether the
> lock was dropped or is still held. Introduce new fault_flag to indicate
> whether the lock got dropped and store it inside vm_fault flags.
> 
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
>  include/linux/mm_types.h | 1 +
>  mm/filemap.c             | 2 ++
>  2 files changed, 3 insertions(+)
> 
> diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> index 79765e3dd8f3..6f0dbef7aa1f 100644
> --- a/include/linux/mm_types.h
> +++ b/include/linux/mm_types.h
> @@ -1169,6 +1169,7 @@ enum fault_flag {
>  	FAULT_FLAG_UNSHARE =		1 << 10,
>  	FAULT_FLAG_ORIG_PTE_VALID =	1 << 11,
>  	FAULT_FLAG_VMA_LOCK =		1 << 12,
> +	FAULT_FLAG_LOCK_DROPPED =	1 << 13,
>  };
>  
>  typedef unsigned int __bitwise zap_flags_t;
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 87b335a93530..8ad06d69895b 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -1723,6 +1723,7 @@ vm_fault_t __folio_lock_fault(struct folio *folio, struct vm_fault *vmf)
>  			return VM_FAULT_RETRY;
>  
>  		mmap_read_unlock(mm);
> +		vmf->flags |= FAULT_FLAG_LOCK_DROPPED;
>  		if (vmf->flags & FAULT_FLAG_KILLABLE)
>  			folio_wait_locked_killable(folio);
>  		else
> @@ -1735,6 +1736,7 @@ vm_fault_t __folio_lock_fault(struct folio *folio, struct vm_fault *vmf)
>  		ret = __folio_lock_killable(folio);
>  		if (ret) {
>  			mmap_read_unlock(mm);
> +			vmf->flags |= FAULT_FLAG_LOCK_DROPPED;
>  			return VM_FAULT_RETRY;
>  		}
>  	} else {

IIRC we've discussed about this bits in previous version, and the consensus
was that we don't need yet another flag?  Just to recap: I think relying on
RETRY|COMPLETE would be enough for vma lock, as NOWAIT is only used by gup
while not affecting vma lockings, no?

As mentioned in the other reply, even COMPLETE won't appear for vma lock
path yet afaict, so mostly only RETRY matters here and it can 100% imply a
lock release happened.  It's just that it's very easy to still cover
COMPLETE altogether in this case, being prepared for any possible shared
support on vma locks, IMHO.

Thanks,
Suren Baghdasaryan June 27, 2023, 4 p.m. UTC | #3 
On Tue, Jun 27, 2023 at 8:32 AM Peter Xu <peterx@redhat.com> wrote:
>
> On Mon, Jun 26, 2023 at 09:23:18PM -0700, Suren Baghdasaryan wrote:
> > folio_lock_fault might drop mmap_lock before returning and to extend it
> > to work with per-VMA locks, the callers will need to know whether the
> > lock was dropped or is still held. Introduce new fault_flag to indicate
> > whether the lock got dropped and store it inside vm_fault flags.
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> >  include/linux/mm_types.h | 1 +
> >  mm/filemap.c             | 2 ++
> >  2 files changed, 3 insertions(+)
> >
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index 79765e3dd8f3..6f0dbef7aa1f 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -1169,6 +1169,7 @@ enum fault_flag {
> >       FAULT_FLAG_UNSHARE =            1 << 10,
> >       FAULT_FLAG_ORIG_PTE_VALID =     1 << 11,
> >       FAULT_FLAG_VMA_LOCK =           1 << 12,
> > +     FAULT_FLAG_LOCK_DROPPED =       1 << 13,
> >  };
> >
> >  typedef unsigned int __bitwise zap_flags_t;
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 87b335a93530..8ad06d69895b 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -1723,6 +1723,7 @@ vm_fault_t __folio_lock_fault(struct folio *folio, struct vm_fault *vmf)
> >                       return VM_FAULT_RETRY;
> >
> >               mmap_read_unlock(mm);
> > +             vmf->flags |= FAULT_FLAG_LOCK_DROPPED;
> >               if (vmf->flags & FAULT_FLAG_KILLABLE)
> >                       folio_wait_locked_killable(folio);
> >               else
> > @@ -1735,6 +1736,7 @@ vm_fault_t __folio_lock_fault(struct folio *folio, struct vm_fault *vmf)
> >               ret = __folio_lock_killable(folio);
> >               if (ret) {
> >                       mmap_read_unlock(mm);
> > +                     vmf->flags |= FAULT_FLAG_LOCK_DROPPED;
> >                       return VM_FAULT_RETRY;
> >               }
> >       } else {
>
> IIRC we've discussed about this bits in previous version, and the consensus
> was that we don't need yet another flag?  Just to recap: I think relying on
> RETRY|COMPLETE would be enough for vma lock, as NOWAIT is only used by gup
> while not affecting vma lockings, no?

Sorry for missing that point. I focused on making VMA locks being
dropped for RETRY|COMPLETE and forgot to check after that change if
RETRY|COMPLETE is enough indication to conclude that VMA lock is
dropped. Looking at that now, I'm not sure that would be always true
for file-backed page faults (including shmem_fault()), but we do not
handle them under VMA locks for now anyway, so this indeed seems like
a safe assumption. When Matthew implements file-backed support he
needs to be careful to ensure this rule still holds. With your
suggestions to drop the VMA lock at the place where we return RETRY
this seems to indeed eliminate the need for FAULT_FLAG_LOCK_DROPPED
and simplifies things. I'll try that approach and see if anything
blows up.

>
> As mentioned in the other reply, even COMPLETE won't appear for vma lock
> path yet afaict, so mostly only RETRY matters here and it can 100% imply a
> lock release happened.  It's just that it's very easy to still cover
> COMPLETE altogether in this case, being prepared for any possible shared
> support on vma locks, IMHO.

Yes and I do introduce one place where we use COMPLETE with VMA locks,
so will cover it the same way as for RETRY.
Thanks,
Suren.

>
> Thanks,
>
> --
> Peter Xu
>
Suren Baghdasaryan June 27, 2023, 4:01 p.m. UTC | #4 
On Tue, Jun 27, 2023 at 1:09 AM Alistair Popple <apopple@nvidia.com> wrote:
>
>
> Suren Baghdasaryan <surenb@google.com> writes:
>
> > folio_lock_fault might drop mmap_lock before returning and to extend it
> > to work with per-VMA locks, the callers will need to know whether the
> > lock was dropped or is still held. Introduce new fault_flag to indicate
> > whether the lock got dropped and store it inside vm_fault flags.
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> >  include/linux/mm_types.h | 1 +
> >  mm/filemap.c             | 2 ++
> >  2 files changed, 3 insertions(+)
> >
> > diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
> > index 79765e3dd8f3..6f0dbef7aa1f 100644
> > --- a/include/linux/mm_types.h
> > +++ b/include/linux/mm_types.h
> > @@ -1169,6 +1169,7 @@ enum fault_flag {
> >       FAULT_FLAG_UNSHARE =            1 << 10,
> >       FAULT_FLAG_ORIG_PTE_VALID =     1 << 11,
> >       FAULT_FLAG_VMA_LOCK =           1 << 12,
> > +     FAULT_FLAG_LOCK_DROPPED =       1 << 13,
>
> Minor nit but this should also be added to the enum documentation
> comment above this.

Thanks! Sounds like we will be dripping the new flag, so hopefully I
won't need to document it :)

>
> >  };
> >
> >  typedef unsigned int __bitwise zap_flags_t;
> > diff --git a/mm/filemap.c b/mm/filemap.c
> > index 87b335a93530..8ad06d69895b 100644
> > --- a/mm/filemap.c
> > +++ b/mm/filemap.c
> > @@ -1723,6 +1723,7 @@ vm_fault_t __folio_lock_fault(struct folio *folio, struct vm_fault *vmf)
> >                       return VM_FAULT_RETRY;
> >
> >               mmap_read_unlock(mm);
> > +             vmf->flags |= FAULT_FLAG_LOCK_DROPPED;
> >               if (vmf->flags & FAULT_FLAG_KILLABLE)
> >                       folio_wait_locked_killable(folio);
> >               else
> > @@ -1735,6 +1736,7 @@ vm_fault_t __folio_lock_fault(struct folio *folio, struct vm_fault *vmf)
> >               ret = __folio_lock_killable(folio);
> >               if (ret) {
> >                       mmap_read_unlock(mm);
> > +                     vmf->flags |= FAULT_FLAG_LOCK_DROPPED;
> >                       return VM_FAULT_RETRY;
> >               }
> >       } else {
>
diff mbox series

Patch

diff --git a/include/linux/mm_types.h b/include/linux/mm_types.h
index 79765e3dd8f3..6f0dbef7aa1f 100644
--- a/include/linux/mm_types.h
+++ b/include/linux/mm_types.h
@@ -1169,6 +1169,7 @@  enum fault_flag {
 	FAULT_FLAG_UNSHARE =		1 << 10,
 	FAULT_FLAG_ORIG_PTE_VALID =	1 << 11,
 	FAULT_FLAG_VMA_LOCK =		1 << 12,
+	FAULT_FLAG_LOCK_DROPPED =	1 << 13,
 };
 
 typedef unsigned int __bitwise zap_flags_t;
diff --git a/mm/filemap.c b/mm/filemap.c
index 87b335a93530..8ad06d69895b 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -1723,6 +1723,7 @@  vm_fault_t __folio_lock_fault(struct folio *folio, struct vm_fault *vmf)
 			return VM_FAULT_RETRY;
 
 		mmap_read_unlock(mm);
+		vmf->flags |= FAULT_FLAG_LOCK_DROPPED;
 		if (vmf->flags & FAULT_FLAG_KILLABLE)
 			folio_wait_locked_killable(folio);
 		else
@@ -1735,6 +1736,7 @@  vm_fault_t __folio_lock_fault(struct folio *folio, struct vm_fault *vmf)
 		ret = __folio_lock_killable(folio);
 		if (ret) {
 			mmap_read_unlock(mm);
+			vmf->flags |= FAULT_FLAG_LOCK_DROPPED;
 			return VM_FAULT_RETRY;
 		}
 	} else {