Re: [PATCH 1/1] mm: replace vma_start_write() with vma_start_write_killable()

[Jann Horn <jannh@google.com>]

On Mon, Feb 9, 2026 at 11:08 PM Suren Baghdasaryan <surenb@google.com> wrote:
> Now that we have vma_start_write_killable() we can replace most of the
> vma_start_write() calls with it, improving reaction time to the kill
> signal.
[...]
> diff --git a/mm/mempolicy.c b/mm/mempolicy.c
> index dbd48502ac24..3de7ab4f4cee 100644
> --- a/mm/mempolicy.c
> +++ b/mm/mempolicy.c
[...]
> @@ -1808,7 +1817,11 @@ SYSCALL_DEFINE4(set_mempolicy_home_node, unsigned long, start, unsigned long, le
>                         break;
>                 }
>
> -               vma_start_write(vma);
> +               if (vma_start_write_killable(vma)) {
> +                       err = -EINTR;

Doesn't this need mpol_put(new)? Or less complicated, move the
vma_start_write_killable() up to somewhere above the mpol_dup() call.

> +                       break;
> +               }
> +
>                 new->home_node = home_node;
>                 err = mbind_range(&vmi, vma, &prev, start, end, new);
>                 mpol_put(new);
[...]
> diff --git a/mm/pagewalk.c b/mm/pagewalk.c
> index a94c401ab2cf..dc9f7a7709c6 100644
> --- a/mm/pagewalk.c
> +++ b/mm/pagewalk.c
> @@ -425,14 +425,13 @@ static inline void process_mm_walk_lock(struct mm_struct *mm,
>                 mmap_assert_write_locked(mm);
>  }
>
> -static inline void process_vma_walk_lock(struct vm_area_struct *vma,
> +static inline int process_vma_walk_lock(struct vm_area_struct *vma,
>                                          enum page_walk_lock walk_lock)
>  {
>  #ifdef CONFIG_PER_VMA_LOCK
>         switch (walk_lock) {
>         case PGWALK_WRLOCK:
> -               vma_start_write(vma);
> -               break;
> +               return vma_start_write_killable(vma);

There are two users of PGWALK_WRLOCK in arch/s390/mm/gmap.c code that
don't check pagewalk return values, have you checked that they are not
negatively affected by this new possible error return?

>         case PGWALK_WRLOCK_VERIFY:
>                 vma_assert_write_locked(vma);
>                 break;
[...]
> diff --git a/mm/vma.c b/mm/vma.c
> index be64f781a3aa..3cfb81b3b7cf 100644
> --- a/mm/vma.c
> +++ b/mm/vma.c
> @@ -540,8 +540,12 @@ __split_vma(struct vma_iterator *vmi, struct vm_area_struct *vma,
>         if (new->vm_ops && new->vm_ops->open)
>                 new->vm_ops->open(new);
>
> -       vma_start_write(vma);
> -       vma_start_write(new);
> +       err = vma_start_write_killable(vma);
> +       if (err)
> +               goto out_fput;
> +       err = vma_start_write_killable(new);
> +       if (err)
> +               goto out_fput;

What about the new->vm_ops->open() call and the anon_vma_clone()
above? I don't think the error path properly undoes either. These
calls should probably be moved further up, so that the point of no
return in this function stays where it was.

>         init_vma_prep(&vp, vma);
>         vp.insert = new;
[...]
> @@ -1155,10 +1168,12 @@ int vma_expand(struct vma_merge_struct *vmg)
>         struct vm_area_struct *next = vmg->next;
>         bool remove_next = false;
>         vm_flags_t sticky_flags;
> -       int ret = 0;
> +       int ret;
>
>         mmap_assert_write_locked(vmg->mm);
> -       vma_start_write(target);
> +       ret = vma_start_write_killable(target);
> +       if (ret)
> +               return ret;
>
>         if (next && target != next && vmg->end == next->vm_end)
>                 remove_next = true;
> @@ -1186,17 +1201,19 @@ int vma_expand(struct vma_merge_struct *vmg)
>          * Note that, by convention, callers ignore OOM for this case, so
>          * we don't need to account for vmg->give_up_on_mm here.
>          */
> -       if (remove_next)
> +       if (remove_next) {
> +               ret = vma_start_write_killable(next);
> +               if (ret)
> +                       return ret;
>                 ret = dup_anon_vma(target, next, &anon_dup);
> +       }
>         if (!ret && vmg->copied_from)
>                 ret = dup_anon_vma(target, vmg->copied_from, &anon_dup);
>         if (ret)
>                 return ret;

nit: the control flow here is kinda chaotic, with some "if (ret)
return ret;" mixed with "if (!ret && ...) ret = ...;".

>
> -       if (remove_next) {
> -               vma_start_write(next);
> +       if (remove_next)
>                 vmg->__remove_next = true;
> -       }
>         if (commit_merge(vmg))
>                 goto nomem;
>
[...]
> @@ -2211,9 +2240,8 @@ int mm_take_all_locks(struct mm_struct *mm)
>          * is reached.
>          */
>         for_each_vma(vmi, vma) {
> -               if (signal_pending(current))
> +               if (vma_start_write_killable(vma))
>                         goto out_unlock;
> -               vma_start_write(vma);

nit: might want to keep the signal_pending() so that this can sort of
be interrupted by non-fatal signals, which seems to be the intention

>         }
>
>         vma_iter_init(&vmi, mm, 0);
> @@ -2549,7 +2577,9 @@ static int __mmap_new_vma(struct mmap_state *map, struct vm_area_struct **vmap)
>  #endif
>
>         /* Lock the VMA since it is modified after insertion into VMA tree */
> -       vma_start_write(vma);
> +       error = vma_start_write_killable(vma);
> +       if (error)
> +               goto free_iter_vma;

This seems way past the point of no return, we've already called the
->mmap() handler which I think means removing the VMA again would
require a ->close() call. The VMA should be locked further up if we
want to do it killably.

>         vma_iter_store_new(vmi, vma);
>         map->mm->map_count++;
>         vma_link_file(vma, map->hold_file_rmap_lock);
>