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

Other mechanisms for querying the peak memory usage of either a process
or v1 memory cgroup allow for resetting the high watermark. Restore
parity with those mechanisms, but with a less racy API.

For example:
 - Any write to memory.max_usage_in_bytes in a cgroup v1 mount resets
   the high watermark.
 - writing "5" to the clear_refs pseudo-file in a processes's proc
   directory resets the peak RSS.

This change is an evolution of a previous patch, which mostly copied the
cgroup v1 behavior, however, there were concerns about races/ownership
issues with a global reset, so instead this change makes the reset
filedescriptor-local.

Writing a specific string to the memory.peak and memory.swap.peak
pseudo-files reset the high watermark to the current usage for
subsequent reads through that same fd.

Notably, following Johannes's suggestion, this implementation moves the
O(fds that have written) behavior onto the fd write(2) path. Instead, on
the page-allocation path, we simply add one additional watermark to
conditionally bump per-hierarchy level in the page-counter.

Additionally, this takes Longman's suggestion of nesting the
page-charging-path checks for the two watermarks to reduce the number of
common-case comparisons.

This behavior is particularly useful for work scheduling systems that
need to track memory usage of worker processes/cgroups per-work-item.
Since memory can't be squeezed like CPU can (the OOM-killer has
opinions), these systems need to track the peak memory usage to compute
system/container fullness when binpacking workitems.

Most notably, Vimeo's use-case involves a system that's doing global
binpacking across many Kubernetes pods/containers, and while we can use
PSI for some local decisions about overload, we strive to avoid packing
workloads too tightly in the first place. To facilitate this, we track
the peak memory usage. However, since we run with long-lived workers (to
amortize startup costs) we need a way to track the high watermark while
a work-item is executing. Polling runs the risk of missing short spikes
that last for timescales below the polling interval, and peak memory
tracking at the cgroup level is otherwise perfect for this use-case.

As this data is used to ensure that binpacked work ends up with
sufficient headroom, this use-case mostly avoids the inaccuracies
surrounding reclaimable memory.

Suggested-by: Johannes Weiner <hannes@cmpxchg.org>
Suggested-by: Waiman Long <longman@redhat.com>
Signed-off-by: David Finkel <davidf@vimeo.com>
---
 Documentation/admin-guide/cgroup-v2.rst |  26 ++++--
 include/linux/cgroup-defs.h             |   5 +
 include/linux/cgroup.h                  |   3 +
 include/linux/memcontrol.h              |   5 +
 include/linux/page_counter.h            |   6 +-
 kernel/cgroup/cgroup-internal.h         |   2 +
 kernel/cgroup/cgroup.c                  |   7 ++
 mm/memcontrol.c                         | 117 ++++++++++++++++++++++--
 mm/page_counter.c                       |  30 ++++--
 9 files changed, 174 insertions(+), 27 deletions(-)

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.

On Wed, Jul 24, 2024 at 7:49 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> 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

Sorry, I forgot that that flag exists.
I'll use that with the next patch. (which I'll send out shortly)

>
> > > @@ -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.
Yeah, it's not explicitly the min. At reset-time, it's the current
value at reset-time, and all the fd-local
watermarks will all be greater than or equal.
Which does effectively make it the min of watermarks at the time it's
being set by the reset code.

However, yeah, the page-charging code will increase it, which makes it
not a min.

>
> 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.
Yeah, I definitely didn't think that comment through.
Deleting.
>
> > > @@ -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.
>
Hmm, since the global watermark update is now conditional on both watermarks
being <= the current usage, it does make sense.
Witht that said, since we're assigning without any barriers, as-is,
the CPU and compiler are quite free to re-order them anyway.

I've swapped them and added a comment.
> > > @@ -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.
Thanks, I didn't realize the compiler had the latitude to decide to
read from that
struct field a second time when referencing the local variable.

Added.
>
> 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.
I've switched fd_peak to `u64`.

Thanks again,

--
David Finkel
Senior Principal Software Engineer, Core Services

Hello, David.

On Tue, Jul 23, 2024 at 07:31:48PM -0400, David Finkel wrote:
...
> +	A write of the string "reset" to this file resets it to the
> +	current memory usage for subsequent reads through the same
> +	file descriptor.
> +	Attempts to write any other non-empty string will return EINVAL
> +	(modulo leading and trailing whitespace).

Let's just please do any write. We don't want to add complex write semantics
to these files. Writing anything to reset these files is an established
pattern and I don't think we gain anything by making this more complicated.

Thanks.

Hi Tejun,

On Fri, Jul 26, 2024 at 4:22 PM Tejun Heo <tj@kernel.org> wrote:
>
> Hello, David.
>
> On Tue, Jul 23, 2024 at 07:31:48PM -0400, David Finkel wrote:
> ...
> > +     A write of the string "reset" to this file resets it to the
> > +     current memory usage for subsequent reads through the same
> > +     file descriptor.
> > +     Attempts to write any other non-empty string will return EINVAL
> > +     (modulo leading and trailing whitespace).
>
> Let's just please do any write. We don't want to add complex write semantics
> to these files. Writing anything to reset these files is an established
> pattern and I don't think we gain anything by making this more complicated.

I still think something more limited is right here, but it seems that
there's consensus
that accepting all non-empty writes is the right option here, so I've
removed the check.
The next patchset will accept any (non-empty) write.

>
> Thanks.
>
> --
> tejun


Thanks,