[PATCH v2 0/2] Try to release mmap_lock temporarily in smaps_rollup

[PATCH v2 0/2] Try to release mmap_lock temporarily in smaps_rollup

[Chinwen Chang]

Recently, we have observed some janky issues caused by unpleasantly long
contention on mmap_lock which is held by smaps_rollup when probing large
processes. To address the problem, we let smaps_rollup detect if anyone
wants to acquire mmap_lock for write attempts. If yes, just release the
lock temporarily to ease the contention.

smaps_rollup is a procfs interface which allows users to summarize the
process's memory usage without the overhead of seq_* calls. Android uses it
to sample the memory usage of various processes to balance its memory pool
sizes. If no one wants to take the lock for write requests, smaps_rollup
with this patch will behave like the original one.

Although there are on-going mmap_lock optimizations like range-based locks,
the lock applied to smaps_rollup would be the coarse one, which is hard to
avoid the occurrence of aforementioned issues. So the detection and
temporary release for write attempts on mmap_lock in smaps_rollup is still
necessary.

Change since v1:
- If current VMA is freed after dropping the lock, it will return
- incomplete result. To fix this issue, refine the code flow as
- suggested by Steve. [1]

[1] https://lore.kernel.org/lkml/bf40676e-b14b-44cd-75ce-419c70194783@arm.com/


Chinwen Chang (2):
  mmap locking API: add mmap_lock_is_contended()
  mm: proc: smaps_rollup: do not stall write attempts on mmap_lock

 fs/proc/task_mmu.c        | 57 ++++++++++++++++++++++++++++++++++++++-
 include/linux/mmap_lock.h |  5 ++++
 2 files changed, 61 insertions(+), 1 deletion(-)

[PATCH v2 1/2] mmap locking API: add mmap_lock_is_contended()

[Chinwen Chang]

Add new API to query if someone wants to acquire mmap_lock
for write attempts.

Using this instead of rwsem_is_contended makes it more tolerant
of future changes to the lock type.

Signed-off-by: Chinwen Chang <chinwen.chang@mediatek.com>
---
 include/linux/mmap_lock.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
index 0707671..18e7eae 100644
--- a/include/linux/mmap_lock.h
+++ b/include/linux/mmap_lock.h
@@ -87,4 +87,9 @@ static inline void mmap_assert_write_locked(struct mm_struct *mm)
 	VM_BUG_ON_MM(!rwsem_is_locked(&mm->mmap_lock), mm);
 }
 
+static inline int mmap_lock_is_contended(struct mm_struct *mm)
+{
+	return rwsem_is_contended(&mm->mmap_lock);
+}
+
 #endif /* _LINUX_MMAP_LOCK_H */
-- 
1.9.1

Re: [PATCH v2 1/2] mmap locking API: add mmap_lock_is_contended()

[Steven Price]

On 13/08/2020 03:13, Chinwen Chang wrote:
> Add new API to query if someone wants to acquire mmap_lock
> for write attempts.
> 
> Using this instead of rwsem_is_contended makes it more tolerant
> of future changes to the lock type.
> 
> Signed-off-by: Chinwen Chang <chinwen.chang@mediatek.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>   include/linux/mmap_lock.h | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/mmap_lock.h b/include/linux/mmap_lock.h
> index 0707671..18e7eae 100644
> --- a/include/linux/mmap_lock.h
> +++ b/include/linux/mmap_lock.h
> @@ -87,4 +87,9 @@ static inline void mmap_assert_write_locked(struct mm_struct *mm)
>   	VM_BUG_ON_MM(!rwsem_is_locked(&mm->mmap_lock), mm);
>   }
>   
> +static inline int mmap_lock_is_contended(struct mm_struct *mm)
> +{
> +	return rwsem_is_contended(&mm->mmap_lock);
> +}
> +
>   #endif /* _LINUX_MMAP_LOCK_H */
> 

Re: [PATCH v2 1/2] mmap locking API: add mmap_lock_is_contended()

[Michel Lespinasse]

On Wed, Aug 12, 2020 at 7:14 PM Chinwen Chang
<chinwen.chang@mediatek.com> wrote:
>
> Add new API to query if someone wants to acquire mmap_lock
> for write attempts.
>
> Using this instead of rwsem_is_contended makes it more tolerant
> of future changes to the lock type.
>
> Signed-off-by: Chinwen Chang <chinwen.chang@mediatek.com>

Acked-by: Michel Lespinasse <walken@google.com>

[PATCH v2 2/2] mm: proc: smaps_rollup: do not stall write attempts on mmap_lock

[Chinwen Chang]

smaps_rollup will try to grab mmap_lock and go through the whole vma
list until it finishes the iterating. When encountering large processes,
the mmap_lock will be held for a longer time, which may block other
write requests like mmap and munmap from progressing smoothly.

There are upcoming mmap_lock optimizations like range-based locks, but
the lock applied to smaps_rollup would be the coarse type, which doesn't
avoid the occurrence of unpleasant contention.

To solve aforementioned issue, we add a check which detects whether
anyone wants to grab mmap_lock for write attempts.

Change since v1:
- If current VMA is freed after dropping the lock, it will return
- incomplete result. To fix this issue, refine the code flow as
- suggested by Steve. [1]

[1] https://lore.kernel.org/lkml/bf40676e-b14b-44cd-75ce-419c70194783@arm.com/

Signed-off-by: Chinwen Chang <chinwen.chang@mediatek.com>
---
 fs/proc/task_mmu.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 55 insertions(+), 1 deletion(-)

diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index dbda449..23b3a447 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -853,9 +853,63 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
 
 	hold_task_mempolicy(priv);
 
-	for (vma = priv->mm->mmap; vma; vma = vma->vm_next) {
+	for (vma = priv->mm->mmap; vma;) {
 		smap_gather_stats(vma, &mss);
 		last_vma_end = vma->vm_end;
+
+		/*
+		 * Release mmap_lock temporarily if someone wants to
+		 * access it for write request.
+		 */
+		if (mmap_lock_is_contended(mm)) {
+			mmap_read_unlock(mm);
+			ret = mmap_read_lock_killable(mm);
+			if (ret) {
+				release_task_mempolicy(priv);
+				goto out_put_mm;
+			}
+
+			/*
+			 * After dropping the lock, there are three cases to
+			 * consider. See the following example for explanation.
+			 *
+			 *   +------+------+-----------+
+			 *   | VMA1 | VMA2 | VMA3      |
+			 *   +------+------+-----------+
+			 *   |      |      |           |
+			 *  4k     8k     16k         400k
+			 *
+			 * Suppose we drop the lock after reading VMA2 due to
+			 * contention, then we get:
+			 *
+			 *	last_vma_end = 16k
+			 *
+			 * 1) VMA2 is freed, but VMA3 exists:
+			 *
+			 *    find_vma(mm, 16k - 1) will return VMA3.
+			 *    In this case, just continue from VMA3.
+			 *
+			 * 2) VMA2 still exists:
+			 *
+			 *    find_vma(mm, 16k - 1) will return VMA2.
+			 *    Iterate the loop like the original one.
+			 *
+			 * 3) No more VMAs can be found:
+			 *
+			 *    find_vma(mm, 16k - 1) will return NULL.
+			 *    No more things to do, just break.
+			 */
+			vma = find_vma(mm, last_vma_end - 1);
+			/* Case 3 above */
+			if (!vma)
+				break;
+
+			/* Case 1 above */
+			if (vma->vm_start >= last_vma_end)
+				continue;
+		}
+		/* Case 2 above */
+		vma = vma->vm_next;
 	}
 
 	show_vma_header_prefix(m, priv->mm->mmap->vm_start,
-- 
1.9.1

Re: [PATCH v2 2/2] mm: proc: smaps_rollup: do not stall write attempts on mmap_lock

[Steven Price]

On 13/08/2020 03:13, Chinwen Chang wrote:
> smaps_rollup will try to grab mmap_lock and go through the whole vma
> list until it finishes the iterating. When encountering large processes,
> the mmap_lock will be held for a longer time, which may block other
> write requests like mmap and munmap from progressing smoothly.
> 
> There are upcoming mmap_lock optimizations like range-based locks, but
> the lock applied to smaps_rollup would be the coarse type, which doesn't
> avoid the occurrence of unpleasant contention.
> 
> To solve aforementioned issue, we add a check which detects whether
> anyone wants to grab mmap_lock for write attempts.
> 
> Change since v1:
> - If current VMA is freed after dropping the lock, it will return
> - incomplete result. To fix this issue, refine the code flow as
> - suggested by Steve. [1]
> 
> [1] https://lore.kernel.org/lkml/bf40676e-b14b-44cd-75ce-419c70194783@arm.com/
> 
> Signed-off-by: Chinwen Chang <chinwen.chang@mediatek.com>

Reviewed-by: Steven Price <steven.price@arm.com>

> ---
>   fs/proc/task_mmu.c | 56 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>   1 file changed, 55 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index dbda449..23b3a447 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -853,9 +853,63 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>   
>   	hold_task_mempolicy(priv);
>   
> -	for (vma = priv->mm->mmap; vma; vma = vma->vm_next) {
> +	for (vma = priv->mm->mmap; vma;) {
>   		smap_gather_stats(vma, &mss);
>   		last_vma_end = vma->vm_end;
> +
> +		/*
> +		 * Release mmap_lock temporarily if someone wants to
> +		 * access it for write request.
> +		 */
> +		if (mmap_lock_is_contended(mm)) {
> +			mmap_read_unlock(mm);
> +			ret = mmap_read_lock_killable(mm);
> +			if (ret) {
> +				release_task_mempolicy(priv);
> +				goto out_put_mm;
> +			}
> +
> +			/*
> +			 * After dropping the lock, there are three cases to
> +			 * consider. See the following example for explanation.
> +			 *
> +			 *   +------+------+-----------+
> +			 *   | VMA1 | VMA2 | VMA3      |
> +			 *   +------+------+-----------+
> +			 *   |      |      |           |
> +			 *  4k     8k     16k         400k
> +			 *
> +			 * Suppose we drop the lock after reading VMA2 due to
> +			 * contention, then we get:
> +			 *
> +			 *	last_vma_end = 16k
> +			 *
> +			 * 1) VMA2 is freed, but VMA3 exists:
> +			 *
> +			 *    find_vma(mm, 16k - 1) will return VMA3.
> +			 *    In this case, just continue from VMA3.
> +			 *
> +			 * 2) VMA2 still exists:
> +			 *
> +			 *    find_vma(mm, 16k - 1) will return VMA2.
> +			 *    Iterate the loop like the original one.
> +			 *
> +			 * 3) No more VMAs can be found:
> +			 *
> +			 *    find_vma(mm, 16k - 1) will return NULL.
> +			 *    No more things to do, just break.
> +			 */
> +			vma = find_vma(mm, last_vma_end - 1);
> +			/* Case 3 above */
> +			if (!vma)
> +				break;
> +
> +			/* Case 1 above */
> +			if (vma->vm_start >= last_vma_end)
> +				continue;
> +		}
> +		/* Case 2 above */
> +		vma = vma->vm_next;
>   	}
>   
>   	show_vma_header_prefix(m, priv->mm->mmap->vm_start,
> 

Re: [PATCH v2 2/2] mm: proc: smaps_rollup: do not stall write attempts on mmap_lock

[Michel Lespinasse]

On Wed, Aug 12, 2020 at 7:13 PM Chinwen Chang
<chinwen.chang@mediatek.com> wrote:
> smaps_rollup will try to grab mmap_lock and go through the whole vma
> list until it finishes the iterating. When encountering large processes,
> the mmap_lock will be held for a longer time, which may block other
> write requests like mmap and munmap from progressing smoothly.
>
> There are upcoming mmap_lock optimizations like range-based locks, but
> the lock applied to smaps_rollup would be the coarse type, which doesn't
> avoid the occurrence of unpleasant contention.
>
> To solve aforementioned issue, we add a check which detects whether
> anyone wants to grab mmap_lock for write attempts.

I think your retry mechanism still doesn't handle all cases. When you
get back the mmap lock, the address where you stopped last time could
now be in the middle of a vma. I think the consistent thing to do in
that case would be to retry scanning from the address you stopped at,
even if it's not on a vma boundary anymore. You may have to change
smap_gather_stats to support that, though.

Re: [PATCH v2 2/2] mm: proc: smaps_rollup: do not stall write attempts on mmap_lock

[Chinwen Chang]

On Fri, 2020-08-14 at 01:35 -0700, Michel Lespinasse wrote:
> On Wed, Aug 12, 2020 at 7:13 PM Chinwen Chang
> <chinwen.chang@mediatek.com> wrote:
> > smaps_rollup will try to grab mmap_lock and go through the whole vma
> > list until it finishes the iterating. When encountering large processes,
> > the mmap_lock will be held for a longer time, which may block other
> > write requests like mmap and munmap from progressing smoothly.
> >
> > There are upcoming mmap_lock optimizations like range-based locks, but
> > the lock applied to smaps_rollup would be the coarse type, which doesn't
> > avoid the occurrence of unpleasant contention.
> >
> > To solve aforementioned issue, we add a check which detects whether
> > anyone wants to grab mmap_lock for write attempts.
> 
> I think your retry mechanism still doesn't handle all cases. When you
> get back the mmap lock, the address where you stopped last time could
> now be in the middle of a vma. I think the consistent thing to do in
> that case would be to retry scanning from the address you stopped at,
> even if it's not on a vma boundary anymore. You may have to change
> smap_gather_stats to support that, though.

Hi Michel,

I think I got your point. Let me try to prepare new patch series for
further reviews.

Thank you for your suggestion :)

Chinwen

Re: [PATCH v2 0/2] Try to release mmap_lock temporarily in smaps_rollup

[Michel Lespinasse]

On Wed, Aug 12, 2020 at 7:14 PM Chinwen Chang
<chinwen.chang@mediatek.com> wrote:
> Recently, we have observed some janky issues caused by unpleasantly long
> contention on mmap_lock which is held by smaps_rollup when probing large
> processes. To address the problem, we let smaps_rollup detect if anyone
> wants to acquire mmap_lock for write attempts. If yes, just release the
> lock temporarily to ease the contention.
>
> smaps_rollup is a procfs interface which allows users to summarize the
> process's memory usage without the overhead of seq_* calls. Android uses it
> to sample the memory usage of various processes to balance its memory pool
> sizes. If no one wants to take the lock for write requests, smaps_rollup
> with this patch will behave like the original one.
>
> Although there are on-going mmap_lock optimizations like range-based locks,
> the lock applied to smaps_rollup would be the coarse one, which is hard to
> avoid the occurrence of aforementioned issues. So the detection and
> temporary release for write attempts on mmap_lock in smaps_rollup is still
> necessary.

I do not mind extending the mmap lock API as needed. However, in the
past I have tried adding rwsem_is_contended to mlock(), and later to
mm_populate() paths, and IIRC gotten pushback on it both times. I
don't feel strongly on this, but would prefer if someone else approved
the rwsem_is_contended() use case.

Couple related questions, how many VMAs are we looking at here ? Would
need_resched() be workable too ?

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.

Re: [PATCH v2 0/2] Try to release mmap_lock temporarily in smaps_rollup

[Chinwen Chang]

On Thu, 2020-08-13 at 02:53 -0700, Michel Lespinasse wrote:
> On Wed, Aug 12, 2020 at 7:14 PM Chinwen Chang
> <chinwen.chang@mediatek.com> wrote:
> > Recently, we have observed some janky issues caused by unpleasantly long
> > contention on mmap_lock which is held by smaps_rollup when probing large
> > processes. To address the problem, we let smaps_rollup detect if anyone
> > wants to acquire mmap_lock for write attempts. If yes, just release the
> > lock temporarily to ease the contention.
> >
> > smaps_rollup is a procfs interface which allows users to summarize the
> > process's memory usage without the overhead of seq_* calls. Android uses it
> > to sample the memory usage of various processes to balance its memory pool
> > sizes. If no one wants to take the lock for write requests, smaps_rollup
> > with this patch will behave like the original one.
> >
> > Although there are on-going mmap_lock optimizations like range-based locks,
> > the lock applied to smaps_rollup would be the coarse one, which is hard to
> > avoid the occurrence of aforementioned issues. So the detection and
> > temporary release for write attempts on mmap_lock in smaps_rollup is still
> > necessary.
> 
> I do not mind extending the mmap lock API as needed. However, in the
> past I have tried adding rwsem_is_contended to mlock(), and later to
> mm_populate() paths, and IIRC gotten pushback on it both times. I
> don't feel strongly on this, but would prefer if someone else approved
> the rwsem_is_contended() use case.
> 
Hi Michel,

Thank you for your kind feedback.

In my opinion, the difference between the case in smaps_rollup and the
one in your example is that, for the former, the interference comes from
the outside of the affected process, for the latter, it doesn't.

In other words, anyone may use smaps_rollup to probe the affected
process without the information about what step the affected one is
executing.

> Couple related questions, how many VMAs are we looking at here ? Would
> need_resched() be workable too ?
> 
It depends on the types of applications. The number of VMAs we are
looking at is around 3000 ~ 4000.

As far as I know, need_resched() is used by the caller to check if it
should release the CPU resource for others. But in the case of
smaps_rollup, the affected process is contended on the mmap_lock, not
waiting for CPU. So need_resched() may not be workable here.

Thanks again for your comment.
Chinwen

Re: [PATCH v2 0/2] Try to release mmap_lock temporarily in smaps_rollup

[Michel Lespinasse]

On Thu, Aug 13, 2020 at 9:11 AM Chinwen Chang
<chinwen.chang@mediatek.com> wrote:
> On Thu, 2020-08-13 at 02:53 -0700, Michel Lespinasse wrote:
> > On Wed, Aug 12, 2020 at 7:14 PM Chinwen Chang
> > <chinwen.chang@mediatek.com> wrote:
> > > Recently, we have observed some janky issues caused by unpleasantly long
> > > contention on mmap_lock which is held by smaps_rollup when probing large
> > > processes. To address the problem, we let smaps_rollup detect if anyone
> > > wants to acquire mmap_lock for write attempts. If yes, just release the
> > > lock temporarily to ease the contention.
> > >
> > > smaps_rollup is a procfs interface which allows users to summarize the
> > > process's memory usage without the overhead of seq_* calls. Android uses it
> > > to sample the memory usage of various processes to balance its memory pool
> > > sizes. If no one wants to take the lock for write requests, smaps_rollup
> > > with this patch will behave like the original one.
> > >
> > > Although there are on-going mmap_lock optimizations like range-based locks,
> > > the lock applied to smaps_rollup would be the coarse one, which is hard to
> > > avoid the occurrence of aforementioned issues. So the detection and
> > > temporary release for write attempts on mmap_lock in smaps_rollup is still
> > > necessary.
> >
> > I do not mind extending the mmap lock API as needed. However, in the
> > past I have tried adding rwsem_is_contended to mlock(), and later to
> > mm_populate() paths, and IIRC gotten pushback on it both times. I
> > don't feel strongly on this, but would prefer if someone else approved
> > the rwsem_is_contended() use case.
> >
> Hi Michel,
>
> Thank you for your kind feedback.
>
> In my opinion, the difference between the case in smaps_rollup and the
> one in your example is that, for the former, the interference comes from
> the outside of the affected process, for the latter, it doesn't.
>
> In other words, anyone may use smaps_rollup to probe the affected
> process without the information about what step the affected one is
> executing.

Thanks, that is a good enough point for me :)

-- 
Michel "Walken" Lespinasse
A program is never fully debugged until the last user dies.