Message-ID: <7a26aae6-8695-4bbc-b00d-47d0c56b9cd6@lucifer.local>
Date: Wed, 11 Jun 2025 11:24:42 +0100
From: Lorenzo Stoakes <lorenzo.stoakes@...cle.com>
To: Suren Baghdasaryan <surenb@...gle.com>
Cc: akpm@...ux-foundation.org, Liam.Howlett@...cle.com, david@...hat.com,
        vbabka@...e.cz, peterx@...hat.com, jannh@...gle.com,
        hannes@...xchg.org, mhocko@...nel.org, paulmck@...nel.org,
        shuah@...nel.org, adobriyan@...il.com, brauner@...nel.org,
        josef@...icpanda.com, yebin10@...wei.com, linux@...ssschuh.net,
        willy@...radead.org, osalvador@...e.de, andrii@...nel.org,
        ryan.roberts@....com, christophe.leroy@...roup.eu,
        tjmercier@...gle.com, kaleshsingh@...gle.com,
        linux-kernel@...r.kernel.org, linux-fsdevel@...r.kernel.org,
        linux-mm@...ck.org, linux-kselftest@...r.kernel.org
Subject: Re: [PATCH v4 6/7] mm/maps: read proc/pid/maps under per-vma lock

Thanks for your patient replies :)

OK to save us both time in such a huuuuge back-and-forth - I agree broadly
with your comments below and I think we are aligned on everything now.

I will try to get you a list of merge scenarios and ideally have a look at
the test code too if I have time this week.

But otherwise hopefully we are good for a respin here?

Cheers, Lorenzo

On Tue, Jun 10, 2025 at 05:16:36PM -0700, Suren Baghdasaryan wrote:
> On Tue, Jun 10, 2025 at 10:43 AM Lorenzo Stoakes
> <lorenzo.stoakes@...cle.com> wrote:
> >
> > On Sat, Jun 07, 2025 at 06:41:35PM -0700, Suren Baghdasaryan wrote:
> > > On Sat, Jun 7, 2025 at 10:43 AM Lorenzo Stoakes
> > > <lorenzo.stoakes@...cle.com> wrote:
> > > >
> > > > Hi Suren,
> > > >
> > > > Forgive me but I am going to ask a lot of questions here :p just want to
> > > > make sure I'm getting everything right here.
> > >
> > > No worries and thank you for reviewing!
> >
> > No problem!
> >
> > >
> > > >
> > > > On Wed, Jun 04, 2025 at 04:11:50PM -0700, Suren Baghdasaryan wrote:
> > > > > With maple_tree supporting vma tree traversal under RCU and per-vma
> > > > > locks, /proc/pid/maps can be read while holding individual vma locks
> > > > > instead of locking the entire address space.
> > > >
> > > > Nice :)
> > > >
> > > > > Completely lockless approach would be quite complex with the main issue
> > > > > being get_vma_name() using callbacks which might not work correctly with
> > > > > a stable vma copy, requiring original (unstable) vma.
> > > >
> > > > Hmmm can you expand on what a 'completely lockless' design might comprise of?
> > >
> > > In my previous implementation
> > > (https://lore.kernel.org/all/20250418174959.1431962-1-surenb@google.com/)
> > > I was doing this under RCU while checking mmap_lock seq counter to
> > > detect address space changes. That's what I meant by a completely
> > > lockless approach here.
> >
> > Oh did that approach not even use VMA locks _at all_?
>
> Correct, it was done under RCU protection.
>
> >
> > >
> > > >
> > > > It's super un-greppable and I've not got clangd set up with an allmod kernel to
> > > > triple-check but I'm seeing at least 2 (are there more?):
> > > >
> > > > gate_vma_name() which is:
> > > >
> > > >         return "[vsyscall]";
> > > >
> > > > special_mapping_name() which is:
> > > >
> > > >          return ((struct vm_special_mapping *)vma->vm_private_data)->name;
> > > >
> > > > Which I'm guessing is the issue because it's a double pointer deref...
> > >
> > > Correct but in more general terms, depending on implementation of the
> > > vm_ops.name callback, vma->vm_ops->name(vma) might not work correctly
> > > with a vma copy. special_mapping_name() is an example of that.
> >
> > Yeah, this is a horrible situation to be in for such a trivial thing. But I
> > guess unavoidable for now.
> >
> > >
> > > >
> > > > Seems such a silly issue to get stuck on, I wonder if we can't just change
> > > > this to function correctly?
> > >
> > > I was thinking about different ways to overcome that but once I
> > > realized per-vma locks result in even less contention and the
> > > implementation is simpler and more robust, I decided that per-vma
> > > locks direction is better.
> >
> > Ack well in that case :)
> >
> > But still it'd be nice to somehow restrict the impact of this callback.
>
> With VMA locked we are back in a safe place, I think.
>
> >
> > >
> > > >
> > > > > When per-vma lock acquisition fails, we take the mmap_lock for reading,
> > > > > lock the vma, release the mmap_lock and continue. This guarantees the
> > > > > reader to make forward progress even during lock contention. This will
> > > >
> > > > Ah that fabled constant forward progress ;)
> > > >
> > > > > interfere with the writer but for a very short time while we are
> > > > > acquiring the per-vma lock and only when there was contention on the
> > > > > vma reader is interested in.
> > > > > One case requiring special handling is when vma changes between the
> > > > > time it was found and the time it got locked. A problematic case would
> > > > > be if vma got shrunk so that it's start moved higher in the address
> > > > > space and a new vma was installed at the beginning:
> > > > >
> > > > > reader found:               |--------VMA A--------|
> > > > > VMA is modified:            |-VMA B-|----VMA A----|
> > > > > reader locks modified VMA A
> > > > > reader reports VMA A:       |  gap  |----VMA A----|
> > > > >
> > > > > This would result in reporting a gap in the address space that does not
> > > > > exist. To prevent this we retry the lookup after locking the vma, however
> > > > > we do that only when we identify a gap and detect that the address space
> > > > > was changed after we found the vma.
> > > >
> > > > OK so in this case we have
> > > >
> > > > 1. Find VMA A - nothing is locked yet, but presumably we are under RCU so
> > > >    are... safe? From unmaps? Or are we? I guess actually the detach
> > > >    mechanism sorts this out for us perhaps?
> > >
> > > Yes, VMAs are RCU-safe and we do detect if it got detached after we
> > > found it but before we locked it.
> >
> > Ack I thought so.
> >
> > >
> > > >
> > > > 2. We got unlucky and did this immediately prior to VMA A having its
> > > >    vma->vm_start, vm_end updated to reflect the split.
> > >
> > > Yes, the split happened after we found it and before we locked it.
> > >
> > > >
> > > > 3. We lock VMA A, now position with an apparent gap after the prior VMA
> > > > which, in practice does not exist.
> > >
> > > Correct.
> >
> > Ack
> >
> > >
> > > >
> > > > So I am guessing that by observing sequence numbers you are able to detect
> > > > that a change has occurred and thus retry the operation in this situation?
> > >
> > > Yes, we detect the gap and we detect that address space has changed,
> > > so to endure we did not miss a split we fall back to mmap_read_lock,
> > > lock the VMA while holding mmap_read_lock, drop mmap_read_lock and
> > > retry.
> > >
> > > >
> > > > I know we previously discussed the possibility of this retry mechanism
> > > > going on forever, I guess I will see the resolution to this in the code :)
> > >
> > > Retry in this case won't go forever because we take mmap_read_lock
> > > during the retry. In the worst case we will be constantly falling back
> > > to mmap_read_lock but that's a very unlikely case (the writer should
> > > be constantly splitting the vma right before the reader locks it).
> >
> > It might be worth adding that to commit message to underline that this has
> > been considered and this is the resolution.
> >
> > Something like:
> >
> >         we guarantee forward progress by always resolving contention via a
> >         fallback to an mmap-read lock.
> >
> >         We shouldn't see a repeated fallback to mmap read locks in
> >         practice, as this require a vanishingly unlikely series of lock
> >         contentions (for instance due to repeated VMA split
> >         operations). However even if this did somehow happen, we would
> >         still progress.
>
> Ack.
>
> >
> > >
> > > >
> > > > > 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
> > > > > updates. 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.
> > > >
> > > > By tearing do you mean for instance seeing a VMA more than once due to
> > > > e.g. a VMA expanding in a racey way?
> > >
> > > Yes.
> > >
> > > >
> > > > Pedantic I know, but it might be worth goiing through all the merge case,
> > > > split and remap scenarios and explaining what might happen in each one (or
> > > > perhaps do that as some form of documentation?)
> > > >
> > > > I can try to put together a list of all of the possibilities if that would
> > > > be helpful.
> > >
> > > Hmm. That might be an interesting exercise. I called out this
> > > particular case because my tests caught it. I spent some time thinking
> > > about other possible scenarios where we would report a gap in a place
> > > where there are no gaps but could not think of anything else.
> >
> > todo++; :)
> >
> > >
> > > >
> > > > >
> > > > > Signed-off-by: Suren Baghdasaryan <surenb@...gle.com>
> > > > > ---
> > > > >  fs/proc/internal.h |   6 ++
> > > > >  fs/proc/task_mmu.c | 177 +++++++++++++++++++++++++++++++++++++++++++--
> > > > >  2 files changed, 175 insertions(+), 8 deletions(-)
> > > >
> > > > I really hate having all this logic in the proc/task_mmu.c file.
> > > >
> > > > This is really delicate stuff and I'd really like it to live in mm if
> > > > possible.
> > > >
> > > > I reallise this might be a total pain, but I'm quite worried about us
> > > > putting super-delicate, carefully written VMA handling code in different
> > > > places.
> > > >
> > > > Also having stuff in mm/vma.c opens the door to userland testing which,
> > > > when I finally have time to really expand that, would allow for some really
> > > > nice stress testing here.
> > >
> > > That would require some sizable refactoring. I assume code for smaps
> > > reading and PROCMAP_QUERY would have to be moved as well?
> >
> > Yeah, I know, and apologies for that, but I really oppose us having this
> > super delicate VMA logic in an fs/proc file, one we don't maintain for that
> > matter.
> >
> > I know it's a total pain, but this just isn't the right place to be doing
> > such a careful dance.
> >
> > I'm not saying relocate code that belongs here, but find a way to abstract
> > the operations.
>
> Ok, I'll take a stab at refactoring purely mm-related code and will
> see how that looks.
>
> >
> > Perhaps could be a walker or something that does all the state transition
> > stuff that you can then just call from the walker functions here?
> >
> > You could then figure out something similar for the PROCMAP_QUERY logic.
> >
> > We're not doing this VMA locking stuff for smaps are we? As that is walking
> > page tables anyway right? So nothing would change for that.
>
> Yeah, smaps would stay as they are but refactoring might affect its
> code portions as well.
>
> >
> > >
> > > >
> > > > >
> > > > > diff --git a/fs/proc/internal.h b/fs/proc/internal.h
> > > > > index 96122e91c645..3728c9012687 100644
> > > > > --- a/fs/proc/internal.h
> > > > > +++ b/fs/proc/internal.h
> > > > > @@ -379,6 +379,12 @@ struct proc_maps_private {
> > > > >       struct task_struct *task;
> > > > >       struct mm_struct *mm;
> > > > >       struct vma_iterator iter;
> > > > > +     loff_t last_pos;
> > > > > +#ifdef CONFIG_PER_VMA_LOCK
> > > > > +     bool mmap_locked;
> > > > > +     unsigned int mm_wr_seq;
> > > >
> > > > Is this the _last_ sequence number observed in the mm_struct? or rather,
> > > > previous? Nitty but maybe worth renaming accordingly.
> > >
> > > It's a copy of the mm->mm_wr_seq. I can add a comment if needed.
> >
> > Right, of course. But I think the problem is the 'when' it refers to. It's
> > the sequence number associatied with the mm here sure, but when was it
> > snapshotted? How do we use it?
> >
> > Something like 'last_seen_seqnum' or 'mm_wr_seq_start' or something plus a
> > comment would be helpful.
> >
> > This is nitty I know... but this stuff is very confusing and I think every
> > little bit we do to help explain things is helpful here.
>
> Ok, I'll add a comment that mm_wr_seq is a snapshot of mm->mm_wr_seq
> before we started the VMA lookup.
>
> >
> > >
> > > >
> > > > > +     struct vm_area_struct *locked_vma;
> > > > > +#endif
> > > > >  #ifdef CONFIG_NUMA
> > > > >       struct mempolicy *task_mempolicy;
> > > > >  #endif
> > > > > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > > > > index 27972c0749e7..36d883c4f394 100644
> > > > > --- a/fs/proc/task_mmu.c
> > > > > +++ b/fs/proc/task_mmu.c
> > > > > @@ -127,13 +127,172 @@ 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 struct vm_area_struct *trylock_vma(struct proc_maps_private *priv,
> > > > > +                                       struct vm_area_struct *vma,
> > > > > +                                       unsigned long last_pos,
> > > > > +                                       bool mm_unstable)
> > > >
> > > > This whole function is a bit weird tbh, you handle both the
> > > > mm_unstable=true and mm_unstable=false cases, in the latter we don't try to
> > > > lock at all...
> > >
> > > Why do you think so? vma_start_read() is always called but in case
> > > mm_unstable=true we double check for the gaps to take care of the case
> > > I mentioned in the changelog.
> >
> > Well the read lock will always succeed if mmap read lock is held right?
> > Actually... no :)
> >
> > I see your point below about vma_start_read_locked() :>)
> >
> > I see below you suggest splitting into two functions, that seems to be a
> > good way forward.
>
> Ack.
>
> >
> > I _think_ we won't even need the checks re: mm and last_pos in that case
> > right? As holding the mmap lock we should be able to guarantee? Or at least
> > the mm check?
>
> Correct. These checks are needed only if we are searching the VMA
> under RCU protection before locking it. If we are holding mmap_lock
> then all this is not needed.
>
> >
> > >
> > > >
> > > > Nitty (sorry I know this is mildly irritating review) but maybe needs to be
> > > > renamed, or split up somehow?
> > > >
> > > > This is only trylocking in the mm_unstable case...
> > >
> > > Nope, I think you misunderstood the intention, as I mentioned above.
> > >
> > > >
> > > > > +{
> > > > > +     vma = vma_start_read(priv->mm, vma);
> > > >
> > > > Do we want to do this with mm_unstable == false?
> > >
> > > Yes, always. mm_unstable=true only indicates that we are already
> > > holding mmap_read_lock, so we don't need to double-check for gaps.
> > > Perhaps I should add some comments to clarify what purpose this
> > > parameter serves...
> > >
> > > >
> > > > I know (from my own documentation :)) taking a VMA read lock while holding
> > > > an mmap read lock is fine (the reverse isn't) but maybe it's suboptimal?
> > >
> > > Ah, right. I should use vma_start_read_locked() instead when we are
> > > holding mmap_read_lock. That's why that function was introduced. Will
> > > change.
> >
> > Yeah, I'll pretend this is what I meant to sound smart :P but this is a
> > really good point!
> >
> > >
> > > >
> > > > > +     if (IS_ERR_OR_NULL(vma))
> > > > > +             return NULL;
> > > >
> > > > Hmm IS_ERR_OR_NULL() is generally a code smell (I learned this some years
> > > > ago from people moaning at me on code review :)
> > > >
> > > > Sorry I know that's annoying but perhaps its indicative of an issue in the
> > > > interface? That's possibly out of scope here however.
> > >
> > > lock_vma_under_rcu() returns NULL or EAGAIN to signal
> > > lock_vma_under_rcu() that it should retry the VMA lookup. In here in
> > > either case we retry under mmap_read_lock, that's why EAGAIN is
> > > ignored.
> >
> > Yeah indeed you're right. I guess I'm just echoing previous review traumas
> > here :P
> >
> > >
> > > >
> > > > Why are we ignoring errors here though? I guess because we don't care if
> > > > the VMA got detached from under us, we don't bother retrying like we do in
> > > > lock_vma_under_rcu()?
> > >
> > > No, we take mmap_read_lock and retry in either case. Perhaps I should
> > > split trylock_vma() into two separate functions - one for the case
> > > when we are holding mmap_read_lock and another one when we don't? I
> > > think that would have prevented many of your questions. I'll try that
> > > and see how it looks.
> >
> > Yeah that'd be helpful. I think this should also simplify things?
>
> Yes. Will try that.
>
> >
> > >
> > > >
> > > > Should we just abstract that part of lock_vma_under_rcu() and use it?
> > >
> > > trylock_vma() is not that similar to lock_vma_under_rcu() for that
> > > IMO. Also lock_vma_under_rcu() is in the pagefault path which is very
> > > hot, so I would not want to add conditions there to make it work for
> > > trylock_vma().
> >
> > Right sure.
> >
> > But I'm just wondering why we don't do the retry stuff, e.g.:
> >
> >                 /* Check if the VMA got isolated after we found it */
> >                 if (PTR_ERR(vma) == -EAGAIN) {
> >                         count_vm_vma_lock_event(VMA_LOCK_MISS);
> >                         /* The area was replaced with another one */
> >                         goto retry;
> >                 }
> >
> > I mean do we need to retry under mmap lock in that case? Can we just retry
> > the lookup? Or is this not a worthwhile optimisation here?
>
> Hmm. That might be applicable here as well. Let me think some more
> about it. Theoretically that might affect our forward progress
> guarantee but for us to retry infinitely the VMA we find has to be
> knocked out from under us each time we find it. So, quite unlikely to
> happen continuously.
>
> >
> > >
> > > >
> > > > > +
> > > > > +     /* Check if the vma we locked is the right one. */
> > > >
> > > > Well it might not be the right one :) but might still belong to the right
> > > > mm, so maybe better to refer to the right virtual address space.
> > >
> > > Ack. Will change to "Check if the vma belongs to the right address space. "
> >
> > Thanks!
> >
> > >
> > > >
> > > > > +     if (unlikely(vma->vm_mm != priv->mm))
> > > > > +             goto err;
> > > > > +
> > > > > +     /* vma should not be ahead of the last search position. */
> > > >
> > > > You mean behind the last search position? Surely a VMA being _ahead_ of it
> > > > is fine?
> > >
> > > Yes, you are correct. "should not" should have been "should".
> >
> > Thanks!
> >
> > >
> > > >
> > > > > +     if (unlikely(last_pos >= vma->vm_end))
> > > >
> > > > Should that be >=? Wouldn't an == just be an adjacent VMA? Why is that
> > > > problematic? Or is last_pos inclusive?
> > >
> > > last_pos is inclusive and vma->vm_end is not inclusive, so if last_pos
> > > == vma->vm_end that would mean the vma is behind the last_pos. Since
> > > we are searching forward from the last_pos, we should not be finding a
> > > vma before last_pos unless it mutated.
> >
> > Ahhh that explains it. Thanks.
> >
> > >
> > > >
> > > > > +             goto err;
> > > >
> > > > Am I correct in thinking thi is what is being checked?
> > > >
> > > >           last_pos
> > > >              |
> > > >              v
> > > > |---------|
> > > > |         |
> > > > |---------|
> > > >         vm_end
> > > >    <--- vma 'next'??? How did we go backwards?
> > >
> > > Exactly.
> > >
> > > >
> > > > When last_pos gets updated, is it possible for a shrink to race to cause
> > > > this somehow?
> > >
> > > No, we update last_pos only after we locked the vma and confirmed it's
> > > the right one.
> >
> > Ack.
> >
> > >
> > > >
> > > > Do we treat this as an entirely unexpected error condition? In which case
> > > > is a WARN_ON_ONCE() warranted?
> > >
> > > No, the VMA might have mutated from under us before we locked it. For
> > > example it might have been remapped to a higher address.
> > >
> > > >
> > > > > +
> > > > > +     /*
> > > > > +      * vma ahead of last search position is possible but we need to
> > > > > +      * verify that it was not shrunk after we found it, and another
> > > > > +      * vma has not been installed ahead of it. Otherwise we might
> > > > > +      * observe a gap that should not be there.
> > > > > +      */
> > > >
> > > > OK so this is the juicy bit.
> > >
> > > Yep, that's the case singled out in the changelog.
> >
> > And rightly so!
> >
> > >
> > > >
> > > >
> > > > > +     if (mm_unstable && last_pos < vma->vm_start) {
> > > > > +             /* Verify only if the address space changed since vma lookup. */
> > > > > +             if ((priv->mm_wr_seq & 1) ||
> > > >
> > > > Can we wrap this into a helper? This is a 'you just have to know that odd
> > > > seq number means a write operation is in effect'. I know you have a comment
> > > > here, but I think something like:
> > > >
> > > >         if (has_mm_been_modified(priv) ||
> > > >
> > > > Would be a lot clearer.
> > >
> > > Yeah, I was thinking about that. I think an even cleaner way would be
> > > to remember the return value of mmap_lock_speculate_try_begin() and
> > > pass it around. I was hoping to avoid that extra parameter but sounds
> > > like for the sake of clarity that would be preferable?
> >
> > You know, it's me so I might have to mention a helper struct here :P it's
> > the two most Lorenzo things - helper sructs and churn...
> >
> > >
> > > >
> > > > Again this speaks to the usefulness of abstracting all this logic from the
> > > > proc code, we are putting super delicate VMA stuff here and it's just not
> > > > the right place.
> > > >
> > > > As an aside, I don't see coverage in the process_addrs documentation on
> > > > sequence number odd/even or speculation?
> > > >
> > > > I think we probably need to cover this to maintain an up-to-date
> > > > description of how the VMA locking mechanism works and is used?
> > >
> > > I think that's a very low level technical detail which I should not
> > > have exposed here. As I mentioned, I should simply store the return
> > > value of mmap_lock_speculate_try_begin() instead of doing these tricky
> > > mm_wr_seq checks.
> >
> > Right yeah I'm all for simplifying if we can! Sounds sensible.
> >
> > >
> > > >
> > > > > +                 mmap_lock_speculate_retry(priv->mm, priv->mm_wr_seq)) {
> > > >
> > > > Nit, again unrelated to this series, but would be useful to add a comment
> > > > to mmap_lock_speculate_retry() to indicate that a true return value
> > > > indicates a retry is needed, or renaming it.
> > >
> > > This is how seqcount API works in general. Note that
> > > mmap_lock_speculate_retry() is just a wrapper around
> > > read_seqcount_retry().
> >
> > Yeah, I guess I can moan to PeterZ about that :P
> >
> > It's not a big deal honestly, but it was just something I found confusing.
> >
> > I think adjusting the comment above to something like:
> >
> >                 /*
> >                  * Verify if the address space changed since vma lookup, or if
> >                  * the speculative lock needs to be retried.
> >                  */
> >
> > Or perhaps somethig more in line with the description you give below?
>
> Ack.
>
> >
> > >
> > > >
> > > > Maybe mmap_lock_speculate_needs_retry()? Also I think that function needs a
> > > > comment.
> > >
> > > See https://elixir.bootlin.com/linux/v6.15.1/source/include/linux/seqlock.h#L395
> >
> > Yeah I saw that, but going 2 levels deep to read a comment isn't great.
> >
> > But again this isn't the end of the world.
> >
> > >
> > > >
> > > > Naming is hard :P
> > > >
> > > > Anyway the totality of this expression is 'something changed' or 'read
> > > > section retry required'.
> > >
> > > Not quite. The expression is "something is changed from under us or
> > > something was changing even before we started VMA lookup". Or in more
> > > technical terms, mmap_write_lock was acquired while we were locking
> > > the VMA or mmap_write_lock was already held even before we started the
> > > VMA search.
> >
> > OK so read section retry required = the seq num changes from under us
> > (checked carefully with memory barriers and carefully considered and
> > thought out such logic), and the priv->mm_wr_seq check before it is the
> > 'was this changed even before we began?'
> >
> > I wonder btw if we could put both into a single helper function to check
> > whether that'd be clearer.
>
> So this will look something like this:
>
> priv->can_speculate = mmap_lock_speculate_try_begin();
> ...
> if (!priv->can_speculate || mmap_lock_speculate_retry()) {
>     // fallback
> }
>
> Is that descriptive enough?
>
> >
> > >
> > > >
> > > > Under what circumstances would this happen?
> > >
> > > See my previous comment and I hope that clarifies it.
> >
> > Thanks!
> >
> > >
> > > >
> > > > OK so we're into the 'retry' logic here:
> > > >
> > > > > +                     vma_iter_init(&priv->iter, priv->mm, last_pos);
> > > >
> > > > I'd definitely want Liam to confirm this is all above board and correct, as
> > > > these operations are pretty sensitive.
> > > >
> > > > But assuming this is safe, we reset the iterator to the last position...
> > > >
> > > > > +                     if (vma != vma_next(&priv->iter))
> > > >
> > > > Then assert the following VMA is the one we seek.
> > > >
> > > > > +                             goto err;
> > > >
> > > > Might this ever be the case in the course of ordinary operation? Is this
> > > > really an error?
> > >
> > > This simply means that the VMA we found before is not at the place we
> > > found it anymore. The locking fails and we should retry.
> >
> > I know it's pedantic but feels like 'err' is not a great name for this.
> >
> > Maybe 'nolock' or something? Or 'lock_failed'?
>
> lock_failed sounds good.
>
>
> >
> > >
> > > >
> > > > > +             }
> > > > > +     }
> > > > > +
> > > > > +     priv->locked_vma = vma;
> > > > > +
> > > > > +     return vma;
> > > > > +err:
> > > >
> > > > As queried above, is this really an error path or something we might expect
> > > > to happen that could simply result in an expected fallback to mmap lock?
> > >
> > > It's a failure to lock the VMA, which is handled by retrying under
> > > mmap_read_lock. So, trylock_vma() failure does not mean a fault in the
> > > logic. It's expected to happen occasionally.
> >
> > Ack yes understood thanks!
> >
> > >
> > > >
> > > > > +     vma_end_read(vma);
> > > > > +     return NULL;
> > > > > +}
> > > > > +
> > > > > +
> > > > > +static void unlock_vma(struct proc_maps_private *priv)
> > > > > +{
> > > > > +     if (priv->locked_vma) {
> > > > > +             vma_end_read(priv->locked_vma);
> > > > > +             priv->locked_vma = NULL;
> > > > > +     }
> > > > > +}
> > > > > +
> > > > > +static const struct seq_operations proc_pid_maps_op;
> > > > > +
> > > > > +static inline bool lock_content(struct seq_file *m,
> > > > > +                             struct proc_maps_private *priv)
> > > >
> > > > Pedantic I know but isn't 'lock_content' a bit generic?
> > > >
> > > > He says, not being able to think of a great alternative...
> > > >
> > > > OK maybe fine... :)
> > >
> > > Yeah, I struggled with this myself. Help in naming is appreciated.
> >
> > This is where it gets difficult haha so easy to point out but not so easy
> > to fix...
> >
> > lock_vma_range()?
>
> Ack.
>
> >
> > >
> > > >
> > > > > +{
> > > > > +     /*
> > > > > +      * smaps and numa_maps perform page table walk, therefore require
> > > > > +      * mmap_lock but maps can be read with locked vma only.
> > > > > +      */
> > > > > +     if (m->op != &proc_pid_maps_op) {
> > > >
> > > > Nit but is there a neater way of checking this? Actually I imagine not...
> > > >
> > > > But maybe worth, instead of forward-declaring proc_pid_maps_op, forward declare e.g.
> > > >
> > > > static inline bool is_maps_op(struct seq_file *m);
> > > >
> > > > And check e.g.
> > > >
> > > > if (is_maps_op(m)) { ... in the above.
> > > >
> > > > Yeah this is nitty not a massive del :)
> > >
> > > I'll try that and see how it looks. Thanks!
> >
> > Thanks!
> >
> > >
> > > >
> > > > > +             if (mmap_read_lock_killable(priv->mm))
> > > > > +                     return false;
> > > > > +
> > > > > +             priv->mmap_locked = true;
> > > > > +     } else {
> > > > > +             rcu_read_lock();
> > > > > +             priv->locked_vma = NULL;
> > > > > +             priv->mmap_locked = false;
> > > > > +     }
> > > > > +
> > > > > +     return true;
> > > > > +}
> > > > > +
> > > > > +static inline void unlock_content(struct proc_maps_private *priv)
> > > > > +{
> > > > > +     if (priv->mmap_locked) {
> > > > > +             mmap_read_unlock(priv->mm);
> > > > > +     } else {
> > > > > +             unlock_vma(priv);
> > > > > +             rcu_read_unlock();
> > > >
> > > > Does this always get called even in error cases?
> > >
> > > What error cases do you have in mind? Error to lock a VMA is handled
> > > by retrying and we should be happily proceeding. Please clarify.
> >
> > Well it was more of a question really - can the traversal through
> > /proc/$pid/maps result in some kind of error that doesn't reach this
> > function, thereby leaving things locked mistakenly?
> >
> > If not then happy days :)
> >
> > I'm guessing there isn't.
>
> There is EINTR in m_start() but unlock_content() won't be called in
> that case, so I think we are good.
>
> >
> > >
> > > >
> > > > > +     }
> > > > > +}
> > > > > +
> > > > > +static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> > > > > +                                        loff_t last_pos)
> > > >
> > > > We really need a generalised RCU multi-VMA locking mechanism (we're looking
> > > > into madvise VMA locking atm with a conservative single VMA lock at the
> > > > moment, but in future we probably want to be able to span multiple for
> > > > instance) and this really really feels like it doesn't belong in this proc
> > > > code.
> > >
> > > Ok, I guess you are building a case to move more code into vma.c? I
> > > see what you are doing :)
> >
> > Haha damn it, my evil plans revealed :P
> >
> > >
> > > >
> > > > >  {
> > > > > -     struct vm_area_struct *vma = vma_next(&priv->iter);
> > > > > +     struct vm_area_struct *vma;
> > > > > +     int ret;
> > > > > +
> > > > > +     if (priv->mmap_locked)
> > > > > +             return vma_next(&priv->iter);
> > > > > +
> > > > > +     unlock_vma(priv);
> > > > > +     /*
> > > > > +      * Record sequence number ahead of vma lookup.
> > > > > +      * Odd seqcount means address space modification is in progress.
> > > > > +      */
> > > > > +     mmap_lock_speculate_try_begin(priv->mm, &priv->mm_wr_seq);
> > > >
> > > > Hmm we're discarding the return value I guess we don't really care about
> > > > that at this stage? Or do we? Do we want to assert the read critical
> > > > section state here?
> > >
> > > Yeah, as I mentioned, instead of relying on priv->mm_wr_seq being odd
> > > I should record the return value of mmap_lock_speculate_try_begin().
> > > In the functional sense these two are interchangeable.
> >
> > Ack, thanks!
> >
> > >
> > > >
> > > > I guess since we have the mm_rq_seq which we use later it's the same thing
> > > > and doesn't matter.
> > >
> > > Yep.
> >
> > Ack
> >
> > >
> > > >
> > > > ~~(off topic a bit)~~
> > > >
> > > > OK so off-topic again afaict we're doing something pretty horribly gross here.
> > > >
> > > > We pass &priv->mm_rw_seq as 'unsigned int *seq' field to
> > > > mmap_lock_speculate_try_begin(), which in turn calls:
> > > >
> > > >         return raw_seqcount_try_begin(&mm->mm_lock_seq, *seq);
> > > >
> > > > And this is defined as a macro of:
> > > >
> > > > #define raw_seqcount_try_begin(s, start)                                \
> > > > ({                                                                      \
> > > >         start = raw_read_seqcount(s);                                   \
> > > >         !(start & 1);                                                   \
> > > > })
> > > >
> > > > So surely this expands to:
> > > >
> > > >         *seq = raw_read_seqcount(&mm->mm_lock_seq);
> > > >         !(*seq & 1) // return true if even, false if odd
> > > >
> > > > So we're basically ostensibly passing an unsigned int, but because we're
> > > > calling a macro it's actually just 'text' and we're instead able to then
> > > > reassign the underlying unsigned int * ptr and... ugh.
> > > >
> > > > ~~(/off topic a bit)~~
> > >
> > > Aaaand we are back...
> >
> > :)) yeah this isn't your fault, just a related 'wtf' moan :P we can pretend
> > like it never happened *ahem*
> >
> > >
> > > >
> > > > > +     vma = vma_next(&priv->iter);
> > > >
> > > >
> > > >
> > > > > +     if (!vma)
> > > > > +             return NULL;
> > > > > +
> > > > > +     vma = trylock_vma(priv, vma, last_pos, true);
> > > > > +     if (vma)
> > > > > +             return vma;
> > > > > +
> > > >
> > > > Really feels like this should be a boolean... I guess neat to reset vma if
> > > > not locked though.
> > >
> > > I guess I can change trylock_vma() to return boolean. We always return
> > > the same vma or NULL I think.
> >
> > Ack, I mean I guess you're looking at reworking it in general so can take
> > this into account.
>
> Ack.
>
> >
> > >
> > > >
> > > > > +     /* Address space got modified, vma might be stale. Re-lock and retry */
> > > >
> > > > > +     rcu_read_unlock();
> > > >
> > > > Might we see a VMA possibly actually legit unmapped in a race here? Do we
> > > > need to update last_pos/ppos to account for this? Otherwise we might just
> > > > fail on the last_pos >= vma->vm_end check in trylock_vma() no?
> > >
> > > Yes, it can happen and trylock_vma() will fail to lock the modified
> > > VMA. That's by design. In such cases we retry the lookup from the same
> > > last_pos.
> >
> > OK and then we're fine with it because the gap we report will be an actual
> > gap.
>
> Yes, either the actual gap or a VMA newly mapped at that address.
>
> >
> > >
> > > >
> > > > > +     ret = mmap_read_lock_killable(priv->mm);
> > > >
> > > > Shouldn't we set priv->mmap_locked here?
> > >
> > > No, we will drop the mmap_read_lock shortly. priv->mmap_locked
> > > indicates the overall mode we operate in. When priv->mmap_locked=false
> > > we can still temporarily take the mmap_read_lock when retrying and
> > > then drop it after we found the VMA.
> >
> > Right yeah, makes sense.
> >
> > >
> > > >
> > > > I guess not as we are simply holding the mmap lock to definitely get the
> > > > next VMA.
> > >
> > > Correct.
> >
> > Ack
> >
> > >
> > > >
> > > > > +     rcu_read_lock();
> > > > > +     if (ret)
> > > > > +             return ERR_PTR(ret);
> > > > > +
> > > > > +     /* Lookup the vma at the last position again under mmap_read_lock */
> > > > > +     vma_iter_init(&priv->iter, priv->mm, last_pos);
> > > > > +     vma = vma_next(&priv->iter);
> > > > > +     if (vma) {
> > > > > +             vma = trylock_vma(priv, vma, last_pos, false);
> > > >
> > > > Be good to use Liam's convention of /* mm_unstable = */ false to make this
> > > > clear.
> > >
> > > Yeah, I'm thinking of splitting trylock_vma() into two separate
> > > functions for mm_unstable=true and mm_unstable=false cases.
> >
> > Yes :) thanks!
> >
> > >
> > > >
> > > > Find it kinda weird again we're 'trylocking' something we already have
> > > > locked via the mmap lock but I already mentioend this... :)
> > > >
> > > > > +             WARN_ON(!vma); /* mm is stable, has to succeed */
> > > >
> > > > I wonder if this is really useful, at any rate seems like there'd be a
> > > > flood here so WARN_ON_ONCE()? Perhaps VM_WARN_ON_ONCE() given this really
> > > > really ought not happen?
> > >
> > > Well, I can't use BUG_ON(), so WARN_ON() is the next tool I have :) In
> > > reality this should never happen, so
> > > WARN_ON/WARN_ON_ONCE/WARN_ON_RATELIMITED/or whatever does not matter
> > > much.
> >
> > I think if you refactor into two separate functions this becomes even more
> > unnecessary because then you are using a vma lock function that can never
> > fail etc.
> >
> > I mean maybe just stick a VM_ in front if it's not going to happen but for
> > debug/dev/early stabilisation purposes we want to keep an eye on it.
>
> Yeah, I think after refactoring we won't need any warnings here.
>
> >
> > >
> > > >
> > > > > +     }
> > > > > +     mmap_read_unlock(priv->mm);
> > > > > +
> > > > > +     return vma;
> > > > > +}
> > > > > +
> > > > > +#else /* CONFIG_PER_VMA_LOCK */
> > > > >
> > > > > +static inline bool lock_content(struct seq_file *m,
> > > > > +                             struct proc_maps_private *priv)
> > > > > +{
> > > > > +     return mmap_read_lock_killable(priv->mm) == 0;
> > > > > +}
> > > > > +
> > > > > +static inline void unlock_content(struct proc_maps_private *priv)
> > > > > +{
> > > > > +     mmap_read_unlock(priv->mm);
> > > > > +}
> > > > > +
> > > > > +static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> > > > > +                                        loff_t last_pos)
> > > > > +{
> > > > > +     return vma_next(&priv->iter);
> > > > > +}
> > > > > +
> > > > > +#endif /* CONFIG_PER_VMA_LOCK */
> > > > > +
> > > > > +static struct vm_area_struct *proc_get_vma(struct seq_file *m, loff_t *ppos)
> > > > > +{
> > > > > +     struct proc_maps_private *priv = m->private;
> > > > > +     struct vm_area_struct *vma;
> > > > > +
> > > > > +     vma = get_next_vma(priv, *ppos);
> > > > > +     if (IS_ERR(vma))
> > > > > +             return vma;
> > > > > +
> > > > > +     /* Store previous position to be able to restart if needed */
> > > > > +     priv->last_pos = *ppos;
> > > > >       if (vma) {
> > > > > -             *ppos = vma->vm_start;
> > > > > +             /*
> > > > > +              * Track the end of the reported vma to ensure position changes
> > > > > +              * even if previous vma was merged with the next vma and we
> > > > > +              * found the extended vma with the same vm_start.
> > > > > +              */
> > > >
> > > > Right, so observing repetitions is acceptable in such circumstances? I mean
> > > > I agree.
> > >
> > > Yep, the VMA will be reported twice in such a case.
> >
> > Ack.
> >
> > >
> > > >
> > > > > +             *ppos = vma->vm_end;
> > > >
> > > > If we store the end, does the last_pos logic which resets the VMA iterator
> > > > later work correctly in all cases?
> > >
> > > I think so. By resetting to vma->vm_end we will start the next search
> > > from the address right next to the last reported VMA, no?
> >
> > Yeah, I was just wondering whether there were any odd corner case that
> > might be problematic.
> >
> > But since we treat last_pos as inclusive as you said in a response above,
> > and of course vma->vm_end is exclusive, then this makes sense.
> >
> > >
> > > >
> > > > >       } else {
> > > > >               *ppos = -2UL;
> > > > >               vma = get_gate_vma(priv->mm);
> > > >
> > > > Is it always the case that !vma here implies a gate VMA (yuck yuck)? I see
> > > > this was the original logic, but maybe put a comment about this as it's
> > > > weird and confusing? (and not your fault obviously :P)
> > >
> > > What comment would you like to see here?
> >
> > It's so gross this. I guess something about the inner workings of gate VMAs
> > and the use of -2UL as a weird sentinel etc.
>
> Ok, I'll try to add a meaningful comment here.
>
> >
> > But this is out of scope here.
> >
> > >
> > > >
> > > > Also, are all locks and state corectly handled in this case? Seems like one
> > > > of this nasty edge case situations that could have jagged edges...
> > >
> > > I think we are fine. get_next_vma() returned NULL, so we did not lock
> > > any VMA and priv->locked_vma should be NULL.
> > >
> > > >
> > > > > @@ -163,19 +322,21 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
> > > > >               return NULL;
> > > > >       }
> > > > >
> > > > > -     if (mmap_read_lock_killable(mm)) {
> > > > > +     if (!lock_content(m, priv)) {
> > > >
> > > > Nice that this just slots in like this! :)
> > > >
> > > > >               mmput(mm);
> > > > >               put_task_struct(priv->task);
> > > > >               priv->task = NULL;
> > > > >               return ERR_PTR(-EINTR);
> > > > >       }
> > > > >
> > > > > +     if (last_addr > 0)
> > > >
> > > > last_addr is an unsigned long, this will always be true.
> > >
> > > Not unless last_addr==0. That's what I'm really checking here: is this
> > > the first invocation of m_start(), in which case we are starting from
> > > the beginning and not restarting from priv->last_pos. Should I add a
> > > comment?
> >
> > Yeah sorry I was being an idiot, I misread this as >= 0 obviously.
> >
> > I had assumed you were checking for the -2 and -1 cases (though -1 early
> > exits above).
> >
> > So in that case, are you handling the gate VMA correctly here? Surely we
> > should exclude that? Wouldn't setting ppos = last_addr = priv->last_pos be
> > incorrect if this were a gate vma?
>
> You are actually right. last_addr can be -2UL here and we should not
> override it. I'll fix it. Thanks!
>
> >
> > Even if we then call get_gate_vma() we've changed these values? Or is that
> > fine?
> >
> > And yeah a comment would be good thanks!
> >
> > >
> > > >
> > > > You probably want to put an explicit check for -1UL, -2UL here or?
> > > >
> > > > God I hate this mechanism for indicating gate VMA... yuck yuck (again, this
> > > > bit not your fault :P)
> > >
> > > No, I don't care here about -1UL, -2UL, just that last_addr==0 or not.
> >
> > OK, so maybe above concerns not a thing.
> >
> > >
> > > >
> > > > > +             *ppos = last_addr = priv->last_pos;
> > > > >       vma_iter_init(&priv->iter, mm, last_addr);
> > > > >       hold_task_mempolicy(priv);
> > > > >       if (last_addr == -2UL)
> > > > >               return get_gate_vma(mm);
> > > > >
> > > > > -     return proc_get_vma(priv, ppos);
> > > > > +     return proc_get_vma(m, ppos);
> > > > >  }
> > > > >
> > > > >  static void *m_next(struct seq_file *m, void *v, loff_t *ppos)
> > > > > @@ -184,7 +345,7 @@ static void *m_next(struct seq_file *m, void *v, loff_t *ppos)
> > > > >               *ppos = -1UL;
> > > > >               return NULL;
> > > > >       }
> > > > > -     return proc_get_vma(m->private, ppos);
> > > > > +     return proc_get_vma(m, ppos);
> > > > >  }
> > > > >
> > > > >  static void m_stop(struct seq_file *m, void *v)
> > > > > @@ -196,7 +357,7 @@ static void m_stop(struct seq_file *m, void *v)
> > > > >               return;
> > > > >
> > > > >       release_task_mempolicy(priv);
> > > > > -     mmap_read_unlock(mm);
> > > > > +     unlock_content(priv);
> > > > >       mmput(mm);
> > > > >       put_task_struct(priv->task);
> > > > >       priv->task = NULL;
> > > > > --
> > > > > 2.49.0.1266.g31b7d2e469-goog
> > > > >
> > > >
> > > > Sorry to add to workload by digging into so many details here, but we
> > > > really need to make sure all the i's are dotted and t's are crossed given
> > > > how fiddly and fragile this stuff is :)
> > > >
> > > > Very much appreciate the work, this is a significant improvement and will
> > > > have a great deal of real world impact!
> > >
> > > Thanks for meticulously going over the code! This is really helpful.
> > > Suren.
> >
> > No problem!
> >
> > >
> > > >
> > > > Cheers, Lorenzo

Powered by blists - more mailing lists