[PATCH v3 next 0/5] Some cleanup for memcg

[PATCH v3 next 0/5] Some cleanup for memcg

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>

Some cleanup for memcg

---

v2->v3:
 - move the wrapper function definitions to header files.
 - add a patch to move the 'local' functions to the memcontrol-v1.c.

v1->v2:
 - drop the patch 'simplify the mem_cgroup_update_lru_size function'.
 - for patch 3, rename '__refill_obj_stock' to replace_stock_objcg, and
   keep the 'objcg equal' check in the calling functions.

Chen Ridong (5):
  memcg: use OFP_PEAK_UNSET instead of -1
  memcg: call the free function when allocation of pn fails
  memcg: factor out the replace_stock_objcg function
  memcg: factor out stat(event)/stat_local(event_local) reading
    functions
  memcg: move the 'local' functions to memcontrol-v1.c

 include/linux/memcontrol.h |  25 +++++--
 mm/memcontrol-v1.c         |  17 +++++
 mm/memcontrol-v1.h         |  11 ++--
 mm/memcontrol.c            | 130 ++++++++++++-------------------------
 4 files changed, 86 insertions(+), 97 deletions(-)

-- 
2.34.1

[PATCH v3 next 1/5] memcg: use OFP_PEAK_UNSET instead of -1

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>

The 'OFP_PEAK_UNSET' has been defined, use it instead of '-1'.

Signed-off-by: Chen Ridong <chenridong@huawei.com>
Reviewed-by: Michal Koutný <mkoutny@suse.com>
Acked-by: David Finkel <davidf@vimeo.com>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 mm/memcontrol.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 46f8b372d212..05a32c860554 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -4004,7 +4004,7 @@ static ssize_t peak_write(struct kernfs_open_file *of, char *buf, size_t nbytes,
 			WRITE_ONCE(peer_ctx->value, usage);
 
 	/* initial write, register watcher */
-	if (ofp->value == -1)
+	if (ofp->value == OFP_PEAK_UNSET)
 		list_add(&ofp->list, watchers);
 
 	WRITE_ONCE(ofp->value, usage);
-- 
2.34.1

Re: [PATCH v3 next 1/5] memcg: use OFP_PEAK_UNSET instead of -1

[Johannes Weiner]

On Fri, Jan 17, 2025 at 01:46:41AM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
> 
> The 'OFP_PEAK_UNSET' has been defined, use it instead of '-1'.
> 
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> Reviewed-by: Michal Koutný <mkoutny@suse.com>
> Acked-by: David Finkel <davidf@vimeo.com>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

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

[PATCH v3 next 2/5] memcg: call the free function when allocation of pn fails

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>

The 'free_mem_cgroup_per_node_info' function is used to free
the 'mem_cgroup_per_node' struct. Using 'pn' as the input for the
free_mem_cgroup_per_node_info function will be much clearer.
Call 'free_mem_cgroup_per_node_info' when 'alloc_mem_cgroup_per_node_info'
fails, to free 'pn' as a whole, which makes the code more cohesive.

Signed-off-by: Chen Ridong <chenridong@huawei.com>
Reviewed-by: Michal Koutný <mkoutny@suse.com>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 mm/memcontrol.c | 27 ++++++++++++---------------
 1 file changed, 12 insertions(+), 15 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 05a32c860554..98f84a9fa228 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -3424,6 +3424,16 @@ struct mem_cgroup *mem_cgroup_get_from_ino(unsigned long ino)
 }
 #endif
 
+static void free_mem_cgroup_per_node_info(struct mem_cgroup_per_node *pn)
+{
+	if (!pn)
+		return;
+
+	free_percpu(pn->lruvec_stats_percpu);
+	kfree(pn->lruvec_stats);
+	kfree(pn);
+}
+
 static bool alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
 {
 	struct mem_cgroup_per_node *pn;
@@ -3448,23 +3458,10 @@ static bool alloc_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
 	memcg->nodeinfo[node] = pn;
 	return true;
 fail:
-	kfree(pn->lruvec_stats);
-	kfree(pn);
+	free_mem_cgroup_per_node_info(pn);
 	return false;
 }
 
-static void free_mem_cgroup_per_node_info(struct mem_cgroup *memcg, int node)
-{
-	struct mem_cgroup_per_node *pn = memcg->nodeinfo[node];
-
-	if (!pn)
-		return;
-
-	free_percpu(pn->lruvec_stats_percpu);
-	kfree(pn->lruvec_stats);
-	kfree(pn);
-}
-
 static void __mem_cgroup_free(struct mem_cgroup *memcg)
 {
 	int node;
@@ -3472,7 +3469,7 @@ static void __mem_cgroup_free(struct mem_cgroup *memcg)
 	obj_cgroup_put(memcg->orig_objcg);
 
 	for_each_node(node)
-		free_mem_cgroup_per_node_info(memcg, node);
+		free_mem_cgroup_per_node_info(memcg->nodeinfo[node]);
 	memcg1_free_events(memcg);
 	kfree(memcg->vmstats);
 	free_percpu(memcg->vmstats_percpu);
-- 
2.34.1

Re: [PATCH v3 next 2/5] memcg: call the free function when allocation of pn fails

[Johannes Weiner]

On Fri, Jan 17, 2025 at 01:46:42AM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
> 
> The 'free_mem_cgroup_per_node_info' function is used to free
> the 'mem_cgroup_per_node' struct. Using 'pn' as the input for the
> free_mem_cgroup_per_node_info function will be much clearer.
> Call 'free_mem_cgroup_per_node_info' when 'alloc_mem_cgroup_per_node_info'
> fails, to free 'pn' as a whole, which makes the code more cohesive.
> 
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> Reviewed-by: Michal Koutný <mkoutny@suse.com>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

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

[PATCH v3 next 3/5] memcg: factor out the replace_stock_objcg function

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>

Factor out the 'replace_stock_objcg' function to make the code more
cohesive.

Signed-off-by: Chen Ridong <chenridong@huawei.com>
Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>
Acked-by: Shakeel Butt <shakeel.butt@linux.dev>
---
 mm/memcontrol.c | 26 ++++++++++++++++----------
 1 file changed, 16 insertions(+), 10 deletions(-)

diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 98f84a9fa228..b10e0a8f3375 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -2691,6 +2691,20 @@ void __memcg_kmem_uncharge_page(struct page *page, int order)
 	obj_cgroup_put(objcg);
 }
 
+/* Replace the stock objcg with objcg, return the old objcg */
+static struct obj_cgroup *replace_stock_objcg(struct memcg_stock_pcp *stock,
+					     struct obj_cgroup *objcg)
+{
+	struct obj_cgroup *old = NULL;
+
+	old = drain_obj_stock(stock);
+	obj_cgroup_get(objcg);
+	stock->nr_bytes = atomic_read(&objcg->nr_charged_bytes)
+			? atomic_xchg(&objcg->nr_charged_bytes, 0) : 0;
+	WRITE_ONCE(stock->cached_objcg, objcg);
+	return old;
+}
+
 static void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat,
 		     enum node_stat_item idx, int nr)
 {
@@ -2708,11 +2722,7 @@ static void mod_objcg_state(struct obj_cgroup *objcg, struct pglist_data *pgdat,
 	 * changes.
 	 */
 	if (READ_ONCE(stock->cached_objcg) != objcg) {
-		old = drain_obj_stock(stock);
-		obj_cgroup_get(objcg);
-		stock->nr_bytes = atomic_read(&objcg->nr_charged_bytes)
-				? atomic_xchg(&objcg->nr_charged_bytes, 0) : 0;
-		WRITE_ONCE(stock->cached_objcg, objcg);
+		old = replace_stock_objcg(stock, objcg);
 		stock->cached_pgdat = pgdat;
 	} else if (stock->cached_pgdat != pgdat) {
 		/* Flush the existing cached vmstat data */
@@ -2866,11 +2876,7 @@ static void refill_obj_stock(struct obj_cgroup *objcg, unsigned int nr_bytes,
 
 	stock = this_cpu_ptr(&memcg_stock);
 	if (READ_ONCE(stock->cached_objcg) != objcg) { /* reset if necessary */
-		old = drain_obj_stock(stock);
-		obj_cgroup_get(objcg);
-		WRITE_ONCE(stock->cached_objcg, objcg);
-		stock->nr_bytes = atomic_read(&objcg->nr_charged_bytes)
-				? atomic_xchg(&objcg->nr_charged_bytes, 0) : 0;
+		old = replace_stock_objcg(stock, objcg);
 		allow_uncharge = true;	/* Allow uncharge when objcg changes */
 	}
 	stock->nr_bytes += nr_bytes;
-- 
2.34.1

Re: [PATCH v3 next 3/5] memcg: factor out the replace_stock_objcg function

[Johannes Weiner]

On Fri, Jan 17, 2025 at 01:46:43AM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
> 
> Factor out the 'replace_stock_objcg' function to make the code more
> cohesive.
> 
> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> Reviewed-by: Roman Gushchin <roman.gushchin@linux.dev>
> Acked-by: Shakeel Butt <shakeel.butt@linux.dev>

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

[PATCH v3 next 4/5] memcg: factor out stat(event)/stat_local(event_local) reading functions

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>

The only difference between 'lruvec_page_state' and
'lruvec_page_state_local' is that they read 'state' and 'state_local',
respectively. Factor out an inner functions to make the code more concise.
Do the same for reading 'memcg_page_stat' and 'memcg_events'.

Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
 include/linux/memcontrol.h | 31 +++++++++++++++---
 mm/memcontrol-v1.h         | 14 ++++++--
 mm/memcontrol.c            | 67 +++++++-------------------------------
 3 files changed, 49 insertions(+), 63 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 6e74b8254d9b..ec469c5f7491 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -936,10 +936,33 @@ static inline void mod_memcg_page_state(struct page *page,
 	rcu_read_unlock();
 }
 
-unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx);
-unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx);
-unsigned long lruvec_page_state_local(struct lruvec *lruvec,
-				      enum node_stat_item idx);
+unsigned long __memcg_page_state(struct mem_cgroup *memcg, int idx, bool local);
+
+/* idx can be of type enum memcg_stat_item or node_stat_item. */
+static inline unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx)
+{
+	return __memcg_page_state(memcg, idx, true);
+}
+
+static inline unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx)
+{
+	return __memcg_page_state(memcg, idx, false);
+}
+
+unsigned long __lruvec_page_state(struct lruvec *lruvec,
+		enum node_stat_item idx, bool local);
+
+static inline unsigned long lruvec_page_state(struct lruvec *lruvec,
+					      enum node_stat_item idx)
+{
+	return __lruvec_page_state(lruvec, idx, false);
+}
+
+static inline unsigned long lruvec_page_state_local(struct lruvec *lruvec,
+						    enum node_stat_item idx)
+{
+	return __lruvec_page_state(lruvec, idx, true);
+}
 
 void mem_cgroup_flush_stats(struct mem_cgroup *memcg);
 void mem_cgroup_flush_stats_ratelimited(struct mem_cgroup *memcg);
diff --git a/mm/memcontrol-v1.h b/mm/memcontrol-v1.h
index 144d71b65907..f68c0064d674 100644
--- a/mm/memcontrol-v1.h
+++ b/mm/memcontrol-v1.h
@@ -59,9 +59,17 @@ unsigned long mem_cgroup_usage(struct mem_cgroup *memcg, bool swap);
 
 void drain_all_stock(struct mem_cgroup *root_memcg);
 
-unsigned long memcg_events(struct mem_cgroup *memcg, int event);
-unsigned long memcg_events_local(struct mem_cgroup *memcg, int event);
-unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx);
+unsigned long __memcg_events(struct mem_cgroup *memcg, int event, bool local);
+static inline unsigned long memcg_events(struct mem_cgroup *memcg, int event)
+{
+	return __memcg_events(memcg, event, false);
+}
+
+static inline unsigned long memcg_events_local(struct mem_cgroup *memcg, int event)
+{
+	return __memcg_events(memcg, event, true);
+}
+
 unsigned long memcg_page_state_output(struct mem_cgroup *memcg, int item);
 unsigned long memcg_page_state_local_output(struct mem_cgroup *memcg, int item);
 int memory_stat_show(struct seq_file *m, void *v);
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index b10e0a8f3375..404bbdfa352f 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -375,7 +375,8 @@ struct lruvec_stats {
 	long state_pending[NR_MEMCG_NODE_STAT_ITEMS];
 };
 
-unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
+unsigned long __lruvec_page_state(struct lruvec *lruvec,
+		enum node_stat_item idx, bool local)
 {
 	struct mem_cgroup_per_node *pn;
 	long x;
@@ -389,30 +390,8 @@ unsigned long lruvec_page_state(struct lruvec *lruvec, enum node_stat_item idx)
 		return 0;
 
 	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
-	x = READ_ONCE(pn->lruvec_stats->state[i]);
-#ifdef CONFIG_SMP
-	if (x < 0)
-		x = 0;
-#endif
-	return x;
-}
-
-unsigned long lruvec_page_state_local(struct lruvec *lruvec,
-				      enum node_stat_item idx)
-{
-	struct mem_cgroup_per_node *pn;
-	long x;
-	int i;
-
-	if (mem_cgroup_disabled())
-		return node_page_state(lruvec_pgdat(lruvec), idx);
-
-	i = memcg_stats_index(idx);
-	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
-		return 0;
-
-	pn = container_of(lruvec, struct mem_cgroup_per_node, lruvec);
-	x = READ_ONCE(pn->lruvec_stats->state_local[i]);
+	x = local ? READ_ONCE(pn->lruvec_stats->state_local[i]) :
+		    READ_ONCE(pn->lruvec_stats->state[i]);
 #ifdef CONFIG_SMP
 	if (x < 0)
 		x = 0;
@@ -651,7 +630,7 @@ static void flush_memcg_stats_dwork(struct work_struct *w)
 	queue_delayed_work(system_unbound_wq, &stats_flush_dwork, FLUSH_TIME);
 }
 
-unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx)
+unsigned long __memcg_page_state(struct mem_cgroup *memcg, int idx, bool local)
 {
 	long x;
 	int i = memcg_stats_index(idx);
@@ -659,7 +638,9 @@ unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx)
 	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
 		return 0;
 
-	x = READ_ONCE(memcg->vmstats->state[i]);
+	x = local ? READ_ONCE(memcg->vmstats->state_local[i]) :
+		    READ_ONCE(memcg->vmstats->state[i]);
+
 #ifdef CONFIG_SMP
 	if (x < 0)
 		x = 0;
@@ -706,23 +687,6 @@ void __mod_memcg_state(struct mem_cgroup *memcg, enum memcg_stat_item idx,
 	trace_mod_memcg_state(memcg, idx, val);
 }
 
-/* idx can be of type enum memcg_stat_item or node_stat_item. */
-unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx)
-{
-	long x;
-	int i = memcg_stats_index(idx);
-
-	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, idx))
-		return 0;
-
-	x = READ_ONCE(memcg->vmstats->state_local[i]);
-#ifdef CONFIG_SMP
-	if (x < 0)
-		x = 0;
-#endif
-	return x;
-}
-
 static void __mod_memcg_lruvec_state(struct lruvec *lruvec,
 				     enum node_stat_item idx,
 				     int val)
@@ -859,24 +823,15 @@ void __count_memcg_events(struct mem_cgroup *memcg, enum vm_event_item idx,
 	memcg_stats_unlock();
 }
 
-unsigned long memcg_events(struct mem_cgroup *memcg, int event)
-{
-	int i = memcg_events_index(event);
-
-	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, event))
-		return 0;
-
-	return READ_ONCE(memcg->vmstats->events[i]);
-}
-
-unsigned long memcg_events_local(struct mem_cgroup *memcg, int event)
+unsigned long __memcg_events(struct mem_cgroup *memcg, int event, bool local)
 {
 	int i = memcg_events_index(event);
 
 	if (WARN_ONCE(BAD_STAT_IDX(i), "%s: missing stat item %d\n", __func__, event))
 		return 0;
 
-	return READ_ONCE(memcg->vmstats->events_local[i]);
+	return local ? READ_ONCE(memcg->vmstats->events_local[i]) :
+		    READ_ONCE(memcg->vmstats->events[i]);
 }
 
 struct mem_cgroup *mem_cgroup_from_task(struct task_struct *p)
-- 
2.34.1

Re: [PATCH v3 next 4/5] memcg: factor out stat(event)/stat_local(event_local) reading functions

[Johannes Weiner]

On Fri, Jan 17, 2025 at 01:46:44AM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
> 
> The only difference between 'lruvec_page_state' and
> 'lruvec_page_state_local' is that they read 'state' and 'state_local',
> respectively. Factor out an inner functions to make the code more concise.
> Do the same for reading 'memcg_page_stat' and 'memcg_events'.
> 
> Signed-off-by: Chen Ridong <chenridong@huawei.com>

bool parameters make for poor readability at the callsites :(

With the next patch moving most of the duplication to memcontrol-v1.c,
I think it's probably not worth refactoring this.

Re: [PATCH v3 next 4/5] memcg: factor out stat(event)/stat_local(event_local) reading functions

[Yosry Ahmed]

On Fri, Jan 17, 2025 at 8:56 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>
> On Fri, Jan 17, 2025 at 01:46:44AM +0000, Chen Ridong wrote:
> > From: Chen Ridong <chenridong@huawei.com>
> >
> > The only difference between 'lruvec_page_state' and
> > 'lruvec_page_state_local' is that they read 'state' and 'state_local',
> > respectively. Factor out an inner functions to make the code more concise.
> > Do the same for reading 'memcg_page_stat' and 'memcg_events'.
> >
> > Signed-off-by: Chen Ridong <chenridong@huawei.com>
>
> bool parameters make for poor readability at the callsites :(
>
> With the next patch moving most of the duplication to memcontrol-v1.c,
> I think it's probably not worth refactoring this.

Arguably the duplication would now be across two different files,
making it more difficult to notice and keep the implementations in
sync.

Re: [PATCH v3 next 4/5] memcg: factor out stat(event)/stat_local(event_local) reading functions

[Johannes Weiner]

On Fri, Jan 17, 2025 at 09:01:59AM -0800, Yosry Ahmed wrote:
> On Fri, Jan 17, 2025 at 8:56 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >
> > On Fri, Jan 17, 2025 at 01:46:44AM +0000, Chen Ridong wrote:
> > > From: Chen Ridong <chenridong@huawei.com>
> > >
> > > The only difference between 'lruvec_page_state' and
> > > 'lruvec_page_state_local' is that they read 'state' and 'state_local',
> > > respectively. Factor out an inner functions to make the code more concise.
> > > Do the same for reading 'memcg_page_stat' and 'memcg_events'.
> > >
> > > Signed-off-by: Chen Ridong <chenridong@huawei.com>
> >
> > bool parameters make for poor readability at the callsites :(
> >
> > With the next patch moving most of the duplication to memcontrol-v1.c,
> > I think it's probably not worth refactoring this.
> 
> Arguably the duplication would now be across two different files,
> making it more difficult to notice and keep the implementations in
> sync.

Dependencies between the files is a bigger pain. E.g. try_charge()
being defined in memcontrol-v1.h makes memcontrol.c more difficult to
work with. That shared state also immediately bitrotted when charge
moving was removed and the last cgroup1 caller disappeared.

The whole point of the cgroup1 split was to simplify cgroup2 code. The
tiny amount of duplication in this case doesn't warrant further
entanglement between the codebases.

Re: [PATCH v3 next 4/5] memcg: factor out stat(event)/stat_local(event_local) reading functions

[Chen Ridong]

On 2025/1/18 2:02, Johannes Weiner wrote:
> On Fri, Jan 17, 2025 at 09:01:59AM -0800, Yosry Ahmed wrote:
>> On Fri, Jan 17, 2025 at 8:56 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>>>
>>> On Fri, Jan 17, 2025 at 01:46:44AM +0000, Chen Ridong wrote:
>>>> From: Chen Ridong <chenridong@huawei.com>
>>>>
>>>> The only difference between 'lruvec_page_state' and
>>>> 'lruvec_page_state_local' is that they read 'state' and 'state_local',
>>>> respectively. Factor out an inner functions to make the code more concise.
>>>> Do the same for reading 'memcg_page_stat' and 'memcg_events'.
>>>>
>>>> Signed-off-by: Chen Ridong <chenridong@huawei.com>
>>>
>>> bool parameters make for poor readability at the callsites :(
>>>
>>> With the next patch moving most of the duplication to memcontrol-v1.c,
>>> I think it's probably not worth refactoring this.
>>
>> Arguably the duplication would now be across two different files,
>> making it more difficult to notice and keep the implementations in
>> sync.
> 
> Dependencies between the files is a bigger pain. E.g. try_charge()
> being defined in memcontrol-v1.h makes memcontrol.c more difficult to
> work with. That shared state also immediately bitrotted when charge
> moving was removed and the last cgroup1 caller disappeared.
> 
> The whole point of the cgroup1 split was to simplify cgroup2 code. The
> tiny amount of duplication in this case doesn't warrant further
> entanglement between the codebases.

Thank you for your review.

I agree with that. However, If I just move the 'local' functions to
memcontrol-v1.c, I have to move some dependent declarations to the
memcontrol-v1.h.
E.g. struct memcg_vmstats, MEMCG_VMSTAT_SIZE and so on.

Is this worth doing?

Best regards,
Ridong

Re: [PATCH v3 next 4/5] memcg: factor out stat(event)/stat_local(event_local) reading functions

[Johannes Weiner]

On Tue, Jan 21, 2025 at 10:15:00PM +0800, Chen Ridong wrote:
> 
> 
> On 2025/1/18 2:02, Johannes Weiner wrote:
> > On Fri, Jan 17, 2025 at 09:01:59AM -0800, Yosry Ahmed wrote:
> >> On Fri, Jan 17, 2025 at 8:56 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
> >>>
> >>> On Fri, Jan 17, 2025 at 01:46:44AM +0000, Chen Ridong wrote:
> >>>> From: Chen Ridong <chenridong@huawei.com>
> >>>>
> >>>> The only difference between 'lruvec_page_state' and
> >>>> 'lruvec_page_state_local' is that they read 'state' and 'state_local',
> >>>> respectively. Factor out an inner functions to make the code more concise.
> >>>> Do the same for reading 'memcg_page_stat' and 'memcg_events'.
> >>>>
> >>>> Signed-off-by: Chen Ridong <chenridong@huawei.com>
> >>>
> >>> bool parameters make for poor readability at the callsites :(
> >>>
> >>> With the next patch moving most of the duplication to memcontrol-v1.c,
> >>> I think it's probably not worth refactoring this.
> >>
> >> Arguably the duplication would now be across two different files,
> >> making it more difficult to notice and keep the implementations in
> >> sync.
> > 
> > Dependencies between the files is a bigger pain. E.g. try_charge()
> > being defined in memcontrol-v1.h makes memcontrol.c more difficult to
> > work with. That shared state also immediately bitrotted when charge
> > moving was removed and the last cgroup1 caller disappeared.
> > 
> > The whole point of the cgroup1 split was to simplify cgroup2 code. The
> > tiny amount of duplication in this case doesn't warrant further
> > entanglement between the codebases.
> 
> Thank you for your review.
> 
> I agree with that. However, If I just move the 'local' functions to
> memcontrol-v1.c, I have to move some dependent declarations to the
> memcontrol-v1.h.
> E.g. struct memcg_vmstats, MEMCG_VMSTAT_SIZE and so on.
> 
> Is this worth doing?

Ah, right. No, that's not worth it.

The easiest way is to slap CONFIG_MEMCG_V1 guards around the local
functions but leave them in memcontrol.c for now. We already have a
few of those ifdefs for where splitting/sharing wasn't practical. At
least then it's clearly marked and they won't get built.

Re: [PATCH v3 next 4/5] memcg: factor out stat(event)/stat_local(event_local) reading functions

[Chen Ridong]

On 2025/1/24 12:23, Johannes Weiner wrote:
> On Tue, Jan 21, 2025 at 10:15:00PM +0800, Chen Ridong wrote:
>>
>>
>> On 2025/1/18 2:02, Johannes Weiner wrote:
>>> On Fri, Jan 17, 2025 at 09:01:59AM -0800, Yosry Ahmed wrote:
>>>> On Fri, Jan 17, 2025 at 8:56 AM Johannes Weiner <hannes@cmpxchg.org> wrote:
>>>>>
>>>>> On Fri, Jan 17, 2025 at 01:46:44AM +0000, Chen Ridong wrote:
>>>>>> From: Chen Ridong <chenridong@huawei.com>
>>>>>>
>>>>>> The only difference between 'lruvec_page_state' and
>>>>>> 'lruvec_page_state_local' is that they read 'state' and 'state_local',
>>>>>> respectively. Factor out an inner functions to make the code more concise.
>>>>>> Do the same for reading 'memcg_page_stat' and 'memcg_events'.
>>>>>>
>>>>>> Signed-off-by: Chen Ridong <chenridong@huawei.com>
>>>>>
>>>>> bool parameters make for poor readability at the callsites :(
>>>>>
>>>>> With the next patch moving most of the duplication to memcontrol-v1.c,
>>>>> I think it's probably not worth refactoring this.
>>>>
>>>> Arguably the duplication would now be across two different files,
>>>> making it more difficult to notice and keep the implementations in
>>>> sync.
>>>
>>> Dependencies between the files is a bigger pain. E.g. try_charge()
>>> being defined in memcontrol-v1.h makes memcontrol.c more difficult to
>>> work with. That shared state also immediately bitrotted when charge
>>> moving was removed and the last cgroup1 caller disappeared.
>>>
>>> The whole point of the cgroup1 split was to simplify cgroup2 code. The
>>> tiny amount of duplication in this case doesn't warrant further
>>> entanglement between the codebases.
>>
>> Thank you for your review.
>>
>> I agree with that. However, If I just move the 'local' functions to
>> memcontrol-v1.c, I have to move some dependent declarations to the
>> memcontrol-v1.h.
>> E.g. struct memcg_vmstats, MEMCG_VMSTAT_SIZE and so on.
>>
>> Is this worth doing?
> 
> Ah, right. No, that's not worth it.
> 
> The easiest way is to slap CONFIG_MEMCG_V1 guards around the local
> functions but leave them in memcontrol.c for now. We already have a
> few of those ifdefs for where splitting/sharing wasn't practical. At
> least then it's clearly marked and they won't get built.

Thank you, will do that.

Best regards,
Ridong

[PATCH v3 next 5/5] memcg: move the 'local' functions to memcontrol-v1.c

[Chen Ridong]

From: Chen Ridong <chenridong@huawei.com>

Move the 'local' functions, which are only used in memcg v1, to the
memcontrol-v1.c.

Signed-off-by: Chen Ridong <chenridong@huawei.com>
---
 include/linux/memcontrol.h |  6 ------
 mm/memcontrol-v1.c         | 17 +++++++++++++++++
 mm/memcontrol-v1.h         |  7 +------
 mm/memcontrol.c            |  8 +-------
 4 files changed, 19 insertions(+), 19 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index ec469c5f7491..6895b2958835 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -938,12 +938,6 @@ static inline void mod_memcg_page_state(struct page *page,
 
 unsigned long __memcg_page_state(struct mem_cgroup *memcg, int idx, bool local);
 
-/* idx can be of type enum memcg_stat_item or node_stat_item. */
-static inline unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx)
-{
-	return __memcg_page_state(memcg, idx, true);
-}
-
 static inline unsigned long memcg_page_state(struct mem_cgroup *memcg, int idx)
 {
 	return __memcg_page_state(memcg, idx, false);
diff --git a/mm/memcontrol-v1.c b/mm/memcontrol-v1.c
index 2be6b9112808..2e8529b63366 100644
--- a/mm/memcontrol-v1.c
+++ b/mm/memcontrol-v1.c
@@ -106,6 +106,23 @@ static struct lockdep_map memcg_oom_lock_dep_map = {
 
 DEFINE_SPINLOCK(memcg_oom_lock);
 
+static unsigned long memcg_events_local(struct mem_cgroup *memcg, int event)
+{
+	return __memcg_events(memcg, event, true);
+}
+
+/* idx can be of type enum memcg_stat_item or node_stat_item. */
+static unsigned long memcg_page_state_local(struct mem_cgroup *memcg, int idx)
+{
+	return __memcg_page_state(memcg, idx, true);
+}
+
+static unsigned long memcg_page_state_local_output(struct mem_cgroup *memcg, int item)
+{
+	return memcg_page_state_local(memcg, item) *
+		memcg_page_state_output_unit(item);
+}
+
 static void __mem_cgroup_insert_exceeded(struct mem_cgroup_per_node *mz,
 					 struct mem_cgroup_tree_per_node *mctz,
 					 unsigned long new_usage_in_excess)
diff --git a/mm/memcontrol-v1.h b/mm/memcontrol-v1.h
index f68c0064d674..d76e9a47adaa 100644
--- a/mm/memcontrol-v1.h
+++ b/mm/memcontrol-v1.h
@@ -65,13 +65,8 @@ static inline unsigned long memcg_events(struct mem_cgroup *memcg, int event)
 	return __memcg_events(memcg, event, false);
 }
 
-static inline unsigned long memcg_events_local(struct mem_cgroup *memcg, int event)
-{
-	return __memcg_events(memcg, event, true);
-}
-
+int memcg_page_state_output_unit(int item);
 unsigned long memcg_page_state_output(struct mem_cgroup *memcg, int item);
-unsigned long memcg_page_state_local_output(struct mem_cgroup *memcg, int item);
 int memory_stat_show(struct seq_file *m, void *v);
 
 /* Cgroup v1-specific declarations */
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 404bbdfa352f..3f32d4ab55b3 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1367,7 +1367,7 @@ static int memcg_page_state_unit(int item)
 }
 
 /* Translate stat items to the correct unit for memory.stat output */
-static int memcg_page_state_output_unit(int item)
+int memcg_page_state_output_unit(int item)
 {
 	/*
 	 * Workingset state is actually in pages, but we export it to userspace
@@ -1402,12 +1402,6 @@ unsigned long memcg_page_state_output(struct mem_cgroup *memcg, int item)
 		memcg_page_state_output_unit(item);
 }
 
-unsigned long memcg_page_state_local_output(struct mem_cgroup *memcg, int item)
-{
-	return memcg_page_state_local(memcg, item) *
-		memcg_page_state_output_unit(item);
-}
-
 #ifdef CONFIG_HUGETLB_PAGE
 static bool memcg_accounts_hugetlb(void)
 {
-- 
2.34.1

Re: [PATCH v3 next 5/5] memcg: move the 'local' functions to memcontrol-v1.c

[Johannes Weiner]

On Fri, Jan 17, 2025 at 01:46:45AM +0000, Chen Ridong wrote:
> From: Chen Ridong <chenridong@huawei.com>
> 
> Move the 'local' functions, which are only used in memcg v1, to the
> memcontrol-v1.c.
> 
> Signed-off-by: Chen Ridong <chenridong@huawei.com>

This makes sense. Without the refactoring from the preceding patch:

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