Re: [RFC 0/3] reading proc/pid/maps under RCU

[Suren Baghdasaryan]

On Thu, Jan 18, 2024 at 9:58 AM Suren Baghdasaryan <surenb@xxxxxxxxxx> wrote:
>
> On Tue, Jan 16, 2024 at 9:57 AM Suren Baghdasaryan <surenb@googlecom> wrote:
> >
> > On Tue, Jan 16, 2024 at 6:46 AM Vlastimil Babka <vbabka@xxxxxxx> wrote:
> > >
> > > On 1/16/24 15:42, Vlastimil Babka wrote:
> > > > On 1/15/24 19:38, Suren Baghdasaryan wrote:
> > > >
> > > > Hi,
> > > >
> > > >> The issue this patchset is trying to address is mmap_lock contention when
> > > >> a low priority task (monitoring, data collecting, etc.) blocks a higher
> > > >> priority task from making updated to the address space. The contention is
> > > >> due to the mmap_lock being held for read when reading proc/pid/maps.
> > > >> With maple_tree introduction, VMA tree traversals are RCU-safe and per-vma
> > > >> locks make VMA access RCU-safe. this provides an opportunity for lock-less
> > > >> reading of proc/pid/maps. We still need to overcome a couple obstacles:
> > > >> 1. Make all VMA pointer fields used for proc/pid/maps content generation
> > > >> RCU-safe;
> > > >> 2. Ensure that proc/pid/maps data tearing, which is currently possible at
> > > >> page boundaries only, does not get worse.
> > > >
> > > > Hm I thought we were to only choose this more complicated in case additional
> > > > tearing becomes a problem, and at first assume that if software can deal
> > > > with page boundary tearing, it can deal with sub-page tearing too?
> >
> > Hi Vlastimil,
> > Thanks for the feedback!
> > Yes, originally I thought we wouldn't be able to avoid additional
> > tearing without a big change but then realized it's not that hard, so
> > I tried to keep the change in behavior transparent to the userspace.
>
> In the absence of other feedback I'm going to implement and post the
> originally envisioned approach: remove validation step and avoid any
> possibility of blocking but allowing for sub-page tearing. Will use
> Matthew's rwsem_wait() to deal with possible inconsistent maple_tree
> state.

I posted v1 at
https://lore.kernel.org/all/20240122071324.2099712-1-surenb@xxxxxxxxxx/
In the RFC I used mm_struct.mm_lock_seq to detect if mm is being
changed but then I realized that won't work. mm_struct.mm_lock_seq is
incremented after mm is changed and right before mmap_lock is
write-unlocked. Instead I need a counter that changes once we
write-lock mmap_lock and before any mm changes. So the new patchset
introduces a separate counter to detect possible mm changes. In
addition, I could not use rwsem_wait() and instead had to take
mmap_lock for read to wait for the writer to finish and then record
the new counter while holding mmap_lock for read. That prevents
concurrent mm changes while we are recording the new counter value.

> Thanks,
> Suren.
>
> >
> > > >
> > > >> The patchset deals with these issues but there is a downside which I would
> > > >> like to get input on:
> > > >> This change introduces unfairness towards the reader of proc/pid/maps,
> > > >> which can be blocked by an overly active/malicious address space modifyer.
> > > >
> > > > So this is a consequence of the validate() operation, right? We could avoid
> > > > this if we allowed sub-page tearing.
> >
> > Yes, if we don't care about sub-page tearing then we could get rid of
> > validate step and this issue with updaters blocking the reader would
> > go away. If we choose that direction there will be one more issue to
> > fix, namely the maple_tree temporary inconsistent state when a VMA is
> > replaced with another one and we might observe NULL there. We might be
> > able to use Matthew's rwsem_wait() to deal with that issue.
> >
> > > >
> > > >> A couple of ways I though we can address this issue are:
> > > >> 1. After several lock-less retries (or some time limit) to fall back to
> > > >> taking mmap_lock.
> > > >> 2. Employ lock-less reading only if the reader has low priority,
> > > >> indicating that blocking it is not critical.
> > > >> 3. Introducing a separate procfs file which publishes the same data in
> > > >> lock-less manner.
> > >
> > > Oh and if this option 3 becomes necessary, then such new file shouldn't
> > > validate() either, and whoever wants to avoid the reader contention and
> > > converts their monitoring to the new file will have to account for this
> > > possible extra tearing from the start. So I would suggest trying to change
> > > the existing file with no validate() first, and if existing userspace gets
> > > broken, employ option 3. This would mean no validate() in either case?
> >
> > Yes but I was trying to avoid introducing additional file which
> > publishes the same content in a slightly different way. We will have
> > to explain when userspace should use one vs the other and that would
> > require going into low level implementation details, I think. Don't
> > know if that's acceptable/preferable.
> > Thanks,
> > Suren.
> >
> > >
> > > >> I imagine a combination of these approaches can also be employed.
> > > >> I would like to get feedback on this from the Linux community.
> > > >>
> > > >> Note: mmap_read_lock/mmap_read_unlock sequence inside validate_map()
> > > >> can be replaced with more efficiend rwsem_wait() proposed by Matthew
> > > >> in [1].
> > > >>
> > > >> [1] https://lore.kernel.org/all/ZZ1+ZicgN8dZ3zj3@xxxxxxxxxxxxxxxxxxxx/
> > > >>
> > > >> Suren Baghdasaryan (3):
> > > >>   mm: make vm_area_struct anon_name field RCU-safe
> > > >>   seq_file: add validate() operation to seq_operations
> > > >>   mm/maps: read proc/pid/maps under RCU
> > > >>
> > > >>  fs/proc/internal.h        |   3 +
> > > >>  fs/proc/task_mmu.c        | 130 ++++++++++++++++++++++++++++++++++----
> > > >>  fs/seq_file.c             |  24 ++++++-
> > > >>  include/linux/mm_inline.h |  10 ++-
> > > >>  include/linux/mm_types.h  |   3 +-
> > > >>  include/linux/seq_file.h  |   1 +
> > > >>  mm/madvise.c              |  30 +++++++--
> > > >>  7 files changed, 181 insertions(+), 20 deletions(-)
> > > >>
> > > >
> > >