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

[Suren Baghdasaryan]

On Tue, Jun 27, 2023 at 8:32 AM Peter Xu <peterx@xxxxxxxxxx> 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@xxxxxxxxxx>
> > ---
> >  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
>