Received-SPF: pass (google.com: domain of linux-kernel+bounces-36179-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Precedence: bulk
MIME-Version: 1.0
References: <20240122071324.2099712-3-surenb@google.com> <20240123053629.365673-1-sj@kernel.org>
 <CAJuCfpHpujSbPcR2_jNTBu6+DTXvLBUoi2PjkYNJyTp62xaP9w@mail.gmail.com>
In-Reply-To: <CAJuCfpHpujSbPcR2_jNTBu6+DTXvLBUoi2PjkYNJyTp62xaP9w@mail.gmail.com>
From: Suren Baghdasaryan <surenb@google.com>
Date: Tue, 23 Jan 2024 15:12:44 -0800
Message-ID: <CAJuCfpHgScS4tud7yLn24RGMMwTaM=G4THOCgBZKGEoYkNPvtg@mail.gmail.com>
Subject: Re: [PATCH 3/3] mm/maps: read proc/pid/maps under RCU
To: SeongJae Park <sj@kernel.org>
Cc: akpm@linux-foundation.org, viro@zeniv.linux.org.uk, brauner@kernel.org, 
	jack@suse.cz, dchinner@redhat.com, casey@schaufler-ca.com, 
	ben.wolsieffer@hefring.com, paulmck@kernel.org, david@redhat.com, 
	avagin@google.com, usama.anjum@collabora.com, peterx@redhat.com, 
	hughd@google.com, ryan.roberts@arm.com, wangkefeng.wang@huawei.com, 
	Liam.Howlett@oracle.com, yuzhao@google.com, axelrasmussen@google.com, 
	lstoakes@gmail.com, talumbau@google.com, willy@infradead.org, vbabka@suse.cz, 
	mgorman@techsingularity.net, jhubbard@nvidia.com, vishal.moola@gmail.com, 
	mathieu.desnoyers@efficios.com, dhowells@redhat.com, jgg@ziepe.ca, 
	sidhartha.kumar@oracle.com, andriy.shevchenko@linux.intel.com, 
	yangxingui@huawei.com, keescook@chromium.org, linux-kernel@vger.kernel.org, 
	linux-fsdevel@vger.kernel.org, linux-mm@kvack.org, kernel-team@android.com
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

On Mon, Jan 22, 2024 at 10:07=E2=80=AFPM Suren Baghdasaryan <surenb@google.=
com> wrote:
>
> On Mon, Jan 22, 2024 at 9:36=E2=80=AFPM SeongJae Park <sj@kernel.org> wro=
te:
> >
> > Hi Suren,
> >
> > On Sun, 21 Jan 2024 23:13:24 -0800 Suren Baghdasaryan <surenb@google.co=
m> wrote:
> >
> > > With maple_tree supporting vma tree traversal under RCU and per-vma l=
ocks
> > > making vma access RCU-safe, /proc/pid/maps can be read under RCU and
> > > without the need to read-lock mmap_lock. However vma content can chan=
ge
> > > from under us, therefore we make a copy of the vma and we pin pointer
> > > fields used when generating the output (currently only vm_file and
> > > anon_name). Afterwards we check for concurrent address space
> > > modifications, wait for them to end and retry. That last check is nee=
ded
> > > to avoid possibility of missing a vma during concurrent maple_tree
> > > node replacement, which might report a NULL when a vma is replaced
> > > with another one. While we take the mmap_lock for reading during such
> > > contention, we do that momentarily only to record new mm_wr_seq count=
er.
> > > This change is designed to reduce mmap_lock contention and prevent a
> > > process reading /proc/pid/maps files (often a low priority task, such=
 as
> > > monitoring/data collection services) from blocking address space upda=
tes.
> > >
> > > Note that this change has a userspace visible disadvantage: it allows=
 for
> > > sub-page data tearing as opposed to the previous mechanism where data
> > > tearing could happen only between pages of generated output data.
> > > Since current userspace considers data tearing between pages to be
> > > acceptable, we assume is will be able to handle sub-page data tearing
> > > as well.
> > >
> > > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > > ---
> > >  fs/proc/internal.h |   2 +
> > >  fs/proc/task_mmu.c | 114 ++++++++++++++++++++++++++++++++++++++++++-=
--
> > >  2 files changed, 109 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> > > index a71ac5379584..e0247225bb68 100644
> > > --- a/fs/proc/internal.h
> > > +++ b/fs/proc/internal.h
> > > @@ -290,6 +290,8 @@ struct proc_maps_private {
> > >       struct task_struct *task;
> > >       struct mm_struct *mm;
> > >       struct vma_iterator iter;
> > > +     unsigned long mm_wr_seq;
> > > +     struct vm_area_struct vma_copy;
> > >  #ifdef CONFIG_NUMA
> > >       struct mempolicy *task_mempolicy;
> > >  #endif
> > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > index 3f78ebbb795f..3886d04afc01 100644
> > > --- a/fs/proc/task_mmu.c
> > > +++ b/fs/proc/task_mmu.c
> > > @@ -126,11 +126,96 @@ static void release_task_mempolicy(struct proc_=
maps_private *priv)
> > >  }
> > >  #endif
> > >
> > > -static struct vm_area_struct *proc_get_vma(struct proc_maps_private =
*priv,
> > > -                                             loff_t *ppos)
> > > +#ifdef CONFIG_PER_VMA_LOCK
> > > +
> > > +static const struct seq_operations proc_pid_maps_op;
> > > +/*
> > > + * Take VMA snapshot and pin vm_file and anon_name as they are used =
by
> > > + * show_map_vma.
> > > + */
> > > +static int get_vma_snapshow(struct proc_maps_private *priv, struct v=
m_area_struct *vma)
> > >  {
> > > +     struct vm_area_struct *copy =3D &priv->vma_copy;
> > > +     int ret =3D -EAGAIN;
> > > +
> > > +     memcpy(copy, vma, sizeof(*vma));
> > > +     if (copy->vm_file && !get_file_rcu(&copy->vm_file))
> > > +             goto out;
> > > +
> > > +     if (copy->anon_name && !anon_vma_name_get_rcu(copy))
> > > +             goto put_file;
> >
> > From today updated mm-unstable which containing this patch, I'm getting=
 below
> > build error when CONFIG_ANON_VMA_NAME is not set.  Seems this patch nee=
ds to
> > handle the case?
>
> Hi SeongJae,
> Thanks for reporting! I'll post an updated version fixing this config.

Fix is posted at
https://lore.kernel.org/all/20240123231014.3801041-3-surenb@google.com/
as part of v2 of this patchset.
Thanks,
Suren.

> Suren.
>
>
> >
> >     .../linux/fs/proc/task_mmu.c: In function =E2=80=98get_vma_snapshow=
=E2=80=99:
> >     .../linux/fs/proc/task_mmu.c:145:19: error: =E2=80=98struct vm_area=
_struct=E2=80=99 has no member named =E2=80=98anon_name=E2=80=99; did you m=
ean =E2=80=98anon_vma=E2=80=99?
> >       145 |         if (copy->anon_name && !anon_vma_name_get_rcu(copy)=
)
> >           |                   ^~~~~~~~~
> >           |                   anon_vma
> >     .../linux/fs/proc/task_mmu.c:161:19: error: =E2=80=98struct vm_area=
_struct=E2=80=99 has no member named =E2=80=98anon_name=E2=80=99; did you m=
ean =E2=80=98anon_vma=E2=80=99?
> >       161 |         if (copy->anon_name)
> >           |                   ^~~~~~~~~
> >           |                   anon_vma
> >     .../linux/fs/proc/task_mmu.c:162:41: error: =E2=80=98struct vm_area=
_struct=E2=80=99 has no member named =E2=80=98anon_name=E2=80=99; did you m=
ean =E2=80=98anon_vma=E2=80=99?
> >       162 |                 anon_vma_name_put(copy->anon_name);
> >           |                                         ^~~~~~~~~
> >           |                                         anon_vma
> >     .../linux/fs/proc/task_mmu.c: In function =E2=80=98put_vma_snapshot=
=E2=80=99:
> >     .../linux/fs/proc/task_mmu.c:174:18: error: =E2=80=98struct vm_area=
_struct=E2=80=99 has no member named =E2=80=98anon_name=E2=80=99; did you m=
ean =E2=80=98anon_vma=E2=80=99?
> >       174 |         if (vma->anon_name)
> >           |                  ^~~~~~~~~
> >           |                  anon_vma
> >     .../linux/fs/proc/task_mmu.c:175:40: error: =E2=80=98struct vm_area=
_struct=E2=80=99 has no member named =E2=80=98anon_name=E2=80=99; did you m=
ean =E2=80=98anon_vma=E2=80=99?
> >       175 |                 anon_vma_name_put(vma->anon_name);
> >           |                                        ^~~~~~~~~
> >           |                                        anon_vma
> >
> > [...]
> >
> >
> > Thanks,
> > SJ