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

[Ridong Chen <ridong.chen@linux.dev>]

On 8/14/2026 10:12 AM, Muchun Song wrote:
> 
> 
>> On Aug 14, 2026, at 09:29, Ridong Chen <ridong.chen@linux.dev> wrote:
>>
>> From: Ridong Chen <chenridong@xiaomi.com>
>>
>> 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@linux.dev?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@xiaomi.com>
>> Acked-by: Johannes Weiner <hannes@cmpxchg.org>
>> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
>> ---
>> 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.
> 

Good idea. Will update.

>> + 	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
>>
> 

-- 
Best regards
Ridong