Re: not issuing vma_start_write() in dup_mmap() if the caller is single-threaded

[Mateusz Guzik <mjguzik@gmail.com>]

On Sat, Mar 29, 2025 at 1:57 AM Suren Baghdasaryan <surenb@google.com> wrote:
> On Wed, Mar 26, 2025 at 10:46 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
> > Here is the original:
> > https://lore.kernel.org/all/20230804214620.btgwhsszsd7rh6nf@f/
> >
> > the thread is weirdly long and I recommend not opening it without a
> > good reason, I link it for reference if needed.
>
> I had to re-read it to remember what it was all about :) To bring
> others up-to-speed, the suggested change would look something like
> this:
>
> @@ -638,9 +640,11 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>         unsigned long charge = 0;
>         LIST_HEAD(uf);
>         VMA_ITERATOR(vmi, mm, 0);
> +       bool only_user;
>
>         if (mmap_write_lock_killable(oldmm))
>                 return -EINTR;
> +       only_user = atomic_read(&oldmm->mm_users) == 1;
>         flush_cache_dup_mm(oldmm);
>         uprobe_dup_mmap(oldmm, mm);
>         /*
> @@ -664,8 +668,11 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
>         mt_clear_in_rcu(vmi.mas.tree);
>         for_each_vma(vmi, mpnt) {
>                 struct file *file;
>
> -               vma_start_write(mpnt);
> +               if (!only_user)
> +                       vma_start_write(mpnt);
>                 if (mpnt->vm_flags & VM_DONTCOPY) {
>                         retval = vma_iter_clear_gfp(&vmi, mpnt->vm_start,
>                                                     mpnt->vm_end, GFP_KERNEL);
>
> >
> > At the time there were woes concerning stability of the new locking
> > scheme, resulting in CC list being rather excessive. As such I'm
> > starting a new thread with a modest list, not sure who to add though.
> >
> > So dup_mmap() holds the caller mmap_sem for writing and calls
> > vma_start_write() to protect against fault handling in another threads
> > using the same mm.
> >
> > If this is the only thread with this ->mm in place, there are no
> > sibling threads to worry about and this can be checked with mm_users
> > == 1.
> >
> > AFAICS all remote accesses require the mmap_sem to also be held, which
> > provides exclusion against dup_mmap, meaning they don't pose a problem
> > either.
>
> Yes, I believe the optimization you proposed would be safe.
>
> >
> > The patch to merely skip locking is a few liner and I would officially
> > submit it myself, but for my taste an assert is needed in fault
> > handling to runtime test the above invariant.
>
> Hmm. Adding an assert in the pagefault path that checks whether the
> fault is triggered during dup_mmap() && mm_users==1 would not be
> trivial. We would need to indicate that we expect no page faults while
> in that section of dup_mmap() (maybe a flag inside mm_struct) and then
> assert that this flag is not set inside the pagefault handling path.
> I'm not sure it's worth the complexity... As was discussed in that
> thread, the only other task that might fault a page would be external
> and therefore would have to go through
> access_remote_vm()->mmap_read_lock_killable() and since dup_mmap()
> already holds mmap_write_lock the new user would have to wait.
>
> > So happens I really
> > can't be bothered to figure out how to sort it out and was hoping you
> > would step in. ;) Alternatively if you guys don't think the assert is
> > warranted, that's your business.
>
> I don't think it's worth it but I'll CC Matthew and Lorenzo (you
> already CC'ed Liam) to get their opinion.
>
> >
> > As for whether this can save any locking -- yes:
>
> Yeah, I'm sure it will make a difference in performance. While forking
> we are currently locking each VMA separately, so skipping that would
> be nice.
> Folks, WDYT? Do we need a separate assertion that pagefault can't
> happen if mm_users==1 and we are holding mmap_write_lock
> (access_remote_vm() will block)?

pseudocode-wise I was thinking in the lines of the following in the fault path:

if (current->mm != vma->vm_mm)
       mmap_assert_locked(vma->vm_mm);

with the assumption that mmap_assert_locked expands to nothing without debug

>
> >
> > I added a probe (below for reference) with two args: whether we are
> > single-threaded and vma_start_write() returning whether it took the
> > down/up cycle and ran make -j 20 in the kernel dir.
> >
> > The lock was taken for every single vma (377913 in total), while all
> > forking processes were single-threaded. Or to put it differently all
> > of these were skippable.
> >
> > the probe (total hack):
> > bpftrace -e 'kprobe:dup_probe { @[arg0, arg1] = count(); }'
> >
> > probe diff:
> > diff --git a/fs/namei.c b/fs/namei.c
> > index ecb7b95c2ca3..d6cde76eda81 100644
> > --- a/fs/namei.c
> > +++ b/fs/namei.c
> > @@ -5459,3 +5459,7 @@ const struct inode_operations
> > page_symlink_inode_operations = {
> >         .get_link       = page_get_link,
> >  };
> >  EXPORT_SYMBOL(page_symlink_inode_operations);
> > +
> > +void dup_probe(int, int);
> > +void dup_probe(int, int) { }
> > +EXPORT_SYMBOL(dup_probe);
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 1f80baddacc5..f7b1f0a02f2e 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -760,12 +760,12 @@ static bool __is_vma_write_locked(struct
> > vm_area_struct *vma, unsigned int *mm_l
> >   * Exclude concurrent readers under the per-VMA lock until the currently
> >   * write-locked mmap_lock is dropped or downgraded.
> >   */
> > -static inline void vma_start_write(struct vm_area_struct *vma)
> > +static inline bool vma_start_write(struct vm_area_struct *vma)
> >  {
> >         unsigned int mm_lock_seq;
> >
> >         if (__is_vma_write_locked(vma, &mm_lock_seq))
> > -               return;
> > +               return false;
> >
> >         down_write(&vma->vm_lock->lock);
> >         /*
> > @@ -776,6 +776,7 @@ static inline void vma_start_write(struct
> > vm_area_struct *vma)
> >          */
> >         WRITE_ONCE(vma->vm_lock_seq, mm_lock_seq);
> >         up_write(&vma->vm_lock->lock);
> > +       return true;
> >  }
> >
> >  static inline void vma_assert_write_locked(struct vm_area_struct *vma)
> > diff --git a/kernel/fork.c b/kernel/fork.c
> > index 735405a9c5f3..0cc56255a339 100644
> > --- a/kernel/fork.c
> > +++ b/kernel/fork.c
> > @@ -629,6 +629,8 @@ static void dup_mm_exe_file(struct mm_struct *mm,
> > struct mm_struct *oldmm)
> >                 pr_warn_once("exe_file_deny_write_access() failed in
> > %s\n", __func__);
> >  }
> >
> > +void dup_probe(int, int);
> > +
> >  #ifdef CONFIG_MMU
> >  static __latent_entropy int dup_mmap(struct mm_struct *mm,
> >                                         struct mm_struct *oldmm)
> > @@ -638,9 +640,11 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
> >         unsigned long charge = 0;
> >         LIST_HEAD(uf);
> >         VMA_ITERATOR(vmi, mm, 0);
> > +       bool only_user;
> >
> >         if (mmap_write_lock_killable(oldmm))
> >                 return -EINTR;
> > +       only_user = atomic_read(&oldmm->mm_users) == 1;
> >         flush_cache_dup_mm(oldmm);
> >         uprobe_dup_mmap(oldmm, mm);
> >         /*
> > @@ -664,8 +668,11 @@ static __latent_entropy int dup_mmap(struct mm_struct *mm,
> >         mt_clear_in_rcu(vmi.mas.tree);
> >         for_each_vma(vmi, mpnt) {
> >                 struct file *file;
> > +               bool locked;
> > +
> > +               locked = vma_start_write(mpnt);
> > +               dup_probe(only_user ? 1 :0, locked ? 1 : 0);
> >
> > -               vma_start_write(mpnt);
> >                 if (mpnt->vm_flags & VM_DONTCOPY) {
> >                         retval = vma_iter_clear_gfp(&vmi, mpnt->vm_start,
> >                                                     mpnt->vm_end, GFP_KERNEL);
> >
> > --
> > Mateusz Guzik <mjguzik gmail.com>



-- 
Mateusz Guzik <mjguzik gmail.com>