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

On 8/7/2026 5:14 PM, [email protected] wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] peak_write() destroys transient peaks for all other watchers by propagating the current usage instead of the old local_watermark.
> --
> 
> --- Patch [1]: [PATCH v2 1/2] memcg: acquire peaks_lock when reading memory.peak ---
> commit 7238a507204c3e18dae7c130c8f9f1da91b169b0
> Author: Ridong Chen <[email protected]>
> 
> memcg: acquire peaks_lock when reading memory.peak
> 
> The commit fixes a race condition where a reader of memory.peak could
> transiently observe a lower peak. It adds peaks_lock acquisition in
> memory_peak_show and swap_peak_show to ensure readers see a consistent
> snapshot of the peak metrics.
> 
> Link: https://sashiko.dev/#/patchset/[email protected]?part=1
> 
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 6061c0ba923bf..15c65249849e3 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
> 
> [ ... ]
> 
>> @@ -5806,8 +5811,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,
> 
> [Severity: High]
> This is a pre-existing issue, but does peak_write() destroy transient peaks
> for other watchers by propagating the current usage instead of the old
> local_watermark?
> 
> 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 its peak when usage has dropped, peak_write() uses the
> current lower usage. If watcher B was tracking a previous peak, it seems
> watcher B's value is updated to the current usage rather than the previous
> peak (the old local_watermark).
> 
> For example, if watcher B resets, and then usage spikes to 100 and drops to
> 50, when watcher A resets, peak_write() will see usage is 50. Watcher B's
> value would be updated to 50 instead of 100, and the previous peak of 100
> would be lost.
> 
> Can this cause workload schedulers reading the peak to receive an
> artificially low value, leading them to over-pack workloads?
> 

This is what I am trying to fix with patch 2/2.

-- 
Best regards
Ridong