* [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock
@ 2026-09-07 6:39 Suren Baghdasaryan
2026-09-07 6:39 ` [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers Suren Baghdasaryan
` (5 more replies)
0 siblings, 6 replies; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-07 6:39 UTC (permalink / raw)
To: akpm
Cc: liam, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel, surenb
proc/pid/smaps_rollup can be read using the combination of RCU and
VMA read locks, similar to proc/pid/{maps|smaps|numa_maps}. RCU is
required to safely traverse the VMA tree and VMA lock stabilizes the
VMA being processed and the pagetable walk.
Note that we have to keep the logic to drop mmap_lock on contention
because even when using per-VMA locks we might have to fall back to
holding the mmap_lock.
The first 3 patches are cleanups making later change simpler. The main
change is in patch 4. Patch 5 extends existing proc-maps-race tearing
test to verify smaps_rollup content.
Changes since v1 [1]
- removed helper functions which are not needed after per-VMA locks
became unconditional in [2];
- added a cover letter, per Lorenzo Stoakes and David Hildenbrand;
- removed is_mmap_lock_contended() as it's no more needed after [2],
per David Hildenbrand;
- fixed typos, per Lorenzo Stoakes and David Hildenbrand;
- refactored duplicate error checks, per Lorenzo Stoakes;
- eliminated special case of start=0 in smap_gather_stats(),
per Lorenzo Stoakes;
- updated comments, per Lorenzo Stoakes;
- moved original comment about 4 possible cases when mmap lock is
dropped into patch 4 changelog;
- added smaps_rollup testing into proc-maps-race test.
[1] https://lore.kernel.org/all/20260606015729.1837935-1-surenb@google.com/
[2] https://lore.kernel.org/all/20260813193433.3318288-1-surenb@google.com/
Suren Baghdasaryan (5):
proc/task_mmu: remove unnecessary helpers
proc/task_mmu: remove unnecessary inlines in function definitions
proc/task_mmu: remove special-casing of smap_gather_stats() start
parameter
proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock
selftests/proc: add /proc/pid/smaps_rollup tearing tests
fs/proc/task_mmu.c | 280 +++++++-----------
tools/testing/selftests/proc/proc-maps-race.c | 187 +++++++++++-
2 files changed, 291 insertions(+), 176 deletions(-)
base-commit: e3fc12b08aadde9cec7b3799ac0e0c9a1aa245c4
--
2.55.0.979.g7e5102b832-goog
^ permalink raw reply [flat|nested] 36+ messages in thread
* [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers
2026-09-07 6:39 [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
@ 2026-09-07 6:39 ` Suren Baghdasaryan
2026-09-07 16:44 ` Usama Arif
` (3 more replies)
2026-09-07 6:39 ` [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions Suren Baghdasaryan
` (4 subsequent siblings)
5 siblings, 4 replies; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-07 6:39 UTC (permalink / raw)
To: akpm
Cc: liam, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel, surenb
When per-vma locks were behind a config option, a number of helper
functions were needed to simplify the locking code. Now that these
locks are universally available, we can do a little cleanup.
Remove lock_vma_range(), unlock_vma_range(), query_vma_setup(),
query_vma_teardown() helpers.
No functional change intended.
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
fs/proc/task_mmu.c | 67 ++++++++++++----------------------------------
1 file changed, 17 insertions(+), 50 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index e671b4fd8ded..2f500d639db5 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -160,25 +160,6 @@ static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
}
}
-static inline bool lock_vma_range(struct seq_file *m,
- struct proc_maps_locking_ctx *lock_ctx)
-{
- rcu_read_lock();
- reset_lock_ctx(lock_ctx);
-
- return true;
-}
-
-static inline void unlock_vma_range(struct proc_maps_locking_ctx *lock_ctx)
-{
- if (lock_ctx->mmap_locked) {
- unlock_ctx_mm(lock_ctx);
- } else {
- unlock_ctx_vma(lock_ctx);
- rcu_read_unlock();
- }
-}
-
static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
loff_t last_pos)
{
@@ -286,13 +267,8 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
return NULL;
}
- if (!lock_vma_range(m, lock_ctx)) {
- mmput(mm);
- put_task_struct(priv->task);
- priv->task = NULL;
- return ERR_PTR(-EINTR);
- }
-
+ rcu_read_lock();
+ reset_lock_ctx(lock_ctx);
/*
* Reset current position if last_addr was set before
* and it's not a sentinel.
@@ -325,7 +301,12 @@ static void m_stop(struct seq_file *m, void *v)
return;
release_task_mempolicy(priv);
- unlock_vma_range(&priv->lock_ctx);
+ if (priv->lock_ctx.mmap_locked) {
+ unlock_ctx_mm(&priv->lock_ctx);
+ } else {
+ unlock_ctx_vma(&priv->lock_ctx);
+ rcu_read_unlock();
+ }
mmput(mm);
put_task_struct(priv->task);
priv->task = NULL;
@@ -518,21 +499,6 @@ static int pid_maps_open(struct inode *inode, struct file *file)
PROCMAP_QUERY_VMA_FLAGS \
)
-static int query_vma_setup(struct proc_maps_locking_ctx *lock_ctx)
-{
- reset_lock_ctx(lock_ctx);
-
- return 0;
-}
-
-static void query_vma_teardown(struct proc_maps_locking_ctx *lock_ctx)
-{
- if (lock_ctx->mmap_locked)
- unlock_ctx_mm(lock_ctx);
- else
- unlock_ctx_vma(lock_ctx);
-}
-
static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ctx *lock_ctx,
unsigned long addr)
{
@@ -653,12 +619,7 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
if (!mm || !mmget_not_zero(mm))
return -ESRCH;
- err = query_vma_setup(&lock_ctx);
- if (err) {
- mmput(mm);
- return err;
- }
-
+ reset_lock_ctx(&lock_ctx);
vma = query_matching_vma(&lock_ctx, karg.query_addr, karg.query_flags);
if (IS_ERR(vma)) {
err = PTR_ERR(vma);
@@ -732,7 +693,10 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
vm_file = get_file(vma->vm_file);
/* unlock vma or mmap_lock, and put mm_struct before copying data to user */
- query_vma_teardown(&lock_ctx);
+ if (lock_ctx.mmap_locked)
+ unlock_ctx_mm(&lock_ctx);
+ else
+ unlock_ctx_vma(&lock_ctx);
mmput(mm);
if (karg.build_id_size) {
@@ -773,7 +737,10 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
return 0;
out:
- query_vma_teardown(&lock_ctx);
+ if (lock_ctx.mmap_locked)
+ unlock_ctx_mm(&lock_ctx);
+ else
+ unlock_ctx_vma(&lock_ctx);
mmput(mm);
out_file:
if (vm_file)
--
2.55.0.979.g7e5102b832-goog
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions
2026-09-07 6:39 [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
2026-09-07 6:39 ` [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers Suren Baghdasaryan
@ 2026-09-07 6:39 ` Suren Baghdasaryan
2026-09-07 16:49 ` Usama Arif
` (3 more replies)
2026-09-07 6:39 ` [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter Suren Baghdasaryan
` (3 subsequent siblings)
5 siblings, 4 replies; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-07 6:39 UTC (permalink / raw)
To: akpm
Cc: liam, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel, surenb
It was pointed out in the previous reviews of this code that many
functions are specified as inline, which is unnecessary as the compile
can make that decision by itself. Cleanup these definitions.
No functional change intended.
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
fs/proc/task_mmu.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 2f500d639db5..9908ba32f180 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -130,7 +130,7 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
}
#endif
-static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
+static int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
{
int ret = mmap_read_lock_killable(lock_ctx->mm);
@@ -140,7 +140,7 @@ static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
return ret;
}
-static inline void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
+static void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
{
mmap_read_unlock(lock_ctx->mm);
lock_ctx->mmap_locked = false;
@@ -177,7 +177,7 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
return vma;
}
-static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
+static bool fallback_to_mmap_lock(struct proc_maps_private *priv,
loff_t pos)
{
struct proc_maps_locking_ctx *lock_ctx = &priv->lock_ctx;
@@ -194,7 +194,7 @@ static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
return true;
}
-static inline void drop_rcu(struct proc_maps_private *priv)
+static void drop_rcu(struct proc_maps_private *priv)
{
if (priv->lock_ctx.mmap_locked)
return;
@@ -202,7 +202,7 @@ static inline void drop_rcu(struct proc_maps_private *priv)
rcu_read_unlock();
}
-static inline void reacquire_rcu(struct proc_maps_private *priv)
+static void reacquire_rcu(struct proc_maps_private *priv)
{
if (priv->lock_ctx.mmap_locked)
return;
@@ -1230,7 +1230,7 @@ static const struct mm_walk_ops smaps_shmem_walk_vma_lock_ops = {
.walk_lock = PGWALK_VMA_RDLOCK_VERIFY,
};
-static inline const struct mm_walk_ops *
+static const struct mm_walk_ops *
get_smaps_walk_ops(struct proc_maps_private *priv)
{
if (priv->lock_ctx.mmap_locked)
@@ -1238,7 +1238,7 @@ get_smaps_walk_ops(struct proc_maps_private *priv)
return &smaps_walk_vma_lock_ops;
}
-static inline const struct mm_walk_ops *
+static const struct mm_walk_ops *
get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
{
if (priv->lock_ctx.mmap_locked)
@@ -1572,7 +1572,7 @@ struct clear_refs_private {
enum clear_refs_types type;
};
-static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
+static bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
{
struct folio *folio;
@@ -1588,8 +1588,8 @@ static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr,
return folio_maybe_dma_pinned(folio);
}
-static inline void clear_soft_dirty(struct vm_area_struct *vma,
- unsigned long addr, pte_t *pte)
+static void clear_soft_dirty(struct vm_area_struct *vma, unsigned long addr,
+ pte_t *pte)
{
if (!pgtable_supports_soft_dirty())
return;
@@ -1620,8 +1620,8 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
}
#if defined(CONFIG_TRANSPARENT_HUGEPAGE)
-static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
- unsigned long addr, pmd_t *pmdp)
+static void clear_soft_dirty_pmd(struct vm_area_struct *vma,
+ unsigned long addr, pmd_t *pmdp)
{
pmd_t old, pmd = *pmdp;
@@ -1646,8 +1646,8 @@ static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
}
}
#else
-static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
- unsigned long addr, pmd_t *pmdp)
+static void clear_soft_dirty_pmd(struct vm_area_struct *vma,
+ unsigned long addr, pmd_t *pmdp)
{
}
#endif
@@ -1846,7 +1846,7 @@ struct pagemapread {
#define PM_END_OF_BUFFER 1
-static inline pagemap_entry_t make_pme(u64 frame, u64 flags)
+static pagemap_entry_t make_pme(u64 frame, u64 flags)
{
return (pagemap_entry_t) { .pme = (frame & PM_PFRAME_MASK) | flags };
}
@@ -3388,7 +3388,7 @@ static const struct mm_walk_ops show_numa_vma_lock_ops = {
.walk_lock = PGWALK_VMA_RDLOCK_VERIFY,
};
-static inline const struct mm_walk_ops *
+static const struct mm_walk_ops *
get_show_numa_ops(struct proc_maps_private *priv)
{
if (priv->lock_ctx.mmap_locked)
--
2.55.0.979.g7e5102b832-goog
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter
2026-09-07 6:39 [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
2026-09-07 6:39 ` [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers Suren Baghdasaryan
2026-09-07 6:39 ` [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions Suren Baghdasaryan
@ 2026-09-07 6:39 ` Suren Baghdasaryan
2026-09-08 18:07 ` Liam R. Howlett
2026-09-09 17:16 ` David Hildenbrand (Arm)
2026-09-07 6:39 ` [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
` (2 subsequent siblings)
5 siblings, 2 replies; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-07 6:39 UTC (permalink / raw)
To: akpm
Cc: liam, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel, surenb
smap_gather_stats() interprets its start parameter to mean vma->vm_start
when it's set to 0. Eliminate this special interpretation and pass
vma->vm_start explicitly when needed.
Since smap_gather_stats() operates within a single VMA, we can replace
walk_page_vma()/walk_page_range() calls with walk_page_range_vma()
which is simpler and also can be called while holding per-VMA lock.
No functional change intended.
Suggested by: Lorenzo Stoakes <ljs@kernel.org>
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
fs/proc/task_mmu.c | 40 ++++++++++++++++++++++------------------
1 file changed, 22 insertions(+), 18 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 9908ba32f180..3351decd1172 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1246,20 +1246,27 @@ get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
return &smaps_shmem_walk_vma_lock_ops;
}
-/*
- * Gather mem stats from @vma with the indicated beginning
- * address @start, and keep them in @mss.
+/**
+ * smap_gather_stats() - Gather mem stats from @vma.
+ * @priv: proc maps private state.
+ * @vma: The VMA to gather stats for.
+ * @mss: The accumulated stats.
+ * @start: The address from which to start.
*
- * Use vm_start of @vma as the beginning address if @start is 0.
+ * This gathers stats for the whole of the VMA unless the lock was dropped
+ * and VMA grew or got merged and we found it again, in which case we only
+ * gather stats for the remainder of the VMA range.
*/
static void smap_gather_stats(struct proc_maps_private *priv,
struct vm_area_struct *vma,
- struct mem_size_stats *mss, unsigned long start)
+ struct mem_size_stats *mss,
+ unsigned long start)
{
const struct mm_walk_ops *ops = get_smaps_walk_ops(priv);
+ const bool is_partial = start > vma->vm_start;
/* Invalid start */
- if (start >= vma->vm_end)
+ if (start < vma->vm_start || start >= vma->vm_end)
return;
if (vma == get_gate_vma(priv->lock_ctx.mm))
@@ -1279,20 +1286,17 @@ static void smap_gather_stats(struct proc_maps_private *priv,
* Unless we know that the shmem object (or the part mapped by
* our VMA) has no swapped out pages at all.
*/
- unsigned long shmem_swapped = shmem_swap_usage(vma);
+ const unsigned long shmem_swapped = shmem_swap_usage(vma);
+ const bool shared_or_ro = vma_test(vma, VMA_SHARED_BIT) ||
+ !vma_test(vma, VMA_WRITE_BIT);
- if (!start && (!shmem_swapped || (vma->vm_flags & VM_SHARED) ||
- !(vma->vm_flags & VM_WRITE))) {
+ if (!is_partial && (!shmem_swapped || shared_or_ro))
mss->swap += shmem_swapped;
- } else {
+ else
ops = get_smaps_shmem_walk_ops(priv);
- }
}
- if (!start)
- walk_page_vma(vma, ops, mss);
- else
- walk_page_range(vma->vm_mm, start, vma->vm_end, ops, mss);
+ walk_page_range_vma(vma, start, vma->vm_end, ops, mss);
reacquire_rcu(priv);
}
@@ -1347,7 +1351,7 @@ static int show_smap(struct seq_file *m, void *v)
struct vm_area_struct *vma = v;
struct mem_size_stats mss = {};
- smap_gather_stats(priv, vma, &mss, 0);
+ smap_gather_stats(priv, vma, &mss, vma->vm_start);
show_map_vma(m, vma);
@@ -1400,7 +1404,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
vma_start = vma->vm_start;
do {
- smap_gather_stats(priv, vma, &mss, 0);
+ smap_gather_stats(priv, vma, &mss, vma->vm_start);
last_vma_end = vma->vm_end;
/*
@@ -1459,7 +1463,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
/* Case 1 and 2 above */
if (vma->vm_start >= last_vma_end) {
- smap_gather_stats(priv, vma, &mss, 0);
+ smap_gather_stats(priv, vma, &mss, vma->vm_start);
last_vma_end = vma->vm_end;
continue;
}
--
2.55.0.979.g7e5102b832-goog
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock
2026-09-07 6:39 [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
` (2 preceding siblings ...)
2026-09-07 6:39 ` [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter Suren Baghdasaryan
@ 2026-09-07 6:39 ` Suren Baghdasaryan
2026-09-08 18:17 ` Liam R. Howlett
` (2 more replies)
2026-09-07 6:39 ` [PATCH v2 5/5] selftests/proc: add /proc/pid/smaps_rollup tearing tests Suren Baghdasaryan
2026-09-08 16:04 ` [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock Xueyuan Chen
5 siblings, 3 replies; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-07 6:39 UTC (permalink / raw)
To: akpm
Cc: liam, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel, surenb
proc/pid/smaps_rollup can be read using the combination of RCU and
VMA read locks, similar to proc/pid/{maps|smaps|numa_maps}. RCU is
required to safely traverse the VMA tree and VMA lock stabilizes the
VMA being processed and the pagetable walk.
Note that we have to keep the logic to drop mmap_lock on contention
because even when using per-VMA locks we might have to fall back to
holding the mmap_lock.
Running Paul's contention benchmark [1] shows considerable improvement
both in median and in the worst case latencies:
Execution command: run-proc-vs-map.sh --nsamples 20 --rawdata -- \
--busyduration 2 --procfile smaps_rollup
Baseline:
Median Minimum Maximum
0.174 0.161 2.553
0.174 0.164 2.663
0.174 0.165 2.664
0.174 0.166 2.679
0.174 0.167 2.691
0.174 0.168 2.704
0.174 0.169 2.729
0.174 0.172 2.741
0.174 0.174 2.745
0.174 0.174 2.755
0.174 0.175 2.790
0.174 0.177 2.809
0.174 0.179 3.096
0.174 0.183 3.144
0.174 0.184 3.158
0.174 0.185 3.175
0.174 0.185 4.568
0.174 0.198 4.821
0.174 0.214 5.143
0.174 0.251 5.220
Patched:
Median Minimum Maximum
0.007 0.007 1.952
0.007 0.007 1.955
0.007 0.007 1.955
0.007 0.007 1.955
0.007 0.007 1.957
0.007 0.007 1.969
0.007 0.007 2.065
0.007 0.007 2.075
0.007 0.007 2.146
0.007 0.007 2.195
0.007 0.007 2.223
0.007 0.007 2.259
0.007 0.007 2.488
0.007 0.007 2.562
0.007 0.007 2.599
0.007 0.007 2.697
0.007 0.007 3.030
0.007 0.007 3.075
0.007 0.007 3.145
0.007 0.007 3.225
Remove now unused lock_ctx_mm() and move unlock_ctx_vma() next to
unlock_ctx_mm() as they are logically related.
Remove a long comment about 4 cases that we handle when dropping the
mmap lock in the middle of VMA walk due to contention. The first 3
cases explained there are handled naturally and only case 4 needs to
be handled in a special way, which is done in smap_gather_stats() by
gathering stats from the portion of the VMA that has not yet been
processed.
For posterity, moving this comment here:
After dropping the lock, there are four 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:
vma_next(vmi) will return VMA3.
In this case, just continue from VMA3.
2) VMA2 still exists:
vma_next(vmi) will return VMA3.
In this case, just continue from VMA3.
3) No more VMAs can be found:
vma_next(vmi) will return NULL.
No more things to do, just break.
4) (last_vma_end - 1) is the middle of a vma (VMA'):
vma_next(vmi) will return VMA' whose range
contains last_vma_end.
Iterate VMA' from last_vma_end.
[1] https://github.com/paulmckrcu/proc-mmap_sem-test
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
fs/proc/task_mmu.c | 153 ++++++++++++++++++---------------------------
1 file changed, 60 insertions(+), 93 deletions(-)
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index 3351decd1172..641a155b0c61 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -130,28 +130,12 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
}
#endif
-static int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
-{
- int ret = mmap_read_lock_killable(lock_ctx->mm);
-
- if (!ret)
- lock_ctx->mmap_locked = true;
-
- return ret;
-}
-
static void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
{
mmap_read_unlock(lock_ctx->mm);
lock_ctx->mmap_locked = false;
}
-static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
-{
- lock_ctx->locked_vma = NULL;
- lock_ctx->mmap_locked = false;
-}
-
static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
{
if (lock_ctx->locked_vma) {
@@ -160,6 +144,12 @@ static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
}
}
+static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
+{
+ lock_ctx->locked_vma = NULL;
+ lock_ctx->mmap_locked = false;
+}
+
static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
loff_t last_pos)
{
@@ -1376,12 +1366,14 @@ static int show_smap(struct seq_file *m, void *v)
static int show_smaps_rollup(struct seq_file *m, void *v)
{
struct proc_maps_private *priv = m->private;
+ struct proc_maps_locking_ctx *lock_ctx = &priv->lock_ctx;
+ struct mm_struct *mm = lock_ctx->mm;
struct mem_size_stats mss = {};
- struct mm_struct *mm = priv->lock_ctx.mm;
+ unsigned long last_vma_end = 0;
+ unsigned long vma_start = 0;
struct vm_area_struct *vma;
- unsigned long vma_start = 0, last_vma_end = 0;
+ loff_t pos = 0;
int ret = 0;
- VMA_ITERATOR(vmi, mm, 0);
priv->task = get_proc_task(priv->inode);
if (!priv->task)
@@ -1392,89 +1384,60 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
goto out_put_task;
}
- ret = lock_ctx_mm(&priv->lock_ctx);
- if (ret)
- goto out_put_mm;
-
hold_task_mempolicy(priv);
- vma = vma_next(&vmi);
+ rcu_read_lock();
+ reset_lock_ctx(lock_ctx);
+ vma_iter_init(&priv->iter, mm, 0);
+ vma = proc_get_vma(m, &pos);
if (unlikely(!vma))
goto empty_set;
- vma_start = vma->vm_start;
- do {
- smap_gather_stats(priv, vma, &mss, vma->vm_start);
- last_vma_end = vma->vm_end;
+ if (!IS_ERR(vma) && vma != get_gate_vma(lock_ctx->mm))
+ vma_start = vma->vm_start;
+
+ while (vma) {
+ if (IS_ERR(vma)) {
+ ret = PTR_ERR(vma);
+ goto out_unlock;
+ }
+
+ if (vma == get_gate_vma(lock_ctx->mm))
+ break;
/*
- * Release mmap_lock temporarily if someone wants to
- * access it for write request.
+ * If after retaking the lock, already reported VMA grew or
+ * merged with the next one, smap_gather_stats() will gather
+ * stats for the remaining portion by starting at last_vma_end.
*/
- if (mmap_lock_is_contended(mm)) {
- vma_iter_invalidate(&vmi);
- unlock_ctx_mm(&priv->lock_ctx);
- ret = lock_ctx_mm(&priv->lock_ctx);
- if (ret) {
- release_task_mempolicy(priv);
- goto out_put_mm;
- }
+ smap_gather_stats(priv, vma, &mss, last_vma_end);
+ last_vma_end = vma->vm_end;
+ /*
+ * If the VMA lock is not taken, we hold the often contended
+ * mmap lock. This can happen if we had to fall back to the
+ * mmap lock.
+ *
+ * To relieve pressure, check if it is indeed contended, then
+ * temporarily release it.
+ */
+ if (lock_ctx->mmap_locked &&
+ mmap_lock_is_contended(lock_ctx->mm)) {
+ unlock_ctx_mm(lock_ctx);
/*
- * After dropping the lock, there are four 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:
- *
- * vma_next(vmi) will return VMA3.
- * In this case, just continue from VMA3.
- *
- * 2) VMA2 still exists:
- *
- * vma_next(vmi) will return VMA3.
- * In this case, just continue from VMA3.
- *
- * 3) No more VMAs can be found:
- *
- * vma_next(vmi) will return NULL.
- * No more things to do, just break.
- *
- * 4) (last_vma_end - 1) is the middle of a vma (VMA'):
- *
- * vma_next(vmi) will return VMA' whose range
- * contains last_vma_end.
- * Iterate VMA' from last_vma_end.
+ * Even though we previously fell back to mmap lock,
+ * we try taking VMA lock for the next VMA, since it
+ * might not be under modification. In the worst case
+ * we will fall back to mmap lock again.
*/
- vma = vma_next(&vmi);
- /* Case 3 above */
- if (!vma)
- break;
-
- /* Case 1 and 2 above */
- if (vma->vm_start >= last_vma_end) {
- smap_gather_stats(priv, vma, &mss, vma->vm_start);
- last_vma_end = vma->vm_end;
- continue;
- }
-
- /* Case 4 above */
- if (vma->vm_end > last_vma_end) {
- smap_gather_stats(priv, vma, &mss, last_vma_end);
- last_vma_end = vma->vm_end;
- }
+ rcu_read_lock();
+ reset_lock_ctx(lock_ctx);
+ /* Resume from the last position. */
+ pos = last_vma_end;
+ vma_iter_init(&priv->iter, mm, pos);
}
- } for_each_vma(vmi, vma);
+ vma = proc_get_vma(m, &pos);
+ }
empty_set:
show_vma_header_prefix(m, vma_start, last_vma_end, 0, 0, 0, 0);
@@ -1483,10 +1446,14 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
__show_smap(m, &mss, true);
+out_unlock:
+ if (lock_ctx->mmap_locked) {
+ unlock_ctx_mm(lock_ctx);
+ } else {
+ unlock_ctx_vma(lock_ctx);
+ rcu_read_unlock();
+ }
release_task_mempolicy(priv);
- unlock_ctx_mm(&priv->lock_ctx);
-
-out_put_mm:
mmput(mm);
out_put_task:
put_task_struct(priv->task);
--
2.55.0.979.g7e5102b832-goog
^ permalink raw reply related [flat|nested] 36+ messages in thread
* [PATCH v2 5/5] selftests/proc: add /proc/pid/smaps_rollup tearing tests
2026-09-07 6:39 [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
` (3 preceding siblings ...)
2026-09-07 6:39 ` [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
@ 2026-09-07 6:39 ` Suren Baghdasaryan
2026-09-08 18:18 ` Liam R. Howlett
2026-09-08 16:04 ` [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock Xueyuan Chen
5 siblings, 1 reply; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-07 6:39 UTC (permalink / raw)
To: akpm
Cc: liam, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel, surenb
During tearing tests, smaps_rollup Pss* metrics should stay constant.
Extend /proc/pid/smaps tearing tests to also check for smaps_rollup
consistency.
Signed-off-by: Suren Baghdasaryan <surenb@google.com>
---
tools/testing/selftests/proc/proc-maps-race.c | 187 +++++++++++++++++-
1 file changed, 182 insertions(+), 5 deletions(-)
diff --git a/tools/testing/selftests/proc/proc-maps-race.c b/tools/testing/selftests/proc/proc-maps-race.c
index 415eccb70468..8d00d7db1c65 100644
--- a/tools/testing/selftests/proc/proc-maps-race.c
+++ b/tools/testing/selftests/proc/proc-maps-race.c
@@ -80,6 +80,61 @@ enum maps_file {
struct vma_modifier_info;
+enum smaps_rollup_stat {
+ Rss,
+ Pss,
+ Pss_Dirty,
+ Pss_Anon,
+ Pss_File,
+ Pss_Shmem,
+ Shared_Clean,
+ Shared_Dirty,
+ Private_Clean,
+ Private_Dirty,
+ Referenced,
+ Anonymous,
+ KSM,
+ LazyFree,
+ AnonHugePages,
+ ShmemPmdMapped,
+ FilePmdMapped,
+ Shared_Hugetlb,
+ Private_Hugetlb,
+ Swap,
+ SwapPss,
+ Locked,
+ RollupFieldCount
+};
+
+static const char *smaps_rollup_stat_names[RollupFieldCount] = {
+ "Rss",
+ "Pss",
+ "Pss_Dirty",
+ "Pss_Anon",
+ "Pss_File",
+ "Pss_Shmem",
+ "Shared_Clean",
+ "Shared_Dirty",
+ "Private_Clean",
+ "Private_Dirty",
+ "Referenced",
+ "Anonymous",
+ "KSM",
+ "LazyFree",
+ "AnonHugePages",
+ "ShmemPmdMapped",
+ "FilePmdMapped",
+ "Shared_Hugetlb",
+ "Private_Hugetlb",
+ "Swap",
+ "SwapPss",
+ "Locked",
+};
+
+struct smaps_rollup_stats {
+ unsigned long values[RollupFieldCount];
+};
+
FIXTURE(proc_maps_race)
{
struct vma_modifier_info *mod_info;
@@ -91,6 +146,7 @@ FIXTURE(proc_maps_race)
enum maps_file maps_file;
int shared_mem_size;
int skip_pages;
+ int rollup_fd;
int page_size;
int vma_count;
bool verbose;
@@ -132,12 +188,12 @@ struct vma_modifier_info {
void *child_mapped_addr[];
};
-static bool read_page(FIXTURE_DATA(proc_maps_race) *self,
+static bool read_page(FIXTURE_DATA(proc_maps_race) *self, int fd,
struct page_content *page)
{
ssize_t bytes_read;
- bytes_read = read(self->maps_fd, page->data, self->page_size);
+ bytes_read = read(fd, page->data, self->page_size);
if (bytes_read <= 0)
return false;
@@ -175,7 +231,7 @@ static int locate_containing_page(FIXTURE_DATA(proc_maps_race) *self,
char *curr_pos;
char *end_pos;
- if (!read_page(self, &self->page1))
+ if (!read_page(self, self->maps_fd, &self->page1))
return -1;
curr_pos = self->page1.data;
@@ -205,10 +261,11 @@ static bool read_two_pages(FIXTURE_DATA(proc_maps_race) *self)
return false;
for (int i = 0; i < self->skip_pages; i++)
- if (!read_page(self, &self->page1))
+ if (!read_page(self, self->maps_fd, &self->page1))
return false;
- return read_page(self, &self->page1) && read_page(self, &self->page2);
+ return read_page(self, self->maps_fd, &self->page1) &&
+ read_page(self, self->maps_fd, &self->page2);
}
static void copy_line(const char *line_start, const char *line_end,
@@ -317,6 +374,61 @@ static bool read_boundary_lines(FIXTURE_DATA(proc_maps_race) *self,
&first_line->end_addr) == 2;
}
+static bool parse_smaps_rollup(FIXTURE_DATA(proc_maps_race) *self,
+ struct smaps_rollup_stats *stats)
+{
+ unsigned int dev_maj, dev_min, inode;
+ unsigned long start, end, offs;
+ unsigned long value;
+ char name[32], perm[5];
+ char *curr_pos;
+ char *end_pos;
+ char *line_end;
+
+ if (lseek(self->rollup_fd, 0, SEEK_SET) < 0)
+ return false;
+
+ if (!read_page(self, self->rollup_fd, &self->page1))
+ return false;
+
+ curr_pos = self->page1.data;
+ end_pos = self->page1.data + self->page1.size;
+
+ line_end = strchr(curr_pos, '\n');
+ if (!line_end)
+ return false;
+
+ if (sscanf(curr_pos, "%lx-%lx %4s %lx %u:%u %u %31s",
+ &start, &end, perm, &offs, &dev_maj, &dev_min, &inode, name) != 8)
+ return false;
+
+ if (strcmp(name, "[rollup]"))
+ return false;
+
+ for (int stat = 0; stat < ARRAY_SIZE(smaps_rollup_stat_names); stat++) {
+ int len;
+
+ curr_pos = line_end + 1;
+ if (curr_pos >= end_pos)
+ return false;
+
+ line_end = strchr(curr_pos, '\n');
+ if (!line_end)
+ return false;
+
+ if (sscanf(curr_pos, "%31s %lu kB", name, &value) != 2)
+ return false;
+
+ len = strlen(name);
+ if (name[len - 1] != ':' || strncmp(name, smaps_rollup_stat_names[stat], len - 1))
+ return false;
+
+ stats->values[stat] = value;
+ }
+
+ return true;
+}
+
/* Thread synchronization routines */
static void wait_for_state(struct vma_modifier_info *mod_info, enum test_state state)
{
@@ -397,6 +509,41 @@ static bool print_boundaries_on(bool condition, const char *title,
return condition;
}
+static void print_smaps_rollup_stats(const char *title, FIXTURE_DATA(proc_maps_race) *self,
+ struct smaps_rollup_stats *stats)
+{
+ printf("%s", title);
+ for (int stat = 0; stat < ARRAY_SIZE(smaps_rollup_stat_names); stat++)
+ printf("%64s %lu kB\n", smaps_rollup_stat_names[stat], stats->values[stat]);
+}
+
+static bool cmp_smaps_rollup_stat(struct smaps_rollup_stats *s1,
+ struct smaps_rollup_stats *s2,
+ enum smaps_rollup_stat stat)
+{
+ return s1->values[stat] == s2->values[stat];
+}
+
+static bool compare_smaps_rollup(FIXTURE_DATA(proc_maps_race) *self,
+ struct smaps_rollup_stats *expected,
+ struct smaps_rollup_stats *actual)
+{
+ /*
+ * Clean/dirty metrics might change but Pss-related ones
+ * should stay constant.
+ */
+ if (cmp_smaps_rollup_stat(expected, actual, Pss) &&
+ cmp_smaps_rollup_stat(expected, actual, Pss_Anon) &&
+ cmp_smaps_rollup_stat(expected, actual, Pss_File) &&
+ cmp_smaps_rollup_stat(expected, actual, Pss_Shmem))
+ return true;
+
+ print_smaps_rollup_stats("Expected stats:", self, expected);
+ print_smaps_rollup_stats("Actual stats:", self, actual);
+
+ return false;
+}
+
static void report_test_start(const char *name, bool verbose)
{
if (verbose)
@@ -572,6 +719,7 @@ FIXTURE_SETUP(proc_maps_race)
unsigned long first_map_addr;
unsigned long last_map_addr;
unsigned long duration_sec;
+ char rollup_fname[32];
char fname[32];
self->page_size = (unsigned long)sysconf(_SC_PAGESIZE);
@@ -649,6 +797,9 @@ FIXTURE_SETUP(proc_maps_race)
break;
case SMAPS:
sprintf(fname, "/proc/%d/smaps", self->pid);
+ sprintf(rollup_fname, "/proc/%d/smaps_rollup", self->pid);
+ self->rollup_fd = open(rollup_fname, O_RDONLY);
+ ASSERT_NE(self->rollup_fd, -1);
break;
default:
ksft_exit_fail();
@@ -711,6 +862,8 @@ FIXTURE_TEARDOWN(proc_maps_race)
for (int i = 0; i < self->vma_count; i++)
munmap(self->mod_info->child_mapped_addr[i], self->page_size);
close(self->maps_fd);
+ if (self->maps_file == SMAPS)
+ close(self->rollup_fd);
waitpid(self->pid, &status, 0);
munmap(self->mod_info, self->shared_mem_size);
}
@@ -723,6 +876,7 @@ TEST_F(proc_maps_race, test_maps_tearing_from_split)
struct line_content split_first_line;
struct line_content restored_last_line;
struct line_content restored_first_line;
+ struct smaps_rollup_stats orig_stats;
wait_for_state(mod_info, SETUP_READY);
@@ -736,6 +890,8 @@ TEST_F(proc_maps_race, test_maps_tearing_from_split)
report_test_start("Tearing from split", self->verbose);
ASSERT_TRUE(capture_mod_pattern(self, &split_last_line, &split_first_line,
&restored_last_line, &restored_first_line));
+ if (self->maps_file == SMAPS)
+ ASSERT_TRUE(parse_smaps_rollup(self, &orig_stats));
/* Now start concurrent modifications for self->duration_sec */
signal_state(mod_info, TEST_READY);
@@ -799,6 +955,11 @@ TEST_F(proc_maps_race, test_maps_tearing_from_split)
vma_end == self->last_line.end_addr) ||
(vma_start == split_first_line.start_addr &&
vma_end == split_first_line.end_addr));
+ } else {
+ struct smaps_rollup_stats stats;
+
+ ASSERT_TRUE(parse_smaps_rollup(self, &stats));
+ ASSERT_TRUE(compare_smaps_rollup(self, &orig_stats, &stats));
}
clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
end_test_iteration(&end_ts, self->verbose);
@@ -817,6 +978,7 @@ TEST_F(proc_maps_race, test_maps_tearing_from_resize)
struct line_content shrunk_first_line;
struct line_content restored_last_line;
struct line_content restored_first_line;
+ struct smaps_rollup_stats orig_stats;
wait_for_state(mod_info, SETUP_READY);
@@ -830,6 +992,8 @@ TEST_F(proc_maps_race, test_maps_tearing_from_resize)
report_test_start("Tearing from resize", self->verbose);
ASSERT_TRUE(capture_mod_pattern(self, &shrunk_last_line, &shrunk_first_line,
&restored_last_line, &restored_first_line));
+ if (self->maps_file == SMAPS)
+ ASSERT_TRUE(parse_smaps_rollup(self, &orig_stats));
/* Now start concurrent modifications for self->duration_sec */
signal_state(mod_info, TEST_READY);
@@ -880,6 +1044,11 @@ TEST_F(proc_maps_race, test_maps_tearing_from_resize)
ASSERT_TRUE(vma_start == self->last_line.start_addr &&
(vma_end - vma_start == self->page_size * 3 ||
vma_end - vma_start == self->page_size));
+ } else {
+ struct smaps_rollup_stats stats;
+
+ ASSERT_TRUE(parse_smaps_rollup(self, &stats));
+ ASSERT_TRUE(compare_smaps_rollup(self, &orig_stats, &stats));
}
clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
end_test_iteration(&end_ts, self->verbose);
@@ -898,6 +1067,7 @@ TEST_F(proc_maps_race, test_maps_tearing_from_remap)
struct line_content remapped_first_line;
struct line_content restored_last_line;
struct line_content restored_first_line;
+ struct smaps_rollup_stats orig_stats;
wait_for_state(mod_info, SETUP_READY);
@@ -911,6 +1081,8 @@ TEST_F(proc_maps_race, test_maps_tearing_from_remap)
report_test_start("Tearing from remap", self->verbose);
ASSERT_TRUE(capture_mod_pattern(self, &remapped_last_line, &remapped_first_line,
&restored_last_line, &restored_first_line));
+ if (self->maps_file == SMAPS)
+ ASSERT_TRUE(parse_smaps_rollup(self, &orig_stats));
/* Now start concurrent modifications for self->duration_sec */
signal_state(mod_info, TEST_READY);
@@ -963,6 +1135,11 @@ TEST_F(proc_maps_race, test_maps_tearing_from_remap)
vma_end - vma_start == self->page_size * 3) ||
(vma_start == self->last_line.start_addr + self->page_size &&
vma_end - vma_start == self->page_size));
+ } else {
+ struct smaps_rollup_stats stats;
+
+ ASSERT_TRUE(parse_smaps_rollup(self, &stats));
+ ASSERT_TRUE(compare_smaps_rollup(self, &orig_stats, &stats));
}
clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
end_test_iteration(&end_ts, self->verbose);
--
2.55.0.979.g7e5102b832-goog
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers
2026-09-07 6:39 ` [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers Suren Baghdasaryan
@ 2026-09-07 16:44 ` Usama Arif
2026-09-08 17:58 ` Liam R. Howlett
` (2 subsequent siblings)
3 siblings, 0 replies; 36+ messages in thread
From: Usama Arif @ 2026-09-07 16:44 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Usama Arif, akpm, liam, ljs, vbabka, david, willy, jannh, paulmck,
pfalcato, linux-mm, linux-kernel, linux-fsdevel
On Sun, 6 Sep 2026 23:39:14 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> When per-vma locks were behind a config option, a number of helper
> functions were needed to simplify the locking code. Now that these
> locks are universally available, we can do a little cleanup.
> Remove lock_vma_range(), unlock_vma_range(), query_vma_setup(),
> query_vma_teardown() helpers.
>
> No functional change intended.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
> fs/proc/task_mmu.c | 67 ++++++++++++----------------------------------
> 1 file changed, 17 insertions(+), 50 deletions(-)
Change makes sense independent of the series as well.
Acked-by: Usama Arif <usama.arif@linux.dev>
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index e671b4fd8ded..2f500d639db5 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -160,25 +160,6 @@ static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
> }
> }
>
> -static inline bool lock_vma_range(struct seq_file *m,
> - struct proc_maps_locking_ctx *lock_ctx)
> -{
> - rcu_read_lock();
> - reset_lock_ctx(lock_ctx);
> -
> - return true;
> -}
> -
> -static inline void unlock_vma_range(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - if (lock_ctx->mmap_locked) {
> - unlock_ctx_mm(lock_ctx);
> - } else {
> - unlock_ctx_vma(lock_ctx);
> - rcu_read_unlock();
> - }
> -}
> -
> static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> loff_t last_pos)
> {
> @@ -286,13 +267,8 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
> return NULL;
> }
>
> - if (!lock_vma_range(m, lock_ctx)) {
> - mmput(mm);
> - put_task_struct(priv->task);
> - priv->task = NULL;
> - return ERR_PTR(-EINTR);
> - }
> -
> + rcu_read_lock();
> + reset_lock_ctx(lock_ctx);
> /*
> * Reset current position if last_addr was set before
> * and it's not a sentinel.
> @@ -325,7 +301,12 @@ static void m_stop(struct seq_file *m, void *v)
> return;
>
> release_task_mempolicy(priv);
> - unlock_vma_range(&priv->lock_ctx);
> + if (priv->lock_ctx.mmap_locked) {
> + unlock_ctx_mm(&priv->lock_ctx);
> + } else {
> + unlock_ctx_vma(&priv->lock_ctx);
> + rcu_read_unlock();
> + }
> mmput(mm);
> put_task_struct(priv->task);
> priv->task = NULL;
> @@ -518,21 +499,6 @@ static int pid_maps_open(struct inode *inode, struct file *file)
> PROCMAP_QUERY_VMA_FLAGS \
> )
>
> -static int query_vma_setup(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - reset_lock_ctx(lock_ctx);
> -
> - return 0;
> -}
> -
> -static void query_vma_teardown(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - if (lock_ctx->mmap_locked)
> - unlock_ctx_mm(lock_ctx);
> - else
> - unlock_ctx_vma(lock_ctx);
> -}
> -
> static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ctx *lock_ctx,
> unsigned long addr)
> {
> @@ -653,12 +619,7 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
> if (!mm || !mmget_not_zero(mm))
> return -ESRCH;
>
> - err = query_vma_setup(&lock_ctx);
> - if (err) {
> - mmput(mm);
> - return err;
> - }
> -
> + reset_lock_ctx(&lock_ctx);
> vma = query_matching_vma(&lock_ctx, karg.query_addr, karg.query_flags);
> if (IS_ERR(vma)) {
> err = PTR_ERR(vma);
> @@ -732,7 +693,10 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
> vm_file = get_file(vma->vm_file);
>
> /* unlock vma or mmap_lock, and put mm_struct before copying data to user */
> - query_vma_teardown(&lock_ctx);
> + if (lock_ctx.mmap_locked)
> + unlock_ctx_mm(&lock_ctx);
> + else
> + unlock_ctx_vma(&lock_ctx);
> mmput(mm);
>
> if (karg.build_id_size) {
> @@ -773,7 +737,10 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
> return 0;
>
> out:
> - query_vma_teardown(&lock_ctx);
> + if (lock_ctx.mmap_locked)
> + unlock_ctx_mm(&lock_ctx);
> + else
> + unlock_ctx_vma(&lock_ctx);
> mmput(mm);
> out_file:
> if (vm_file)
> --
> 2.55.0.979.g7e5102b832-goog
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions
2026-09-07 6:39 ` [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions Suren Baghdasaryan
@ 2026-09-07 16:49 ` Usama Arif
2026-09-10 15:35 ` Suren Baghdasaryan
2026-09-08 18:01 ` Liam R. Howlett
` (2 subsequent siblings)
3 siblings, 1 reply; 36+ messages in thread
From: Usama Arif @ 2026-09-07 16:49 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Usama Arif, akpm, liam, ljs, vbabka, david, willy, jannh, paulmck,
pfalcato, linux-mm, linux-kernel, linux-fsdevel
On Sun, 6 Sep 2026 23:39:15 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> It was pointed out in the previous reviews of this code that many
> functions are specified as inline, which is unnecessary as the compile
> can make that decision by itself. Cleanup these definitions.
>
> No functional change intended.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
> fs/proc/task_mmu.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
Most of these functions would be inlined anyways so should be ok.
Curious if there was a change in binary size with this?
Acked-by: Usama Arif <usama.arif@linux.dev>
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 2f500d639db5..9908ba32f180 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -130,7 +130,7 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
> }
> #endif
>
> -static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> +static int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> {
> int ret = mmap_read_lock_killable(lock_ctx->mm);
>
> @@ -140,7 +140,7 @@ static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> return ret;
> }
>
> -static inline void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> +static void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> {
> mmap_read_unlock(lock_ctx->mm);
> lock_ctx->mmap_locked = false;
> @@ -177,7 +177,7 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> return vma;
> }
>
> -static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> +static bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> loff_t pos)
> {
> struct proc_maps_locking_ctx *lock_ctx = &priv->lock_ctx;
> @@ -194,7 +194,7 @@ static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> return true;
> }
>
> -static inline void drop_rcu(struct proc_maps_private *priv)
> +static void drop_rcu(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> return;
> @@ -202,7 +202,7 @@ static inline void drop_rcu(struct proc_maps_private *priv)
> rcu_read_unlock();
> }
>
> -static inline void reacquire_rcu(struct proc_maps_private *priv)
> +static void reacquire_rcu(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> return;
> @@ -1230,7 +1230,7 @@ static const struct mm_walk_ops smaps_shmem_walk_vma_lock_ops = {
> .walk_lock = PGWALK_VMA_RDLOCK_VERIFY,
> };
>
> -static inline const struct mm_walk_ops *
> +static const struct mm_walk_ops *
> get_smaps_walk_ops(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> @@ -1238,7 +1238,7 @@ get_smaps_walk_ops(struct proc_maps_private *priv)
> return &smaps_walk_vma_lock_ops;
> }
>
> -static inline const struct mm_walk_ops *
> +static const struct mm_walk_ops *
> get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> @@ -1572,7 +1572,7 @@ struct clear_refs_private {
> enum clear_refs_types type;
> };
>
> -static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> +static bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> {
> struct folio *folio;
>
> @@ -1588,8 +1588,8 @@ static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr,
> return folio_maybe_dma_pinned(folio);
> }
>
> -static inline void clear_soft_dirty(struct vm_area_struct *vma,
> - unsigned long addr, pte_t *pte)
> +static void clear_soft_dirty(struct vm_area_struct *vma, unsigned long addr,
> + pte_t *pte)
> {
> if (!pgtable_supports_soft_dirty())
> return;
> @@ -1620,8 +1620,8 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
> }
>
> #if defined(CONFIG_TRANSPARENT_HUGEPAGE)
> -static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> - unsigned long addr, pmd_t *pmdp)
> +static void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> + unsigned long addr, pmd_t *pmdp)
> {
> pmd_t old, pmd = *pmdp;
>
> @@ -1646,8 +1646,8 @@ static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> }
> }
> #else
> -static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> - unsigned long addr, pmd_t *pmdp)
> +static void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> + unsigned long addr, pmd_t *pmdp)
> {
> }
> #endif
> @@ -1846,7 +1846,7 @@ struct pagemapread {
>
> #define PM_END_OF_BUFFER 1
>
> -static inline pagemap_entry_t make_pme(u64 frame, u64 flags)
> +static pagemap_entry_t make_pme(u64 frame, u64 flags)
> {
> return (pagemap_entry_t) { .pme = (frame & PM_PFRAME_MASK) | flags };
> }
> @@ -3388,7 +3388,7 @@ static const struct mm_walk_ops show_numa_vma_lock_ops = {
> .walk_lock = PGWALK_VMA_RDLOCK_VERIFY,
> };
>
> -static inline const struct mm_walk_ops *
> +static const struct mm_walk_ops *
> get_show_numa_ops(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> --
> 2.55.0.979.g7e5102b832-goog
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock
2026-09-07 6:39 [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
` (4 preceding siblings ...)
2026-09-07 6:39 ` [PATCH v2 5/5] selftests/proc: add /proc/pid/smaps_rollup tearing tests Suren Baghdasaryan
@ 2026-09-08 16:04 ` Xueyuan Chen
2026-09-08 16:08 ` Suren Baghdasaryan
5 siblings, 1 reply; 36+ messages in thread
From: Xueyuan Chen @ 2026-09-08 16:04 UTC (permalink / raw)
To: surenb
Cc: akpm, liam, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel, Xueyuan Chen
On Sun, Sep 06, 2026 at 11:39:13PM -0700, Suren Baghdasaryan wrote:
>proc/pid/smaps_rollup can be read using the combination of RCU and
>VMA read locks, similar to proc/pid/{maps|smaps|numa_maps}. RCU is
>required to safely traverse the VMA tree and VMA lock stabilizes the
>VMA being processed and the pagetable walk.
>Note that we have to keep the logic to drop mmap_lock on contention
>because even when using per-VMA locks we might have to fall back to
>holding the mmap_lock.
>
Hi Suren,
I tested this series on my arm64 24-core machine, using the same
command:
run-proc-vs-map.sh --nsamples 20 --rawdata -- \
--busyduration 2 --procfile smaps_rollup
baseline:
Median Minimum Maximum
0.132 0.121 2.600
0.132 0.123 2.768
0.132 0.123 2.804
0.132 0.125 2.835
0.132 0.125 2.837
0.132 0.125 2.849
0.132 0.125 2.858
0.132 0.125 2.888
0.132 0.130 2.894
0.132 0.132 2.896
0.132 0.132 2.915
0.132 0.134 2.921
0.132 0.134 2.927
0.132 0.137 2.945
0.132 0.141 2.984
0.132 0.246 3.004
0.132 0.270 3.381
0.132 1.316 3.533
0.132 1.321 3.551
0.132 1.343 3.924
patched:
Median Minimum Maximum
0.006 0.006 1.950
0.006 0.006 1.955
0.006 0.006 1.955
0.006 0.006 1.956
0.006 0.006 1.956
0.006 0.006 1.957
0.006 0.006 1.958
0.006 0.006 1.959
0.006 0.006 1.959
0.006 0.006 1.961
0.006 0.006 1.962
0.006 0.006 1.963
0.006 0.006 1.964
0.006 0.006 1.966
0.006 0.006 1.969
0.006 0.006 1.976
0.006 0.006 1.982
0.006 0.006 1.995
0.006 0.006 1.996
0.006 0.006 2.043
So the median is 0.132 -> 0.006 ms, about 22x, and the worst case
drops from 3.9 to 2.0 ms.
Tested-by: Xueyuan Chen <xueyuan.chen21@gmail.com>
Thanks,
Xueyuan
>The first 3 patches are cleanups making later change simpler. The main
>change is in patch 4. Patch 5 extends existing proc-maps-race tearing
>test to verify smaps_rollup content.
>
>Changes since v1 [1]
>- removed helper functions which are not needed after per-VMA locks
>became unconditional in [2];
>- added a cover letter, per Lorenzo Stoakes and David Hildenbrand;
>- removed is_mmap_lock_contended() as it's no more needed after [2],
>per David Hildenbrand;
>- fixed typos, per Lorenzo Stoakes and David Hildenbrand;
>- refactored duplicate error checks, per Lorenzo Stoakes;
>- eliminated special case of start=0 in smap_gather_stats(),
>per Lorenzo Stoakes;
>- updated comments, per Lorenzo Stoakes;
>- moved original comment about 4 possible cases when mmap lock is
>dropped into patch 4 changelog;
>- added smaps_rollup testing into proc-maps-race test.
>
>[1] https://lore.kernel.org/all/20260606015729.1837935-1-surenb@google.com/
>[2] https://lore.kernel.org/all/20260813193433.3318288-1-surenb@google.com/
>
>Suren Baghdasaryan (5):
> proc/task_mmu: remove unnecessary helpers
> proc/task_mmu: remove unnecessary inlines in function definitions
> proc/task_mmu: remove special-casing of smap_gather_stats() start
> parameter
> proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock
> selftests/proc: add /proc/pid/smaps_rollup tearing tests
>
> fs/proc/task_mmu.c | 280 +++++++-----------
> tools/testing/selftests/proc/proc-maps-race.c | 187 +++++++++++-
> 2 files changed, 291 insertions(+), 176 deletions(-)
>
>
>base-commit: e3fc12b08aadde9cec7b3799ac0e0c9a1aa245c4
>--
>2.55.0.979.g7e5102b832-goog
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock
2026-09-08 16:04 ` [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock Xueyuan Chen
@ 2026-09-08 16:08 ` Suren Baghdasaryan
0 siblings, 0 replies; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-08 16:08 UTC (permalink / raw)
To: Xueyuan Chen
Cc: akpm, liam, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On Tue, Sep 8, 2026 at 9:05 AM Xueyuan Chen <xueyuan.chen21@gmail.com> wrote:
>
>
> On Sun, Sep 06, 2026 at 11:39:13PM -0700, Suren Baghdasaryan wrote:
> >proc/pid/smaps_rollup can be read using the combination of RCU and
> >VMA read locks, similar to proc/pid/{maps|smaps|numa_maps}. RCU is
> >required to safely traverse the VMA tree and VMA lock stabilizes the
> >VMA being processed and the pagetable walk.
> >Note that we have to keep the logic to drop mmap_lock on contention
> >because even when using per-VMA locks we might have to fall back to
> >holding the mmap_lock.
> >
>
> Hi Suren,
>
> I tested this series on my arm64 24-core machine, using the same
> command:
>
> run-proc-vs-map.sh --nsamples 20 --rawdata -- \
> --busyduration 2 --procfile smaps_rollup
>
> baseline:
> Median Minimum Maximum
> 0.132 0.121 2.600
> 0.132 0.123 2.768
> 0.132 0.123 2.804
> 0.132 0.125 2.835
> 0.132 0.125 2.837
> 0.132 0.125 2.849
> 0.132 0.125 2.858
> 0.132 0.125 2.888
> 0.132 0.130 2.894
> 0.132 0.132 2.896
> 0.132 0.132 2.915
> 0.132 0.134 2.921
> 0.132 0.134 2.927
> 0.132 0.137 2.945
> 0.132 0.141 2.984
> 0.132 0.246 3.004
> 0.132 0.270 3.381
> 0.132 1.316 3.533
> 0.132 1.321 3.551
> 0.132 1.343 3.924
>
> patched:
> Median Minimum Maximum
> 0.006 0.006 1.950
> 0.006 0.006 1.955
> 0.006 0.006 1.955
> 0.006 0.006 1.956
> 0.006 0.006 1.956
> 0.006 0.006 1.957
> 0.006 0.006 1.958
> 0.006 0.006 1.959
> 0.006 0.006 1.959
> 0.006 0.006 1.961
> 0.006 0.006 1.962
> 0.006 0.006 1.963
> 0.006 0.006 1.964
> 0.006 0.006 1.966
> 0.006 0.006 1.969
> 0.006 0.006 1.976
> 0.006 0.006 1.982
> 0.006 0.006 1.995
> 0.006 0.006 1.996
> 0.006 0.006 2.043
>
> So the median is 0.132 -> 0.006 ms, about 22x, and the worst case
> drops from 3.9 to 2.0 ms.
>
> Tested-by: Xueyuan Chen <xueyuan.chen21@gmail.com>
Thanks for the confirmation!
>
> Thanks,
> Xueyuan
>
> >The first 3 patches are cleanups making later change simpler. The main
> >change is in patch 4. Patch 5 extends existing proc-maps-race tearing
> >test to verify smaps_rollup content.
> >
> >Changes since v1 [1]
> >- removed helper functions which are not needed after per-VMA locks
> >became unconditional in [2];
> >- added a cover letter, per Lorenzo Stoakes and David Hildenbrand;
> >- removed is_mmap_lock_contended() as it's no more needed after [2],
> >per David Hildenbrand;
> >- fixed typos, per Lorenzo Stoakes and David Hildenbrand;
> >- refactored duplicate error checks, per Lorenzo Stoakes;
> >- eliminated special case of start=0 in smap_gather_stats(),
> >per Lorenzo Stoakes;
> >- updated comments, per Lorenzo Stoakes;
> >- moved original comment about 4 possible cases when mmap lock is
> >dropped into patch 4 changelog;
> >- added smaps_rollup testing into proc-maps-race test.
> >
> >[1] https://lore.kernel.org/all/20260606015729.1837935-1-surenb@google.com/
> >[2] https://lore.kernel.org/all/20260813193433.3318288-1-surenb@google.com/
> >
> >Suren Baghdasaryan (5):
> > proc/task_mmu: remove unnecessary helpers
> > proc/task_mmu: remove unnecessary inlines in function definitions
> > proc/task_mmu: remove special-casing of smap_gather_stats() start
> > parameter
> > proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock
> > selftests/proc: add /proc/pid/smaps_rollup tearing tests
> >
> > fs/proc/task_mmu.c | 280 +++++++-----------
> > tools/testing/selftests/proc/proc-maps-race.c | 187 +++++++++++-
> > 2 files changed, 291 insertions(+), 176 deletions(-)
> >
> >
> >base-commit: e3fc12b08aadde9cec7b3799ac0e0c9a1aa245c4
> >--
> >2.55.0.979.g7e5102b832-goog
> >
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers
2026-09-07 6:39 ` [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers Suren Baghdasaryan
2026-09-07 16:44 ` Usama Arif
@ 2026-09-08 17:58 ` Liam R. Howlett
2026-09-09 17:06 ` David Hildenbrand (Arm)
2026-09-10 15:43 ` Lorenzo Stoakes (ARM)
3 siblings, 0 replies; 36+ messages in thread
From: Liam R. Howlett @ 2026-09-08 17:58 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On 26/09/06 11:39PM, Suren Baghdasaryan wrote:
> When per-vma locks were behind a config option, a number of helper
> functions were needed to simplify the locking code. Now that these
> locks are universally available, we can do a little cleanup.
> Remove lock_vma_range(), unlock_vma_range(), query_vma_setup(),
> query_vma_teardown() helpers.
>
> No functional change intended.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Reviewed-by: Liam R. Howlett (Oracle) <liam@infradead.org>
> ---
> fs/proc/task_mmu.c | 67 ++++++++++++----------------------------------
> 1 file changed, 17 insertions(+), 50 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index e671b4fd8ded..2f500d639db5 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -160,25 +160,6 @@ static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
> }
> }
>
> -static inline bool lock_vma_range(struct seq_file *m,
> - struct proc_maps_locking_ctx *lock_ctx)
> -{
> - rcu_read_lock();
> - reset_lock_ctx(lock_ctx);
> -
> - return true;
> -}
> -
> -static inline void unlock_vma_range(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - if (lock_ctx->mmap_locked) {
> - unlock_ctx_mm(lock_ctx);
> - } else {
> - unlock_ctx_vma(lock_ctx);
> - rcu_read_unlock();
> - }
> -}
> -
> static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> loff_t last_pos)
> {
> @@ -286,13 +267,8 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
> return NULL;
> }
>
> - if (!lock_vma_range(m, lock_ctx)) {
> - mmput(mm);
> - put_task_struct(priv->task);
> - priv->task = NULL;
> - return ERR_PTR(-EINTR);
> - }
> -
> + rcu_read_lock();
> + reset_lock_ctx(lock_ctx);
> /*
> * Reset current position if last_addr was set before
> * and it's not a sentinel.
> @@ -325,7 +301,12 @@ static void m_stop(struct seq_file *m, void *v)
> return;
>
> release_task_mempolicy(priv);
> - unlock_vma_range(&priv->lock_ctx);
> + if (priv->lock_ctx.mmap_locked) {
> + unlock_ctx_mm(&priv->lock_ctx);
> + } else {
> + unlock_ctx_vma(&priv->lock_ctx);
> + rcu_read_unlock();
> + }
> mmput(mm);
> put_task_struct(priv->task);
> priv->task = NULL;
> @@ -518,21 +499,6 @@ static int pid_maps_open(struct inode *inode, struct file *file)
> PROCMAP_QUERY_VMA_FLAGS \
> )
>
> -static int query_vma_setup(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - reset_lock_ctx(lock_ctx);
> -
> - return 0;
> -}
> -
> -static void query_vma_teardown(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - if (lock_ctx->mmap_locked)
> - unlock_ctx_mm(lock_ctx);
> - else
> - unlock_ctx_vma(lock_ctx);
> -}
> -
> static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ctx *lock_ctx,
> unsigned long addr)
> {
> @@ -653,12 +619,7 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
> if (!mm || !mmget_not_zero(mm))
> return -ESRCH;
>
> - err = query_vma_setup(&lock_ctx);
> - if (err) {
> - mmput(mm);
> - return err;
> - }
> -
> + reset_lock_ctx(&lock_ctx);
> vma = query_matching_vma(&lock_ctx, karg.query_addr, karg.query_flags);
> if (IS_ERR(vma)) {
> err = PTR_ERR(vma);
> @@ -732,7 +693,10 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
> vm_file = get_file(vma->vm_file);
>
> /* unlock vma or mmap_lock, and put mm_struct before copying data to user */
> - query_vma_teardown(&lock_ctx);
> + if (lock_ctx.mmap_locked)
> + unlock_ctx_mm(&lock_ctx);
> + else
> + unlock_ctx_vma(&lock_ctx);
> mmput(mm);
>
> if (karg.build_id_size) {
> @@ -773,7 +737,10 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
> return 0;
>
> out:
> - query_vma_teardown(&lock_ctx);
> + if (lock_ctx.mmap_locked)
> + unlock_ctx_mm(&lock_ctx);
> + else
> + unlock_ctx_vma(&lock_ctx);
> mmput(mm);
> out_file:
> if (vm_file)
> --
> 2.55.0.979.g7e5102b832-goog
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions
2026-09-07 6:39 ` [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions Suren Baghdasaryan
2026-09-07 16:49 ` Usama Arif
@ 2026-09-08 18:01 ` Liam R. Howlett
2026-09-09 17:07 ` David Hildenbrand (Arm)
2026-09-10 15:55 ` Lorenzo Stoakes (ARM)
3 siblings, 0 replies; 36+ messages in thread
From: Liam R. Howlett @ 2026-09-08 18:01 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On 26/09/06 11:39PM, Suren Baghdasaryan wrote:
> It was pointed out in the previous reviews of this code that many
> functions are specified as inline, which is unnecessary as the compile
> can make that decision by itself. Cleanup these definitions.
>
> No functional change intended.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
I'd rather two tabs for the multiple lines of arguments to a function,
but it's fine either way.
Reviewed-by: Liam R. Howlett (Oracle) <liam@infradead.org>
> ---
> fs/proc/task_mmu.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 2f500d639db5..9908ba32f180 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -130,7 +130,7 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
> }
> #endif
>
> -static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> +static int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> {
> int ret = mmap_read_lock_killable(lock_ctx->mm);
>
> @@ -140,7 +140,7 @@ static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> return ret;
> }
>
> -static inline void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> +static void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> {
> mmap_read_unlock(lock_ctx->mm);
> lock_ctx->mmap_locked = false;
> @@ -177,7 +177,7 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> return vma;
> }
>
> -static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> +static bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> loff_t pos)
> {
> struct proc_maps_locking_ctx *lock_ctx = &priv->lock_ctx;
> @@ -194,7 +194,7 @@ static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> return true;
> }
>
> -static inline void drop_rcu(struct proc_maps_private *priv)
> +static void drop_rcu(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> return;
> @@ -202,7 +202,7 @@ static inline void drop_rcu(struct proc_maps_private *priv)
> rcu_read_unlock();
> }
>
> -static inline void reacquire_rcu(struct proc_maps_private *priv)
> +static void reacquire_rcu(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> return;
> @@ -1230,7 +1230,7 @@ static const struct mm_walk_ops smaps_shmem_walk_vma_lock_ops = {
> .walk_lock = PGWALK_VMA_RDLOCK_VERIFY,
> };
>
> -static inline const struct mm_walk_ops *
> +static const struct mm_walk_ops *
> get_smaps_walk_ops(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> @@ -1238,7 +1238,7 @@ get_smaps_walk_ops(struct proc_maps_private *priv)
> return &smaps_walk_vma_lock_ops;
> }
>
> -static inline const struct mm_walk_ops *
> +static const struct mm_walk_ops *
> get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> @@ -1572,7 +1572,7 @@ struct clear_refs_private {
> enum clear_refs_types type;
> };
>
> -static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> +static bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> {
> struct folio *folio;
>
> @@ -1588,8 +1588,8 @@ static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr,
> return folio_maybe_dma_pinned(folio);
> }
>
> -static inline void clear_soft_dirty(struct vm_area_struct *vma,
> - unsigned long addr, pte_t *pte)
> +static void clear_soft_dirty(struct vm_area_struct *vma, unsigned long addr,
> + pte_t *pte)
> {
> if (!pgtable_supports_soft_dirty())
> return;
> @@ -1620,8 +1620,8 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
> }
>
> #if defined(CONFIG_TRANSPARENT_HUGEPAGE)
> -static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> - unsigned long addr, pmd_t *pmdp)
> +static void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> + unsigned long addr, pmd_t *pmdp)
> {
> pmd_t old, pmd = *pmdp;
>
> @@ -1646,8 +1646,8 @@ static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> }
> }
> #else
> -static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> - unsigned long addr, pmd_t *pmdp)
> +static void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> + unsigned long addr, pmd_t *pmdp)
> {
> }
> #endif
> @@ -1846,7 +1846,7 @@ struct pagemapread {
>
> #define PM_END_OF_BUFFER 1
>
> -static inline pagemap_entry_t make_pme(u64 frame, u64 flags)
> +static pagemap_entry_t make_pme(u64 frame, u64 flags)
> {
> return (pagemap_entry_t) { .pme = (frame & PM_PFRAME_MASK) | flags };
> }
> @@ -3388,7 +3388,7 @@ static const struct mm_walk_ops show_numa_vma_lock_ops = {
> .walk_lock = PGWALK_VMA_RDLOCK_VERIFY,
> };
>
> -static inline const struct mm_walk_ops *
> +static const struct mm_walk_ops *
> get_show_numa_ops(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> --
> 2.55.0.979.g7e5102b832-goog
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter
2026-09-07 6:39 ` [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter Suren Baghdasaryan
@ 2026-09-08 18:07 ` Liam R. Howlett
2026-09-09 17:16 ` David Hildenbrand (Arm)
1 sibling, 0 replies; 36+ messages in thread
From: Liam R. Howlett @ 2026-09-08 18:07 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On 26/09/06 11:39PM, Suren Baghdasaryan wrote:
> smap_gather_stats() interprets its start parameter to mean vma->vm_start
> when it's set to 0. Eliminate this special interpretation and pass
> vma->vm_start explicitly when needed.
>
> Since smap_gather_stats() operates within a single VMA, we can replace
> walk_page_vma()/walk_page_range() calls with walk_page_range_vma()
> which is simpler and also can be called while holding per-VMA lock.
>
> No functional change intended.
>
> Suggested by: Lorenzo Stoakes <ljs@kernel.org>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Reviewed-by: Liam R. Howlett (Oracle) <liam@infradead.org>
> ---
> fs/proc/task_mmu.c | 40 ++++++++++++++++++++++------------------
> 1 file changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 9908ba32f180..3351decd1172 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1246,20 +1246,27 @@ get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
> return &smaps_shmem_walk_vma_lock_ops;
> }
>
> -/*
> - * Gather mem stats from @vma with the indicated beginning
> - * address @start, and keep them in @mss.
> +/**
> + * smap_gather_stats() - Gather mem stats from @vma.
> + * @priv: proc maps private state.
> + * @vma: The VMA to gather stats for.
> + * @mss: The accumulated stats.
> + * @start: The address from which to start.
> *
> - * Use vm_start of @vma as the beginning address if @start is 0.
> + * This gathers stats for the whole of the VMA unless the lock was dropped
> + * and VMA grew or got merged and we found it again, in which case we only
> + * gather stats for the remainder of the VMA range.
> */
> static void smap_gather_stats(struct proc_maps_private *priv,
> struct vm_area_struct *vma,
> - struct mem_size_stats *mss, unsigned long start)
> + struct mem_size_stats *mss,
> + unsigned long start)
> {
> const struct mm_walk_ops *ops = get_smaps_walk_ops(priv);
> + const bool is_partial = start > vma->vm_start;
>
> /* Invalid start */
> - if (start >= vma->vm_end)
> + if (start < vma->vm_start || start >= vma->vm_end)
> return;
>
> if (vma == get_gate_vma(priv->lock_ctx.mm))
> @@ -1279,20 +1286,17 @@ static void smap_gather_stats(struct proc_maps_private *priv,
> * Unless we know that the shmem object (or the part mapped by
> * our VMA) has no swapped out pages at all.
> */
> - unsigned long shmem_swapped = shmem_swap_usage(vma);
> + const unsigned long shmem_swapped = shmem_swap_usage(vma);
> + const bool shared_or_ro = vma_test(vma, VMA_SHARED_BIT) ||
> + !vma_test(vma, VMA_WRITE_BIT);
>
> - if (!start && (!shmem_swapped || (vma->vm_flags & VM_SHARED) ||
> - !(vma->vm_flags & VM_WRITE))) {
> + if (!is_partial && (!shmem_swapped || shared_or_ro))
> mss->swap += shmem_swapped;
> - } else {
> + else
> ops = get_smaps_shmem_walk_ops(priv);
> - }
> }
>
> - if (!start)
> - walk_page_vma(vma, ops, mss);
> - else
> - walk_page_range(vma->vm_mm, start, vma->vm_end, ops, mss);
> + walk_page_range_vma(vma, start, vma->vm_end, ops, mss);
>
> reacquire_rcu(priv);
> }
> @@ -1347,7 +1351,7 @@ static int show_smap(struct seq_file *m, void *v)
> struct vm_area_struct *vma = v;
> struct mem_size_stats mss = {};
>
> - smap_gather_stats(priv, vma, &mss, 0);
> + smap_gather_stats(priv, vma, &mss, vma->vm_start);
>
> show_map_vma(m, vma);
>
> @@ -1400,7 +1404,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>
> vma_start = vma->vm_start;
> do {
> - smap_gather_stats(priv, vma, &mss, 0);
> + smap_gather_stats(priv, vma, &mss, vma->vm_start);
> last_vma_end = vma->vm_end;
>
> /*
> @@ -1459,7 +1463,7 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>
> /* Case 1 and 2 above */
> if (vma->vm_start >= last_vma_end) {
> - smap_gather_stats(priv, vma, &mss, 0);
> + smap_gather_stats(priv, vma, &mss, vma->vm_start);
> last_vma_end = vma->vm_end;
> continue;
> }
> --
> 2.55.0.979.g7e5102b832-goog
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock
2026-09-07 6:39 ` [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
@ 2026-09-08 18:17 ` Liam R. Howlett
2026-09-09 14:25 ` Usama Arif
2026-09-09 17:23 ` David Hildenbrand (Arm)
2 siblings, 0 replies; 36+ messages in thread
From: Liam R. Howlett @ 2026-09-08 18:17 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On 26/09/06 11:39PM, Suren Baghdasaryan wrote:
> proc/pid/smaps_rollup can be read using the combination of RCU and
> VMA read locks, similar to proc/pid/{maps|smaps|numa_maps}. RCU is
> required to safely traverse the VMA tree and VMA lock stabilizes the
> VMA being processed and the pagetable walk.
> Note that we have to keep the logic to drop mmap_lock on contention
> because even when using per-VMA locks we might have to fall back to
> holding the mmap_lock.
>
> Running Paul's contention benchmark [1] shows considerable improvement
> both in median and in the worst case latencies:
>
> Execution command: run-proc-vs-map.sh --nsamples 20 --rawdata -- \
> --busyduration 2 --procfile smaps_rollup
>
> Baseline:
> Median Minimum Maximum
> 0.174 0.161 2.553
> 0.174 0.164 2.663
> 0.174 0.165 2.664
> 0.174 0.166 2.679
> 0.174 0.167 2.691
> 0.174 0.168 2.704
> 0.174 0.169 2.729
> 0.174 0.172 2.741
> 0.174 0.174 2.745
> 0.174 0.174 2.755
> 0.174 0.175 2.790
> 0.174 0.177 2.809
> 0.174 0.179 3.096
> 0.174 0.183 3.144
> 0.174 0.184 3.158
> 0.174 0.185 3.175
> 0.174 0.185 4.568
> 0.174 0.198 4.821
> 0.174 0.214 5.143
> 0.174 0.251 5.220
>
> Patched:
> Median Minimum Maximum
> 0.007 0.007 1.952
> 0.007 0.007 1.955
> 0.007 0.007 1.955
> 0.007 0.007 1.955
> 0.007 0.007 1.957
> 0.007 0.007 1.969
> 0.007 0.007 2.065
> 0.007 0.007 2.075
> 0.007 0.007 2.146
> 0.007 0.007 2.195
> 0.007 0.007 2.223
> 0.007 0.007 2.259
> 0.007 0.007 2.488
> 0.007 0.007 2.562
> 0.007 0.007 2.599
> 0.007 0.007 2.697
> 0.007 0.007 3.030
> 0.007 0.007 3.075
> 0.007 0.007 3.145
> 0.007 0.007 3.225
>
> Remove now unused lock_ctx_mm() and move unlock_ctx_vma() next to
> unlock_ctx_mm() as they are logically related.
>
> Remove a long comment about 4 cases that we handle when dropping the
> mmap lock in the middle of VMA walk due to contention. The first 3
> cases explained there are handled naturally and only case 4 needs to
> be handled in a special way, which is done in smap_gather_stats() by
> gathering stats from the portion of the VMA that has not yet been
> processed.
> For posterity, moving this comment here:
>
> After dropping the lock, there are four 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:
>
> vma_next(vmi) will return VMA3.
> In this case, just continue from VMA3.
>
> 2) VMA2 still exists:
>
> vma_next(vmi) will return VMA3.
> In this case, just continue from VMA3.
>
> 3) No more VMAs can be found:
>
> vma_next(vmi) will return NULL.
> No more things to do, just break.
>
> 4) (last_vma_end - 1) is the middle of a vma (VMA'):
>
> vma_next(vmi) will return VMA' whose range
> contains last_vma_end.
> Iterate VMA' from last_vma_end.
>
> [1] https://github.com/paulmckrcu/proc-mmap_sem-test
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Reviewed-by: Liam R. Howlett (Oracle) <liam@infradead.org>
> ---
> fs/proc/task_mmu.c | 153 ++++++++++++++++++---------------------------
> 1 file changed, 60 insertions(+), 93 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 3351decd1172..641a155b0c61 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -130,28 +130,12 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
> }
> #endif
>
> -static int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - int ret = mmap_read_lock_killable(lock_ctx->mm);
> -
> - if (!ret)
> - lock_ctx->mmap_locked = true;
> -
> - return ret;
> -}
> -
> static void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> {
> mmap_read_unlock(lock_ctx->mm);
> lock_ctx->mmap_locked = false;
> }
>
> -static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - lock_ctx->locked_vma = NULL;
> - lock_ctx->mmap_locked = false;
> -}
> -
> static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
> {
> if (lock_ctx->locked_vma) {
> @@ -160,6 +144,12 @@ static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
> }
> }
>
> +static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
> +{
> + lock_ctx->locked_vma = NULL;
> + lock_ctx->mmap_locked = false;
> +}
> +
> static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> loff_t last_pos)
> {
> @@ -1376,12 +1366,14 @@ static int show_smap(struct seq_file *m, void *v)
> static int show_smaps_rollup(struct seq_file *m, void *v)
> {
> struct proc_maps_private *priv = m->private;
> + struct proc_maps_locking_ctx *lock_ctx = &priv->lock_ctx;
> + struct mm_struct *mm = lock_ctx->mm;
> struct mem_size_stats mss = {};
> - struct mm_struct *mm = priv->lock_ctx.mm;
> + unsigned long last_vma_end = 0;
> + unsigned long vma_start = 0;
> struct vm_area_struct *vma;
> - unsigned long vma_start = 0, last_vma_end = 0;
> + loff_t pos = 0;
> int ret = 0;
> - VMA_ITERATOR(vmi, mm, 0);
>
> priv->task = get_proc_task(priv->inode);
> if (!priv->task)
> @@ -1392,89 +1384,60 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
> goto out_put_task;
> }
>
> - ret = lock_ctx_mm(&priv->lock_ctx);
> - if (ret)
> - goto out_put_mm;
> -
> hold_task_mempolicy(priv);
> - vma = vma_next(&vmi);
> + rcu_read_lock();
> + reset_lock_ctx(lock_ctx);
>
> + vma_iter_init(&priv->iter, mm, 0);
> + vma = proc_get_vma(m, &pos);
> if (unlikely(!vma))
> goto empty_set;
>
> - vma_start = vma->vm_start;
> - do {
> - smap_gather_stats(priv, vma, &mss, vma->vm_start);
> - last_vma_end = vma->vm_end;
> + if (!IS_ERR(vma) && vma != get_gate_vma(lock_ctx->mm))
> + vma_start = vma->vm_start;
> +
> + while (vma) {
> + if (IS_ERR(vma)) {
> + ret = PTR_ERR(vma);
> + goto out_unlock;
> + }
> +
> + if (vma == get_gate_vma(lock_ctx->mm))
> + break;
>
> /*
> - * Release mmap_lock temporarily if someone wants to
> - * access it for write request.
> + * If after retaking the lock, already reported VMA grew or
> + * merged with the next one, smap_gather_stats() will gather
> + * stats for the remaining portion by starting at last_vma_end.
> */
> - if (mmap_lock_is_contended(mm)) {
> - vma_iter_invalidate(&vmi);
> - unlock_ctx_mm(&priv->lock_ctx);
> - ret = lock_ctx_mm(&priv->lock_ctx);
> - if (ret) {
> - release_task_mempolicy(priv);
> - goto out_put_mm;
> - }
> + smap_gather_stats(priv, vma, &mss, last_vma_end);
> + last_vma_end = vma->vm_end;
>
> + /*
> + * If the VMA lock is not taken, we hold the often contended
> + * mmap lock. This can happen if we had to fall back to the
> + * mmap lock.
> + *
> + * To relieve pressure, check if it is indeed contended, then
> + * temporarily release it.
> + */
> + if (lock_ctx->mmap_locked &&
> + mmap_lock_is_contended(lock_ctx->mm)) {
> + unlock_ctx_mm(lock_ctx);
> /*
> - * After dropping the lock, there are four 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:
> - *
> - * vma_next(vmi) will return VMA3.
> - * In this case, just continue from VMA3.
> - *
> - * 2) VMA2 still exists:
> - *
> - * vma_next(vmi) will return VMA3.
> - * In this case, just continue from VMA3.
> - *
> - * 3) No more VMAs can be found:
> - *
> - * vma_next(vmi) will return NULL.
> - * No more things to do, just break.
> - *
> - * 4) (last_vma_end - 1) is the middle of a vma (VMA'):
> - *
> - * vma_next(vmi) will return VMA' whose range
> - * contains last_vma_end.
> - * Iterate VMA' from last_vma_end.
> + * Even though we previously fell back to mmap lock,
> + * we try taking VMA lock for the next VMA, since it
> + * might not be under modification. In the worst case
> + * we will fall back to mmap lock again.
> */
> - vma = vma_next(&vmi);
> - /* Case 3 above */
> - if (!vma)
> - break;
> -
> - /* Case 1 and 2 above */
> - if (vma->vm_start >= last_vma_end) {
> - smap_gather_stats(priv, vma, &mss, vma->vm_start);
> - last_vma_end = vma->vm_end;
> - continue;
> - }
> -
> - /* Case 4 above */
> - if (vma->vm_end > last_vma_end) {
> - smap_gather_stats(priv, vma, &mss, last_vma_end);
> - last_vma_end = vma->vm_end;
> - }
Thanks for this :)
> + rcu_read_lock();
> + reset_lock_ctx(lock_ctx);
> + /* Resume from the last position. */
> + pos = last_vma_end;
> + vma_iter_init(&priv->iter, mm, pos);
> }
> - } for_each_vma(vmi, vma);
> + vma = proc_get_vma(m, &pos);
> + }
>
> empty_set:
> show_vma_header_prefix(m, vma_start, last_vma_end, 0, 0, 0, 0);
> @@ -1483,10 +1446,14 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>
> __show_smap(m, &mss, true);
>
> +out_unlock:
> + if (lock_ctx->mmap_locked) {
> + unlock_ctx_mm(lock_ctx);
> + } else {
> + unlock_ctx_vma(lock_ctx);
> + rcu_read_unlock();
> + }
> release_task_mempolicy(priv);
> - unlock_ctx_mm(&priv->lock_ctx);
> -
> -out_put_mm:
> mmput(mm);
> out_put_task:
> put_task_struct(priv->task);
> --
> 2.55.0.979.g7e5102b832-goog
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 5/5] selftests/proc: add /proc/pid/smaps_rollup tearing tests
2026-09-07 6:39 ` [PATCH v2 5/5] selftests/proc: add /proc/pid/smaps_rollup tearing tests Suren Baghdasaryan
@ 2026-09-08 18:18 ` Liam R. Howlett
0 siblings, 0 replies; 36+ messages in thread
From: Liam R. Howlett @ 2026-09-08 18:18 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On 26/09/06 11:39PM, Suren Baghdasaryan wrote:
> During tearing tests, smaps_rollup Pss* metrics should stay constant.
> Extend /proc/pid/smaps tearing tests to also check for smaps_rollup
> consistency.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Acked-by: Liam R. Howlett (Oracle) <liam@infradead.org>
> ---
> tools/testing/selftests/proc/proc-maps-race.c | 187 +++++++++++++++++-
> 1 file changed, 182 insertions(+), 5 deletions(-)
>
> diff --git a/tools/testing/selftests/proc/proc-maps-race.c b/tools/testing/selftests/proc/proc-maps-race.c
> index 415eccb70468..8d00d7db1c65 100644
> --- a/tools/testing/selftests/proc/proc-maps-race.c
> +++ b/tools/testing/selftests/proc/proc-maps-race.c
> @@ -80,6 +80,61 @@ enum maps_file {
>
> struct vma_modifier_info;
>
> +enum smaps_rollup_stat {
> + Rss,
> + Pss,
> + Pss_Dirty,
> + Pss_Anon,
> + Pss_File,
> + Pss_Shmem,
> + Shared_Clean,
> + Shared_Dirty,
> + Private_Clean,
> + Private_Dirty,
> + Referenced,
> + Anonymous,
> + KSM,
> + LazyFree,
> + AnonHugePages,
> + ShmemPmdMapped,
> + FilePmdMapped,
> + Shared_Hugetlb,
> + Private_Hugetlb,
> + Swap,
> + SwapPss,
> + Locked,
> + RollupFieldCount
> +};
> +
> +static const char *smaps_rollup_stat_names[RollupFieldCount] = {
> + "Rss",
> + "Pss",
> + "Pss_Dirty",
> + "Pss_Anon",
> + "Pss_File",
> + "Pss_Shmem",
> + "Shared_Clean",
> + "Shared_Dirty",
> + "Private_Clean",
> + "Private_Dirty",
> + "Referenced",
> + "Anonymous",
> + "KSM",
> + "LazyFree",
> + "AnonHugePages",
> + "ShmemPmdMapped",
> + "FilePmdMapped",
> + "Shared_Hugetlb",
> + "Private_Hugetlb",
> + "Swap",
> + "SwapPss",
> + "Locked",
> +};
> +
> +struct smaps_rollup_stats {
> + unsigned long values[RollupFieldCount];
> +};
> +
> FIXTURE(proc_maps_race)
> {
> struct vma_modifier_info *mod_info;
> @@ -91,6 +146,7 @@ FIXTURE(proc_maps_race)
> enum maps_file maps_file;
> int shared_mem_size;
> int skip_pages;
> + int rollup_fd;
> int page_size;
> int vma_count;
> bool verbose;
> @@ -132,12 +188,12 @@ struct vma_modifier_info {
> void *child_mapped_addr[];
> };
>
> -static bool read_page(FIXTURE_DATA(proc_maps_race) *self,
> +static bool read_page(FIXTURE_DATA(proc_maps_race) *self, int fd,
> struct page_content *page)
> {
> ssize_t bytes_read;
>
> - bytes_read = read(self->maps_fd, page->data, self->page_size);
> + bytes_read = read(fd, page->data, self->page_size);
> if (bytes_read <= 0)
> return false;
>
> @@ -175,7 +231,7 @@ static int locate_containing_page(FIXTURE_DATA(proc_maps_race) *self,
> char *curr_pos;
> char *end_pos;
>
> - if (!read_page(self, &self->page1))
> + if (!read_page(self, self->maps_fd, &self->page1))
> return -1;
>
> curr_pos = self->page1.data;
> @@ -205,10 +261,11 @@ static bool read_two_pages(FIXTURE_DATA(proc_maps_race) *self)
> return false;
>
> for (int i = 0; i < self->skip_pages; i++)
> - if (!read_page(self, &self->page1))
> + if (!read_page(self, self->maps_fd, &self->page1))
> return false;
>
> - return read_page(self, &self->page1) && read_page(self, &self->page2);
> + return read_page(self, self->maps_fd, &self->page1) &&
> + read_page(self, self->maps_fd, &self->page2);
> }
>
> static void copy_line(const char *line_start, const char *line_end,
> @@ -317,6 +374,61 @@ static bool read_boundary_lines(FIXTURE_DATA(proc_maps_race) *self,
> &first_line->end_addr) == 2;
> }
>
> +static bool parse_smaps_rollup(FIXTURE_DATA(proc_maps_race) *self,
> + struct smaps_rollup_stats *stats)
> +{
> + unsigned int dev_maj, dev_min, inode;
> + unsigned long start, end, offs;
> + unsigned long value;
> + char name[32], perm[5];
> + char *curr_pos;
> + char *end_pos;
> + char *line_end;
> +
> + if (lseek(self->rollup_fd, 0, SEEK_SET) < 0)
> + return false;
> +
> + if (!read_page(self, self->rollup_fd, &self->page1))
> + return false;
> +
> + curr_pos = self->page1.data;
> + end_pos = self->page1.data + self->page1.size;
> +
> + line_end = strchr(curr_pos, '\n');
> + if (!line_end)
> + return false;
> +
> + if (sscanf(curr_pos, "%lx-%lx %4s %lx %u:%u %u %31s",
> + &start, &end, perm, &offs, &dev_maj, &dev_min, &inode, name) != 8)
> + return false;
> +
> + if (strcmp(name, "[rollup]"))
> + return false;
> +
> + for (int stat = 0; stat < ARRAY_SIZE(smaps_rollup_stat_names); stat++) {
> + int len;
> +
> + curr_pos = line_end + 1;
> + if (curr_pos >= end_pos)
> + return false;
> +
> + line_end = strchr(curr_pos, '\n');
> + if (!line_end)
> + return false;
> +
> + if (sscanf(curr_pos, "%31s %lu kB", name, &value) != 2)
> + return false;
> +
> + len = strlen(name);
> + if (name[len - 1] != ':' || strncmp(name, smaps_rollup_stat_names[stat], len - 1))
> + return false;
> +
> + stats->values[stat] = value;
> + }
> +
> + return true;
> +}
> +
> /* Thread synchronization routines */
> static void wait_for_state(struct vma_modifier_info *mod_info, enum test_state state)
> {
> @@ -397,6 +509,41 @@ static bool print_boundaries_on(bool condition, const char *title,
> return condition;
> }
>
> +static void print_smaps_rollup_stats(const char *title, FIXTURE_DATA(proc_maps_race) *self,
> + struct smaps_rollup_stats *stats)
> +{
> + printf("%s", title);
> + for (int stat = 0; stat < ARRAY_SIZE(smaps_rollup_stat_names); stat++)
> + printf("%64s %lu kB\n", smaps_rollup_stat_names[stat], stats->values[stat]);
> +}
> +
> +static bool cmp_smaps_rollup_stat(struct smaps_rollup_stats *s1,
> + struct smaps_rollup_stats *s2,
> + enum smaps_rollup_stat stat)
> +{
> + return s1->values[stat] == s2->values[stat];
> +}
> +
> +static bool compare_smaps_rollup(FIXTURE_DATA(proc_maps_race) *self,
> + struct smaps_rollup_stats *expected,
> + struct smaps_rollup_stats *actual)
> +{
> + /*
> + * Clean/dirty metrics might change but Pss-related ones
> + * should stay constant.
> + */
> + if (cmp_smaps_rollup_stat(expected, actual, Pss) &&
> + cmp_smaps_rollup_stat(expected, actual, Pss_Anon) &&
> + cmp_smaps_rollup_stat(expected, actual, Pss_File) &&
> + cmp_smaps_rollup_stat(expected, actual, Pss_Shmem))
> + return true;
> +
> + print_smaps_rollup_stats("Expected stats:", self, expected);
> + print_smaps_rollup_stats("Actual stats:", self, actual);
> +
> + return false;
> +}
> +
> static void report_test_start(const char *name, bool verbose)
> {
> if (verbose)
> @@ -572,6 +719,7 @@ FIXTURE_SETUP(proc_maps_race)
> unsigned long first_map_addr;
> unsigned long last_map_addr;
> unsigned long duration_sec;
> + char rollup_fname[32];
> char fname[32];
>
> self->page_size = (unsigned long)sysconf(_SC_PAGESIZE);
> @@ -649,6 +797,9 @@ FIXTURE_SETUP(proc_maps_race)
> break;
> case SMAPS:
> sprintf(fname, "/proc/%d/smaps", self->pid);
> + sprintf(rollup_fname, "/proc/%d/smaps_rollup", self->pid);
> + self->rollup_fd = open(rollup_fname, O_RDONLY);
> + ASSERT_NE(self->rollup_fd, -1);
> break;
> default:
> ksft_exit_fail();
> @@ -711,6 +862,8 @@ FIXTURE_TEARDOWN(proc_maps_race)
> for (int i = 0; i < self->vma_count; i++)
> munmap(self->mod_info->child_mapped_addr[i], self->page_size);
> close(self->maps_fd);
> + if (self->maps_file == SMAPS)
> + close(self->rollup_fd);
> waitpid(self->pid, &status, 0);
> munmap(self->mod_info, self->shared_mem_size);
> }
> @@ -723,6 +876,7 @@ TEST_F(proc_maps_race, test_maps_tearing_from_split)
> struct line_content split_first_line;
> struct line_content restored_last_line;
> struct line_content restored_first_line;
> + struct smaps_rollup_stats orig_stats;
>
> wait_for_state(mod_info, SETUP_READY);
>
> @@ -736,6 +890,8 @@ TEST_F(proc_maps_race, test_maps_tearing_from_split)
> report_test_start("Tearing from split", self->verbose);
> ASSERT_TRUE(capture_mod_pattern(self, &split_last_line, &split_first_line,
> &restored_last_line, &restored_first_line));
> + if (self->maps_file == SMAPS)
> + ASSERT_TRUE(parse_smaps_rollup(self, &orig_stats));
>
> /* Now start concurrent modifications for self->duration_sec */
> signal_state(mod_info, TEST_READY);
> @@ -799,6 +955,11 @@ TEST_F(proc_maps_race, test_maps_tearing_from_split)
> vma_end == self->last_line.end_addr) ||
> (vma_start == split_first_line.start_addr &&
> vma_end == split_first_line.end_addr));
> + } else {
> + struct smaps_rollup_stats stats;
> +
> + ASSERT_TRUE(parse_smaps_rollup(self, &stats));
> + ASSERT_TRUE(compare_smaps_rollup(self, &orig_stats, &stats));
> }
> clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
> end_test_iteration(&end_ts, self->verbose);
> @@ -817,6 +978,7 @@ TEST_F(proc_maps_race, test_maps_tearing_from_resize)
> struct line_content shrunk_first_line;
> struct line_content restored_last_line;
> struct line_content restored_first_line;
> + struct smaps_rollup_stats orig_stats;
>
> wait_for_state(mod_info, SETUP_READY);
>
> @@ -830,6 +992,8 @@ TEST_F(proc_maps_race, test_maps_tearing_from_resize)
> report_test_start("Tearing from resize", self->verbose);
> ASSERT_TRUE(capture_mod_pattern(self, &shrunk_last_line, &shrunk_first_line,
> &restored_last_line, &restored_first_line));
> + if (self->maps_file == SMAPS)
> + ASSERT_TRUE(parse_smaps_rollup(self, &orig_stats));
>
> /* Now start concurrent modifications for self->duration_sec */
> signal_state(mod_info, TEST_READY);
> @@ -880,6 +1044,11 @@ TEST_F(proc_maps_race, test_maps_tearing_from_resize)
> ASSERT_TRUE(vma_start == self->last_line.start_addr &&
> (vma_end - vma_start == self->page_size * 3 ||
> vma_end - vma_start == self->page_size));
> + } else {
> + struct smaps_rollup_stats stats;
> +
> + ASSERT_TRUE(parse_smaps_rollup(self, &stats));
> + ASSERT_TRUE(compare_smaps_rollup(self, &orig_stats, &stats));
> }
> clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
> end_test_iteration(&end_ts, self->verbose);
> @@ -898,6 +1067,7 @@ TEST_F(proc_maps_race, test_maps_tearing_from_remap)
> struct line_content remapped_first_line;
> struct line_content restored_last_line;
> struct line_content restored_first_line;
> + struct smaps_rollup_stats orig_stats;
>
> wait_for_state(mod_info, SETUP_READY);
>
> @@ -911,6 +1081,8 @@ TEST_F(proc_maps_race, test_maps_tearing_from_remap)
> report_test_start("Tearing from remap", self->verbose);
> ASSERT_TRUE(capture_mod_pattern(self, &remapped_last_line, &remapped_first_line,
> &restored_last_line, &restored_first_line));
> + if (self->maps_file == SMAPS)
> + ASSERT_TRUE(parse_smaps_rollup(self, &orig_stats));
>
> /* Now start concurrent modifications for self->duration_sec */
> signal_state(mod_info, TEST_READY);
> @@ -963,6 +1135,11 @@ TEST_F(proc_maps_race, test_maps_tearing_from_remap)
> vma_end - vma_start == self->page_size * 3) ||
> (vma_start == self->last_line.start_addr + self->page_size &&
> vma_end - vma_start == self->page_size));
> + } else {
> + struct smaps_rollup_stats stats;
> +
> + ASSERT_TRUE(parse_smaps_rollup(self, &stats));
> + ASSERT_TRUE(compare_smaps_rollup(self, &orig_stats, &stats));
> }
> clock_gettime(CLOCK_MONOTONIC_COARSE, &end_ts);
> end_test_iteration(&end_ts, self->verbose);
> --
> 2.55.0.979.g7e5102b832-goog
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock
2026-09-07 6:39 ` [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
2026-09-08 18:17 ` Liam R. Howlett
@ 2026-09-09 14:25 ` Usama Arif
2026-09-09 16:13 ` Suren Baghdasaryan
2026-09-09 17:23 ` David Hildenbrand (Arm)
2 siblings, 1 reply; 36+ messages in thread
From: Usama Arif @ 2026-09-09 14:25 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: Usama Arif, akpm, liam, ljs, vbabka, david, willy, jannh, paulmck,
pfalcato, linux-mm, linux-kernel, linux-fsdevel
On Sun, 6 Sep 2026 23:39:17 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
> proc/pid/smaps_rollup can be read using the combination of RCU and
> VMA read locks, similar to proc/pid/{maps|smaps|numa_maps}. RCU is
> required to safely traverse the VMA tree and VMA lock stabilizes the
> VMA being processed and the pagetable walk.
> Note that we have to keep the logic to drop mmap_lock on contention
> because even when using per-VMA locks we might have to fall back to
> holding the mmap_lock.
>
> Running Paul's contention benchmark [1] shows considerable improvement
> both in median and in the worst case latencies:
>
> Execution command: run-proc-vs-map.sh --nsamples 20 --rawdata -- \
> --busyduration 2 --procfile smaps_rollup
>
> Baseline:
> Median Minimum Maximum
> 0.174 0.161 2.553
> 0.174 0.164 2.663
> 0.174 0.165 2.664
> 0.174 0.166 2.679
> 0.174 0.167 2.691
> 0.174 0.168 2.704
> 0.174 0.169 2.729
> 0.174 0.172 2.741
> 0.174 0.174 2.745
> 0.174 0.174 2.755
> 0.174 0.175 2.790
> 0.174 0.177 2.809
> 0.174 0.179 3.096
> 0.174 0.183 3.144
> 0.174 0.184 3.158
> 0.174 0.185 3.175
> 0.174 0.185 4.568
> 0.174 0.198 4.821
> 0.174 0.214 5.143
> 0.174 0.251 5.220
>
> Patched:
> Median Minimum Maximum
> 0.007 0.007 1.952
> 0.007 0.007 1.955
> 0.007 0.007 1.955
> 0.007 0.007 1.955
> 0.007 0.007 1.957
> 0.007 0.007 1.969
> 0.007 0.007 2.065
> 0.007 0.007 2.075
> 0.007 0.007 2.146
> 0.007 0.007 2.195
> 0.007 0.007 2.223
> 0.007 0.007 2.259
> 0.007 0.007 2.488
> 0.007 0.007 2.562
> 0.007 0.007 2.599
> 0.007 0.007 2.697
> 0.007 0.007 3.030
> 0.007 0.007 3.075
> 0.007 0.007 3.145
> 0.007 0.007 3.225
>
> Remove now unused lock_ctx_mm() and move unlock_ctx_vma() next to
> unlock_ctx_mm() as they are logically related.
>
> Remove a long comment about 4 cases that we handle when dropping the
> mmap lock in the middle of VMA walk due to contention. The first 3
> cases explained there are handled naturally and only case 4 needs to
> be handled in a special way, which is done in smap_gather_stats() by
> gathering stats from the portion of the VMA that has not yet been
> processed.
> For posterity, moving this comment here:
>
> After dropping the lock, there are four 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:
>
> vma_next(vmi) will return VMA3.
> In this case, just continue from VMA3.
>
> 2) VMA2 still exists:
>
> vma_next(vmi) will return VMA3.
> In this case, just continue from VMA3.
>
> 3) No more VMAs can be found:
>
> vma_next(vmi) will return NULL.
> No more things to do, just break.
>
> 4) (last_vma_end - 1) is the middle of a vma (VMA'):
>
> vma_next(vmi) will return VMA' whose range
> contains last_vma_end.
> Iterate VMA' from last_vma_end.
>
> [1] https://github.com/paulmckrcu/proc-mmap_sem-test
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
> fs/proc/task_mmu.c | 153 ++++++++++++++++++---------------------------
> 1 file changed, 60 insertions(+), 93 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 3351decd1172..641a155b0c61 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -130,28 +130,12 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
> }
> #endif
>
> -static int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - int ret = mmap_read_lock_killable(lock_ctx->mm);
> -
> - if (!ret)
> - lock_ctx->mmap_locked = true;
> -
> - return ret;
> -}
> -
> static void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> {
> mmap_read_unlock(lock_ctx->mm);
> lock_ctx->mmap_locked = false;
> }
>
> -static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - lock_ctx->locked_vma = NULL;
> - lock_ctx->mmap_locked = false;
> -}
> -
> static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
> {
> if (lock_ctx->locked_vma) {
> @@ -160,6 +144,12 @@ static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
> }
> }
>
> +static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
> +{
> + lock_ctx->locked_vma = NULL;
> + lock_ctx->mmap_locked = false;
> +}
> +
> static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> loff_t last_pos)
> {
> @@ -1376,12 +1366,14 @@ static int show_smap(struct seq_file *m, void *v)
> static int show_smaps_rollup(struct seq_file *m, void *v)
> {
> struct proc_maps_private *priv = m->private;
> + struct proc_maps_locking_ctx *lock_ctx = &priv->lock_ctx;
> + struct mm_struct *mm = lock_ctx->mm;
> struct mem_size_stats mss = {};
> - struct mm_struct *mm = priv->lock_ctx.mm;
> + unsigned long last_vma_end = 0;
> + unsigned long vma_start = 0;
> struct vm_area_struct *vma;
> - unsigned long vma_start = 0, last_vma_end = 0;
> + loff_t pos = 0;
> int ret = 0;
> - VMA_ITERATOR(vmi, mm, 0);
>
> priv->task = get_proc_task(priv->inode);
> if (!priv->task)
> @@ -1392,89 +1384,60 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
> goto out_put_task;
> }
>
> - ret = lock_ctx_mm(&priv->lock_ctx);
> - if (ret)
> - goto out_put_mm;
> -
> hold_task_mempolicy(priv);
> - vma = vma_next(&vmi);
> + rcu_read_lock();
> + reset_lock_ctx(lock_ctx);
>
> + vma_iter_init(&priv->iter, mm, 0);
> + vma = proc_get_vma(m, &pos);
> if (unlikely(!vma))
> goto empty_set;
>
> - vma_start = vma->vm_start;
> - do {
> - smap_gather_stats(priv, vma, &mss, vma->vm_start);
> - last_vma_end = vma->vm_end;
> + if (!IS_ERR(vma) && vma != get_gate_vma(lock_ctx->mm))
> + vma_start = vma->vm_start;
> +
> + while (vma) {
> + if (IS_ERR(vma)) {
> + ret = PTR_ERR(vma);
> + goto out_unlock;
> + }
> +
> + if (vma == get_gate_vma(lock_ctx->mm))
> + break;
>
> /*
> - * Release mmap_lock temporarily if someone wants to
> - * access it for write request.
> + * If after retaking the lock, already reported VMA grew or
> + * merged with the next one, smap_gather_stats() will gather
> + * stats for the remaining portion by starting at last_vma_end.
> */
> - if (mmap_lock_is_contended(mm)) {
> - vma_iter_invalidate(&vmi);
> - unlock_ctx_mm(&priv->lock_ctx);
> - ret = lock_ctx_mm(&priv->lock_ctx);
> - if (ret) {
> - release_task_mempolicy(priv);
> - goto out_put_mm;
> - }
> + smap_gather_stats(priv, vma, &mss, last_vma_end);
Patch 3 made smap_gather_stats() reject starts below the VMA, while this
function initializes last_vma_end to zero.
The first ordinary VMA is therefore skipped. lock_next_vma() can also
return a VMA beginning after the requested position, so the first VMA
after every unmapped gap is skipped as well. This causes
smaps_rollup to underreport RSS, PSS, swap, and the other accumulated
values.
I think you need:
unsigned long start = max(last_vma_end, vma->vm_start);
smap_gather_stats(priv, vma, &mss, start);
> + last_vma_end = vma->vm_end;
>
> + /*
> + * If the VMA lock is not taken, we hold the often contended
> + * mmap lock. This can happen if we had to fall back to the
> + * mmap lock.
> + *
> + * To relieve pressure, check if it is indeed contended, then
> + * temporarily release it.
> + */
> + if (lock_ctx->mmap_locked &&
> + mmap_lock_is_contended(lock_ctx->mm)) {
> + unlock_ctx_mm(lock_ctx);
> /*
> - * After dropping the lock, there are four 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:
> - *
> - * vma_next(vmi) will return VMA3.
> - * In this case, just continue from VMA3.
> - *
> - * 2) VMA2 still exists:
> - *
> - * vma_next(vmi) will return VMA3.
> - * In this case, just continue from VMA3.
> - *
> - * 3) No more VMAs can be found:
> - *
> - * vma_next(vmi) will return NULL.
> - * No more things to do, just break.
> - *
> - * 4) (last_vma_end - 1) is the middle of a vma (VMA'):
> - *
> - * vma_next(vmi) will return VMA' whose range
> - * contains last_vma_end.
> - * Iterate VMA' from last_vma_end.
> + * Even though we previously fell back to mmap lock,
> + * we try taking VMA lock for the next VMA, since it
> + * might not be under modification. In the worst case
> + * we will fall back to mmap lock again.
> */
> - vma = vma_next(&vmi);
> - /* Case 3 above */
> - if (!vma)
> - break;
> -
> - /* Case 1 and 2 above */
> - if (vma->vm_start >= last_vma_end) {
> - smap_gather_stats(priv, vma, &mss, vma->vm_start);
> - last_vma_end = vma->vm_end;
> - continue;
> - }
> -
> - /* Case 4 above */
> - if (vma->vm_end > last_vma_end) {
> - smap_gather_stats(priv, vma, &mss, last_vma_end);
> - last_vma_end = vma->vm_end;
> - }
> + rcu_read_lock();
> + reset_lock_ctx(lock_ctx);
> + /* Resume from the last position. */
> + pos = last_vma_end;
> + vma_iter_init(&priv->iter, mm, pos);
> }
> - } for_each_vma(vmi, vma);
> + vma = proc_get_vma(m, &pos);
> + }
>
> empty_set:
> show_vma_header_prefix(m, vma_start, last_vma_end, 0, 0, 0, 0);
> @@ -1483,10 +1446,14 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
>
> __show_smap(m, &mss, true);
>
> +out_unlock:
> + if (lock_ctx->mmap_locked) {
> + unlock_ctx_mm(lock_ctx);
> + } else {
> + unlock_ctx_vma(lock_ctx);
> + rcu_read_unlock();
> + }
> release_task_mempolicy(priv);
> - unlock_ctx_mm(&priv->lock_ctx);
> -
> -out_put_mm:
> mmput(mm);
> out_put_task:
> put_task_struct(priv->task);
> --
> 2.55.0.979.g7e5102b832-goog
>
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock
2026-09-09 14:25 ` Usama Arif
@ 2026-09-09 16:13 ` Suren Baghdasaryan
0 siblings, 0 replies; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-09 16:13 UTC (permalink / raw)
To: Usama Arif
Cc: akpm, liam, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On Wed, Sep 9, 2026 at 7:25 AM Usama Arif <usama.arif@linux.dev> wrote:
>
> On Sun, 6 Sep 2026 23:39:17 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
>
> > proc/pid/smaps_rollup can be read using the combination of RCU and
> > VMA read locks, similar to proc/pid/{maps|smaps|numa_maps}. RCU is
> > required to safely traverse the VMA tree and VMA lock stabilizes the
> > VMA being processed and the pagetable walk.
> > Note that we have to keep the logic to drop mmap_lock on contention
> > because even when using per-VMA locks we might have to fall back to
> > holding the mmap_lock.
> >
> > Running Paul's contention benchmark [1] shows considerable improvement
> > both in median and in the worst case latencies:
> >
> > Execution command: run-proc-vs-map.sh --nsamples 20 --rawdata -- \
> > --busyduration 2 --procfile smaps_rollup
> >
> > Baseline:
> > Median Minimum Maximum
> > 0.174 0.161 2.553
> > 0.174 0.164 2.663
> > 0.174 0.165 2.664
> > 0.174 0.166 2.679
> > 0.174 0.167 2.691
> > 0.174 0.168 2.704
> > 0.174 0.169 2.729
> > 0.174 0.172 2.741
> > 0.174 0.174 2.745
> > 0.174 0.174 2.755
> > 0.174 0.175 2.790
> > 0.174 0.177 2.809
> > 0.174 0.179 3.096
> > 0.174 0.183 3.144
> > 0.174 0.184 3.158
> > 0.174 0.185 3.175
> > 0.174 0.185 4.568
> > 0.174 0.198 4.821
> > 0.174 0.214 5.143
> > 0.174 0.251 5.220
> >
> > Patched:
> > Median Minimum Maximum
> > 0.007 0.007 1.952
> > 0.007 0.007 1.955
> > 0.007 0.007 1.955
> > 0.007 0.007 1.955
> > 0.007 0.007 1.957
> > 0.007 0.007 1.969
> > 0.007 0.007 2.065
> > 0.007 0.007 2.075
> > 0.007 0.007 2.146
> > 0.007 0.007 2.195
> > 0.007 0.007 2.223
> > 0.007 0.007 2.259
> > 0.007 0.007 2.488
> > 0.007 0.007 2.562
> > 0.007 0.007 2.599
> > 0.007 0.007 2.697
> > 0.007 0.007 3.030
> > 0.007 0.007 3.075
> > 0.007 0.007 3.145
> > 0.007 0.007 3.225
> >
> > Remove now unused lock_ctx_mm() and move unlock_ctx_vma() next to
> > unlock_ctx_mm() as they are logically related.
> >
> > Remove a long comment about 4 cases that we handle when dropping the
> > mmap lock in the middle of VMA walk due to contention. The first 3
> > cases explained there are handled naturally and only case 4 needs to
> > be handled in a special way, which is done in smap_gather_stats() by
> > gathering stats from the portion of the VMA that has not yet been
> > processed.
> > For posterity, moving this comment here:
> >
> > After dropping the lock, there are four 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:
> >
> > vma_next(vmi) will return VMA3.
> > In this case, just continue from VMA3.
> >
> > 2) VMA2 still exists:
> >
> > vma_next(vmi) will return VMA3.
> > In this case, just continue from VMA3.
> >
> > 3) No more VMAs can be found:
> >
> > vma_next(vmi) will return NULL.
> > No more things to do, just break.
> >
> > 4) (last_vma_end - 1) is the middle of a vma (VMA'):
> >
> > vma_next(vmi) will return VMA' whose range
> > contains last_vma_end.
> > Iterate VMA' from last_vma_end.
> >
> > [1] https://github.com/paulmckrcu/proc-mmap_sem-test
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> > fs/proc/task_mmu.c | 153 ++++++++++++++++++---------------------------
> > 1 file changed, 60 insertions(+), 93 deletions(-)
> >
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index 3351decd1172..641a155b0c61 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -130,28 +130,12 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
> > }
> > #endif
> >
> > -static int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> > -{
> > - int ret = mmap_read_lock_killable(lock_ctx->mm);
> > -
> > - if (!ret)
> > - lock_ctx->mmap_locked = true;
> > -
> > - return ret;
> > -}
> > -
> > static void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> > {
> > mmap_read_unlock(lock_ctx->mm);
> > lock_ctx->mmap_locked = false;
> > }
> >
> > -static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
> > -{
> > - lock_ctx->locked_vma = NULL;
> > - lock_ctx->mmap_locked = false;
> > -}
> > -
> > static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
> > {
> > if (lock_ctx->locked_vma) {
> > @@ -160,6 +144,12 @@ static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
> > }
> > }
> >
> > +static void reset_lock_ctx(struct proc_maps_locking_ctx *lock_ctx)
> > +{
> > + lock_ctx->locked_vma = NULL;
> > + lock_ctx->mmap_locked = false;
> > +}
> > +
> > static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> > loff_t last_pos)
> > {
> > @@ -1376,12 +1366,14 @@ static int show_smap(struct seq_file *m, void *v)
> > static int show_smaps_rollup(struct seq_file *m, void *v)
> > {
> > struct proc_maps_private *priv = m->private;
> > + struct proc_maps_locking_ctx *lock_ctx = &priv->lock_ctx;
> > + struct mm_struct *mm = lock_ctx->mm;
> > struct mem_size_stats mss = {};
> > - struct mm_struct *mm = priv->lock_ctx.mm;
> > + unsigned long last_vma_end = 0;
> > + unsigned long vma_start = 0;
> > struct vm_area_struct *vma;
> > - unsigned long vma_start = 0, last_vma_end = 0;
> > + loff_t pos = 0;
> > int ret = 0;
> > - VMA_ITERATOR(vmi, mm, 0);
> >
> > priv->task = get_proc_task(priv->inode);
> > if (!priv->task)
> > @@ -1392,89 +1384,60 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
> > goto out_put_task;
> > }
> >
> > - ret = lock_ctx_mm(&priv->lock_ctx);
> > - if (ret)
> > - goto out_put_mm;
> > -
> > hold_task_mempolicy(priv);
> > - vma = vma_next(&vmi);
> > + rcu_read_lock();
> > + reset_lock_ctx(lock_ctx);
> >
> > + vma_iter_init(&priv->iter, mm, 0);
> > + vma = proc_get_vma(m, &pos);
> > if (unlikely(!vma))
> > goto empty_set;
> >
> > - vma_start = vma->vm_start;
> > - do {
> > - smap_gather_stats(priv, vma, &mss, vma->vm_start);
> > - last_vma_end = vma->vm_end;
> > + if (!IS_ERR(vma) && vma != get_gate_vma(lock_ctx->mm))
> > + vma_start = vma->vm_start;
> > +
> > + while (vma) {
> > + if (IS_ERR(vma)) {
> > + ret = PTR_ERR(vma);
> > + goto out_unlock;
> > + }
> > +
> > + if (vma == get_gate_vma(lock_ctx->mm))
> > + break;
> >
> > /*
> > - * Release mmap_lock temporarily if someone wants to
> > - * access it for write request.
> > + * If after retaking the lock, already reported VMA grew or
> > + * merged with the next one, smap_gather_stats() will gather
> > + * stats for the remaining portion by starting at last_vma_end.
> > */
> > - if (mmap_lock_is_contended(mm)) {
> > - vma_iter_invalidate(&vmi);
> > - unlock_ctx_mm(&priv->lock_ctx);
> > - ret = lock_ctx_mm(&priv->lock_ctx);
> > - if (ret) {
> > - release_task_mempolicy(priv);
> > - goto out_put_mm;
> > - }
> > + smap_gather_stats(priv, vma, &mss, last_vma_end);
>
> Patch 3 made smap_gather_stats() reject starts below the VMA, while this
> function initializes last_vma_end to zero.
>
> The first ordinary VMA is therefore skipped. lock_next_vma() can also
> return a VMA beginning after the requested position, so the first VMA
> after every unmapped gap is skipped as well. This causes
> smaps_rollup to underreport RSS, PSS, swap, and the other accumulated
> values.
>
> I think you need:
> unsigned long start = max(last_vma_end, vma->vm_start);
> smap_gather_stats(priv, vma, &mss, start);
Oops, you are right. I didn't realize the first VMA would be silently
skipped. I'll fix it. Thanks!
>
>
>
> > + last_vma_end = vma->vm_end;
> >
> > + /*
> > + * If the VMA lock is not taken, we hold the often contended
> > + * mmap lock. This can happen if we had to fall back to the
> > + * mmap lock.
> > + *
> > + * To relieve pressure, check if it is indeed contended, then
> > + * temporarily release it.
> > + */
> > + if (lock_ctx->mmap_locked &&
> > + mmap_lock_is_contended(lock_ctx->mm)) {
> > + unlock_ctx_mm(lock_ctx);
> > /*
> > - * After dropping the lock, there are four 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:
> > - *
> > - * vma_next(vmi) will return VMA3.
> > - * In this case, just continue from VMA3.
> > - *
> > - * 2) VMA2 still exists:
> > - *
> > - * vma_next(vmi) will return VMA3.
> > - * In this case, just continue from VMA3.
> > - *
> > - * 3) No more VMAs can be found:
> > - *
> > - * vma_next(vmi) will return NULL.
> > - * No more things to do, just break.
> > - *
> > - * 4) (last_vma_end - 1) is the middle of a vma (VMA'):
> > - *
> > - * vma_next(vmi) will return VMA' whose range
> > - * contains last_vma_end.
> > - * Iterate VMA' from last_vma_end.
> > + * Even though we previously fell back to mmap lock,
> > + * we try taking VMA lock for the next VMA, since it
> > + * might not be under modification. In the worst case
> > + * we will fall back to mmap lock again.
> > */
> > - vma = vma_next(&vmi);
> > - /* Case 3 above */
> > - if (!vma)
> > - break;
> > -
> > - /* Case 1 and 2 above */
> > - if (vma->vm_start >= last_vma_end) {
> > - smap_gather_stats(priv, vma, &mss, vma->vm_start);
> > - last_vma_end = vma->vm_end;
> > - continue;
> > - }
> > -
> > - /* Case 4 above */
> > - if (vma->vm_end > last_vma_end) {
> > - smap_gather_stats(priv, vma, &mss, last_vma_end);
> > - last_vma_end = vma->vm_end;
> > - }
> > + rcu_read_lock();
> > + reset_lock_ctx(lock_ctx);
> > + /* Resume from the last position. */
> > + pos = last_vma_end;
> > + vma_iter_init(&priv->iter, mm, pos);
> > }
> > - } for_each_vma(vmi, vma);
> > + vma = proc_get_vma(m, &pos);
> > + }
> >
> > empty_set:
> > show_vma_header_prefix(m, vma_start, last_vma_end, 0, 0, 0, 0);
> > @@ -1483,10 +1446,14 @@ static int show_smaps_rollup(struct seq_file *m, void *v)
> >
> > __show_smap(m, &mss, true);
> >
> > +out_unlock:
> > + if (lock_ctx->mmap_locked) {
> > + unlock_ctx_mm(lock_ctx);
> > + } else {
> > + unlock_ctx_vma(lock_ctx);
> > + rcu_read_unlock();
> > + }
> > release_task_mempolicy(priv);
> > - unlock_ctx_mm(&priv->lock_ctx);
> > -
> > -out_put_mm:
> > mmput(mm);
> > out_put_task:
> > put_task_struct(priv->task);
> > --
> > 2.55.0.979.g7e5102b832-goog
> >
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers
2026-09-07 6:39 ` [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers Suren Baghdasaryan
2026-09-07 16:44 ` Usama Arif
2026-09-08 17:58 ` Liam R. Howlett
@ 2026-09-09 17:06 ` David Hildenbrand (Arm)
2026-09-09 17:13 ` Suren Baghdasaryan
2026-09-10 15:43 ` Lorenzo Stoakes (ARM)
3 siblings, 1 reply; 36+ messages in thread
From: David Hildenbrand (Arm) @ 2026-09-09 17:06 UTC (permalink / raw)
To: Suren Baghdasaryan, akpm
Cc: liam, ljs, vbabka, willy, jannh, paulmck, pfalcato, linux-mm,
linux-kernel, linux-fsdevel
On 9/7/26 08:39, Suren Baghdasaryan wrote:
> When per-vma locks were behind a config option, a number of helper
> functions were needed to simplify the locking code. Now that these
> locks are universally available, we can do a little cleanup.
> Remove lock_vma_range(), unlock_vma_range(), query_vma_setup(),
> query_vma_teardown() helpers.
>
> No functional change intended.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
As Usama says, this could get picked up early independent of the rest (maybe
even as part of the other patch set)
Acked-by: David Hildenbrand (Arm) <david@kernel.org>
--
Cheers,
David
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions
2026-09-07 6:39 ` [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions Suren Baghdasaryan
2026-09-07 16:49 ` Usama Arif
2026-09-08 18:01 ` Liam R. Howlett
@ 2026-09-09 17:07 ` David Hildenbrand (Arm)
2026-09-09 17:15 ` Suren Baghdasaryan
2026-09-10 15:55 ` Lorenzo Stoakes (ARM)
3 siblings, 1 reply; 36+ messages in thread
From: David Hildenbrand (Arm) @ 2026-09-09 17:07 UTC (permalink / raw)
To: Suren Baghdasaryan, akpm
Cc: liam, ljs, vbabka, willy, jannh, paulmck, pfalcato, linux-mm,
linux-kernel, linux-fsdevel
On 9/7/26 08:39, Suren Baghdasaryan wrote:
> It was pointed out in the previous reviews of this code that many
> functions are specified as inline, which is unnecessary as the compile
> can make that decision by itself. Cleanup these definitions.
>
> No functional change intended.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
> fs/proc/task_mmu.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 2f500d639db5..9908ba32f180 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -130,7 +130,7 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
> }
> #endif
>
> -static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> +static int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> {
> int ret = mmap_read_lock_killable(lock_ctx->mm);
>
> @@ -140,7 +140,7 @@ static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> return ret;
> }
>
> -static inline void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> +static void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> {
> mmap_read_unlock(lock_ctx->mm);
> lock_ctx->mmap_locked = false;
> @@ -177,7 +177,7 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> return vma;
> }
>
> -static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> +static bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> loff_t pos)
If you touch these, please convert them to two-tab indent.
--
Cheers,
David
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers
2026-09-09 17:06 ` David Hildenbrand (Arm)
@ 2026-09-09 17:13 ` Suren Baghdasaryan
2026-09-09 17:17 ` David Hildenbrand (Arm)
0 siblings, 1 reply; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-09 17:13 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: akpm, liam, ljs, vbabka, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On Wed, Sep 9, 2026 at 10:06 AM David Hildenbrand (Arm)
<david@kernel.org> wrote:
>
> On 9/7/26 08:39, Suren Baghdasaryan wrote:
> > When per-vma locks were behind a config option, a number of helper
> > functions were needed to simplify the locking code. Now that these
> > locks are universally available, we can do a little cleanup.
> > Remove lock_vma_range(), unlock_vma_range(), query_vma_setup(),
> > query_vma_teardown() helpers.
> >
> > No functional change intended.
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
>
> As Usama says, this could get picked up early independent of the rest (maybe
> even as part of the other patch set)
I can post the 3 cleanup patches separately from the one that switches
to using per-VMA locks if that's preferable. One downside: I'll have
to wait until the cleanups are picked up before posting the last
patch, as it depends on these cleanups. Please let me know if I should
split them anyway; otherwise, will keep v3 as a single patchset.
>
> Acked-by: David Hildenbrand (Arm) <david@kernel.org>
Thanks!
>
> --
> Cheers,
>
> David
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions
2026-09-09 17:07 ` David Hildenbrand (Arm)
@ 2026-09-09 17:15 ` Suren Baghdasaryan
0 siblings, 0 replies; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-09 17:15 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: akpm, liam, ljs, vbabka, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On Wed, Sep 9, 2026 at 10:07 AM David Hildenbrand (Arm)
<david@kernel.org> wrote:
>
> On 9/7/26 08:39, Suren Baghdasaryan wrote:
> > It was pointed out in the previous reviews of this code that many
> > functions are specified as inline, which is unnecessary as the compile
> > can make that decision by itself. Cleanup these definitions.
> >
> > No functional change intended.
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> > fs/proc/task_mmu.c | 32 ++++++++++++++++----------------
> > 1 file changed, 16 insertions(+), 16 deletions(-)
> >
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index 2f500d639db5..9908ba32f180 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -130,7 +130,7 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
> > }
> > #endif
> >
> > -static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> > +static int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> > {
> > int ret = mmap_read_lock_killable(lock_ctx->mm);
> >
> > @@ -140,7 +140,7 @@ static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> > return ret;
> > }
> >
> > -static inline void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> > +static void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> > {
> > mmap_read_unlock(lock_ctx->mm);
> > lock_ctx->mmap_locked = false;
> > @@ -177,7 +177,7 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> > return vma;
> > }
> >
> > -static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> > +static bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> > loff_t pos)
>
> If you touch these, please convert them to two-tab indent.
Ack. I didn't realize that's the preferred formatting.
>
> --
> Cheers,
>
> David
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter
2026-09-07 6:39 ` [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter Suren Baghdasaryan
2026-09-08 18:07 ` Liam R. Howlett
@ 2026-09-09 17:16 ` David Hildenbrand (Arm)
2026-09-09 18:28 ` Suren Baghdasaryan
1 sibling, 1 reply; 36+ messages in thread
From: David Hildenbrand (Arm) @ 2026-09-09 17:16 UTC (permalink / raw)
To: Suren Baghdasaryan, akpm
Cc: liam, ljs, vbabka, willy, jannh, paulmck, pfalcato, linux-mm,
linux-kernel, linux-fsdevel
On 9/7/26 08:39, Suren Baghdasaryan wrote:
> smap_gather_stats() interprets its start parameter to mean vma->vm_start
> when it's set to 0. Eliminate this special interpretation and pass
> vma->vm_start explicitly when needed.
>
> Since smap_gather_stats() operates within a single VMA, we can replace
> walk_page_vma()/walk_page_range() calls with walk_page_range_vma()
> which is simpler and also can be called while holding per-VMA lock.
>
> No functional change intended.
>
> Suggested by: Lorenzo Stoakes <ljs@kernel.org>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> ---
> fs/proc/task_mmu.c | 40 ++++++++++++++++++++++------------------
> 1 file changed, 22 insertions(+), 18 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 9908ba32f180..3351decd1172 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1246,20 +1246,27 @@ get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
> return &smaps_shmem_walk_vma_lock_ops;
> }
>
> -/*
> - * Gather mem stats from @vma with the indicated beginning
> - * address @start, and keep them in @mss.
> +/**
> + * smap_gather_stats() - Gather mem stats from @vma.
> + * @priv: proc maps private state.
> + * @vma: The VMA to gather stats for.
> + * @mss: The accumulated stats.
> + * @start: The address from which to start.
> *
> - * Use vm_start of @vma as the beginning address if @start is 0.
> + * This gathers stats for the whole of the VMA unless the lock was dropped
> + * and VMA grew or got merged and we found it again, in which case we only
> + * gather stats for the remainder of the VMA range.
> */
> static void smap_gather_stats(struct proc_maps_private *priv,
> struct vm_area_struct *vma,
> - struct mem_size_stats *mss, unsigned long start)
> + struct mem_size_stats *mss,
> + unsigned long start)
> {
> const struct mm_walk_ops *ops = get_smaps_walk_ops(priv);
> + const bool is_partial = start > vma->vm_start;
>
> /* Invalid start */
> - if (start >= vma->vm_end)
> + if (start < vma->vm_start || start >= vma->vm_end)
> return;
>
> if (vma == get_gate_vma(priv->lock_ctx.mm))
> @@ -1279,20 +1286,17 @@ static void smap_gather_stats(struct proc_maps_private *priv,
> * Unless we know that the shmem object (or the part mapped by
> * our VMA) has no swapped out pages at all.
> */
> - unsigned long shmem_swapped = shmem_swap_usage(vma);
> + const unsigned long shmem_swapped = shmem_swap_usage(vma);
> + const bool shared_or_ro = vma_test(vma, VMA_SHARED_BIT) ||
> + !vma_test(vma, VMA_WRITE_BIT);
>
> - if (!start && (!shmem_swapped || (vma->vm_flags & VM_SHARED) ||
> - !(vma->vm_flags & VM_WRITE))) {
> + if (!is_partial && (!shmem_swapped || shared_or_ro))
> mss->swap += shmem_swapped;
> - } else {
> + else
> ops = get_smaps_shmem_walk_ops(priv);
> - }
Horrible, horrible code, really. But not your fault :)
I think we can just make the shared_or_ro less odd by just checking for cow
mappings (as described in the comment).
const bool is_cow = vma_is_cow_mapping(vma);
...
if (is_partial || (shmem_swapped && is_cow))
ops = get_smaps_shmem_walk_ops(priv);
else
mss->swap += shmem_swapped;
That's almost in a form that I could understand what's happening.
--
Cheers,
David
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers
2026-09-09 17:13 ` Suren Baghdasaryan
@ 2026-09-09 17:17 ` David Hildenbrand (Arm)
2026-09-09 18:29 ` Suren Baghdasaryan
0 siblings, 1 reply; 36+ messages in thread
From: David Hildenbrand (Arm) @ 2026-09-09 17:17 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, liam, ljs, vbabka, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On 9/9/26 19:13, Suren Baghdasaryan wrote:
> On Wed, Sep 9, 2026 at 10:06 AM David Hildenbrand (Arm)
> <david@kernel.org> wrote:
>>
>> On 9/7/26 08:39, Suren Baghdasaryan wrote:
>>> When per-vma locks were behind a config option, a number of helper
>>> functions were needed to simplify the locking code. Now that these
>>> locks are universally available, we can do a little cleanup.
>>> Remove lock_vma_range(), unlock_vma_range(), query_vma_setup(),
>>> query_vma_teardown() helpers.
>>>
>>> No functional change intended.
>>>
>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>>> ---
>>
>> As Usama says, this could get picked up early independent of the rest (maybe
>> even as part of the other patch set)
>
> I can post the 3 cleanup patches separately from the one that switches
> to using per-VMA locks if that's preferable. One downside: I'll have
> to wait until the cleanups are picked up before posting the last
> patch, as it depends on these cleanups. Please let me know if I should
> split them anyway; otherwise, will keep v3 as a single patchset.
Let's keep it as is. We can reconsider if we realize that the other patches get
stuck.
--
Cheers,
David
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock
2026-09-07 6:39 ` [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
2026-09-08 18:17 ` Liam R. Howlett
2026-09-09 14:25 ` Usama Arif
@ 2026-09-09 17:23 ` David Hildenbrand (Arm)
2026-09-09 17:58 ` Suren Baghdasaryan
2 siblings, 1 reply; 36+ messages in thread
From: David Hildenbrand (Arm) @ 2026-09-09 17:23 UTC (permalink / raw)
To: Suren Baghdasaryan, akpm
Cc: liam, ljs, vbabka, willy, jannh, paulmck, pfalcato, linux-mm,
linux-kernel, linux-fsdevel
> - vma_start = vma->vm_start;
> - do {
> - smap_gather_stats(priv, vma, &mss, vma->vm_start);
> - last_vma_end = vma->vm_end;
> + if (!IS_ERR(vma) && vma != get_gate_vma(lock_ctx->mm))
> + vma_start = vma->vm_start;
> +
> + while (vma) {
> + if (IS_ERR(vma)) {
> + ret = PTR_ERR(vma);
> + goto out_unlock;
> + }
> +
Can we add a comment whey we break (and not e.g., continue) whenw e hit the gate
VMA?
(I seriously don't kmow ... should I know? :) )
> + if (vma == get_gate_vma(lock_ctx->mm))
> + break;
>
> /*
> - * Release mmap_lock temporarily if someone wants to
> - * access it for write request.
> + * If after retaking the lock, already reported VMA grew or
> + * merged with the next one, smap_gather_stats() will gather
> + * stats for the remaining portion by starting at last_vma_end.
> */
> - if (mmap_lock_is_contended(mm)) {
> - vma_iter_invalidate(&vmi);
> - unlock_ctx_mm(&priv->lock_ctx);
> - ret = lock_ctx_mm(&priv->lock_ctx);
> - if (ret) {
> - release_task_mempolicy(priv);
> - goto out_put_mm;
> - }
> + smap_gather_stats(priv, vma, &mss, last_vma_end);
> + last_vma_end = vma->vm_end;
I skimmed over the remaining bits, hoping the VMA lock experts will review in
detail :)
--
Cheers,
David
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock
2026-09-09 17:23 ` David Hildenbrand (Arm)
@ 2026-09-09 17:58 ` Suren Baghdasaryan
2026-09-10 7:44 ` David Hildenbrand (Arm)
0 siblings, 1 reply; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-09 17:58 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: akpm, liam, ljs, vbabka, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On Wed, Sep 9, 2026 at 10:23 AM David Hildenbrand (Arm)
<david@kernel.org> wrote:
>
>
> > - vma_start = vma->vm_start;
> > - do {
> > - smap_gather_stats(priv, vma, &mss, vma->vm_start);
> > - last_vma_end = vma->vm_end;
> > + if (!IS_ERR(vma) && vma != get_gate_vma(lock_ctx->mm))
> > + vma_start = vma->vm_start;
> > +
> > + while (vma) {
> > + if (IS_ERR(vma)) {
> > + ret = PTR_ERR(vma);
> > + goto out_unlock;
> > + }
> > +
>
> Can we add a comment whey we break (and not e.g., continue) whenw e hit the gate
> VMA?
>
> (I seriously don't kmow ... should I know? :) )
The way m_next() is implemented, the gate VMA always placed at the end
of the address space, so the next VMA will be NULL and we can break
once we see the gate. But now that I'm looking closer into this code,
reading smaps_rollup file does not invoke m_next(), so we should never
encounter a gate VMA (it's not in the maple tree, so for_each_vma()
should never return it). I think I can remove the special handling for
that case.
Thanks for the question, David! It made me realize we can simplify this further.
>
> > + if (vma == get_gate_vma(lock_ctx->mm))
> > + break;
> >
> > /*
> > - * Release mmap_lock temporarily if someone wants to
> > - * access it for write request.
> > + * If after retaking the lock, already reported VMA grew or
> > + * merged with the next one, smap_gather_stats() will gather
> > + * stats for the remaining portion by starting at last_vma_end.
> > */
> > - if (mmap_lock_is_contended(mm)) {
> > - vma_iter_invalidate(&vmi);
> > - unlock_ctx_mm(&priv->lock_ctx);
> > - ret = lock_ctx_mm(&priv->lock_ctx);
> > - if (ret) {
> > - release_task_mempolicy(priv);
> > - goto out_put_mm;
> > - }
> > + smap_gather_stats(priv, vma, &mss, last_vma_end);
> > + last_vma_end = vma->vm_end;
>
> I skimmed over the remaining bits, hoping the VMA lock experts will review in
> detail :)
Thanks! I'll wait another day and if there are no more comments, I
will post the next version.
>
> --
> Cheers,
>
> David
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter
2026-09-09 17:16 ` David Hildenbrand (Arm)
@ 2026-09-09 18:28 ` Suren Baghdasaryan
2026-09-09 19:16 ` David Hildenbrand (Arm)
0 siblings, 1 reply; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-09 18:28 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: akpm, liam, ljs, vbabka, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On Wed, Sep 9, 2026 at 10:16 AM David Hildenbrand (Arm)
<david@kernel.org> wrote:
>
> On 9/7/26 08:39, Suren Baghdasaryan wrote:
> > smap_gather_stats() interprets its start parameter to mean vma->vm_start
> > when it's set to 0. Eliminate this special interpretation and pass
> > vma->vm_start explicitly when needed.
> >
> > Since smap_gather_stats() operates within a single VMA, we can replace
> > walk_page_vma()/walk_page_range() calls with walk_page_range_vma()
> > which is simpler and also can be called while holding per-VMA lock.
> >
> > No functional change intended.
> >
> > Suggested by: Lorenzo Stoakes <ljs@kernel.org>
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> > fs/proc/task_mmu.c | 40 ++++++++++++++++++++++------------------
> > 1 file changed, 22 insertions(+), 18 deletions(-)
> >
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index 9908ba32f180..3351decd1172 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -1246,20 +1246,27 @@ get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
> > return &smaps_shmem_walk_vma_lock_ops;
> > }
> >
> > -/*
> > - * Gather mem stats from @vma with the indicated beginning
> > - * address @start, and keep them in @mss.
> > +/**
> > + * smap_gather_stats() - Gather mem stats from @vma.
> > + * @priv: proc maps private state.
> > + * @vma: The VMA to gather stats for.
> > + * @mss: The accumulated stats.
> > + * @start: The address from which to start.
> > *
> > - * Use vm_start of @vma as the beginning address if @start is 0.
> > + * This gathers stats for the whole of the VMA unless the lock was dropped
> > + * and VMA grew or got merged and we found it again, in which case we only
> > + * gather stats for the remainder of the VMA range.
> > */
> > static void smap_gather_stats(struct proc_maps_private *priv,
> > struct vm_area_struct *vma,
> > - struct mem_size_stats *mss, unsigned long start)
> > + struct mem_size_stats *mss,
> > + unsigned long start)
> > {
> > const struct mm_walk_ops *ops = get_smaps_walk_ops(priv);
> > + const bool is_partial = start > vma->vm_start;
> >
> > /* Invalid start */
> > - if (start >= vma->vm_end)
> > + if (start < vma->vm_start || start >= vma->vm_end)
> > return;
> >
> > if (vma == get_gate_vma(priv->lock_ctx.mm))
> > @@ -1279,20 +1286,17 @@ static void smap_gather_stats(struct proc_maps_private *priv,
> > * Unless we know that the shmem object (or the part mapped by
> > * our VMA) has no swapped out pages at all.
> > */
> > - unsigned long shmem_swapped = shmem_swap_usage(vma);
> > + const unsigned long shmem_swapped = shmem_swap_usage(vma);
> > + const bool shared_or_ro = vma_test(vma, VMA_SHARED_BIT) ||
> > + !vma_test(vma, VMA_WRITE_BIT);
> >
> > - if (!start && (!shmem_swapped || (vma->vm_flags & VM_SHARED) ||
> > - !(vma->vm_flags & VM_WRITE))) {
> > + if (!is_partial && (!shmem_swapped || shared_or_ro))
> > mss->swap += shmem_swapped;
> > - } else {
> > + else
> > ops = get_smaps_shmem_walk_ops(priv);
> > - }
>
> Horrible, horrible code, really. But not your fault :)
>
> I think we can just make the shared_or_ro less odd by just checking for cow
> mappings (as described in the comment).
>
> const bool is_cow = vma_is_cow_mapping(vma);
>
> ...
>
> if (is_partial || (shmem_swapped && is_cow))
> ops = get_smaps_shmem_walk_ops(priv);
> else
> mss->swap += shmem_swapped;
>
> That's almost in a form that I could understand what's happening.
Hmm. So, are you saying that !is_cow always implies shared_or_ro? Or
maybe you are stating that vma_is_cow_mapping() was the actual intent
here?
shared_or_ro = VMA_SHARED_BIT || !VMA_WRITE_BIT
is_cow = !VMA_SHARED_BIT && VMA_MAYWRITE_BIT
!is_cow = VMA_SHARED_BIT || !VMA_MAYWRITE_BIT
so, !is_cow would impy shared_or_ro only if !VMA_MAYWRITE_BIT always
implies !VMA_WRITE_BIT. But I think it's possible to have a VMA that
has VMA_WRITE_BIT but not VMA_MAYWRITE_BIT, right? For example, a
driver can create such a VMA to allow writing to the VMA but to lock
its content once mprotect(PROT_READ) gets called.
>
> --
> Cheers,
>
> David
>
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers
2026-09-09 17:17 ` David Hildenbrand (Arm)
@ 2026-09-09 18:29 ` Suren Baghdasaryan
0 siblings, 0 replies; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-09 18:29 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: akpm, liam, ljs, vbabka, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On Wed, Sep 9, 2026 at 10:17 AM David Hildenbrand (Arm)
<david@kernel.org> wrote:
>
> On 9/9/26 19:13, Suren Baghdasaryan wrote:
> > On Wed, Sep 9, 2026 at 10:06 AM David Hildenbrand (Arm)
> > <david@kernel.org> wrote:
> >>
> >> On 9/7/26 08:39, Suren Baghdasaryan wrote:
> >>> When per-vma locks were behind a config option, a number of helper
> >>> functions were needed to simplify the locking code. Now that these
> >>> locks are universally available, we can do a little cleanup.
> >>> Remove lock_vma_range(), unlock_vma_range(), query_vma_setup(),
> >>> query_vma_teardown() helpers.
> >>>
> >>> No functional change intended.
> >>>
> >>> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> >>> ---
> >>
> >> As Usama says, this could get picked up early independent of the rest (maybe
> >> even as part of the other patch set)
> >
> > I can post the 3 cleanup patches separately from the one that switches
> > to using per-VMA locks if that's preferable. One downside: I'll have
> > to wait until the cleanups are picked up before posting the last
> > patch, as it depends on these cleanups. Please let me know if I should
> > split them anyway; otherwise, will keep v3 as a single patchset.
>
> Let's keep it as is. We can reconsider if we realize that the other patches get
> stuck.
Ack. Thanks!
>
> --
> Cheers,
>
> David
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter
2026-09-09 18:28 ` Suren Baghdasaryan
@ 2026-09-09 19:16 ` David Hildenbrand (Arm)
2026-09-09 21:51 ` Suren Baghdasaryan
0 siblings, 1 reply; 36+ messages in thread
From: David Hildenbrand (Arm) @ 2026-09-09 19:16 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, liam, ljs, vbabka, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On 9/9/26 20:28, Suren Baghdasaryan wrote:
> On Wed, Sep 9, 2026 at 10:16 AM David Hildenbrand (Arm)
> <david@kernel.org> wrote:
>>
>> On 9/7/26 08:39, Suren Baghdasaryan wrote:
>>> smap_gather_stats() interprets its start parameter to mean vma->vm_start
>>> when it's set to 0. Eliminate this special interpretation and pass
>>> vma->vm_start explicitly when needed.
>>>
>>> Since smap_gather_stats() operates within a single VMA, we can replace
>>> walk_page_vma()/walk_page_range() calls with walk_page_range_vma()
>>> which is simpler and also can be called while holding per-VMA lock.
>>>
>>> No functional change intended.
>>>
>>> Suggested by: Lorenzo Stoakes <ljs@kernel.org>
>>> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
>>> ---
>>> fs/proc/task_mmu.c | 40 ++++++++++++++++++++++------------------
>>> 1 file changed, 22 insertions(+), 18 deletions(-)
>>>
>>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>>> index 9908ba32f180..3351decd1172 100644
>>> --- a/fs/proc/task_mmu.c
>>> +++ b/fs/proc/task_mmu.c
>>> @@ -1246,20 +1246,27 @@ get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
>>> return &smaps_shmem_walk_vma_lock_ops;
>>> }
>>>
>>> -/*
>>> - * Gather mem stats from @vma with the indicated beginning
>>> - * address @start, and keep them in @mss.
>>> +/**
>>> + * smap_gather_stats() - Gather mem stats from @vma.
>>> + * @priv: proc maps private state.
>>> + * @vma: The VMA to gather stats for.
>>> + * @mss: The accumulated stats.
>>> + * @start: The address from which to start.
>>> *
>>> - * Use vm_start of @vma as the beginning address if @start is 0.
>>> + * This gathers stats for the whole of the VMA unless the lock was dropped
>>> + * and VMA grew or got merged and we found it again, in which case we only
>>> + * gather stats for the remainder of the VMA range.
>>> */
>>> static void smap_gather_stats(struct proc_maps_private *priv,
>>> struct vm_area_struct *vma,
>>> - struct mem_size_stats *mss, unsigned long start)
>>> + struct mem_size_stats *mss,
>>> + unsigned long start)
>>> {
>>> const struct mm_walk_ops *ops = get_smaps_walk_ops(priv);
>>> + const bool is_partial = start > vma->vm_start;
>>>
>>> /* Invalid start */
>>> - if (start >= vma->vm_end)
>>> + if (start < vma->vm_start || start >= vma->vm_end)
>>> return;
>>>
>>> if (vma == get_gate_vma(priv->lock_ctx.mm))
>>> @@ -1279,20 +1286,17 @@ static void smap_gather_stats(struct proc_maps_private *priv,
>>> * Unless we know that the shmem object (or the part mapped by
>>> * our VMA) has no swapped out pages at all.
>>> */
>>> - unsigned long shmem_swapped = shmem_swap_usage(vma);
>>> + const unsigned long shmem_swapped = shmem_swap_usage(vma);
>>> + const bool shared_or_ro = vma_test(vma, VMA_SHARED_BIT) ||
>>> + !vma_test(vma, VMA_WRITE_BIT);
>>>
>>> - if (!start && (!shmem_swapped || (vma->vm_flags & VM_SHARED) ||
>>> - !(vma->vm_flags & VM_WRITE))) {
>>> + if (!is_partial && (!shmem_swapped || shared_or_ro))
>>> mss->swap += shmem_swapped;
>>> - } else {
>>> + else
>>> ops = get_smaps_shmem_walk_ops(priv);
>>> - }
>>
>> Horrible, horrible code, really. But not your fault :)
>>
>> I think we can just make the shared_or_ro less odd by just checking for cow
>> mappings (as described in the comment).
>>
>> const bool is_cow = vma_is_cow_mapping(vma);
>>
>> ...
>>
>> if (is_partial || (shmem_swapped && is_cow))
>> ops = get_smaps_shmem_walk_ops(priv);
>> else
>> mss->swap += shmem_swapped;
>>
>> That's almost in a form that I could understand what's happening.
>
> Hmm. So, are you saying that !is_cow always implies shared_or_ro? Or
> maybe you are stating that vma_is_cow_mapping() was the actual intent
> here?
So the comment says:
"For private writable mappings, we might have COW pages that .."
Which translates to:
private writable == vma_is_cow_mapping()
>
> shared_or_ro = VMA_SHARED_BIT || !VMA_WRITE_BIT
>
> is_cow = !VMA_SHARED_BIT && VMA_MAYWRITE_BIT
> !is_cow = VMA_SHARED_BIT || !VMA_MAYWRITE_BIT
>
> so, !is_cow would impy shared_or_ro only if !VMA_MAYWRITE_BIT always
> implies !VMA_WRITE_BIT. But I think it's possible to have a VMA that
> has VMA_WRITE_BIT but not VMA_MAYWRITE_BIT, right?
VMA_WRITE should imply VMA_MAYWRITE
(in sanitize_fault_flags() we even disallow write faults entirely if VM_MAYWRITE
is missing)
For example, a
> driver can create such a VMA to allow writing to the VMA but to lock
> its content once mprotect(PROT_READ) gets called.
I don't think that would be valid for a driver to do. But it wouldn't matter
here because
shmem_mapping(vma->vm_file->f_mapping)
I think we could simplify the comment as well to:
diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
index e671b4fd8dedd..4b7e7089cafa7 100644
--- a/fs/proc/task_mmu.c
+++ b/fs/proc/task_mmu.c
@@ -1303,12 +1303,10 @@ static void smap_gather_stats(struct proc_maps_private
*priv,
if (vma->vm_file && shmem_mapping(vma->vm_file->f_mapping)) {
/*
- * For shared or readonly shmem mappings we know that all
- * swapped out pages belong to the shmem object, and we can
- * obtain the swap value much more efficiently. For private
- * writable mappings, we might have COW pages that are
- * not affected by the parent swapped out pages of the shmem
- * object, so we have to distinguish them during the page walk.
+ * In CoW mappings, we might have anon folios that are
+ * independent of the shmem object. So fallback to the less
+ * efficient mechanism in such mappings.
+ *
* Unless we know that the shmem object (or the part mapped by
* our VMA) has no swapped out pages at all.
*/
--
Cheers,
David
^ permalink raw reply related [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter
2026-09-09 19:16 ` David Hildenbrand (Arm)
@ 2026-09-09 21:51 ` Suren Baghdasaryan
2026-09-10 7:41 ` David Hildenbrand (Arm)
0 siblings, 1 reply; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-09 21:51 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: akpm, liam, ljs, vbabka, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On Wed, Sep 9, 2026 at 12:16 PM David Hildenbrand (Arm)
<david@kernel.org> wrote:
>
> On 9/9/26 20:28, Suren Baghdasaryan wrote:
> > On Wed, Sep 9, 2026 at 10:16 AM David Hildenbrand (Arm)
> > <david@kernel.org> wrote:
> >>
> >> On 9/7/26 08:39, Suren Baghdasaryan wrote:
> >>> smap_gather_stats() interprets its start parameter to mean vma->vm_start
> >>> when it's set to 0. Eliminate this special interpretation and pass
> >>> vma->vm_start explicitly when needed.
> >>>
> >>> Since smap_gather_stats() operates within a single VMA, we can replace
> >>> walk_page_vma()/walk_page_range() calls with walk_page_range_vma()
> >>> which is simpler and also can be called while holding per-VMA lock.
> >>>
> >>> No functional change intended.
> >>>
> >>> Suggested by: Lorenzo Stoakes <ljs@kernel.org>
> >>> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> >>> ---
> >>> fs/proc/task_mmu.c | 40 ++++++++++++++++++++++------------------
> >>> 1 file changed, 22 insertions(+), 18 deletions(-)
> >>>
> >>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> >>> index 9908ba32f180..3351decd1172 100644
> >>> --- a/fs/proc/task_mmu.c
> >>> +++ b/fs/proc/task_mmu.c
> >>> @@ -1246,20 +1246,27 @@ get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
> >>> return &smaps_shmem_walk_vma_lock_ops;
> >>> }
> >>>
> >>> -/*
> >>> - * Gather mem stats from @vma with the indicated beginning
> >>> - * address @start, and keep them in @mss.
> >>> +/**
> >>> + * smap_gather_stats() - Gather mem stats from @vma.
> >>> + * @priv: proc maps private state.
> >>> + * @vma: The VMA to gather stats for.
> >>> + * @mss: The accumulated stats.
> >>> + * @start: The address from which to start.
> >>> *
> >>> - * Use vm_start of @vma as the beginning address if @start is 0.
> >>> + * This gathers stats for the whole of the VMA unless the lock was dropped
> >>> + * and VMA grew or got merged and we found it again, in which case we only
> >>> + * gather stats for the remainder of the VMA range.
> >>> */
> >>> static void smap_gather_stats(struct proc_maps_private *priv,
> >>> struct vm_area_struct *vma,
> >>> - struct mem_size_stats *mss, unsigned long start)
> >>> + struct mem_size_stats *mss,
> >>> + unsigned long start)
> >>> {
> >>> const struct mm_walk_ops *ops = get_smaps_walk_ops(priv);
> >>> + const bool is_partial = start > vma->vm_start;
> >>>
> >>> /* Invalid start */
> >>> - if (start >= vma->vm_end)
> >>> + if (start < vma->vm_start || start >= vma->vm_end)
> >>> return;
> >>>
> >>> if (vma == get_gate_vma(priv->lock_ctx.mm))
> >>> @@ -1279,20 +1286,17 @@ static void smap_gather_stats(struct proc_maps_private *priv,
> >>> * Unless we know that the shmem object (or the part mapped by
> >>> * our VMA) has no swapped out pages at all.
> >>> */
> >>> - unsigned long shmem_swapped = shmem_swap_usage(vma);
> >>> + const unsigned long shmem_swapped = shmem_swap_usage(vma);
> >>> + const bool shared_or_ro = vma_test(vma, VMA_SHARED_BIT) ||
> >>> + !vma_test(vma, VMA_WRITE_BIT);
> >>>
> >>> - if (!start && (!shmem_swapped || (vma->vm_flags & VM_SHARED) ||
> >>> - !(vma->vm_flags & VM_WRITE))) {
> >>> + if (!is_partial && (!shmem_swapped || shared_or_ro))
> >>> mss->swap += shmem_swapped;
> >>> - } else {
> >>> + else
> >>> ops = get_smaps_shmem_walk_ops(priv);
> >>> - }
> >>
> >> Horrible, horrible code, really. But not your fault :)
> >>
> >> I think we can just make the shared_or_ro less odd by just checking for cow
> >> mappings (as described in the comment).
> >>
> >> const bool is_cow = vma_is_cow_mapping(vma);
> >>
> >> ...
> >>
> >> if (is_partial || (shmem_swapped && is_cow))
> >> ops = get_smaps_shmem_walk_ops(priv);
> >> else
> >> mss->swap += shmem_swapped;
> >>
> >> That's almost in a form that I could understand what's happening.
> >
> > Hmm. So, are you saying that !is_cow always implies shared_or_ro? Or
> > maybe you are stating that vma_is_cow_mapping() was the actual intent
> > here?
>
> So the comment says:
>
> "For private writable mappings, we might have COW pages that .."
>
> Which translates to:
>
> private writable == vma_is_cow_mapping()
Ok, just want to make sure I'm not missing something subtle.
>
> >
> > shared_or_ro = VMA_SHARED_BIT || !VMA_WRITE_BIT
> >
> > is_cow = !VMA_SHARED_BIT && VMA_MAYWRITE_BIT
> > !is_cow = VMA_SHARED_BIT || !VMA_MAYWRITE_BIT
> >
> > so, !is_cow would impy shared_or_ro only if !VMA_MAYWRITE_BIT always
> > implies !VMA_WRITE_BIT. But I think it's possible to have a VMA that
> > has VMA_WRITE_BIT but not VMA_MAYWRITE_BIT, right?
>
> VMA_WRITE should imply VMA_MAYWRITE
Ah, good to know.
>
> (in sanitize_fault_flags() we even disallow write faults entirely if VM_MAYWRITE
> is missing)
I see.
>
> For example, a
> > driver can create such a VMA to allow writing to the VMA but to lock
> > its content once mprotect(PROT_READ) gets called.
>
> I don't think that would be valid for a driver to do. But it wouldn't matter
> here because
>
> shmem_mapping(vma->vm_file->f_mapping)
I was obviously overthinking this :)
>
>
> I think we could simplify the comment as well to:
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index e671b4fd8dedd..4b7e7089cafa7 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -1303,12 +1303,10 @@ static void smap_gather_stats(struct proc_maps_private
> *priv,if (is_partial || (shmem_swapped && is_cow))
>
> if (vma->vm_file && shmem_mapping(vma->vm_file->f_mapping)) {
> /*
> - * For shared or readonly shmem mappings we know that all
> - * swapped out pages belong to the shmem object, and we can
> - * obtain the swap value much more efficiently. For private
> - * writable mappings, we might have COW pages that are
> - * not affected by the parent swapped out pages of the shmem
> - * object, so we have to distinguish them during the page walk.
> + * In CoW mappings, we might have anon folios that are
> + * independent of the shmem object. So fallback to the less
> + * efficient mechanism in such mappings.
> + *
Sounds good. Will update.
> * Unless we know that the shmem object (or the part mapped by
> * our VMA) has no swapped out pages at all.
> */
>
> --
> Cheers,
>
> David
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter
2026-09-09 21:51 ` Suren Baghdasaryan
@ 2026-09-10 7:41 ` David Hildenbrand (Arm)
2026-09-10 15:45 ` Suren Baghdasaryan
0 siblings, 1 reply; 36+ messages in thread
From: David Hildenbrand (Arm) @ 2026-09-10 7:41 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, liam, ljs, vbabka, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On 9/9/26 23:51, Suren Baghdasaryan wrote:
> On Wed, Sep 9, 2026 at 12:16 PM David Hildenbrand (Arm)
> <david@kernel.org> wrote:
>>
>> On 9/9/26 20:28, Suren Baghdasaryan wrote:
>>> On Wed, Sep 9, 2026 at 10:16 AM David Hildenbrand (Arm)
>>> <david@kernel.org> wrote:
>>>
>>> Hmm. So, are you saying that !is_cow always implies shared_or_ro? Or
>>> maybe you are stating that vma_is_cow_mapping() was the actual intent
>>> here?
>>
>> So the comment says:
>>
>> "For private writable mappings, we might have COW pages that .."
>>
>> Which translates to:
>>
>> private writable == vma_is_cow_mapping()
>
> Ok, just want to make sure I'm not missing something subtle.
>
>>
>>>
>>> shared_or_ro = VMA_SHARED_BIT || !VMA_WRITE_BIT
>>>
>>> is_cow = !VMA_SHARED_BIT && VMA_MAYWRITE_BIT
>>> !is_cow = VMA_SHARED_BIT || !VMA_MAYWRITE_BIT
>>>
>>> so, !is_cow would impy shared_or_ro only if !VMA_MAYWRITE_BIT always
>>> implies !VMA_WRITE_BIT. But I think it's possible to have a VMA that
>>> has VMA_WRITE_BIT but not VMA_MAYWRITE_BIT, right?
>>
>> VMA_WRITE should imply VMA_MAYWRITE
>
> Ah, good to know.
>
>>
>> (in sanitize_fault_flags() we even disallow write faults entirely if VM_MAYWRITE
>> is missing)
>
> I see.
>
>>
>> For example, a
>>> driver can create such a VMA to allow writing to the VMA but to lock
>>> its content once mprotect(PROT_READ) gets called.
>>
>> I don't think that would be valid for a driver to do. But it wouldn't matter
>> here because
>>
>> shmem_mapping(vma->vm_file->f_mapping)
>
> I was obviously overthinking this :)
>
>>
>>
>> I think we could simplify the comment as well to:
>>
>> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
>> index e671b4fd8dedd..4b7e7089cafa7 100644
>> --- a/fs/proc/task_mmu.c
>> +++ b/fs/proc/task_mmu.c
>> @@ -1303,12 +1303,10 @@ static void smap_gather_stats(struct proc_maps_private
>> *priv,if (is_partial || (shmem_swapped && is_cow))
>>
>> if (vma->vm_file && shmem_mapping(vma->vm_file->f_mapping)) {
>> /*
>> - * For shared or readonly shmem mappings we know that all
>> - * swapped out pages belong to the shmem object, and we can
>> - * obtain the swap value much more efficiently. For private
>> - * writable mappings, we might have COW pages that are
>> - * not affected by the parent swapped out pages of the shmem
>> - * object, so we have to distinguish them during the page walk.
>> + * In CoW mappings, we might have anon folios that are
>> + * independent of the shmem object. So fallback to the less
>> + * efficient mechanism in such mappings.
>> + *
>
> Sounds good. Will update.
Likely worth putting that into a prior cleanup patch, so there is less noise in
this patch.
--
Cheers,
David
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock
2026-09-09 17:58 ` Suren Baghdasaryan
@ 2026-09-10 7:44 ` David Hildenbrand (Arm)
0 siblings, 0 replies; 36+ messages in thread
From: David Hildenbrand (Arm) @ 2026-09-10 7:44 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, liam, ljs, vbabka, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On 9/9/26 19:58, Suren Baghdasaryan wrote:
> On Wed, Sep 9, 2026 at 10:23 AM David Hildenbrand (Arm)
> <david@kernel.org> wrote:
>>
>>
>>> - vma_start = vma->vm_start;
>>> - do {
>>> - smap_gather_stats(priv, vma, &mss, vma->vm_start);
>>> - last_vma_end = vma->vm_end;
>>> + if (!IS_ERR(vma) && vma != get_gate_vma(lock_ctx->mm))
>>> + vma_start = vma->vm_start;
>>> +
>>> + while (vma) {
>>> + if (IS_ERR(vma)) {
>>> + ret = PTR_ERR(vma);
>>> + goto out_unlock;
>>> + }
>>> +
>>
>> Can we add a comment whey we break (and not e.g., continue) whenw e hit the gate
>> VMA?
>>
>> (I seriously don't kmow ... should I know? :) )
>
> The way m_next() is implemented, the gate VMA always placed at the end
> of the address space, so the next VMA will be NULL and we can break
> once we see the gate. But now that I'm looking closer into this code,
> reading smaps_rollup file does not invoke m_next(), so we should never
> encounter a gate VMA (it's not in the maple tree, so for_each_vma()
> should never return it). I think I can remove the special handling for
> that case.
>
> Thanks for the question, David! It made me realize we can simplify this further.
good! :)
--
Cheers,
David
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions
2026-09-07 16:49 ` Usama Arif
@ 2026-09-10 15:35 ` Suren Baghdasaryan
0 siblings, 0 replies; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-10 15:35 UTC (permalink / raw)
To: Usama Arif
Cc: akpm, liam, ljs, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On Mon, Sep 7, 2026 at 9:49 AM Usama Arif <usama.arif@linux.dev> wrote:
>
> On Sun, 6 Sep 2026 23:39:15 -0700 Suren Baghdasaryan <surenb@google.com> wrote:
>
> > It was pointed out in the previous reviews of this code that many
> > functions are specified as inline, which is unnecessary as the compile
> > can make that decision by itself. Cleanup these definitions.
> >
> > No functional change intended.
> >
> > Signed-off-by: Suren Baghdasaryan <surenb@google.com>
> > ---
> > fs/proc/task_mmu.c | 32 ++++++++++++++++----------------
> > 1 file changed, 16 insertions(+), 16 deletions(-)
>
> Most of these functions would be inlined anyways so should be ok.
>
> Curious if there was a change in binary size with this?
No change whatsoever:
W/o this patch:
text data bss dec hex filename
31321399 9024030 1127028 41472457 278d1c9 vmlinux
With this patch:
text data bss dec hex filename
31321399 9024030 1127028 41472457 278d1c9 vmlinux
>
> Acked-by: Usama Arif <usama.arif@linux.dev>
>
> >
> > diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> > index 2f500d639db5..9908ba32f180 100644
> > --- a/fs/proc/task_mmu.c
> > +++ b/fs/proc/task_mmu.c
> > @@ -130,7 +130,7 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
> > }
> > #endif
> >
> > -static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> > +static int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> > {
> > int ret = mmap_read_lock_killable(lock_ctx->mm);
> >
> > @@ -140,7 +140,7 @@ static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> > return ret;
> > }
> >
> > -static inline void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> > +static void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> > {
> > mmap_read_unlock(lock_ctx->mm);
> > lock_ctx->mmap_locked = false;
> > @@ -177,7 +177,7 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> > return vma;
> > }
> >
> > -static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> > +static bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> > loff_t pos)
> > {
> > struct proc_maps_locking_ctx *lock_ctx = &priv->lock_ctx;
> > @@ -194,7 +194,7 @@ static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> > return true;
> > }
> >
> > -static inline void drop_rcu(struct proc_maps_private *priv)
> > +static void drop_rcu(struct proc_maps_private *priv)
> > {
> > if (priv->lock_ctx.mmap_locked)
> > return;
> > @@ -202,7 +202,7 @@ static inline void drop_rcu(struct proc_maps_private *priv)
> > rcu_read_unlock();
> > }
> >
> > -static inline void reacquire_rcu(struct proc_maps_private *priv)
> > +static void reacquire_rcu(struct proc_maps_private *priv)
> > {
> > if (priv->lock_ctx.mmap_locked)
> > return;
> > @@ -1230,7 +1230,7 @@ static const struct mm_walk_ops smaps_shmem_walk_vma_lock_ops = {
> > .walk_lock = PGWALK_VMA_RDLOCK_VERIFY,
> > };
> >
> > -static inline const struct mm_walk_ops *
> > +static const struct mm_walk_ops *
> > get_smaps_walk_ops(struct proc_maps_private *priv)
> > {
> > if (priv->lock_ctx.mmap_locked)
> > @@ -1238,7 +1238,7 @@ get_smaps_walk_ops(struct proc_maps_private *priv)
> > return &smaps_walk_vma_lock_ops;
> > }
> >
> > -static inline const struct mm_walk_ops *
> > +static const struct mm_walk_ops *
> > get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
> > {
> > if (priv->lock_ctx.mmap_locked)
> > @@ -1572,7 +1572,7 @@ struct clear_refs_private {
> > enum clear_refs_types type;
> > };
> >
> > -static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> > +static bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> > {
> > struct folio *folio;
> >
> > @@ -1588,8 +1588,8 @@ static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr,
> > return folio_maybe_dma_pinned(folio);
> > }
> >
> > -static inline void clear_soft_dirty(struct vm_area_struct *vma,
> > - unsigned long addr, pte_t *pte)
> > +static void clear_soft_dirty(struct vm_area_struct *vma, unsigned long addr,
> > + pte_t *pte)
> > {
> > if (!pgtable_supports_soft_dirty())
> > return;
> > @@ -1620,8 +1620,8 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
> > }
> >
> > #if defined(CONFIG_TRANSPARENT_HUGEPAGE)
> > -static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> > - unsigned long addr, pmd_t *pmdp)
> > +static void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> > + unsigned long addr, pmd_t *pmdp)
> > {
> > pmd_t old, pmd = *pmdp;
> >
> > @@ -1646,8 +1646,8 @@ static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> > }
> > }
> > #else
> > -static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> > - unsigned long addr, pmd_t *pmdp)
> > +static void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> > + unsigned long addr, pmd_t *pmdp)
> > {
> > }
> > #endif
> > @@ -1846,7 +1846,7 @@ struct pagemapread {
> >
> > #define PM_END_OF_BUFFER 1
> >
> > -static inline pagemap_entry_t make_pme(u64 frame, u64 flags)
> > +static pagemap_entry_t make_pme(u64 frame, u64 flags)
> > {
> > return (pagemap_entry_t) { .pme = (frame & PM_PFRAME_MASK) | flags };
> > }
> > @@ -3388,7 +3388,7 @@ static const struct mm_walk_ops show_numa_vma_lock_ops = {
> > .walk_lock = PGWALK_VMA_RDLOCK_VERIFY,
> > };
> >
> > -static inline const struct mm_walk_ops *
> > +static const struct mm_walk_ops *
> > get_show_numa_ops(struct proc_maps_private *priv)
> > {
> > if (priv->lock_ctx.mmap_locked)
> > --
> > 2.55.0.979.g7e5102b832-goog
> >
> >
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers
2026-09-07 6:39 ` [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers Suren Baghdasaryan
` (2 preceding siblings ...)
2026-09-09 17:06 ` David Hildenbrand (Arm)
@ 2026-09-10 15:43 ` Lorenzo Stoakes (ARM)
3 siblings, 0 replies; 36+ messages in thread
From: Lorenzo Stoakes (ARM) @ 2026-09-10 15:43 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, liam, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On Sun, Sep 06, 2026 at 11:39:14PM -0700, Suren Baghdasaryan wrote:
> When per-vma locks were behind a config option, a number of helper
> functions were needed to simplify the locking code. Now that these
> locks are universally available, we can do a little cleanup.
> Remove lock_vma_range(), unlock_vma_range(), query_vma_setup(),
> query_vma_teardown() helpers.
Oh nice :)
>
> No functional change intended.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
LGTM so:
Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> ---
> fs/proc/task_mmu.c | 67 ++++++++++++----------------------------------
> 1 file changed, 17 insertions(+), 50 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index e671b4fd8ded..2f500d639db5 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -160,25 +160,6 @@ static void unlock_ctx_vma(struct proc_maps_locking_ctx *lock_ctx)
> }
> }
>
> -static inline bool lock_vma_range(struct seq_file *m,
> - struct proc_maps_locking_ctx *lock_ctx)
> -{
> - rcu_read_lock();
> - reset_lock_ctx(lock_ctx);
> -
> - return true;
> -}
> -
> -static inline void unlock_vma_range(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - if (lock_ctx->mmap_locked) {
> - unlock_ctx_mm(lock_ctx);
> - } else {
> - unlock_ctx_vma(lock_ctx);
> - rcu_read_unlock();
> - }
> -}
> -
> static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> loff_t last_pos)
> {
> @@ -286,13 +267,8 @@ static void *m_start(struct seq_file *m, loff_t *ppos)
> return NULL;
> }
>
> - if (!lock_vma_range(m, lock_ctx)) {
> - mmput(mm);
> - put_task_struct(priv->task);
> - priv->task = NULL;
> - return ERR_PTR(-EINTR);
> - }
> -
> + rcu_read_lock();
> + reset_lock_ctx(lock_ctx);
> /*
> * Reset current position if last_addr was set before
> * and it's not a sentinel.
> @@ -325,7 +301,12 @@ static void m_stop(struct seq_file *m, void *v)
> return;
>
> release_task_mempolicy(priv);
> - unlock_vma_range(&priv->lock_ctx);
> + if (priv->lock_ctx.mmap_locked) {
> + unlock_ctx_mm(&priv->lock_ctx);
> + } else {
> + unlock_ctx_vma(&priv->lock_ctx);
> + rcu_read_unlock();
> + }
> mmput(mm);
> put_task_struct(priv->task);
> priv->task = NULL;
> @@ -518,21 +499,6 @@ static int pid_maps_open(struct inode *inode, struct file *file)
> PROCMAP_QUERY_VMA_FLAGS \
> )
>
> -static int query_vma_setup(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - reset_lock_ctx(lock_ctx);
> -
> - return 0;
> -}
> -
> -static void query_vma_teardown(struct proc_maps_locking_ctx *lock_ctx)
> -{
> - if (lock_ctx->mmap_locked)
> - unlock_ctx_mm(lock_ctx);
> - else
> - unlock_ctx_vma(lock_ctx);
> -}
> -
> static struct vm_area_struct *query_vma_find_by_addr(struct proc_maps_locking_ctx *lock_ctx,
> unsigned long addr)
> {
> @@ -653,12 +619,7 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
> if (!mm || !mmget_not_zero(mm))
> return -ESRCH;
>
> - err = query_vma_setup(&lock_ctx);
> - if (err) {
> - mmput(mm);
> - return err;
> - }
> -
> + reset_lock_ctx(&lock_ctx);
> vma = query_matching_vma(&lock_ctx, karg.query_addr, karg.query_flags);
> if (IS_ERR(vma)) {
> err = PTR_ERR(vma);
> @@ -732,7 +693,10 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
> vm_file = get_file(vma->vm_file);
>
> /* unlock vma or mmap_lock, and put mm_struct before copying data to user */
> - query_vma_teardown(&lock_ctx);
> + if (lock_ctx.mmap_locked)
> + unlock_ctx_mm(&lock_ctx);
> + else
> + unlock_ctx_vma(&lock_ctx);
> mmput(mm);
>
> if (karg.build_id_size) {
> @@ -773,7 +737,10 @@ static int do_procmap_query(struct mm_struct *mm, void __user *uarg)
> return 0;
>
> out:
> - query_vma_teardown(&lock_ctx);
> + if (lock_ctx.mmap_locked)
> + unlock_ctx_mm(&lock_ctx);
> + else
> + unlock_ctx_vma(&lock_ctx);
> mmput(mm);
> out_file:
> if (vm_file)
> --
> 2.55.0.979.g7e5102b832-goog
>
--
Cheers, Lorenzo
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter
2026-09-10 7:41 ` David Hildenbrand (Arm)
@ 2026-09-10 15:45 ` Suren Baghdasaryan
2026-09-10 16:01 ` David Hildenbrand (Arm)
0 siblings, 1 reply; 36+ messages in thread
From: Suren Baghdasaryan @ 2026-09-10 15:45 UTC (permalink / raw)
To: David Hildenbrand (Arm)
Cc: akpm, liam, ljs, vbabka, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On Thu, Sep 10, 2026 at 12:41 AM David Hildenbrand (Arm)
<david@kernel.org> wrote:
>
> On 9/9/26 23:51, Suren Baghdasaryan wrote:
> > On Wed, Sep 9, 2026 at 12:16 PM David Hildenbrand (Arm)
> > <david@kernel.org> wrote:
> >>
> >> On 9/9/26 20:28, Suren Baghdasaryan wrote:
> >>> On Wed, Sep 9, 2026 at 10:16 AM David Hildenbrand (Arm)
> >>> <david@kernel.org> wrote:
> >>>
> >>> Hmm. So, are you saying that !is_cow always implies shared_or_ro? Or
> >>> maybe you are stating that vma_is_cow_mapping() was the actual intent
> >>> here?
> >>
> >> So the comment says:
> >>
> >> "For private writable mappings, we might have COW pages that .."
> >>
> >> Which translates to:
> >>
> >> private writable == vma_is_cow_mapping()
> >
> > Ok, just want to make sure I'm not missing something subtle.
> >
> >>
> >>>
> >>> shared_or_ro = VMA_SHARED_BIT || !VMA_WRITE_BIT
> >>>
> >>> is_cow = !VMA_SHARED_BIT && VMA_MAYWRITE_BIT
> >>> !is_cow = VMA_SHARED_BIT || !VMA_MAYWRITE_BIT
> >>>
> >>> so, !is_cow would impy shared_or_ro only if !VMA_MAYWRITE_BIT always
> >>> implies !VMA_WRITE_BIT. But I think it's possible to have a VMA that
> >>> has VMA_WRITE_BIT but not VMA_MAYWRITE_BIT, right?
> >>
> >> VMA_WRITE should imply VMA_MAYWRITE
> >
> > Ah, good to know.
> >
> >>
> >> (in sanitize_fault_flags() we even disallow write faults entirely if VM_MAYWRITE
> >> is missing)
> >
> > I see.
> >
> >>
> >> For example, a
> >>> driver can create such a VMA to allow writing to the VMA but to lock
> >>> its content once mprotect(PROT_READ) gets called.
> >>
> >> I don't think that would be valid for a driver to do. But it wouldn't matter
> >> here because
> >>
> >> shmem_mapping(vma->vm_file->f_mapping)
> >
> > I was obviously overthinking this :)
> >
> >>
> >>
> >> I think we could simplify the comment as well to:
> >>
> >> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> >> index e671b4fd8dedd..4b7e7089cafa7 100644
> >> --- a/fs/proc/task_mmu.c
> >> +++ b/fs/proc/task_mmu.c
> >> @@ -1303,12 +1303,10 @@ static void smap_gather_stats(struct proc_maps_private
> >> *priv,if (is_partial || (shmem_swapped && is_cow))
> >>
> >> if (vma->vm_file && shmem_mapping(vma->vm_file->f_mapping)) {
> >> /*
> >> - * For shared or readonly shmem mappings we know that all
> >> - * swapped out pages belong to the shmem object, and we can
> >> - * obtain the swap value much more efficiently. For private
> >> - * writable mappings, we might have COW pages that are
> >> - * not affected by the parent swapped out pages of the shmem
> >> - * object, so we have to distinguish them during the page walk.
> >> + * In CoW mappings, we might have anon folios that are
> >> + * independent of the shmem object. So fallback to the less
> >> + * efficient mechanism in such mappings.
> >> + *
> >
> > Sounds good. Will update.
>
> Likely worth putting that into a prior cleanup patch, so there is less noise in
> this patch.
Well, this is a cleanup specific for smap_gather_stats() funciton, so
I would prefer to keep all the pieces in one place.
>
> --
> Cheers,
>
> David
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions
2026-09-07 6:39 ` [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions Suren Baghdasaryan
` (2 preceding siblings ...)
2026-09-09 17:07 ` David Hildenbrand (Arm)
@ 2026-09-10 15:55 ` Lorenzo Stoakes (ARM)
3 siblings, 0 replies; 36+ messages in thread
From: Lorenzo Stoakes (ARM) @ 2026-09-10 15:55 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, liam, vbabka, david, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On Sun, Sep 06, 2026 at 11:39:15PM -0700, Suren Baghdasaryan wrote:
> It was pointed out in the previous reviews of this code that many
> functions are specified as inline, which is unnecessary as the compile
> can make that decision by itself. Cleanup these definitions.
>
> No functional change intended.
>
> Signed-off-by: Suren Baghdasaryan <surenb@google.com>
Yes :) Nice, thanks!
Reviewed-by: Lorenzo Stoakes (ARM) <ljs@kernel.org>
> ---
> fs/proc/task_mmu.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/fs/proc/task_mmu.c b/fs/proc/task_mmu.c
> index 2f500d639db5..9908ba32f180 100644
> --- a/fs/proc/task_mmu.c
> +++ b/fs/proc/task_mmu.c
> @@ -130,7 +130,7 @@ static void release_task_mempolicy(struct proc_maps_private *priv)
> }
> #endif
>
> -static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> +static int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> {
> int ret = mmap_read_lock_killable(lock_ctx->mm);
>
> @@ -140,7 +140,7 @@ static inline int lock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> return ret;
> }
>
> -static inline void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> +static void unlock_ctx_mm(struct proc_maps_locking_ctx *lock_ctx)
> {
> mmap_read_unlock(lock_ctx->mm);
> lock_ctx->mmap_locked = false;
> @@ -177,7 +177,7 @@ static struct vm_area_struct *get_next_vma(struct proc_maps_private *priv,
> return vma;
> }
>
> -static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> +static bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> loff_t pos)
> {
> struct proc_maps_locking_ctx *lock_ctx = &priv->lock_ctx;
> @@ -194,7 +194,7 @@ static inline bool fallback_to_mmap_lock(struct proc_maps_private *priv,
> return true;
> }
>
> -static inline void drop_rcu(struct proc_maps_private *priv)
> +static void drop_rcu(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> return;
> @@ -202,7 +202,7 @@ static inline void drop_rcu(struct proc_maps_private *priv)
> rcu_read_unlock();
> }
>
> -static inline void reacquire_rcu(struct proc_maps_private *priv)
> +static void reacquire_rcu(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> return;
> @@ -1230,7 +1230,7 @@ static const struct mm_walk_ops smaps_shmem_walk_vma_lock_ops = {
> .walk_lock = PGWALK_VMA_RDLOCK_VERIFY,
> };
>
> -static inline const struct mm_walk_ops *
> +static const struct mm_walk_ops *
> get_smaps_walk_ops(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> @@ -1238,7 +1238,7 @@ get_smaps_walk_ops(struct proc_maps_private *priv)
> return &smaps_walk_vma_lock_ops;
> }
>
> -static inline const struct mm_walk_ops *
> +static const struct mm_walk_ops *
> get_smaps_shmem_walk_ops(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> @@ -1572,7 +1572,7 @@ struct clear_refs_private {
> enum clear_refs_types type;
> };
>
> -static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> +static bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr, pte_t pte)
> {
> struct folio *folio;
>
> @@ -1588,8 +1588,8 @@ static inline bool pte_is_pinned(struct vm_area_struct *vma, unsigned long addr,
> return folio_maybe_dma_pinned(folio);
> }
>
> -static inline void clear_soft_dirty(struct vm_area_struct *vma,
> - unsigned long addr, pte_t *pte)
> +static void clear_soft_dirty(struct vm_area_struct *vma, unsigned long addr,
> + pte_t *pte)
> {
> if (!pgtable_supports_soft_dirty())
> return;
> @@ -1620,8 +1620,8 @@ static inline void clear_soft_dirty(struct vm_area_struct *vma,
> }
>
> #if defined(CONFIG_TRANSPARENT_HUGEPAGE)
> -static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> - unsigned long addr, pmd_t *pmdp)
> +static void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> + unsigned long addr, pmd_t *pmdp)
> {
> pmd_t old, pmd = *pmdp;
>
> @@ -1646,8 +1646,8 @@ static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> }
> }
> #else
> -static inline void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> - unsigned long addr, pmd_t *pmdp)
> +static void clear_soft_dirty_pmd(struct vm_area_struct *vma,
> + unsigned long addr, pmd_t *pmdp)
> {
> }
> #endif
> @@ -1846,7 +1846,7 @@ struct pagemapread {
>
> #define PM_END_OF_BUFFER 1
>
> -static inline pagemap_entry_t make_pme(u64 frame, u64 flags)
> +static pagemap_entry_t make_pme(u64 frame, u64 flags)
> {
> return (pagemap_entry_t) { .pme = (frame & PM_PFRAME_MASK) | flags };
> }
> @@ -3388,7 +3388,7 @@ static const struct mm_walk_ops show_numa_vma_lock_ops = {
> .walk_lock = PGWALK_VMA_RDLOCK_VERIFY,
> };
>
> -static inline const struct mm_walk_ops *
> +static const struct mm_walk_ops *
> get_show_numa_ops(struct proc_maps_private *priv)
> {
> if (priv->lock_ctx.mmap_locked)
> --
> 2.55.0.979.g7e5102b832-goog
>
--
Cheers, Lorenzo
^ permalink raw reply [flat|nested] 36+ messages in thread
* Re: [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter
2026-09-10 15:45 ` Suren Baghdasaryan
@ 2026-09-10 16:01 ` David Hildenbrand (Arm)
0 siblings, 0 replies; 36+ messages in thread
From: David Hildenbrand (Arm) @ 2026-09-10 16:01 UTC (permalink / raw)
To: Suren Baghdasaryan
Cc: akpm, liam, ljs, vbabka, willy, jannh, paulmck, pfalcato,
linux-mm, linux-kernel, linux-fsdevel
On 9/10/26 17:45, Suren Baghdasaryan wrote:
> On Thu, Sep 10, 2026 at 12:41 AM David Hildenbrand (Arm)
> <david@kernel.org> wrote:
>>
>> On 9/9/26 23:51, Suren Baghdasaryan wrote:
>>> On Wed, Sep 9, 2026 at 12:16 PM David Hildenbrand (Arm)
>>> <david@kernel.org> wrote:
>>>
>>> Ok, just want to make sure I'm not missing something subtle.
>>>
>>>
>>> Ah, good to know.
>>>
>>>
>>> I see.
>>>
>>>
>>> I was obviously overthinking this :)
>>>
>>>
>>> Sounds good. Will update.
>>
>> Likely worth putting that into a prior cleanup patch, so there is less noise in
>> this patch.
>
> Well, this is a cleanup specific for smap_gather_stats() funciton, so
> I would prefer to keep all the pieces in one place.
I meant as part of this series of course.
--
Cheers,
David
^ permalink raw reply [flat|nested] 36+ messages in thread
end of thread, other threads:[~2026-09-10 16:02 UTC | newest]
Thread overview: 36+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-09-07 6:39 [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
2026-09-07 6:39 ` [PATCH v2 1/5] proc/task_mmu: remove unnecessary helpers Suren Baghdasaryan
2026-09-07 16:44 ` Usama Arif
2026-09-08 17:58 ` Liam R. Howlett
2026-09-09 17:06 ` David Hildenbrand (Arm)
2026-09-09 17:13 ` Suren Baghdasaryan
2026-09-09 17:17 ` David Hildenbrand (Arm)
2026-09-09 18:29 ` Suren Baghdasaryan
2026-09-10 15:43 ` Lorenzo Stoakes (ARM)
2026-09-07 6:39 ` [PATCH v2 2/5] proc/task_mmu: remove unnecessary inlines in function definitions Suren Baghdasaryan
2026-09-07 16:49 ` Usama Arif
2026-09-10 15:35 ` Suren Baghdasaryan
2026-09-08 18:01 ` Liam R. Howlett
2026-09-09 17:07 ` David Hildenbrand (Arm)
2026-09-09 17:15 ` Suren Baghdasaryan
2026-09-10 15:55 ` Lorenzo Stoakes (ARM)
2026-09-07 6:39 ` [PATCH v2 3/5] proc/task_mmu: remove special-casing of smap_gather_stats() start parameter Suren Baghdasaryan
2026-09-08 18:07 ` Liam R. Howlett
2026-09-09 17:16 ` David Hildenbrand (Arm)
2026-09-09 18:28 ` Suren Baghdasaryan
2026-09-09 19:16 ` David Hildenbrand (Arm)
2026-09-09 21:51 ` Suren Baghdasaryan
2026-09-10 7:41 ` David Hildenbrand (Arm)
2026-09-10 15:45 ` Suren Baghdasaryan
2026-09-10 16:01 ` David Hildenbrand (Arm)
2026-09-07 6:39 ` [PATCH v2 4/5] proc/task_mmu: read proc/pid/smaps_rollup under per-vma lock Suren Baghdasaryan
2026-09-08 18:17 ` Liam R. Howlett
2026-09-09 14:25 ` Usama Arif
2026-09-09 16:13 ` Suren Baghdasaryan
2026-09-09 17:23 ` David Hildenbrand (Arm)
2026-09-09 17:58 ` Suren Baghdasaryan
2026-09-10 7:44 ` David Hildenbrand (Arm)
2026-09-07 6:39 ` [PATCH v2 5/5] selftests/proc: add /proc/pid/smaps_rollup tearing tests Suren Baghdasaryan
2026-09-08 18:18 ` Liam R. Howlett
2026-09-08 16:04 ` [PATCH v2 0/5] read proc/pid/smaps_rollup under per-vma lock Xueyuan Chen
2026-09-08 16:08 ` Suren Baghdasaryan
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox