[PATCH v3 0/3] mm: memcg: page counters optimizations

[PATCH v3 0/3] mm: memcg: page counters optimizations

[Roman Gushchin]

This patchset contains 3 independent small optimizations of page counters.

v3:
  - dropped the main part based on a feedback from Johannes
  - rebased on top of current mm-unstable
v2:
  - two page_counter structures per hugetlb cgroup instead of one
  - rebased to the current mm branch
  - many minor fixes and improvements
v1:
  https://lore.kernel.org/lkml/20240503201835.2969707-1-roman.gushchin@linux.dev/T/#m77151ed83451a49132e29ef13d55e08b95ac867f


Roman Gushchin (3):
  mm: memcg: don't call propagate_protected_usage() needlessly
  mm: page_counters: put page_counter_calculate_protection() under
    CONFIG_MEMCG
  mm: page_counters: initialize usage using ATOMIC_LONG_INIT() macro

 include/linux/page_counter.h | 16 ++++++++++++++--
 mm/hugetlb_cgroup.c          |  4 ++--
 mm/memcontrol.c              | 16 ++++++++--------
 mm/page_counter.c            | 18 +++++++++++++++---
 4 files changed, 39 insertions(+), 15 deletions(-)

-- 
2.46.0.rc1.232.g9752f9e123-goog

[PATCH v3 1/3] mm: memcg: don't call propagate_protected_usage() needlessly

[Roman Gushchin]

Memory protection (min/low) requires a constant tracking of
protected memory usage. propagate_protected_usage() is called
on each page counters update and does a number of operations
even in cases when the actual memory protection functionality
is not supported (e.g. hugetlb cgroups or memcg swap counters).

It's obviously inefficient and leads to a waste of CPU cycles.
It can be addressed by calling propagate_protected_usage() only
for the counters which do support memory guarantees. As of now
it's only memcg->memory - the unified memory memcg counter.

Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 include/linux/page_counter.h |  8 +++++++-
 mm/hugetlb_cgroup.c          |  4 ++--
 mm/memcontrol.c              | 16 ++++++++--------
 mm/page_counter.c            | 16 +++++++++++++---
 4 files changed, 30 insertions(+), 14 deletions(-)

diff --git a/include/linux/page_counter.h b/include/linux/page_counter.h
index e1295e3e1f19..aadd42f5ab7b 100644
--- a/include/linux/page_counter.h
+++ b/include/linux/page_counter.h
@@ -32,6 +32,7 @@ struct page_counter {
 	/* Keep all the read most fields in a separete cacheline. */
 	CACHELINE_PADDING(_pad2_);
 
+	bool protection_support;
 	unsigned long min;
 	unsigned long low;
 	unsigned long high;
@@ -45,12 +46,17 @@ struct page_counter {
 #define PAGE_COUNTER_MAX (LONG_MAX / PAGE_SIZE)
 #endif
 
+/*
+ * Protection is supported only for the first counter (with id 0).
+ */
 static inline void page_counter_init(struct page_counter *counter,
-				     struct page_counter *parent)
+				     struct page_counter *parent,
+				     bool protection_support)
 {
 	atomic_long_set(&counter->usage, 0);
 	counter->max = PAGE_COUNTER_MAX;
 	counter->parent = parent;
+	counter->protection_support = protection_support;
 }
 
 static inline unsigned long page_counter_read(struct page_counter *counter)
diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
index f443a56409a9..d8d0e665caed 100644
--- a/mm/hugetlb_cgroup.c
+++ b/mm/hugetlb_cgroup.c
@@ -114,10 +114,10 @@ static void hugetlb_cgroup_init(struct hugetlb_cgroup *h_cgroup,
 		}
 		page_counter_init(hugetlb_cgroup_counter_from_cgroup(h_cgroup,
 								     idx),
-				  fault_parent);
+				  fault_parent, false);
 		page_counter_init(
 			hugetlb_cgroup_counter_from_cgroup_rsvd(h_cgroup, idx),
-			rsvd_parent);
+			rsvd_parent, false);
 
 		limit = round_down(PAGE_COUNTER_MAX,
 				   pages_per_huge_page(&hstates[idx]));
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index eb92c21615eb..58169c34ae55 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3585,21 +3585,21 @@ mem_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
 	if (parent) {
 		WRITE_ONCE(memcg->swappiness, mem_cgroup_swappiness(parent));
 
-		page_counter_init(&memcg->memory, &parent->memory);
-		page_counter_init(&memcg->swap, &parent->swap);
+		page_counter_init(&memcg->memory, &parent->memory, true);
+		page_counter_init(&memcg->swap, &parent->swap, false);
 #ifdef CONFIG_MEMCG_V1
 		WRITE_ONCE(memcg->oom_kill_disable, READ_ONCE(parent->oom_kill_disable));
-		page_counter_init(&memcg->kmem, &parent->kmem);
-		page_counter_init(&memcg->tcpmem, &parent->tcpmem);
+		page_counter_init(&memcg->kmem, &parent->kmem, false);
+		page_counter_init(&memcg->tcpmem, &parent->tcpmem, false);
 #endif
 	} else {
 		init_memcg_stats();
 		init_memcg_events();
-		page_counter_init(&memcg->memory, NULL);
-		page_counter_init(&memcg->swap, NULL);
+		page_counter_init(&memcg->memory, NULL, true);
+		page_counter_init(&memcg->swap, NULL, false);
 #ifdef CONFIG_MEMCG_V1
-		page_counter_init(&memcg->kmem, NULL);
-		page_counter_init(&memcg->tcpmem, NULL);
+		page_counter_init(&memcg->kmem, NULL, false);
+		page_counter_init(&memcg->tcpmem, NULL, false);
 #endif
 		root_mem_cgroup = memcg;
 		return &memcg->css;
diff --git a/mm/page_counter.c b/mm/page_counter.c
index ad9bdde5d5d2..a54382a58ace 100644
--- a/mm/page_counter.c
+++ b/mm/page_counter.c
@@ -13,6 +13,11 @@
 #include <linux/bug.h>
 #include <asm/page.h>
 
+static bool track_protection(struct page_counter *c)
+{
+	return c->protection_support;
+}
+
 static void propagate_protected_usage(struct page_counter *c,
 				      unsigned long usage)
 {
@@ -57,7 +62,8 @@ void page_counter_cancel(struct page_counter *counter, unsigned long nr_pages)
 		new = 0;
 		atomic_long_set(&counter->usage, new);
 	}
-	propagate_protected_usage(counter, new);
+	if (track_protection(counter))
+		propagate_protected_usage(counter, new);
 }
 
 /**
@@ -70,12 +76,14 @@ void page_counter_cancel(struct page_counter *counter, unsigned long nr_pages)
 void page_counter_charge(struct page_counter *counter, unsigned long nr_pages)
 {
 	struct page_counter *c;
+	bool protection = track_protection(counter);
 
 	for (c = counter; c; c = c->parent) {
 		long new;
 
 		new = atomic_long_add_return(nr_pages, &c->usage);
-		propagate_protected_usage(c, new);
+		if (protection)
+			propagate_protected_usage(c, new);
 		/*
 		 * This is indeed racy, but we can live with some
 		 * inaccuracy in the watermark.
@@ -112,6 +120,7 @@ bool page_counter_try_charge(struct page_counter *counter,
 			     struct page_counter **fail)
 {
 	struct page_counter *c;
+	bool protection = track_protection(counter);
 
 	for (c = counter; c; c = c->parent) {
 		long new;
@@ -141,7 +150,8 @@ bool page_counter_try_charge(struct page_counter *counter,
 			*fail = c;
 			goto failed;
 		}
-		propagate_protected_usage(c, new);
+		if (protection)
+			propagate_protected_usage(c, new);
 
 		/* see comment on page_counter_charge */
 		if (new > READ_ONCE(c->local_watermark)) {
-- 
2.46.0.rc1.232.g9752f9e123-goog

[PATCH v3 2/3] mm: page_counters: put page_counter_calculate_protection() under CONFIG_MEMCG

[Roman Gushchin]

Put page_counter_calculate_protection() under CONFIG_MEMCG.

The protection functionality (min/low limits) is not supported by any
other cgroup subsystem, so page_counter_calculate_protection() and
related static effective_protection() can be compiled out if
CONFIG_MEMCG is not enabled.

Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 include/linux/page_counter.h | 6 ++++++
 mm/page_counter.c            | 2 ++
 2 files changed, 8 insertions(+)

diff --git a/include/linux/page_counter.h b/include/linux/page_counter.h
index aadd42f5ab7b..cf837d0f8ed1 100644
--- a/include/linux/page_counter.h
+++ b/include/linux/page_counter.h
@@ -95,8 +95,14 @@ static inline void page_counter_reset_watermark(struct page_counter *counter)
 	counter->watermark = usage;
 }
 
+#ifdef CONFIG_MEMCG
 void page_counter_calculate_protection(struct page_counter *root,
 				       struct page_counter *counter,
 				       bool recursive_protection);
+#else
+static inline void page_counter_calculate_protection(struct page_counter *root,
+						     struct page_counter *counter,
+						     bool recursive_protection) {}
+#endif
 
 #endif /* _LINUX_PAGE_COUNTER_H */
diff --git a/mm/page_counter.c b/mm/page_counter.c
index a54382a58ace..b249d15af9dd 100644
--- a/mm/page_counter.c
+++ b/mm/page_counter.c
@@ -288,6 +288,7 @@ int page_counter_memparse(const char *buf, const char *max,
 }
 
 
+#ifdef CONFIG_MEMCG
 /*
  * This function calculates an individual page counter's effective
  * protection which is derived from its own memory.min/low, its
@@ -459,3 +460,4 @@ void page_counter_calculate_protection(struct page_counter *root,
 			atomic_long_read(&parent->children_low_usage),
 			recursive_protection));
 }
+#endif /* CONFIG_MEMCG */
-- 
2.46.0.rc1.232.g9752f9e123-goog

[PATCH v3 3/3] mm: page_counters: initialize usage using ATOMIC_LONG_INIT() macro

[Roman Gushchin]

When a page_counter structure is initialized, there is no need to
use an atomic set operation to initialize the usage counter because
at this point the structure is not visible to anybody else.
ATOMIC_LONG_INIT() is what should be used in such cases.

Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
---
 include/linux/page_counter.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/linux/page_counter.h b/include/linux/page_counter.h
index cf837d0f8ed1..5da11392b382 100644
--- a/include/linux/page_counter.h
+++ b/include/linux/page_counter.h
@@ -53,7 +53,7 @@ static inline void page_counter_init(struct page_counter *counter,
 				     struct page_counter *parent,
 				     bool protection_support)
 {
-	atomic_long_set(&counter->usage, 0);
+	counter->usage = (atomic_long_t)ATOMIC_LONG_INIT(0);
 	counter->max = PAGE_COUNTER_MAX;
 	counter->parent = parent;
 	counter->protection_support = protection_support;
-- 
2.46.0.rc1.232.g9752f9e123-goog

Re: [PATCH v3 3/3] mm: page_counters: initialize usage using ATOMIC_LONG_INIT() macro

[Shakeel Butt]

On Fri, Jul 26, 2024 at 08:31:10PM GMT, Roman Gushchin wrote:
> When a page_counter structure is initialized, there is no need to
> use an atomic set operation to initialize the usage counter because
> at this point the structure is not visible to anybody else.
> ATOMIC_LONG_INIT() is what should be used in such cases.
> 
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>

Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

Re: [PATCH v3 1/3] mm: memcg: don't call propagate_protected_usage() needlessly

[Johannes Weiner]

On Fri, Jul 26, 2024 at 08:31:08PM +0000, Roman Gushchin wrote:
> Memory protection (min/low) requires a constant tracking of
> protected memory usage. propagate_protected_usage() is called
> on each page counters update and does a number of operations
> even in cases when the actual memory protection functionality
> is not supported (e.g. hugetlb cgroups or memcg swap counters).
> 
> It's obviously inefficient and leads to a waste of CPU cycles.
> It can be addressed by calling propagate_protected_usage() only
> for the counters which do support memory guarantees. As of now
> it's only memcg->memory - the unified memory memcg counter.
> 
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

Makes perfect sense.

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

> @@ -13,6 +13,11 @@
>  #include <linux/bug.h>
>  #include <asm/page.h>
>  
> +static bool track_protection(struct page_counter *c)
> +{
> +	return c->protection_support;
> +}

IMO it's a bit easier to follow without this. page_counter.c should be
able to access struct page_counter members directly :)

Re: [PATCH v3 2/3] mm: page_counters: put page_counter_calculate_protection() under CONFIG_MEMCG

[Johannes Weiner]

On Fri, Jul 26, 2024 at 08:31:09PM +0000, Roman Gushchin wrote:
> Put page_counter_calculate_protection() under CONFIG_MEMCG.
> 
> The protection functionality (min/low limits) is not supported by any
> other cgroup subsystem, so page_counter_calculate_protection() and
> related static effective_protection() can be compiled out if
> CONFIG_MEMCG is not enabled.
> 
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH v3 3/3] mm: page_counters: initialize usage using ATOMIC_LONG_INIT() macro

[Johannes Weiner]

On Fri, Jul 26, 2024 at 08:31:10PM +0000, Roman Gushchin wrote:
> When a page_counter structure is initialized, there is no need to
> use an atomic set operation to initialize the usage counter because
> at this point the structure is not visible to anybody else.
> ATOMIC_LONG_INIT() is what should be used in such cases.
> 
> Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> ---
>  include/linux/page_counter.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/linux/page_counter.h b/include/linux/page_counter.h
> index cf837d0f8ed1..5da11392b382 100644
> --- a/include/linux/page_counter.h
> +++ b/include/linux/page_counter.h
> @@ -53,7 +53,7 @@ static inline void page_counter_init(struct page_counter *counter,
>  				     struct page_counter *parent,
>  				     bool protection_support)
>  {
> -	atomic_long_set(&counter->usage, 0);
> +	counter->usage = (atomic_long_t)ATOMIC_LONG_INIT(0);

Pretty cool that ATOMIC_LONG_INIT() return value needs a cast to
atomic_long_t! ^_^

Acked-by: Johannes Weiner <hannes@cmpxchg.org>

Re: [PATCH v3 3/3] mm: page_counters: initialize usage using ATOMIC_LONG_INIT() macro

[Andrew Morton]

On Fri, 26 Jul 2024 19:11:10 -0400 Johannes Weiner <hannes@cmpxchg.org> wrote:

> On Fri, Jul 26, 2024 at 08:31:10PM +0000, Roman Gushchin wrote:
> > When a page_counter structure is initialized, there is no need to
> > use an atomic set operation to initialize the usage counter because
> > at this point the structure is not visible to anybody else.
> > ATOMIC_LONG_INIT() is what should be used in such cases.
> > 
> > Signed-off-by: Roman Gushchin <roman.gushchin@linux.dev>
> > ---
> >  include/linux/page_counter.h | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/include/linux/page_counter.h b/include/linux/page_counter.h
> > index cf837d0f8ed1..5da11392b382 100644
> > --- a/include/linux/page_counter.h
> > +++ b/include/linux/page_counter.h
> > @@ -53,7 +53,7 @@ static inline void page_counter_init(struct page_counter *counter,
> >  				     struct page_counter *parent,
> >  				     bool protection_support)
> >  {
> > -	atomic_long_set(&counter->usage, 0);
> > +	counter->usage = (atomic_long_t)ATOMIC_LONG_INIT(0);
> 
> Pretty cool that ATOMIC_LONG_INIT() return value needs a cast to
> atomic_long_t! ^_^

That's because this wicked patch passed in an `int'. 

	counter->usage = ATOMIC_LONG_INIT((atomic_long_t)0);