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

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

[Mateusz Guzik]

Hi Suren,

I brought this up long time ago, you agreed the idea works and stated
you will sort it out, but it apparently fell to the wayside (no hard
feelings though :>)

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.

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.

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. 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.

As for whether this can save any locking -- yes:

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>

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

[Suren Baghdasaryan]

Hi Mateusz,

On Wed, Mar 26, 2025 at 10:46 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> Hi Suren,
>
> I brought this up long time ago, you agreed the idea works and stated
> you will sort it out, but it apparently fell to the wayside (no hard
> feelings though :>)

Sorry about that. Yeah, that was a hectic time and I completely forgot
to look into the single-user case.

>
> 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)?
Thanks,
Suren.

>
> 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>

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

[Mateusz Guzik]

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>

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

[Suren Baghdasaryan]

On Fri, Mar 28, 2025 at 6:16 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> 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 see. IOW, if someone external is faulting a page then it has to be
holding at least mmap_read_lock. So, it's a more general check but
seems reasonable. I think adding it at the end of lock_vma_under_rcu()
under the "#ifdef CONFIG_DEBUG_VM" condition would be enough.

>
> >
> > >
> > > 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>

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

[Mateusz Guzik]

top post warning

It just hit me that userfaultfd() may be a problem. Creating it only
bumps mm_count, while mm_users gets transiently modified during
various ops.

So in particular you may end up pounding off the userfaultfd instance
to another process while being single-threaded, then mm_users == 1 and
userfaultfd may be trying to do something. I have no idea if this one
is guaranteed to take the lock.

However, the good news is that mm_count tends to be 1. If both
mm_count and mm_users are 1, then there is no usefaultfd in use and
nobody to add it either.

State of mm_count verified with: bpftrace -e 'kprobe:copy_process {
@[curtask->mm->mm_count.counter] = count(); }'

On Sat, Mar 29, 2025 at 2:35 AM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Fri, Mar 28, 2025 at 6:16 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
> >
> > 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 see. IOW, if someone external is faulting a page then it has to be
> holding at least mmap_read_lock. So, it's a more general check but
> seems reasonable. I think adding it at the end of lock_vma_under_rcu()
> under the "#ifdef CONFIG_DEBUG_VM" condition would be enough.
>
> >
> > >
> > > >
> > > > 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>



-- 
Mateusz Guzik <mjguzik gmail.com>

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

[Suren Baghdasaryan]

On Fri, Mar 28, 2025 at 6:51 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> top post warning
>
> It just hit me that userfaultfd() may be a problem. Creating it only
> bumps mm_count, while mm_users gets transiently modified during
> various ops.
>
> So in particular you may end up pounding off the userfaultfd instance
> to another process while being single-threaded, then mm_users == 1 and
> userfaultfd may be trying to do something. I have no idea if this one
> is guaranteed to take the lock.

Hmm, yeah. uffd is nasty. Even in the cases when it does
mmget_not_zero(), there is still the possibility of a race. For
example:

dup_mmap
    only_user = atomic_read(&oldmm->mm_users) == 1;

                                             userfaultfd_move()
                                                 mmget_not_zero() <--
inc mm_users

    if (!only_user)
        vma_start_write(mpnt); <-- gets skipped

                                                    move_pages()
                                                         uffd_move_lock()
                                                             uffd_lock_vma()

lock_vma_under_rcu() <-- succeeds
                                                         move_pages_pte()
    copy_page_range()

So, userfaultfd_move() will happily move pages while dup_mmap() is
doing copy_page_range(). The copied range might look quite
interesting...

>
> However, the good news is that mm_count tends to be 1. If both
> mm_count and mm_users are 1, then there is no usefaultfd in use and
> nobody to add it either.

I'm not sure... IIUC new_userfaultfd() does not take mmap_lock while
calling mmgrab(), therefore I think it can race with the code checking
its value.

>
> State of mm_count verified with: bpftrace -e 'kprobe:copy_process {
> @[curtask->mm->mm_count.counter] = count(); }'
>
> On Sat, Mar 29, 2025 at 2:35 AM Suren Baghdasaryan <surenb@google.com> wrote:
> >
> > On Fri, Mar 28, 2025 at 6:16 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
> > >
> > > 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 see. IOW, if someone external is faulting a page then it has to be
> > holding at least mmap_read_lock. So, it's a more general check but
> > seems reasonable. I think adding it at the end of lock_vma_under_rcu()
> > under the "#ifdef CONFIG_DEBUG_VM" condition would be enough.
> >
> > >
> > > >
> > > > >
> > > > > 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>
>
>
>
> --
> Mateusz Guzik <mjguzik gmail.com>

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

[Suren Baghdasaryan]

On Sun, Mar 30, 2025 at 12:23 PM Suren Baghdasaryan <surenb@google.com> wrote:
>
> On Fri, Mar 28, 2025 at 6:51 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
> >
> > top post warning
> >
> > It just hit me that userfaultfd() may be a problem. Creating it only
> > bumps mm_count, while mm_users gets transiently modified during
> > various ops.
> >
> > So in particular you may end up pounding off the userfaultfd instance
> > to another process while being single-threaded, then mm_users == 1 and
> > userfaultfd may be trying to do something. I have no idea if this one
> > is guaranteed to take the lock.
>
> Hmm, yeah. uffd is nasty. Even in the cases when it does
> mmget_not_zero(), there is still the possibility of a race. For
> example:
>
> dup_mmap
>     only_user = atomic_read(&oldmm->mm_users) == 1;
>
>                                              userfaultfd_move()
>                                                  mmget_not_zero() <--
> inc mm_users
>
>     if (!only_user)
>         vma_start_write(mpnt); <-- gets skipped
>
>                                                     move_pages()
>                                                          uffd_move_lock()
>                                                              uffd_lock_vma()
>
> lock_vma_under_rcu() <-- succeeds
>                                                          move_pages_pte()
>     copy_page_range()

Sorry about formatting. This might look a bit better:

Task 1                           Task 2
dup_mmap
     only_user = atomic_read(&oldmm->mm_users) == 1;

                                      userfaultfd_move()
                                          mmget_not_zero()

     if (!only_user)
         vma_start_write(mpnt); <-- gets skipped

                                             move_pages()
                                                  uffd_move_lock()
                                                      uffd_lock_vma()
                                                          lock_vma_under_rcu()
                                             move_pages_pte()
     copy_page_range()

>
> So, userfaultfd_move() will happily move pages while dup_mmap() is
> doing copy_page_range(). The copied range might look quite
> interesting...
>
> >
> > However, the good news is that mm_count tends to be 1. If both
> > mm_count and mm_users are 1, then there is no usefaultfd in use and
> > nobody to add it either.
>
> I'm not sure... IIUC new_userfaultfd() does not take mmap_lock while
> calling mmgrab(), therefore I think it can race with the code checking
> its value.
>
> >
> > State of mm_count verified with: bpftrace -e 'kprobe:copy_process {
> > @[curtask->mm->mm_count.counter] = count(); }'
> >
> > On Sat, Mar 29, 2025 at 2:35 AM Suren Baghdasaryan <surenb@google.com> wrote:
> > >
> > > On Fri, Mar 28, 2025 at 6:16 PM Mateusz Guzik <mjguzik@gmail.com> wrote:
> > > >
> > > > 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 see. IOW, if someone external is faulting a page then it has to be
> > > holding at least mmap_read_lock. So, it's a more general check but
> > > seems reasonable. I think adding it at the end of lock_vma_under_rcu()
> > > under the "#ifdef CONFIG_DEBUG_VM" condition would be enough.
> > >
> > > >
> > > > >
> > > > > >
> > > > > > 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>
> >
> >
> >
> > --
> > Mateusz Guzik <mjguzik gmail.com>

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

[Mateusz Guzik]

On Sun, Mar 30, 2025 at 9:23 PM Suren Baghdasaryan <surenb@google.com> wrote:
> > However, the good news is that mm_count tends to be 1. If both
> > mm_count and mm_users are 1, then there is no usefaultfd in use and
> > nobody to add it either.
>
> I'm not sure... IIUC new_userfaultfd() does not take mmap_lock while
> calling mmgrab(), therefore I think it can race with the code checking
> its value.
>

It issues:
ctx->mm = current->mm;
...
mmgrab(ctx->mm);

Thus I claim if mm_count is 1 *and* mm_users is 1 *and* we are in
dup_mmap(), nobody has a userfaultfd for our mm and there is nobody to
create it either and the optimization is saved.

-- 
Mateusz Guzik <mjguzik gmail.com>

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

[Liam R. Howlett]

* Mateusz Guzik <mjguzik@gmail.com> [250330 15:43]:
> On Sun, Mar 30, 2025 at 9:23 PM Suren Baghdasaryan <surenb@google.com> wrote:
> > > However, the good news is that mm_count tends to be 1. If both
> > > mm_count and mm_users are 1, then there is no usefaultfd in use and
> > > nobody to add it either.
> >
> > I'm not sure... IIUC new_userfaultfd() does not take mmap_lock while
> > calling mmgrab(), therefore I think it can race with the code checking
> > its value.
> >

Considering the mm struct isn't the only way to find the vmas and there
are users who use other locks to ensure the mm and vma don't go away
(rmap, for example).  It is reasonable to think that other users may use
the vma lock to avoid mm_struct accesses racing.

Although I don't know of a way this is unsafe today, we are complicating
the locking story of the mm with this change and no data has been given
on the benefit.  I don't recall any regression caused by the addition of
per-vma locking?

> 
> It issues:
> ctx->mm = current->mm;
> ...
> mmgrab(ctx->mm);
> 
> Thus I claim if mm_count is 1 *and* mm_users is 1 *and* we are in
> dup_mmap(), nobody has a userfaultfd for our mm and there is nobody to
> create it either and the optimization is saved.

mm_count is lazy, so I am not entirely sure we can trust what it says.
But maybe that's only true of mmgrab_lazy_tlb() now?

Thanks,
Liam

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

[Mateusz Guzik]

On Mon, Mar 31, 2025 at 6:43 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
>
> * Mateusz Guzik <mjguzik@gmail.com> [250330 15:43]:
> > On Sun, Mar 30, 2025 at 9:23 PM Suren Baghdasaryan <surenb@google.com> wrote:
> > > > However, the good news is that mm_count tends to be 1. If both
> > > > mm_count and mm_users are 1, then there is no usefaultfd in use and
> > > > nobody to add it either.
> > >
> > > I'm not sure... IIUC new_userfaultfd() does not take mmap_lock while
> > > calling mmgrab(), therefore I think it can race with the code checking
> > > its value.
> > >
>
> Considering the mm struct isn't the only way to find the vmas and there
> are users who use other locks to ensure the mm and vma don't go away
> (rmap, for example).  It is reasonable to think that other users may use
> the vma lock to avoid mm_struct accesses racing.
>
> Although I don't know of a way this is unsafe today, we are complicating
> the locking story of the mm with this change and no data has been given
> on the benefit.  I don't recall any regression caused by the addition of
> per-vma locking?
>

I was not involved in any of this, but I learned about the issue from lwn:
https://lwn.net/Articles/937943/

the new (at the time) per-vma locking was suffering weird crashes in
multithreaded programs and Suren ultimately fixed it by locking parent
vma at a 5% hit,
see fb49c455323f ("fork: lock VMAs of the parent process when
forking"). The patch merely adds vma_start_write(mpnt) in dup_mmap.

What I'm proposing here remedies the problem for most commonly forking
consumers (single-threaded), assuming it does work. ;)

To that end see below.

> >
> > It issues:
> > ctx->mm = current->mm;
> > ...
> > mmgrab(ctx->mm);
> >
> > Thus I claim if mm_count is 1 *and* mm_users is 1 *and* we are in
> > dup_mmap(), nobody has a userfaultfd for our mm and there is nobody to
> > create it either and the optimization is saved.
>
> mm_count is lazy, so I am not entirely sure we can trust what it says.
> But maybe that's only true of mmgrab_lazy_tlb() now?
>

warning: I don't know the Linux nomenclature here. I'm going to
outline how I see it.

There is an idiomatic way of splitting ref counts into two:
- something to prevent the struct itself from getting freed ("hold
count" where I'm from, in this case ->mm_count)
- something to prevent data used by the structure from getting freed
("use count" where I'm from, in this case ->mm_users)

mm_users > 0 keeps one ref on ->mm_count

AFAICS the scheme employed for mm follows the mold.

So with that mmgrab_lazy_tlb() example, the call bumps the count on first use.

Suppose we are left with one thread in the process and a lazy tlb
somewhere as the only consumers. mm_users is 1 because of the only
thread and mm_count is 2 -- one ref for mm_users > 0 and one ref for
lazy tlb.

Then my proposed check: ->mm_count == 1 && mm->mm_users == 1

... fails and the optimization is not used.

Now, per the above, the lock was added to protect against faults
happening in parallel. The only cases I found where this is of note
are remote accesses (e.g., from /proc/pid/cmdline) and userfaultfd.

I'm not an mm person and this is why I referred to Suren to sort this
out, hoping he would both have interest and enough knowledge about mm
to validate it.

That is to say I don't *vouch* the idea myself (otherwise I would sign
off on a patch), I am merely bringing it up again long after the dust
has settled. If the idea is a nogo, then so be it, but then it would
be nice to document somewhere why is it so.
-- 
Mateusz Guzik <mjguzik gmail.com>

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

[Suren Baghdasaryan]

On Mon, Mar 31, 2025 at 10:51 AM Mateusz Guzik <mjguzik@gmail.com> wrote:
>
> On Mon, Mar 31, 2025 at 6:43 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> >
> > * Mateusz Guzik <mjguzik@gmail.com> [250330 15:43]:
> > > On Sun, Mar 30, 2025 at 9:23 PM Suren Baghdasaryan <surenb@google.com> wrote:
> > > > > However, the good news is that mm_count tends to be 1. If both
> > > > > mm_count and mm_users are 1, then there is no usefaultfd in use and
> > > > > nobody to add it either.
> > > >
> > > > I'm not sure... IIUC new_userfaultfd() does not take mmap_lock while
> > > > calling mmgrab(), therefore I think it can race with the code checking
> > > > its value.
> > > >
> >
> > Considering the mm struct isn't the only way to find the vmas and there
> > are users who use other locks to ensure the mm and vma don't go away
> > (rmap, for example).  It is reasonable to think that other users may use
> > the vma lock to avoid mm_struct accesses racing.
> >
> > Although I don't know of a way this is unsafe today, we are complicating
> > the locking story of the mm with this change and no data has been given
> > on the benefit.  I don't recall any regression caused by the addition of
> > per-vma locking?
> >
>
> I was not involved in any of this, but I learned about the issue from lwn:
> https://lwn.net/Articles/937943/
>
> the new (at the time) per-vma locking was suffering weird crashes in
> multithreaded programs and Suren ultimately fixed it by locking parent
> vma at a 5% hit,
> see fb49c455323f ("fork: lock VMAs of the parent process when
> forking"). The patch merely adds vma_start_write(mpnt) in dup_mmap.
>
> What I'm proposing here remedies the problem for most commonly forking
> consumers (single-threaded), assuming it does work. ;)
>
> To that end see below.
>
> > >
> > > It issues:
> > > ctx->mm = current->mm;
> > > ...
> > > mmgrab(ctx->mm);
> > >
> > > Thus I claim if mm_count is 1 *and* mm_users is 1 *and* we are in
> > > dup_mmap(), nobody has a userfaultfd for our mm and there is nobody to
> > > create it either and the optimization is saved.
> >
> > mm_count is lazy, so I am not entirely sure we can trust what it says.
> > But maybe that's only true of mmgrab_lazy_tlb() now?
> >
>
> warning: I don't know the Linux nomenclature here. I'm going to
> outline how I see it.
>
> There is an idiomatic way of splitting ref counts into two:
> - something to prevent the struct itself from getting freed ("hold
> count" where I'm from, in this case ->mm_count)
> - something to prevent data used by the structure from getting freed
> ("use count" where I'm from, in this case ->mm_users)
>
> mm_users > 0 keeps one ref on ->mm_count
>
> AFAICS the scheme employed for mm follows the mold.
>
> So with that mmgrab_lazy_tlb() example, the call bumps the count on first use.
>
> Suppose we are left with one thread in the process and a lazy tlb
> somewhere as the only consumers. mm_users is 1 because of the only
> thread and mm_count is 2 -- one ref for mm_users > 0 and one ref for
> lazy tlb.
>
> Then my proposed check: ->mm_count == 1 && mm->mm_users == 1
>
> ... fails and the optimization is not used.
>
> Now, per the above, the lock was added to protect against faults
> happening in parallel. The only cases I found where this is of note
> are remote accesses (e.g., from /proc/pid/cmdline) and userfaultfd.
>
> I'm not an mm person and this is why I referred to Suren to sort this
> out, hoping he would both have interest and enough knowledge about mm
> to validate it.
>
> That is to say I don't *vouch* the idea myself (otherwise I would sign
> off on a patch), I am merely bringing it up again long after the dust
> has settled. If the idea is a nogo, then so be it, but then it would
> be nice to document somewhere why is it so.

I think it would be worth optimizing if it's as straight-forward as we
think so far. I would like to spend some more time checking uffd code
to see if anything else might be lurking there before posting the
patch. If I don't find anything new I'll post a patch sometime next
week.
Thanks,
Suren.

> --
> Mateusz Guzik <mjguzik gmail.com>

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

[Liam R. Howlett]

* Mateusz Guzik <mjguzik@gmail.com> [250331 13:51]:
> On Mon, Mar 31, 2025 at 6:43 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> >
> > * Mateusz Guzik <mjguzik@gmail.com> [250330 15:43]:
> > > On Sun, Mar 30, 2025 at 9:23 PM Suren Baghdasaryan <surenb@google.com> wrote:
> > > > > However, the good news is that mm_count tends to be 1. If both
> > > > > mm_count and mm_users are 1, then there is no usefaultfd in use and
> > > > > nobody to add it either.
> > > >
> > > > I'm not sure... IIUC new_userfaultfd() does not take mmap_lock while
> > > > calling mmgrab(), therefore I think it can race with the code checking
> > > > its value.
> > > >
> >
> > Considering the mm struct isn't the only way to find the vmas and there
> > are users who use other locks to ensure the mm and vma don't go away
> > (rmap, for example).  It is reasonable to think that other users may use
> > the vma lock to avoid mm_struct accesses racing.
> >
> > Although I don't know of a way this is unsafe today, we are complicating
> > the locking story of the mm with this change and no data has been given
> > on the benefit.  I don't recall any regression caused by the addition of
> > per-vma locking?
> >
> 
> I was not involved in any of this, but I learned about the issue from lwn:
> https://lwn.net/Articles/937943/
> 
> the new (at the time) per-vma locking was suffering weird crashes in
> multithreaded programs and Suren ultimately fixed it by locking parent
> vma at a 5% hit,
> see fb49c455323f ("fork: lock VMAs of the parent process when
> forking"). The patch merely adds vma_start_write(mpnt) in dup_mmap.

Ah, thanks for the pointer.  I forgot about that part.

You are referencing the 5% on 5,000 forks of 10,000 vmas in a tight
loop.  The kernel build time was not affected.  I'm sticking with this
being an inconsequential increase on any meaningful benchmark.

Reading the commit did trigger my memory about the 5% regression, but I
disregarded it as it seems so unlikely in real life that the benefits
outweighed the upper bounds of 5% negative.

The 5,000 forks of 10,000 vmas benchmark would also be at least a bit
faster with Suren's more recent rcu safe vma change [1].

Although, that doesn't rule out changing this to get higher number, I
still don't think the complexity of the change is worth it for a
contrived benchmark.

> 
> What I'm proposing here remedies the problem for most commonly forking
> consumers (single-threaded), assuming it does work. ;)
> 
> To that end see below.
> 
> > >
> > > It issues:
> > > ctx->mm = current->mm;
> > > ...
> > > mmgrab(ctx->mm);
> > >
> > > Thus I claim if mm_count is 1 *and* mm_users is 1 *and* we are in
> > > dup_mmap(), nobody has a userfaultfd for our mm and there is nobody to
> > > create it either and the optimization is saved.
> >
> > mm_count is lazy, so I am not entirely sure we can trust what it says.
> > But maybe that's only true of mmgrab_lazy_tlb() now?
> >
> 
> warning: I don't know the Linux nomenclature here. I'm going to
> outline how I see it.
> 
> There is an idiomatic way of splitting ref counts into two:
> - something to prevent the struct itself from getting freed ("hold
> count" where I'm from, in this case ->mm_count)
> - something to prevent data used by the structure from getting freed
> ("use count" where I'm from, in this case ->mm_users)
> 
> mm_users > 0 keeps one ref on ->mm_count
> 
> AFAICS the scheme employed for mm follows the mold.
> 
> So with that mmgrab_lazy_tlb() example, the call bumps the count on first use.
> 
> Suppose we are left with one thread in the process and a lazy tlb
> somewhere as the only consumers. mm_users is 1 because of the only
> thread and mm_count is 2 -- one ref for mm_users > 0 and one ref for
> lazy tlb.
> 
> Then my proposed check: ->mm_count == 1 && mm->mm_users == 1
> 
> ... fails and the optimization is not used.
> 
> Now, per the above, the lock was added to protect against faults
> happening in parallel. The only cases I found where this is of note
> are remote accesses (e.g., from /proc/pid/cmdline) and userfaultfd.
> 
> I'm not an mm person and this is why I referred to Suren to sort this
> out, hoping he would both have interest and enough knowledge about mm
> to validate it.
> 
> That is to say I don't *vouch* the idea myself (otherwise I would sign
> off on a patch), I am merely bringing it up again long after the dust
> has settled. If the idea is a nogo, then so be it, but then it would
> be nice to document somewhere why is it so.

Thanks for the breakdown here.

I am also not sure, but I felt it worth the mention.

Reading Documentation/mm/active_mm.rst makes me uncertain of the
mm_count == 1 and mm_users == 1 test.  Since mm_count is the number of
'lazy' users, and it may no longer include the lazy users..

Since there are no real workloads that suffer, is this worth it?

[1]. https://lore.kernel.org/all/202503311656.e3596aaf-lkp@intel.com/

Thanks,
Liam

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

[Mateusz Guzik]

On Mon, Mar 31, 2025 at 9:24 PM Liam R. Howlett <Liam.Howlett@oracle.com> wrote:
> Reading the commit did trigger my memory about the 5% regression, but I
> disregarded it as it seems so unlikely in real life that the benefits
> outweighed the upper bounds of 5% negative.
>
> The 5,000 forks of 10,000 vmas benchmark would also be at least a bit
> faster with Suren's more recent rcu safe vma change [1].
>

Well it follows it would be more than 5% now? ;)

> Although, that doesn't rule out changing this to get higher number, I
> still don't think the complexity of the change is worth it for a
> contrived benchmark.
>

I assumed this would be easy enough to reason through for someone
familiar with mm (which I'm not).

If this is deemed too hairy, then I completely agree it should not be
pursued (at least for now, see below).

> Reading Documentation/mm/active_mm.rst makes me uncertain of the
> mm_count == 1 and mm_users == 1 test.  Since mm_count is the number of
> 'lazy' users, and it may no longer include the lazy users..
>

The doc states interested callers need to use mmgrab_lazy_tlb() and
mmdrop_lazy_tlb() and that the *exact* count wont be known.

This is perfectly fine for my proposal as we don't care specifically
how many other users are out there (lazy tlb or otherwise), what we do
care to know is if there is at least one and that much we are being
told in the expected manner: pounding the mm off to lazy tlb also
grabs a ->mm_count on it.

To be more exact, mmgrab_lazy_tlb() starts with pinning the struct
while the mmap semaphore is held. This synchronizes ->mm_count
visibility against dup_mmap() which write-locks it.

Suppose a process with two threads races with one issuing dup_mmap()
and the other one exiting. Further suppose the forking thread got the
lock after the thread executing exit_mmap() finished and the mm
remains in active use for lazy tlb. Per my previous e-mail it is an
invariant that in this case ->mm_count is at least two:
1. the forking thread uses the mm, which implies mm_users > 0, which
implies mm_count of at least one.
2. employing the mm for lazy tlb use bumped the count, which means it
has to be at least two, regardless of how many lazy uses managed to
elide refcount manipulation afterwards

But the count being two disables the optimization. Also note the
forking thread got the lock first, ->mm_count cannot transition 1 -> 2
on the account of lazy tlb flush as in the worst case it is waiting
for the lock (besides, this implies ->mm_users of at least two due to
the thread waiting which also disables the optimization).

> Since there are no real workloads that suffer, is this worth it?
>

It is unclear to me if you are criticizing this idea specifically
(given your doubts of whether it even works) or the notion of this
kind of optimization to begin with.

And this brings me back to kernel build, which I'm looking at for kicks.

There is a lot of evil going on there from userspace pov, which
partially obfuscates kernel-side slowdowns, of which there are plenty
(and I whacked some of them on the VFS side, more in the pipeline). I
don't expect dodging vma locking to be worth much in isolation in a
real workload. I do expect consistent effort to provide smooth
execution in commonly used code paths (fork being one) to add up.
Apart from being algorithmically sound in its own right, smooth
execution means no stalls for no stalls which can be easily avoided
(notably cache misses and lock-prefixed ops).

To give you a specific example, compilation of C files ran by gnu make
goes through the shell -- as in extra fork + exec trip on top of
spawning the compiler. In order to model this, with focus on the
kernel side, I slapped in a system("gcc -c -o /tmp/crap.o src.c")
testcase into will-it-scale (src.c is hello world with one #include
clause, important for later). The use of system() suffers the shell
trip in the same manner.

I'm seeing about 20% system time, about a third of which looks
suspicious at best. For example over 1% of the total (or over 4% of
kernel time) is spent in sync_regs() copying 168 bytes using rep
movsq. Convincing the compiler to use regular stores instead dropped
the routine to about 0.1% and gave me a measurable boost in throughput
of these compiles. Will this speed up compilation of a non-toy file?
In isolation, no. I do however claim that:
1. consistent effort to whack avoidable slowdowns adds up, you don't
need to do anything revolutionary (albeit that's welcome)
2. the kernel is chock full of slowdowns which don't need to be there,
but I'm not going to rant about specific examples
3. the famed real workloads are shooting themselves in their feet on
the regular, making it really hard to show improvement on the kernel
side, apart from massive changes or fixing utter breakage

So, for the vma lock avoidance idea, it may be it requires too much
analysis to justify it. If so, screw it. I do stand by what I tries to
accomplish tho (whacking atomics in the fast path in the common case).
-- 
Mateusz Guzik <mjguzik gmail.com>