Re: [PATCH] mm, memcg: fix memory.peak reset clobbering other fds' watermark

On 7/31/2026 12:03 AM, Johannes Weiner wrote:
> On Thu, Jul 30, 2026 at 07:53:14PM +0800, Ridong wrote:
>> From: Ridong Chen <[email protected]>
>>
>> Writing to memory.peak resets the peak for that fd only. Each fd is a
>> watcher and reads back max(its own value, the shared local_watermark).
>>
>> peak_write() resets by lowering local_watermark to the current usage.
>> To keep the other watchers' peaks it then walks the watcher list, but it
>> stores the current usage into them instead of the old watermark. So once
>> usage has dropped from a peak, a reset on one fd wrongly drags every
>> other fd's peak down too, even fds that never reset.
>>
>> Reproduced on 7.2.0-rc5-next under QEMU, two fds A and B on one cgroup:
>> B sees the peak (410624 KB), usage drops, then A resets -- and B's peak
>> collapses to 1060 KB although B never reset. With this patch B keeps
>> reading 410624 KB.
>>
>> Fix: save the old watermark before lowering it and use that to floor the
>> other watchers, so a reset only affects the fd that issued it.
>>
>> Fixes: c6f53ed8f213 ("mm, memcg: cg2 memory{.swap,}.peak write handlers")
>> Assisted-by: Claude:claude-opus-4-8
>> Signed-off-by: Ridong Chen <[email protected]>
>> ---
>>   mm/memcontrol.c | 7 ++++---
>>   1 file changed, 4 insertions(+), 3 deletions(-)
>>
>> diff --git a/mm/memcontrol.c b/mm/memcontrol.c
>> index 60145aadfc5e..881e7c459c64 100644
>> --- a/mm/memcontrol.c
>> +++ b/mm/memcontrol.c
>> @@ -4692,7 +4692,7 @@ static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
>>   			  loff_t off, struct page_counter *pc,
>>   			  struct list_head *watchers)
>>   {
>> -	unsigned long usage;
>> +	unsigned long usage, old_watermark;
>>   	struct cgroup_of_peak *peer_ctx;
>>   	struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
>>   	struct cgroup_of_peak *ofp = of_peak(of);
>> @@ -4700,11 +4700,12 @@ static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
>>   	spin_lock(&memcg->peaks_lock);
>>   
>>   	usage = page_counter_read(pc);
>> +	old_watermark = READ_ONCE(pc->local_watermark);
>>   	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 (peer_ctx != ofp && old_watermark > peer_ctx->value)
>> +			WRITE_ONCE(peer_ctx->value, old_watermark);
> 

Hi Johannes,

Thank you for your reply.

> Ah, because B was previously reporting the higher local_watermark, and
> its peer_ctx->value was actually low. Fixing it to current usage is
> wrong in that case. It must remember local_watermark.
> 
> What about if usage is bigger than old_watermark? Then we don't update
> the peer_ctx just yet. local_watermark is updated and propagated into
> the peers on the next reset. I guess it's correct, but it's kind of
> tricky to follow.
> 

IIUC, usage > old_watermark shouldn't actually happen, because local_watermark 
is a running peak maintained by the charge path:

page_counter_charge() {
[...]
	if (new > READ_ONCE(c->local_watermark))
		WRITE_ONCE(c->local_watermark, new);
[...]
}

> Would it be easier to understand if we mirrored the max() from
> peak_show() here?
> 
> 	usage = page_counter_read(pc);
> 	local_watermark = READ_ONCE(pc->local_watermark);
> 	WRITE_ONCE(pc->local_watermark, usage);
> 
> 	peer_watermark = max(usage, local_watermark);

In peak_show() the max we have is:
	peak = max(fd_peak, local_watermark);

I'd like to clarify that fd_peak here is a different thing from usage. fd_peak 
is the peak recorded by this fd (ofp->value), not the current usage. So 
max(usage, local_watermark) isn't really mirroring the max() in peak_show(). 
it's a different expression.

> 	list_for_each_entry(...)
> 		if (peer_ctx != ofp && peer_watermark > peer_ctx->value)
> 			WRITE_ONCE(peer_ctx->value, peer_watermark);
> 
> This code hurts my head. No strong feelings either way ;)

-- 
Best regards
Ridong