Skip to contentSkip to conversation

This patchset reorganizes page_counter structures which helps to make

PATCH Superseded Roman Gushchin 5 participants 22 messages Jul 24, 2024 – Jul 26, 2024

Subsystems (4)
Memory management MM core memcg HugeTLB

v2 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Roman Gushchin

author maintainer
#0 lore raw

v2 cover This patchset reorganizes page_counter structures which helps to make

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/[email protected]/T/#m77151ed83451a49132e29ef13d55e08b95ac867f

Roman Gushchin (5):
mm: memcg: don't call propagate_protected_usage() needlessly
mm: page_counters: put page_counter_calculate_protection() under
CONFIG_MEMCG
mm: memcg: merge multiple page_counters into a single structure
mm: page_counters: initialize usage using ATOMIC_LONG_INIT() macro
mm: memcg: convert enum res_type to mem_counter_type

 include/linux/hugetlb.h        |   4 +-
 include/linux/hugetlb_cgroup.h |   8 +-
 include/linux/memcontrol.h     |  19 +--
 include/linux/page_counter.h   | 128 ++++++++++++++++----
 mm/hugetlb.c                   |  14 +--
 mm/hugetlb_cgroup.c            | 150 +++++++++--------------
 mm/memcontrol-v1.c             | 154 ++++++++++--------------
 mm/memcontrol-v1.h             |  10 +-
 mm/memcontrol.c                | 211 ++++++++++++++++-----------------
 mm/page_counter.c              |  94 +++++++++------
 10 files changed, 403 insertions(+), 389 deletions(-)
Signature · 1 line
2.46.0.rc1.232.g9752f9e123-goog
author maintainer
#1 lore raw

PATCH v2 1/5 mm: memcg: don't call propagate_protected_usage() needlessly

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 <[email protected]>
---
 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(-)
include/linux/page_counter.h +7 -1
index 860f313182e7..b31fd5b208aa 100644
@@ -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)
mm/hugetlb_cgroup.c +2 -2
index f443a56409a9..d8d0e665caed 100644
@@ -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]));
mm/memcontrol.c +8 -8
index 87fa448b731f..45c0f816a974 100644
@@ -3579,21 +3579,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;
mm/page_counter.c +13 -3
index ad9bdde5d5d2..a54382a58ace 100644
@@ -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)) {
Signature · 1 line
2.46.0.rc1.232.g9752f9e123-goog
maintainer Acked-by on v2 1/5
#2 lore raw

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

Quoted 6 lines · 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).

Quoted 6 lines

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 <[email protected]>
Acked-by Shakeel Butt <[email protected]>
#3 lore raw

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

In reply to Roman Gushchin
Quoted 33 lines · 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 <[email protected]>
---
 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(-)
include/linux/page_counter.h +1 -0
index 860f313182e7..b31fd5b208aa 100644
@@ -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;

include/linux/page_counter.h struct page_counter { · line 54

Quoted 10 lines
include/linux/page_counter.h +5 -1
@@ -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)

Would it be better to make this an internal helper (e.g. __page_counter_init()), and add another API function that passes in protection_support=true, for example:

static inline void page_counter_init_protected(..)

{
       __page_counter_init(.., true);
}

This will get rid of the naked booleans at the callsites of page_counter_init(), which are more difficult to interpret. It will also reduce the diff as we only need to change the page_counter_init() calls of memcg->memory.

WDYT?

Quoted 6 lines
include/linux/page_counter.h +1 -0
 {
        atomic_long_set(&counter->usage, 0);
        counter->max = PAGE_COUNTER_MAX;
        counter->parent = parent;
+       counter->protection_support = protection_support;
 }

[..]

author maintainer
#4 lore raw

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

Quoted 54 lines · Yosry Ahmed
Quoted 33 lines · 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 <[email protected]>
---
 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(-)
include/linux/page_counter.h +1 -0
index 860f313182e7..b31fd5b208aa 100644
@@ -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;

include/linux/page_counter.h struct page_counter { · line 54

Quoted 10 lines
include/linux/page_counter.h +5 -1
@@ -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)

Would it be better to make this an internal helper (e.g. __page_counter_init()), and add another API function that passes in protection_support=true, for example:

static inline void page_counter_init_protected(..)

{
       __page_counter_init(.., true);
}
Quoted 6 lines

This will get rid of the naked booleans at the callsites of page_counter_init(), which are more difficult to interpret. It will also reduce the diff as we only need to change the page_counter_init() calls of memcg->memory.

WDYT?

No strong opinion here. There are basically 2 call sites and I don't expect this number to grow, so not sure if it makes sense to add 2 new helpers.

Another option I thought about is to leave page_counter_init() as it is and add a separate function to enable the protection tracking.

Thanks!

#5 lore raw

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

Quoted 61 lines · Roman Gushchin
Quoted 59 lines · Yosry Ahmed
Quoted 33 lines · 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 <[email protected]>
---
 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(-)
include/linux/page_counter.h +1 -0
index 860f313182e7..b31fd5b208aa 100644
@@ -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;

include/linux/page_counter.h struct page_counter { · line 54

Quoted 10 lines
include/linux/page_counter.h +5 -1
@@ -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)

Would it be better to make this an internal helper (e.g. __page_counter_init()), and add another API function that passes in protection_support=true, for example:

static inline void page_counter_init_protected(..)

{
       __page_counter_init(.., true);
}

This will get rid of the naked booleans at the callsites of page_counter_init(), which are more difficult to interpret. It will also reduce the diff as we only need to change the page_counter_init() calls of memcg->memory.

Quoted 7 lines
Quoted 1 line

WDYT?

No strong opinion here. There are basically 2 call sites and I don't expect this number to grow, so not sure if it makes sense to add 2 new helpers.

Another option I thought about is to leave page_counter_init() as it is and add a separate function to enable the protection tracking.

Yeah this should work too, in fact it should give us a chance to choose a more descriptive name that page_counter_init_protected() (e.g. page_counter_enable_protection()).

My main concern is the naked booleans, and secondarily the unnecessary diff to other init sites. I also don't feel strongly about it though.

Quoted 2 lines

Thanks!

author maintainer
#6 lore raw

PATCH v2 2/5 mm: page_counters: put page_counter_calculate_protection() under CONFIG_MEMCG

In reply to 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 <[email protected]>
---
 include/linux/page_counter.h | 6 ++++++
 mm/page_counter.c            | 2 ++
 2 files changed, 8 insertions(+)
include/linux/page_counter.h +6 -0
index b31fd5b208aa..d85fc7df3280 100644
@@ -91,8 +91,14 @@ static inline void page_counter_reset_watermark(struct page_counter *counter)
 	counter->local_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 */
mm/page_counter.c +2 -0
index a54382a58ace..b249d15af9dd 100644
@@ -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 */
Signature · 1 line
2.46.0.rc1.232.g9752f9e123-goog
maintainer Acked-by on v2 2/5
#7 lore raw

Re: [PATCH v2 2/5] mm: page_counters: put page_counter_calculate_protection() under CONFIG_MEMCG

Quoted 8 lines · 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 <[email protected]>
Acked-by Shakeel Butt <[email protected]>
author maintainer
#8 lore raw

PATCH v2 3/5 mm: memcg: merge multiple page_counters into a single structure

In reply to Roman Gushchin

Struct page_counter is used by memory and hugetlb cgroups to track the resource usage. Memory cgroups are using up to four different counters (on cgroup v1). Hugetlb cgroups are using 2 * HUGE_MAX_HSTATE counters.

Every page_counter structure (except the one belonging to the root cgroup) contains a pointer to the corresponding parent structure. It's obviously wasteful because for each cgroup there are several similar chains of page_counter structures. It's not particularly cache-effective too.

Also every page_counter structure contains fields required for tracking the usage of the memory protection, while it's used only for the main ("memory") counter and only by memory cgroups.

To address these issues let's make page_counter structures to support N counters and use a single page_counter structure for a memory cgroup and 2 page_counter structures for a hugetlb cgroup (to track the actual usage and reservations).

N is determined at the compile time depending on the configuration and HUGE_MAX_HSTATE value. HUGE_MAX_HSTATE is relatively small on all existing platforms (max is 5 on sparc), so it's not too far from 4 memcg memory types.

Memory min/low functionality is supported only for the first counter.

Memory savings (on x86_64):

|-----------------------------+----------------------------+----------------------------|
|                             | memcg                      | hugetlb                    |
|-----------------------------+----------------------------+----------------------------|
| original, CONFIG_MEMCG_V1=n | size: 1600, cachelines: 25 | size: 1280, cachelines: 20 |
| patched, CONFIG_MEMCG_V1=n  | size: 1472, cachelines: 23 | size: 1024, cachelines: 16 |
|-----------------------------+----------------------------+----------------------------|
| original, CONFIG_MEMCG_V1=y | size: 2176, cachelines: 34 | size: 1280, cachelines: 20 |
| patched, CONFIG_MEMCG_V1=y  | size: 1664, cachelines: 26 | size: 1152, cachelines: 18 |
|-----------------------------+----------------------------+----------------------------|

So it's 10-35% reduction in the number of cache lines depending on the kernel configuration.

Signed-off-by Roman Gushchin <[email protected]>
---
 include/linux/hugetlb.h        |   4 +-
 include/linux/hugetlb_cgroup.h |   8 +-
 include/linux/memcontrol.h     |  19 +--
 include/linux/page_counter.h   | 114 ++++++++++++++----
 mm/hugetlb.c                   |  14 +--
 mm/hugetlb_cgroup.c            | 150 +++++++++---------------
 mm/memcontrol-v1.c             |  88 +++++---------
 mm/memcontrol-v1.h             |   2 +-
 mm/memcontrol.c                | 207 ++++++++++++++++-----------------
 mm/page_counter.c              |  86 ++++++++------
 10 files changed, 345 insertions(+), 347 deletions(-)
include/linux/hugetlb.h +2 -2
index c9bf68c239a0..ac27331dcae6 100644
@@ -61,7 +61,7 @@ struct resv_map {
 	 * here. If these fields are 0, then either the mapping is shared, or
 	 * cgroup accounting is disabled for this resv_map.
 	 */
-	struct page_counter *reservation_counter;
+	unsigned long reservation_idx;
 	unsigned long pages_per_hpage;
 	struct cgroup_subsys_state *css;
 #endif

@@ -96,7 +96,7 @@ struct file_region {
 	 * file_region in resv_map. These fields hold the info needed to
 	 * uncharge each reservation.
 	 */
-	struct page_counter *reservation_counter;
+	unsigned long reservation_idx;
 	struct cgroup_subsys_state *css;
 #endif
 };
include/linux/hugetlb_cgroup.h +4 -4
index e5d64b8b59c2..1cc89a50be7d 100644
@@ -36,14 +36,14 @@ struct hugetlb_cgroup {
 	struct cgroup_subsys_state css;
 
 	/*
-	 * the counter to account for hugepages from hugetlb.
+	 * counters to account for hugepages from hugetlb.
 	 */
-	struct page_counter hugepage[HUGE_MAX_HSTATE];
+	struct page_counter usage;
 
 	/*
-	 * the counter to account for hugepage reservations from hugetlb.
+	 * counters to account for hugepage reservations from hugetlb.
 	 */
-	struct page_counter rsvd_hugepage[HUGE_MAX_HSTATE];
+	struct page_counter rsvd;
 
 	atomic_long_t events[HUGE_MAX_HSTATE][HUGETLB_NR_MEMORY_EVENTS];
 	atomic_long_t events_local[HUGE_MAX_HSTATE][HUGETLB_NR_MEMORY_EVENTS];
include/linux/memcontrol.h +5 -14
index 1b79760af685..90eda0b577fa 100644
@@ -186,12 +186,7 @@ struct mem_cgroup {
 	struct mem_cgroup_id id;
 
 	/* Accounted resources */
-	struct page_counter memory;		/* Both v1 & v2 */
-
-	union {
-		struct page_counter swap;	/* v2 only */
-		struct page_counter memsw;	/* v1 only */
-	};
+	struct page_counter memory;
 
 	/* registered local peak watchers */
 	struct list_head memory_peaks;

@@ -271,10 +266,6 @@ struct mem_cgroup {
 #endif
 
 #ifdef CONFIG_MEMCG_V1
-	/* Legacy consumer-oriented counters */
-	struct page_counter kmem;		/* v1 only */
-	struct page_counter tcpmem;		/* v1 only */
-
 	unsigned long soft_limit;
 
 	/* protected by memcg_oom_lock */

@@ -638,7 +629,7 @@ static inline bool mem_cgroup_below_low(struct mem_cgroup *target,
 		return false;
 
 	return READ_ONCE(memcg->memory.elow) >=
-		page_counter_read(&memcg->memory);
+		page_counter_read(&memcg->memory, MCT_MEM);
 }
 
 static inline bool mem_cgroup_below_min(struct mem_cgroup *target,

@@ -648,7 +639,7 @@ static inline bool mem_cgroup_below_min(struct mem_cgroup *target,
 		return false;
 
 	return READ_ONCE(memcg->memory.emin) >=
-		page_counter_read(&memcg->memory);
+		page_counter_read(&memcg->memory, MCT_MEM);
 }
 
 void mem_cgroup_commit_charge(struct folio *folio, struct mem_cgroup *memcg);

@@ -824,8 +815,8 @@ static inline void mem_cgroup_put(struct mem_cgroup *memcg)
 		css_put(&memcg->css);
 }
 
-#define mem_cgroup_from_counter(counter, member)	\
-	container_of(counter, struct mem_cgroup, member)
+#define mem_cgroup_from_counter(counter)	\
+	container_of(counter, struct mem_cgroup, memory)
 
 struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *,
 				   struct mem_cgroup *,
include/linux/page_counter.h +93 -21

166 lines not on this page · raw message

mm/hugetlb.c +6 -8
index 0858a1827207..9ada48757bd8 100644
@@ -455,7 +455,7 @@ static void copy_hugetlb_cgroup_uncharge_info(struct file_region *nrg,
 					      struct file_region *rg)
 {
 #ifdef CONFIG_CGROUP_HUGETLB
-	nrg->reservation_counter = rg->reservation_counter;
+	nrg->reservation_idx = rg->reservation_idx;
 	nrg->css = rg->css;
 	if (rg->css)
 		css_get(rg->css);

@@ -470,8 +470,7 @@ static void record_hugetlb_cgroup_uncharge_info(struct hugetlb_cgroup *h_cg,
 {
 #ifdef CONFIG_CGROUP_HUGETLB
 	if (h_cg) {
-		nrg->reservation_counter =
-			&h_cg->rsvd_hugepage[hstate_index(h)];
+		nrg->reservation_idx = hstate_index(h);
 		nrg->css = &h_cg->css;
 		/*
 		 * The caller will hold exactly one h_cg->css reference for the

@@ -491,7 +490,7 @@ static void record_hugetlb_cgroup_uncharge_info(struct hugetlb_cgroup *h_cg,
 		 */
 		VM_BUG_ON(resv->pages_per_hpage != pages_per_huge_page(h));
 	} else {
-		nrg->reservation_counter = NULL;
+		nrg->reservation_idx = 0;
 		nrg->css = NULL;
 	}
 #endif

@@ -509,7 +508,7 @@ static bool has_same_uncharge_info(struct file_region *rg,
 				   struct file_region *org)
 {
 #ifdef CONFIG_CGROUP_HUGETLB
-	return rg->reservation_counter == org->reservation_counter &&
+	return rg->reservation_idx == org->reservation_idx &&
 	       rg->css == org->css;
 
 #else

@@ -1068,12 +1067,11 @@ resv_map_set_hugetlb_cgroup_uncharge_info(struct resv_map *resv_map,
 {
 #ifdef CONFIG_CGROUP_HUGETLB
 	if (!h_cg || !h) {
-		resv_map->reservation_counter = NULL;
+		resv_map->reservation_idx = 0;
 		resv_map->pages_per_hpage = 0;
 		resv_map->css = NULL;
 	} else {
-		resv_map->reservation_counter =
-			&h_cg->rsvd_hugepage[hstate_index(h)];
+		resv_map->reservation_idx = hstate_index(h);
 		resv_map->pages_per_hpage = pages_per_huge_page(h);
 		resv_map->css = &h_cg->css;
 	}
mm/hugetlb_cgroup.c +56 -94

300 lines not on this page · raw message

mm/memcontrol-v1.c +28 -60

185 lines not on this page · raw message

mm/memcontrol-v1.h +1 -1
index 56d7eaa98274..9a4d8205aed5 100644
@@ -112,7 +112,7 @@ bool memcg1_charge_skmem(struct mem_cgroup *memcg, unsigned int nr_pages,
 			 gfp_t gfp_mask);
 static inline void memcg1_uncharge_skmem(struct mem_cgroup *memcg, unsigned int nr_pages)
 {
-	page_counter_uncharge(&memcg->tcpmem, nr_pages);
+	page_counter_uncharge(&memcg->memory, MCT_TCPMEM, nr_pages);
 }
 
 extern struct cftype memsw_files[];
mm/memcontrol.c +100 -107

525 lines not on this page · raw message

mm/page_counter.c +50 -36

249 lines not on this page · raw message

Signature · 1 line
2.46.0.rc1.232.g9752f9e123-goog
maintainer Acked-by on v2 3/5
#9 lore raw

Re: [PATCH v2 3/5] mm: memcg: merge multiple page_counters into a single structure

Quoted 39 lines · Roman Gushchin

Struct page_counter is used by memory and hugetlb cgroups to track the resource usage. Memory cgroups are using up to four different counters (on cgroup v1). Hugetlb cgroups are using 2 * HUGE_MAX_HSTATE counters.

Every page_counter structure (except the one belonging to the root cgroup) contains a pointer to the corresponding parent structure. It's obviously wasteful because for each cgroup there are several similar chains of page_counter structures. It's not particularly cache-effective too.

Also every page_counter structure contains fields required for tracking the usage of the memory protection, while it's used only for the main ("memory") counter and only by memory cgroups.

To address these issues let's make page_counter structures to support N counters and use a single page_counter structure for a memory cgroup and 2 page_counter structures for a hugetlb cgroup (to track the actual usage and reservations).

N is determined at the compile time depending on the configuration and HUGE_MAX_HSTATE value. HUGE_MAX_HSTATE is relatively small on all existing platforms (max is 5 on sparc), so it's not too far from 4 memcg memory types.

Memory min/low functionality is supported only for the first counter.

Memory savings (on x86_64):

|-----------------------------+----------------------------+----------------------------|
|                             | memcg                      | hugetlb                    |
|-----------------------------+----------------------------+----------------------------|
| original, CONFIG_MEMCG_V1=n | size: 1600, cachelines: 25 | size: 1280, cachelines: 20 |
| patched, CONFIG_MEMCG_V1=n  | size: 1472, cachelines: 23 | size: 1024, cachelines: 16 |
|-----------------------------+----------------------------+----------------------------|
| original, CONFIG_MEMCG_V1=y | size: 2176, cachelines: 34 | size: 1280, cachelines: 20 |
| patched, CONFIG_MEMCG_V1=y  | size: 1664, cachelines: 26 | size: 1152, cachelines: 18 |
|-----------------------------+----------------------------+----------------------------|
Quoted 4 lines

So it's 10-35% reduction in the number of cache lines depending on the kernel configuration.

Signed-off-by Roman Gushchin <[email protected]>
Acked-by Shakeel Butt <[email protected]>
maintainer
#10 lore raw

Re: [PATCH v2 3/5] mm: memcg: merge multiple page_counters into a single structure

In reply to Roman Gushchin
Quoted 89 lines · Roman Gushchin
include/linux/page_counter.h +61 -4
@@ -5,14 +5,71 @@
 #include <linux/atomic.h>
 #include <linux/cache.h>
 #include <linux/limits.h>
+#include <linux/mm_types.h>
 #include <asm/page.h>
 
+/*
+ * Page counters are used by memory and hugetlb cgroups.
+ * Memory cgroups are using up to 4 separate counters:
+ * memory, swap (memory + swap on cgroup v1), kmem and tcpmem.
+ * Hugetlb cgroups are using 2 arrays of counters with HUGE_MAX_HSTATE
+ * in each to track the usage and reservations of each supported
+ * hugepage size.
+ *
+ * Protection (min/low) is supported only for the first counter
+ * with idx 0 and only if the counter was initialized with the protection
+ * support.
+ */
+
+enum mem_counter_type {
+#ifdef CONFIG_MEMCG
+	/* Unified memory counter */
+	MCT_MEM,
+
+	/* Swap */
+	MCT_SWAP,
+
+	/* Memory + swap */
+	MCT_MEMSW = MCT_SWAP,
+
+#ifdef CONFIG_MEMCG_V1
+	/* Kernel memory */
+	MCT_KMEM,
+
+	/* Tcp memory */
+	MCT_TCPMEM,
+#endif /* CONFIG_MEMCG_V1 */
+#endif /* CONFIG_MEMCG */
+
+	/* Maximum number of memcg counters */
+	MCT_NR_MEMCG_ITEMS,
+};
+
+#ifdef CONFIG_CGROUP_HUGETLB
+#ifdef HUGE_MAX_HSTATE
+#define MCT_NR_HUGETLB_ITEMS HUGE_MAX_HSTATE
+#else
+#define MCT_NR_HUGETLB_ITEMS 1
+#endif
+
+/*
+ * max() can't be used here: even though __builtin_choose_expr() evaluates
+ * to true, the false clause generates a compiler error:
+ * error: braced-group within expression allowed only inside a function .
+ */
+#define MCT_NR_ITEMS (__builtin_choose_expr(MCT_NR_MEMCG_ITEMS > MCT_NR_HUGETLB_ITEMS, \
+					    MCT_NR_MEMCG_ITEMS, MCT_NR_HUGETLB_ITEMS))
+
+#else /* CONFIG_CGROUP_HUGETLB */
+#define MCT_NR_ITEMS MCT_NR_MEMCG_ITEMS
+#endif /* CONFIG_CGROUP_HUGETLB */
+
 struct page_counter {
 	/*
 	 * Make sure 'usage' does not share cacheline with any other field. The
 	 * memcg->memory.usage is a hot member of struct mem_cgroup.
 	 */
-	atomic_long_t usage;
+	atomic_long_t usage[MCT_NR_ITEMS];
 	CACHELINE_PADDING(_pad1_);
 
 	/* effective memory.min and memory.min usage tracking */

@@ -25,9 +82,9 @@ struct page_counter {
 	atomic_long_t low_usage;
 	atomic_long_t children_low_usage;
 
-	unsigned long watermark;
-	unsigned long local_watermark; /* track min of fd-local resets */
-	unsigned long failcnt;
+	unsigned long watermark[MCT_NR_ITEMS];
+	unsigned long local_watermark[MCT_NR_ITEMS]; /* track min of fd-local resets */
+	unsigned long failcnt[MCT_NR_ITEMS];
 
 	/* Keep all the read most fields in a separete cacheline. */
 	CACHELINE_PADDING(_pad2_);

include/linux/page_counter.h struct page_counter { · line 99

Quoted 10 lines
include/linux/page_counter.h +3 -2
@@ -35,8 +92,9 @@ struct page_counter {
 	bool protection_support;
 	unsigned long min;
 	unsigned long low;
-	unsigned long high;
-	unsigned long max;
+	unsigned long high[MCT_NR_ITEMS];
+	unsigned long max[MCT_NR_ITEMS];
+
 	struct page_counter *parent;
 } ____cacheline_internodealigned_in_smp;

This hardcodes way too much user specifics into what should be a self-contained piece of abstraction. Should anybody else try to use the 'struct page_counter' for a single resource elsewhere, they'd end up with up to 5 counters, watermarks, failcnts, highs and maxs etc.

It's clear that the hierarchical aspect of the page_counter no longer works. It was okay-ish when all we had was a simple hard limit. Now we carry to much stuff in it that is only useful to a small set of users.

Instead of doubling down and repeating the mistakes we made for struct page, it would be better to move user specific stuff out of it entirely. I think there are two patterns that would be more natural:
a) pass a callback function and have a priv pointer in page_counter for user-specifics; or b) remove the hierarchy aspect from the page counter entirely, let the *caller* walk the tree, call the page counter code to trycharge (and log watermark) only that level, and handle everything caller specific on the caller side.

All users already have parent linkage in the css, so this would eliminate the parent pointer in page_counter altogether.

The protection stuff could be moved to memcg proper, eliminating yet more fields that aren't interesting to any other user.

You'd save more memory, while at the same time keeping struct page_counter clean and generic.

author maintainer
#11 lore raw

Re: [PATCH v2 3/5] mm: memcg: merge multiple page_counters into a single structure

Quoted 118 lines · Johannes Weiner
Quoted 89 lines · Roman Gushchin
include/linux/page_counter.h +61 -4
@@ -5,14 +5,71 @@
 #include <linux/atomic.h>
 #include <linux/cache.h>
 #include <linux/limits.h>
+#include <linux/mm_types.h>
 #include <asm/page.h>
 
+/*
+ * Page counters are used by memory and hugetlb cgroups.
+ * Memory cgroups are using up to 4 separate counters:
+ * memory, swap (memory + swap on cgroup v1), kmem and tcpmem.
+ * Hugetlb cgroups are using 2 arrays of counters with HUGE_MAX_HSTATE
+ * in each to track the usage and reservations of each supported
+ * hugepage size.
+ *
+ * Protection (min/low) is supported only for the first counter
+ * with idx 0 and only if the counter was initialized with the protection
+ * support.
+ */
+
+enum mem_counter_type {
+#ifdef CONFIG_MEMCG
+	/* Unified memory counter */
+	MCT_MEM,
+
+	/* Swap */
+	MCT_SWAP,
+
+	/* Memory + swap */
+	MCT_MEMSW = MCT_SWAP,
+
+#ifdef CONFIG_MEMCG_V1
+	/* Kernel memory */
+	MCT_KMEM,
+
+	/* Tcp memory */
+	MCT_TCPMEM,
+#endif /* CONFIG_MEMCG_V1 */
+#endif /* CONFIG_MEMCG */
+
+	/* Maximum number of memcg counters */
+	MCT_NR_MEMCG_ITEMS,
+};
+
+#ifdef CONFIG_CGROUP_HUGETLB
+#ifdef HUGE_MAX_HSTATE
+#define MCT_NR_HUGETLB_ITEMS HUGE_MAX_HSTATE
+#else
+#define MCT_NR_HUGETLB_ITEMS 1
+#endif
+
+/*
+ * max() can't be used here: even though __builtin_choose_expr() evaluates
+ * to true, the false clause generates a compiler error:
+ * error: braced-group within expression allowed only inside a function .
+ */
+#define MCT_NR_ITEMS (__builtin_choose_expr(MCT_NR_MEMCG_ITEMS > MCT_NR_HUGETLB_ITEMS, \
+					    MCT_NR_MEMCG_ITEMS, MCT_NR_HUGETLB_ITEMS))
+
+#else /* CONFIG_CGROUP_HUGETLB */
+#define MCT_NR_ITEMS MCT_NR_MEMCG_ITEMS
+#endif /* CONFIG_CGROUP_HUGETLB */
+
 struct page_counter {
 	/*
 	 * Make sure 'usage' does not share cacheline with any other field. The
 	 * memcg->memory.usage is a hot member of struct mem_cgroup.
 	 */
-	atomic_long_t usage;
+	atomic_long_t usage[MCT_NR_ITEMS];
 	CACHELINE_PADDING(_pad1_);
 
 	/* effective memory.min and memory.min usage tracking */

@@ -25,9 +82,9 @@ struct page_counter {
 	atomic_long_t low_usage;
 	atomic_long_t children_low_usage;
 
-	unsigned long watermark;
-	unsigned long local_watermark; /* track min of fd-local resets */
-	unsigned long failcnt;
+	unsigned long watermark[MCT_NR_ITEMS];
+	unsigned long local_watermark[MCT_NR_ITEMS]; /* track min of fd-local resets */
+	unsigned long failcnt[MCT_NR_ITEMS];
 
 	/* Keep all the read most fields in a separete cacheline. */
 	CACHELINE_PADDING(_pad2_);

include/linux/page_counter.h struct page_counter { · line 99

Quoted 10 lines
include/linux/page_counter.h +3 -2
@@ -35,8 +92,9 @@ struct page_counter {
 	bool protection_support;
 	unsigned long min;
 	unsigned long low;
-	unsigned long high;
-	unsigned long max;
+	unsigned long high[MCT_NR_ITEMS];
+	unsigned long max[MCT_NR_ITEMS];
+
 	struct page_counter *parent;
 } ____cacheline_internodealigned_in_smp;

This hardcodes way too much user specifics into what should be a self-contained piece of abstraction. Should anybody else try to use the 'struct page_counter' for a single resource elsewhere, they'd end up with up to 5 counters, watermarks, failcnts, highs and maxs etc.

It's clear that the hierarchical aspect of the page_counter no longer works. It was okay-ish when all we had was a simple hard limit. Now we carry to much stuff in it that is only useful to a small set of users. Instead of doubling down and repeating the mistakes we made for struct page, it would be better to move user specific stuff out of it entirely. I think there are two patterns that would be more natural:
a) pass a callback function and have a priv pointer in page_counter for user-specifics; or b) remove the hierarchy aspect from the page counter entirely, let the *caller* walk the tree, call the page counter code to trycharge (and log watermark) only that level, and handle everything caller specific on the caller side.

Quoted 5 lines

All users already have parent linkage in the css, so this would eliminate the parent pointer in page_counter altogether.

The protection stuff could be moved to memcg proper, eliminating yet more fields that aren't interesting to any other user.

Hm, Idk, I do agree with what you're saying about the self-contained piece of abstraction (and I had very similar thoughts in the process), but there are also some complications.

First, funny enough, the protection calculation code was just moved to mm/page_counter.c by a8585ac68621 ("mm/page_counter: move calculating protection values to page_counter"). The commit log says that it's gonna be used by the drm cgroup controller, but the code is not (yet?) upstream apparently. I don't have any insights here. If there will be the second user for the protection functionality, moving it back to memcontrol.c is not feasible.

Second, I agree that it would be nice to get rid of the parent pointer in struct page_cgroup entirely and use one in css. But Idk how to do it without making the code way more messy or duplicate a lot of tree walks. In C++ (or another language with generics) we could make struct page_counter taking the number of counters and the set of features as template parameters.

I feel like with memcg1 code being factored out the benefit of this reorg lowered, so how about me resending 2 first patches separately and spending more time on thinking on the best long-term strategy? I have some ideas here and I thought about this work as a preparatory step, but maybe you're right and it makes sense to approach it more comprehensively.

Thanks!

maintainer
#12 lore raw

Re: [PATCH v2 3/5] mm: memcg: merge multiple page_counters into a single structure

Quoted 10 lines · Roman Gushchin

Hm, Idk, I do agree with what you're saying about the self-contained piece of abstraction (and I had very similar thoughts in the process), but there are also some complications.

First, funny enough, the protection calculation code was just moved to mm/page_counter.c by a8585ac68621 ("mm/page_counter: move calculating protection values to page_counter"). The commit log says that it's gonna be used by the drm cgroup controller, but the code is not (yet?) upstream apparently. I don't have any insights here. If there will be the second user for the protection functionality, moving it back to memcontrol.c is not feasible.

If it comes to that, I think it can be its own abstraction as well, with a struct containing all the elow, low_usage, children_low_usage stuff etc. and API functions to propagate and get effective protection.

Both memcg and drm can embed this into their css descriptors and use those helpers as necessary.

Quoted 5 lines

Second, I agree that it would be nice to get rid of the parent pointer in struct page_cgroup entirely and use one in css. But Idk how to do it without making the code way more messy or duplicate a lot of tree walks. In C++ (or another language with generics) we could make struct page_counter taking the number of counters and the set of features as template parameters.

It shouldn't be more than a for loop right + the unwind on failure, right? *Somewhat* duplicative, but pretty simple code that's easy to wrap in a controller function and have it out of the way.

Actually, we could keep a simple hierarchical version of the page counter functions, but expose non-hiearchical ones for users that want to do additional operations on each level.

IOW, e.g. page_counter_charge_one(), page_counter_try_charge_one() etc, then implement page_counter_charge() et al on top of these.

swap, memsw, kmem, tcpmem, hugetlb etc. could remain unchanged. The memory counter could have a memcg version of the charge function that uses *charge_one() and handles protection propagation.

drm could do the same.

That would keep the page_counter->parent pointer, but would save a bit of complexity for simple users at least.

Quoted 5 lines

I feel like with memcg1 code being factored out the benefit of this reorg lowered, so how about me resending 2 first patches separately and spending more time on thinking on the best long-term strategy? I have some ideas here and I thought about this work as a preparatory step, but maybe you're right and it makes sense to approach it more comprehensively.

Sounds good to me.

Thanks!

author maintainer
#13 lore raw

PATCH v2 4/5 mm: page_counters: initialize usage using ATOMIC_LONG_INIT() macro

In reply to Roman Gushchin

When a page_counter structure is initialized, there is no need to use an atomic set operation to initialize usage counters 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 <[email protected]>
---
 include/linux/page_counter.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
include/linux/page_counter.h +1 -1
index 701e1255c456..ee740ed8cb90 100644
@@ -114,7 +114,7 @@ static inline void page_counter_init(struct page_counter *counter,
 	unsigned long i;
 
 	for (i = 0; i < MCT_NR_ITEMS; i++) {
-		atomic_long_set(&counter->usage[i], 0);
+		counter->usage[i] = (atomic_long_t)ATOMIC_LONG_INIT(0);
 		counter->max[i] = PAGE_COUNTER_MAX;
 	}
 	counter->parent = parent;
Signature · 1 line
2.46.0.rc1.232.g9752f9e123-goog
maintainer Acked-by on v2 4/5
#14 lore raw

Re: [PATCH v2 4/5] mm: page_counters: initialize usage using ATOMIC_LONG_INIT() macro

Quoted 6 lines · Roman Gushchin

When a page_counter structure is initialized, there is no need to use an atomic set operation to initialize usage counters 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 <[email protected]>
Acked-by Shakeel Butt <[email protected]>
author maintainer
#15 lore raw

PATCH v2 5/5 mm: memcg: convert enum res_type to mem_counter_type

In reply to Roman Gushchin

The res_type enum is listing all types of memory tracked by memory
cgroups: generic memory, swap, kernel memory, tcp etc; and it's currently used only for dealing with corresponding sysfs files.

To prepare for tracking of various types of memory by a single page_counter structure, a similar enumeration is needed. Instead of introducing a completely new enumeration, let's re-purpose the existing one: rename it into mem_counter_type, change items names to be more meaningful and move to page_counter.h. The latter is needed to have the total number of different memory types available.

This change doesn't bring any functional difference, it's a pure refactoring.

Signed-off-by Roman Gushchin <[email protected]>
---
 mm/memcontrol-v1.c | 66 +++++++++++++++++++++++-----------------------
 mm/memcontrol-v1.h |  8 ------
 2 files changed, 33 insertions(+), 41 deletions(-)
mm/memcontrol-v1.c +33 -33
index d8bf89c76cc5..05433c0a73dc 100644
@@ -1497,7 +1497,7 @@ static void mem_cgroup_oom_notify(struct mem_cgroup *memcg)
 }
 
 static int __mem_cgroup_usage_register_event(struct mem_cgroup *memcg,
-	struct eventfd_ctx *eventfd, const char *args, enum res_type type)
+	struct eventfd_ctx *eventfd, const char *args, enum mem_counter_type type)
 {
 	struct mem_cgroup_thresholds *thresholds;
 	struct mem_cgroup_threshold_ary *new;

@@ -1511,10 +1511,10 @@ static int __mem_cgroup_usage_register_event(struct mem_cgroup *memcg,
 
 	mutex_lock(&memcg->thresholds_lock);
 
-	if (type == _MEM) {
+	if (type == MCT_MEM) {
 		thresholds = &memcg->thresholds;
 		usage = mem_cgroup_usage(memcg, false);
-	} else if (type == _MEMSWAP) {
+	} else if (type == MCT_MEMSW) {
 		thresholds = &memcg->memsw_thresholds;
 		usage = mem_cgroup_usage(memcg, true);
 	} else

@@ -1522,7 +1522,7 @@ static int __mem_cgroup_usage_register_event(struct mem_cgroup *memcg,
 
 	/* Check if a threshold crossed before adding a new one */
 	if (thresholds->primary)
-		__mem_cgroup_threshold(memcg, type == _MEMSWAP);
+		__mem_cgroup_threshold(memcg, type == MCT_MEMSW);
 
 	size = thresholds->primary ? thresholds->primary->size + 1 : 1;
 

@@ -1579,17 +1579,17 @@ static int __mem_cgroup_usage_register_event(struct mem_cgroup *memcg,
 static int mem_cgroup_usage_register_event(struct mem_cgroup *memcg,
 	struct eventfd_ctx *eventfd, const char *args)
 {
-	return __mem_cgroup_usage_register_event(memcg, eventfd, args, _MEM);
+	return __mem_cgroup_usage_register_event(memcg, eventfd, args, MCT_MEM);
 }
 
 static int memsw_cgroup_usage_register_event(struct mem_cgroup *memcg,
 	struct eventfd_ctx *eventfd, const char *args)
 {
-	return __mem_cgroup_usage_register_event(memcg, eventfd, args, _MEMSWAP);
+	return __mem_cgroup_usage_register_event(memcg, eventfd, args, MCT_MEMSW);
 }
 
 static void __mem_cgroup_usage_unregister_event(struct mem_cgroup *memcg,
-	struct eventfd_ctx *eventfd, enum res_type type)
+	struct eventfd_ctx *eventfd, enum mem_counter_type type)
 {
 	struct mem_cgroup_thresholds *thresholds;
 	struct mem_cgroup_threshold_ary *new;

@@ -1598,10 +1598,10 @@ static void __mem_cgroup_usage_unregister_event(struct mem_cgroup *memcg,
 
 	mutex_lock(&memcg->thresholds_lock);
 
-	if (type == _MEM) {
+	if (type == MCT_MEM) {
 		thresholds = &memcg->thresholds;
 		usage = mem_cgroup_usage(memcg, false);
-	} else if (type == _MEMSWAP) {
+	} else if (type == MCT_MEMSW) {
 		thresholds = &memcg->memsw_thresholds;
 		usage = mem_cgroup_usage(memcg, true);
 	} else

@@ -1611,7 +1611,7 @@ static void __mem_cgroup_usage_unregister_event(struct mem_cgroup *memcg,
 		goto unlock;
 
 	/* Check if a threshold crossed before removing */
-	__mem_cgroup_threshold(memcg, type == _MEMSWAP);
+	__mem_cgroup_threshold(memcg, type == MCT_MEMSW);
 
 	/* Calculate new number of threshold */
 	size = entries = 0;

@@ -1676,13 +1676,13 @@ static void __mem_cgroup_usage_unregister_event(struct mem_cgroup *memcg,
 static void mem_cgroup_usage_unregister_event(struct mem_cgroup *memcg,
 	struct eventfd_ctx *eventfd)
 {
-	return __mem_cgroup_usage_unregister_event(memcg, eventfd, _MEM);
+	return __mem_cgroup_usage_unregister_event(memcg, eventfd, MCT_MEM);
 }
 
 static void memsw_cgroup_usage_unregister_event(struct mem_cgroup *memcg,
 	struct eventfd_ctx *eventfd)
 {
-	return __mem_cgroup_usage_unregister_event(memcg, eventfd, _MEMSWAP);
+	return __mem_cgroup_usage_unregister_event(memcg, eventfd, MCT_MEMSW);
 }
 
 static int mem_cgroup_oom_register_event(struct mem_cgroup *memcg,

@@ -2414,20 +2414,20 @@ static ssize_t mem_cgroup_write(struct kernfs_open_file *of,
 			break;
 		}
 		switch (MEMFILE_TYPE(of_cft(of)->private)) {
-		case _MEM:
+		case MCT_MEM:
 			ret = mem_cgroup_resize_max(memcg, nr_pages, false);
 			break;
-		case _MEMSWAP:
+		case MCT_MEMSW:
 			ret = mem_cgroup_resize_max(memcg, nr_pages, true);
 			break;
-		case _KMEM:
+		case MCT_KMEM:
 			pr_warn_once("kmem.limit_in_bytes is deprecated and will be removed. "
 				     "Writing any value to this file has no effect. "
 				     "Please report your usecase to [email protected] if you "
 				     "depend on this functionality.\n");
 			ret = 0;
 			break;
-		case _TCP:
+		case MCT_TCPMEM:
 			ret = memcg_update_tcp_max(memcg, nr_pages);
 			break;
 		}

@@ -2738,30 +2738,30 @@ static int mem_cgroup_slab_show(struct seq_file *m, void *p)
 struct cftype mem_cgroup_legacy_files[] = {
 	{
 		.name = "usage_in_bytes",
-		.private = MEMFILE_PRIVATE(_MEM, RES_USAGE),
+		.private = MEMFILE_PRIVATE(MCT_MEM, RES_USAGE),
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "max_usage_in_bytes",
-		.private = MEMFILE_PRIVATE(_MEM, RES_MAX_USAGE),
+		.private = MEMFILE_PRIVATE(MCT_MEM, RES_MAX_USAGE),
 		.write = mem_cgroup_reset,
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "limit_in_bytes",
-		.private = MEMFILE_PRIVATE(_MEM, RES_LIMIT),
+		.private = MEMFILE_PRIVATE(MCT_MEM, RES_LIMIT),
 		.write = mem_cgroup_write,
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "soft_limit_in_bytes",
-		.private = MEMFILE_PRIVATE(_MEM, RES_SOFT_LIMIT),
+		.private = MEMFILE_PRIVATE(MCT_MEM, RES_SOFT_LIMIT),
 		.write = mem_cgroup_write,
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "failcnt",
-		.private = MEMFILE_PRIVATE(_MEM, RES_FAILCNT),
+		.private = MEMFILE_PRIVATE(MCT_MEM, RES_FAILCNT),
 		.write = mem_cgroup_reset,
 		.read_u64 = mem_cgroup_read_u64,
 	},

@@ -2810,24 +2810,24 @@ struct cftype mem_cgroup_legacy_files[] = {
 #endif
 	{
 		.name = "kmem.limit_in_bytes",
-		.private = MEMFILE_PRIVATE(_KMEM, RES_LIMIT),
+		.private = MEMFILE_PRIVATE(MCT_KMEM, RES_LIMIT),
 		.write = mem_cgroup_write,
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "kmem.usage_in_bytes",
-		.private = MEMFILE_PRIVATE(_KMEM, RES_USAGE),
+		.private = MEMFILE_PRIVATE(MCT_KMEM, RES_USAGE),
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "kmem.failcnt",
-		.private = MEMFILE_PRIVATE(_KMEM, RES_FAILCNT),
+		.private = MEMFILE_PRIVATE(MCT_KMEM, RES_FAILCNT),
 		.write = mem_cgroup_reset,
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "kmem.max_usage_in_bytes",
-		.private = MEMFILE_PRIVATE(_KMEM, RES_MAX_USAGE),
+		.private = MEMFILE_PRIVATE(MCT_KMEM, RES_MAX_USAGE),
 		.write = mem_cgroup_reset,
 		.read_u64 = mem_cgroup_read_u64,
 	},

@@ -2839,24 +2839,24 @@ struct cftype mem_cgroup_legacy_files[] = {
 #endif
 	{
 		.name = "kmem.tcp.limit_in_bytes",
-		.private = MEMFILE_PRIVATE(_TCP, RES_LIMIT),
+		.private = MEMFILE_PRIVATE(MCT_TCPMEM, RES_LIMIT),
 		.write = mem_cgroup_write,
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "kmem.tcp.usage_in_bytes",
-		.private = MEMFILE_PRIVATE(_TCP, RES_USAGE),
+		.private = MEMFILE_PRIVATE(MCT_TCPMEM, RES_USAGE),
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "kmem.tcp.failcnt",
-		.private = MEMFILE_PRIVATE(_TCP, RES_FAILCNT),
+		.private = MEMFILE_PRIVATE(MCT_TCPMEM, RES_FAILCNT),
 		.write = mem_cgroup_reset,
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "kmem.tcp.max_usage_in_bytes",
-		.private = MEMFILE_PRIVATE(_TCP, RES_MAX_USAGE),
+		.private = MEMFILE_PRIVATE(MCT_TCPMEM, RES_MAX_USAGE),
 		.write = mem_cgroup_reset,
 		.read_u64 = mem_cgroup_read_u64,
 	},

@@ -2866,24 +2866,24 @@ struct cftype mem_cgroup_legacy_files[] = {
 struct cftype memsw_files[] = {
 	{
 		.name = "memsw.usage_in_bytes",
-		.private = MEMFILE_PRIVATE(_MEMSWAP, RES_USAGE),
+		.private = MEMFILE_PRIVATE(MCT_MEMSW, RES_USAGE),
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "memsw.max_usage_in_bytes",
-		.private = MEMFILE_PRIVATE(_MEMSWAP, RES_MAX_USAGE),
+		.private = MEMFILE_PRIVATE(MCT_MEMSW, RES_MAX_USAGE),
 		.write = mem_cgroup_reset,
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "memsw.limit_in_bytes",
-		.private = MEMFILE_PRIVATE(_MEMSWAP, RES_LIMIT),
+		.private = MEMFILE_PRIVATE(MCT_MEMSW, RES_LIMIT),
 		.write = mem_cgroup_write,
 		.read_u64 = mem_cgroup_read_u64,
 	},
 	{
 		.name = "memsw.failcnt",
-		.private = MEMFILE_PRIVATE(_MEMSWAP, RES_FAILCNT),
+		.private = MEMFILE_PRIVATE(MCT_MEMSW, RES_FAILCNT),
 		.write = mem_cgroup_reset,
 		.read_u64 = mem_cgroup_read_u64,
 	},
mm/memcontrol-v1.h +0 -8
index 9a4d8205aed5..b2795c0493f8 100644
@@ -87,14 +87,6 @@ void memcg1_cancel_attach(struct cgroup_taskset *tset);
 void memcg1_move_task(void);
 void memcg1_css_offline(struct mem_cgroup *memcg);
 
-/* for encoding cft->private value on file */
-enum res_type {
-	_MEM,
-	_MEMSWAP,
-	_KMEM,
-	_TCP,
-};
-
 bool memcg1_oom_prepare(struct mem_cgroup *memcg, bool *locked);
 void memcg1_oom_finish(struct mem_cgroup *memcg, bool locked);
 void memcg1_oom_recover(struct mem_cgroup *memcg);
Signature · 1 line
2.46.0.rc1.232.g9752f9e123-goog
maintainer Acked-by on v2 5/5
#16 lore raw

Re: [PATCH v2 5/5] mm: memcg: convert enum res_type to mem_counter_type

Quoted 12 lines · Roman Gushchin

The res_type enum is listing all types of memory tracked by memory
cgroups: generic memory, swap, kernel memory, tcp etc; and it's currently used only for dealing with corresponding sysfs files.

To prepare for tracking of various types of memory by a single page_counter structure, a similar enumeration is needed. Instead of introducing a completely new enumeration, let's re-purpose the existing one: rename it into mem_counter_type, change items names to be more meaningful and move to page_counter.h. The latter is needed to have the total number of different memory types available.

Quoted 4 lines

This change doesn't bring any functional difference, it's a pure refactoring.

Signed-off-by Roman Gushchin <[email protected]>
Acked-by Shakeel Butt <[email protected]>
author maintainer
#17 lore raw

Re: [PATCH v2 5/5] mm: memcg: convert enum res_type to mem_counter_type

Quoted 13 lines · Shakeel Butt
Quoted 12 lines · Roman Gushchin

The res_type enum is listing all types of memory tracked by memory
cgroups: generic memory, swap, kernel memory, tcp etc; and it's currently used only for dealing with corresponding sysfs files.

To prepare for tracking of various types of memory by a single page_counter structure, a similar enumeration is needed. Instead of introducing a completely new enumeration, let's re-purpose the existing one: rename it into mem_counter_type, change items names to be more meaningful and move to page_counter.h. The latter is needed to have the total number of different memory types available.

Quoted 6 lines
Quoted 4 lines

This change doesn't bring any functional difference, it's a pure refactoring.

Signed-off-by Roman Gushchin <[email protected]>
Acked-by Shakeel Butt <[email protected]>

Thank you for reviewing the series!

author maintainer
#18 lore raw
In reply to Roman Gushchin

Ooops, there was a formatting error in the cover letter, my apologies.

Here is a fixed version:

--

From a08acc7e656d279b461a645309745bca5218d7d2 Mon Sep 17 00:00:00 2001
From: Roman Gushchin <[email protected]>
Date: Wed, 24 Jul 2024 20:38:13 +0000
Subject: [PATCH v2 0/5] mm: memcg: merge page_counter trees

This patchset reorganizes page_counter structures which helps to make memory cgroup and hugetlb cgroup structures smaller (10%-35% depending on the kernel configuration) and more cache-effective. It also eliminates useless tracking of protected memory usage when it's not needed.

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/[email protected]/T/#m77151ed83451a49132e29ef13d55e08b95ac867f

Roman Gushchin (5):
mm: memcg: don't call propagate_protected_usage() needlessly
mm: page_counters: put page_counter_calculate_protection() under
CONFIG_MEMCG
mm: memcg: merge multiple page_counters into a single structure
mm: page_counters: initialize usage using ATOMIC_LONG_INIT() macro
mm: memcg: convert enum res_type to mem_counter_type

 include/linux/hugetlb.h        |   4 +-
 include/linux/hugetlb_cgroup.h |   8 +-
 include/linux/memcontrol.h     |  19 +--
 include/linux/page_counter.h   | 128 ++++++++++++++++----
 mm/hugetlb.c                   |  14 +--
 mm/hugetlb_cgroup.c            | 150 +++++++++--------------
 mm/memcontrol-v1.c             | 154 ++++++++++--------------
 mm/memcontrol-v1.h             |  10 +-
 mm/memcontrol.c                | 211 ++++++++++++++++-----------------
 mm/page_counter.c              |  94 +++++++++------
 10 files changed, 403 insertions(+), 389 deletions(-)
Signature · 1 line
2.46.0.rc1.232.g9752f9e123-goog
maintainer
#19 lore raw
Quoted 4 lines · Roman Gushchin

Ooops, there was a formatting error in the cover letter, my apologies.

Here is a fixed version:

Hi Roman, what is the base of this series? Is it linus tree or mm-unstable?

author maintainer
#20 lore raw
Quoted 9 lines · Shakeel Butt
Quoted 4 lines · Roman Gushchin

Ooops, there was a formatting error in the cover letter, my apologies.

Here is a fixed version:

Hi Roman, what is the base of this series? Is it linus tree or mm-unstable?

Hi Shakeel!

It's a today's snapshot of mm-unstable (d8492982371a). It had some conflicts with the recent memory.peak series, which I resolved today.

The whole series can be found here:
https://github.com/rgushchin/linux/tree/page_counters.2 .

Thanks!

maintainer
#21 lore raw
In reply to Roman Gushchin
Quoted 3 lines · On Wed, 24 Jul 2024 20:40:27 +0000 Roman Gushchin <[email protected]>…

Ooops, there was a formatting error in the cover letter, my apologies.

Here is a fixed version:

Yes, the [0/n] is pretty screwy and I'm hitting some rejects against the latest patchpile. Can we please have a v2 against latest mm-unstable, which I expect to push out a few hours from now? Thanks.