Re: [PATCH v3 1/2] memcg: acquire peaks_lock when reading memory.peak

[Muchun Song]

> On Aug 14, 2026, at 09:29, Ridong Chen <ridong.chen@xxxxxxxxx> wrote:
> 
> From: Ridong Chen <chenridong@xxxxxxxxxx>
> 
> Sashiko reported that a reader can transiently observe a lower peak
> within a race window [1]. peak_show() returns
> max(local_watermark, ofp->value), but peak_write() updates those two
> under peaks_lock while the reader takes no lock. The interleaving is:
> 
>  writer (reset on fd A)                 reader (fd B)
>  ----------------------                 -------------
>  usage = page_counter_read(pc)
>  WRITE_ONCE(local_watermark, usage)
>  // watermark lowered to usage
>                                         lw  = READ_ONCE(local_watermark)
>                                         // sees the lowered usage
>                                         val = READ_ONCE(ofp->value)
>                                         // B's value not updated yet
>                                         return max(lw, val)
>                                         // both low -> low peak
>  WRITE_ONCE(peer_ctx->value, usage)
>  // B updated, but too late
> 
> Fix it by acquiring peaks_lock when reading the peak, so the reader sees
> a consistent snapshot of local_watermark and the per-fd values. The same
> race applies to memory.swap.peak, which shares peaks_lock and the
> peak_write() path, so take the lock there as well.
> 
> [1] https://sashiko.dev/#/patchset/20260730115314.1069089-1-ridong.chen@xxxxxxxxx?part=1
> Fixes: c6f53ed8f213 ("mm, memcg: cg2 memory{.swap,}.peak write handlers")
> Assisted-by: Claude:claude-opus-4-8
> Signed-off-by: Ridong Chen <chenridong@xxxxxxxxxx>
> Acked-by: Johannes Weiner <hannes@xxxxxxxxxxx>
> Acked-by: Shakeel Butt <shakeel.butt@xxxxxxxxx>
> ---
> mm/memcontrol.c | 14 ++++++++++++--
> 1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
> index 17da1f43b7d3..6dd8756870ff 100644
> --- a/mm/memcontrol.c
> +++ b/mm/memcontrol.c
> @@ -4713,8 +4713,13 @@ static int peak_show(struct seq_file *sf, void *v, struct page_counter *pc)
> static int memory_peak_show(struct seq_file *sf, void *v)
> {
> 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(sf));
> + 	int ret;
> 
> - 	return peak_show(sf, v, &memcg->memory);
> + 	spin_lock(&memcg->peaks_lock);

Why not to use guard(spinlock)(&memcg->peaks_lock) to simplify the code.

Muchun,
Thanks.

> + 	ret = peak_show(sf, v, &memcg->memory);
> + 	spin_unlock(&memcg->peaks_lock);
> +
> + 	return ret;
> }
> 
> static int peak_open(struct kernfs_open_file *of)
> @@ -5858,8 +5863,13 @@ static u64 swap_current_read(struct cgroup_subsys_state *css,
> static int swap_peak_show(struct seq_file *sf, void *v)
> {
> 	struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(sf));
> + 	int ret;
> 
> - 	return peak_show(sf, v, &memcg->swap);
> + 	spin_lock(&memcg->peaks_lock);
> + 	ret = peak_show(sf, v, &memcg->swap);
> + 	spin_unlock(&memcg->peaks_lock);
> +
> + 	return ret;
> }
> 
> static ssize_t swap_peak_write(struct kernfs_open_file *of, char *buf,
> -- 
> 2.34.1
>