Re: [PATCH 1/2] mm, memcg: cg2 memory{.swap,}.peak write handlers

[Johannes Weiner <hannes@cmpxchg.org>]

On Tue, Jul 23, 2024 at 09:55:19PM -0400, Waiman Long wrote:
> Could you use the "-v <n>" option of git-format-patch to add a version 
> number to the patch title? Without that, it can be confusing as to 
> whether the patch is new or a resend of the previous one.

+1

> > @@ -775,6 +775,11 @@ struct cgroup_subsys {
> >   
> >   extern struct percpu_rw_semaphore cgroup_threadgroup_rwsem;
> >   
> > +struct cgroup_of_peak {
> > +	long			value;
> > +	struct list_head	list;
> > +};
> The name "cgroup_of_peak" is kind of confusing. Maybe local_peak?

It's the peak associated with an 'of' (which is a known concept in
cgroup code), and it pairs up nicely with of_peak(). I'd prefer
keeping that over local_peak.

> > @@ -26,6 +26,7 @@ struct page_counter {
> >   	atomic_long_t children_low_usage;
> >   
> >   	unsigned long watermark;
> > +	unsigned long local_watermark; /* track min of fd-local resets */
> track "min"? I thought it is used to track local maximum after a reset.

Yeah, the comment doesn't sound quite right.

However, I think we'd be hard-pressed to explain correctly and
comprehensively what this thing does in <40 characters.

I'd just remove the comment tbh.

> > @@ -78,7 +79,10 @@ int page_counter_memparse(const char *buf, const char *max,
> >   
> >   static inline void page_counter_reset_watermark(struct page_counter *counter)
> >   {
> > -	counter->watermark = page_counter_read(counter);
> > +	unsigned long usage = page_counter_read(counter);
> > +
> > +	counter->watermark = usage;
> > +	counter->local_watermark = usage;
> >   }
> >   
> 
> Could you set the local_watermark first before setting watermark? There 
> is a very small time window that the invariant "local_watermark <= 
> watermark" is not true.

Does it matter? Only cgroup1 supports global resets; only cgroup2
supports local peaks watching. This doesn't add anything to the race
that already exists between reset and global watermark update on cg1.

> > @@ -3950,12 +3955,90 @@ static u64 memory_current_read(struct cgroup_subsys_state *css,
> >   	return (u64)page_counter_read(&memcg->memory) * PAGE_SIZE;
> >   }
> >   
> > -static u64 memory_peak_read(struct cgroup_subsys_state *css,
> > -			    struct cftype *cft)
> > +static int peak_show(struct seq_file *sf, void *v, struct page_counter *pc)
> >   {
> > -	struct mem_cgroup *memcg = mem_cgroup_from_css(css);
> > +	struct cgroup_of_peak *ofp = of_peak(sf->private);
> > +	s64 fd_peak = ofp->value, peak;
> > +
> > +	/* User wants global or local peak? */
> > +	if (fd_peak == -1)
> > +		peak = pc->watermark;
> > +	else
> > +		peak = max(fd_peak, (s64)pc->local_watermark);
> Should you save the local_watermark value into ofp->value if 
> local_watermark is bigger? This will ensure that each successive read of 
> the fd is monotonically increasing. Otherwise the value may go up or 
> down if there are multiple resets in between.

The reset saves local_watermark into ofp->value if it's bigger..?

I do see another problem, though. The compiler might issue multiple
reads to ofp->value in arbitrary order. We could print max(-1, ...)
which is nonsense. Saving ofp->value into a local variable is the
right idea, but the compiler might still issue two reads anyway. It
needs a READ_ONCE() to force a single read.

I'd use unsigned long for fd_peak. This way the "specialness" is on
the -1UL comparison. The max() must be between two positive numbers,
so the (s64) there is confusing.