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

Hi David,

thanks for pursuing this! A couple of comments below.

On Mon, Jul 22, 2024 at 07:55:53PM -0400, David Finkel wrote:

@@ -1322,11 +1322,16 @@ PAGE_SIZE multiple when read back. reclaim induced by memory.reclaim. memory.peak

• A read-only single value file which exists on non-root
• cgroups.

• A read-write single value file which exists on non-root cgroups.
• The max memory usage recorded for the cgroup and its descendants since
• either the creation of the cgroup or the most recent reset for that FD.

• The max memory usage recorded for the cgroup and its
• descendants since the creation of the cgroup.

• 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).

Why not allow any write to reset? This makes it harder to use, and I'm not sure accidental writes are a likely mistake to make.

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h index 2150ca60394b..7001ed74e339 100644 --- a/include/linux/cgroup.h +++ b/include/linux/cgroup.h @@ -12,6 +12,7 @@ #include <linux/sched.h> #include <linux/cpumask.h> #include <linux/nodemask.h> +#include <linux/list.h> #include <linux/rculist.h> #include <linux/cgroupstats.h> #include <linux/fs.h> @@ -855,4 +856,11 @@ static inline void cgroup_bpf_put(struct cgroup *cgrp) {} struct cgroup *task_get_cgroup1(struct task_struct *tsk, int hierarchy_id); +struct memcg_peak_mem_ctx {

• long local_watermark;
• struct list_head peers;

+};

Since this is generic cgroup code, and can be conceivably used by other controllers, let's keep the naming generic as well. How about:

struct cgroup_of_peak { long value; struct list_head list; };

cgroup-defs.h would be a better place for it.

+struct memcg_peak_mem_ctx *memcg_extract_peak_mem_ctx(struct kernfs_open_file *of);

of_peak()

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h index 030d34e9d117..cbc390234605 100644 --- a/include/linux/memcontrol.h +++ b/include/linux/memcontrol.h @@ -198,6 +198,11 @@ struct mem_cgroup { struct page_counter kmem; /* v1 only */ struct page_counter tcpmem; /* v1 only */

• /* lists of memcg peak watching contexts on swap and memory */
• struct list_head peak_memory_local_watermark_watchers;
• struct list_head peak_swap_local_watermark_watchers;
• spinlock_t swap_memory_peak_watchers_lock;

These names are too long. How about:

/* Registered local usage peak watchers */ struct list_head memory_peaks; struct list_head swap_peaks; spinlock_t peaks_lock;

diff --git a/include/linux/page_counter.h b/include/linux/page_counter.h index 8cd858d912c4..06bb84218960 100644 --- a/include/linux/page_counter.h +++ b/include/linux/page_counter.h @@ -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 */ unsigned long failcnt;

/* Keep all the read most fields in a separete cacheline. */ @@ -78,7 +79,15 @@ 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 cur = page_counter_read(counter);

cur -> usage

@@ -6907,12 +6912,109 @@ 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)
  

+inline int swap_memory_peak_show(

• struct seq_file *sf, void *v, bool swap_cg)

{

Leave inlining to the compiler. Just static int.

The name can be simply peak_show().

Customary coding style is to line wrap at the last parameter that fits. Don't wrap if the line fits within 80 cols.

static int peak_show(struct seq_file *sf, void *v, ..., ...) { ... }

• struct cgroup_subsys_state *css = seq_css(sf); struct mem_cgroup *memcg = mem_cgroup_from_css(css);
• struct page_counter *pc;
• struct kernfs_open_file *of = sf->private;
• struct memcg_peak_mem_ctx *ctx = memcg_extract_peak_mem_ctx(of);
• s64 fd_peak = ctx->local_watermark;

• return (u64)memcg->memory.watermark * PAGE_SIZE;

• if (swap_cg)

• pc = &memcg->swap;
  

• else

• pc = &memcg->memory;
  

• if (fd_peak == -1) {

• seq_printf(sf, "%llu\n", (u64)pc->watermark * PAGE_SIZE);
  

• return 0;
  

• }
• s64 pc_peak = pc->local_watermark;
• s64 wm = fd_peak > pc_peak ? fd_peak : pc_peak;
• seq_printf(sf, "%lld\n", wm * PAGE_SIZE);
• return 0;

+}

As per Roman's feedback, don't mix decls and code.

You can simplify it by extracting css and memcg in the callers, then pass the right struct page counter *pc directly.

That should eliminate most local variables as well.

static int peak_show(struct seq_file *sf, void *v, struct page_counter *pc) { struct cgroup_of_peak *ofp = of_peak(sf->private); u64 peak;

/* User wants global or local peak? */ if (ofp->value == -1) peak = pc->watermark; else peak = max(ofp->value, pc->local_watermark);

seq_printf(sf, "%lld\n", peak * PAGE_SIZE); }

+static int memory_peak_show(struct seq_file *sf, void *v) +{

• return swap_memory_peak_show(sf, v, false);

And then do:

struct mem_cgroup *memcg = mem_cgroup_from_css(seq_css(sf));

return peak_show(sf, v, &memcg->memory);

Then do the same with ... &memcg->swap.

+inline ssize_t swap_memory_peak_write(

• struct kernfs_open_file *of,
• char *buf, size_t nbytes, loff_t off, bool swap_cg)

+{

Same feedback as above. Please don't inline explicitly (unless it really is measurably a performance improvement in a critical path), and stick to surrounding coding style.

Here too, pass page_counter directly and save the branches.

• unsigned long cur;
• struct memcg_peak_mem_ctx *peer_ctx;
• struct mem_cgroup *memcg = mem_cgroup_from_css(of_css(of));
• struct memcg_peak_mem_ctx *ctx = memcg_extract_peak_mem_ctx(of);
• struct page_counter *pc;
• struct list_head *watchers, *pos;
• buf = strstrip(buf);
• /* Only allow "reset" to keep the API clear */
• if (strcmp(buf, "reset"))

• return -EINVAL;
  

• if (swap_cg) {

• pc = &memcg->swap;
  

• watchers = &memcg->peak_swap_local_watermark_watchers;
  

• } else {

• pc = &memcg->memory;
  

• watchers = &memcg->peak_memory_local_watermark_watchers;
  

• }
• spin_lock(&memcg->swap_memory_peak_watchers_lock);
• page_counter_reset_local_watermark(pc);
• cur = pc->local_watermark;
• list_for_each(pos, watchers) {

list_for_each_entry()

• peer_ctx = list_entry(pos, typeof(*ctx), peers);
  

• if (cur > peer_ctx->local_watermark)
  

• 	peer_ctx->local_watermark = cur;
  

• }

I don't think this is quite right. local_peak could be higher than the current usage when a new watcher shows up. The other watchers should retain the higher local_peak, not the current usage.

• if (ctx->local_watermark == -1)

• /* only append to the list if we're not already there */
  

• list_add_tail(&ctx->peers, watchers);
  

• ctx->local_watermark = cur;

This makes me think that page_counter_reset_local_watermark() is not a good helper. It obscures what's going on. Try without it.

AFAICS the list ordering doesn't matter, so keep it simple and use a plain list_add().

/* * A new local peak is being tracked in pc->local_watermark. * Save current local peak in all watchers. */ list_for_each_entry(pos, ...) if (pc->local_watermark > pos->value) pos->value = pc->local_watermark;

pc->local_watermark = page_counter_read(pc);

/* Initital write, register watcher */ if (ofp->value == -1) list_add()

ofp->value = pc->local_watermark;

diff --git a/mm/page_counter.c b/mm/page_counter.c index db20d6452b71..724d31508664 100644 --- a/mm/page_counter.c +++ b/mm/page_counter.c @@ -79,9 +79,22 @@ void page_counter_charge(struct page_counter *counter, unsigned long nr_pages) /* * This is indeed racy, but we can live with some * inaccuracy in the watermark.

•  *
  

•  * Notably, we have two watermarks to allow for both a globally
  

•  * visible peak and one that can be reset at a smaller scope.
  

•  *
  

•  * Since we reset both watermarks when the global reset occurs,
  

•  * we can guarantee that watermark >= local_watermark, so we
  

•  * don't need to do both comparisons every time.
  

•  *
  

•  * On systems with branch predictors, the inner condition should
  

•  * be almost free.
  

  */

• if (new > READ_ONCE(c->watermark))
  

• 	WRITE_ONCE(c->watermark, new);
  

• if (new > READ_ONCE(c->local_watermark)) {
  

• 	WRITE_ONCE(c->local_watermark, new);
  

• 	if (new > READ_ONCE(c->watermark))
  

• 		WRITE_ONCE(c->watermark, new);
  

• }
  

  }

} @@ -131,10 +144,23 @@ bool page_counter_try_charge(struct page_counter *counter, propagate_protected_usage(c, new); /* * Just like with failcnt, we can live with some

•  * inaccuracy in the watermark.
  

•  * inaccuracy in the watermarks.
  

•  *
  

•  * Notably, we have two watermarks to allow for both a globally
  

•  * visible peak and one that can be reset at a smaller scope.
  

•  *
  

•  * Since we reset both watermarks when the global reset occurs,
  

•  * we can guarantee that watermark >= local_watermark, so we
  

•  * don't need to do both comparisons every time.
  

•  *
  

•  * On systems with branch predictors, the inner condition should
  

•  * be almost free.
  

/* See comment in page_counter_charge() */

diff --git a/tools/testing/selftests/cgroup/cgroup_util.c b/tools/testing/selftests/cgroup/cgroup_util.c index 432db923bced..1e2d46636a0c 100644 --- a/tools/testing/selftests/cgroup/cgroup_util.c +++ b/tools/testing/selftests/cgroup/cgroup_util.c @@ -141,6 +141,16 @@ long cg_read_long(const char *cgroup, const char *control) return atol(buf); }

This should be in patch #2.