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

On 8/14/2026 10:06 AM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] peak_write() overwrites pc->local_watermark without preserving the historical peak for other watchers.
> --
> 
> --- Patch [1]: [PATCH v3 1/2] memcg: acquire peaks_lock when reading memory.peak ---
> commit 4ced2059ac574bcfd14e2b9ed8899640facbe49f
> Author: Ridong Chen <[email protected]>
> 
>      memcg: acquire peaks_lock when reading memory.peak
> 
> This patch fixes a race condition when reading memory.peak or memory.swap.peak.
> It acquires peaks_lock in memory_peak_show() and swap_peak_show() so that
> readers observe a consistent snapshot of local_watermark and per-fd values.
> 
> [1] https://sashiko.dev/#/patchset/[email protected]?part=1
> 
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 1d3339520809f..0495b844c3750 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -4712,8 +4712,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);
>> +	ret = peak_show(sf, v, &memcg->memory);
>> +	spin_unlock(&memcg->peaks_lock);
>> +
>> +	return ret;
>>   }
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but while reviewing the peaks_lock
> synchronization changes, does the shared peak_write() path permanently lose
> the historical peak for other watchers?
> 
> Looking at peak_write() in mm/memcontrol.c:
> 
> 	usage = page_counter_read(pc);
> 	WRITE_ONCE(pc->local_watermark, usage);
> 
> 	list_for_each_entry(peer_ctx, watchers, list)
> 		if (usage > peer_ctx->value)
> 			WRITE_ONCE(peer_ctx->value, usage);
> 
> If watcher A resets memory.peak, peak_write() updates pc->local_watermark to
> the current usage. It then updates watcher B's tracking value to that same
> current usage.
> 
> If memory usage hit a peak of 100 but dropped to 50 before watcher A reset it,
> won't watcher B incorrectly read 50 instead of the actual peak of 100?
> 

The patch 2 will fix this issue.

-- 
Best regards
Ridong