[PATCH RESEND 1/6] memcontrol: rename mem_cgroup_scan_tasks()

xu.xin16@zte.com.cn posted 6 patches 9 months, 3 weeks ago
There is a newer version of this series
[PATCH RESEND 1/6] memcontrol: rename mem_cgroup_scan_tasks()
Posted by xu xin 9 months, 3 weeks ago
Current Issue:
==============
The function mem_cgroup_scan_tasks in memcontrol.c has a naming ambiguity.
While its name suggests it only iterates through processes belonging to
the current memcgroup, it actually scans all descendant cgroups under the
subtree rooted at this memcgroup. This discrepancy can cause confusion
for developers relying on the semantic meaning of the function name.

Resolution:
=========
Renaming: We have renamed the original function to
**mem_cgroup_tree_scan_tasks** to explicitly reflect its subtree-traversal
behavior.

A subsequent patch will introduce a new mem_cgroup_scan_tasks function that
strictly iterates processes only within the current memcgroup, aligning its
behavior with its name.

Signed-off-by: xu xin <xu.xin16@zte.com.cn>
---
 include/linux/memcontrol.h | 4 ++--
 mm/memcontrol.c            | 4 ++--
 mm/oom_kill.c              | 6 +++---
 3 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/linux/memcontrol.h b/include/linux/memcontrol.h
index 5264d148bdd9..1c1ce25fae4c 100644
--- a/include/linux/memcontrol.h
+++ b/include/linux/memcontrol.h
@@ -795,7 +795,7 @@ struct mem_cgroup *mem_cgroup_iter(struct mem_cgroup *,
 				   struct mem_cgroup *,
 				   struct mem_cgroup_reclaim_cookie *);
 void mem_cgroup_iter_break(struct mem_cgroup *, struct mem_cgroup *);
-void mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
+void mem_cgroup_tree_scan_tasks(struct mem_cgroup *memcg,
 			   int (*)(struct task_struct *, void *), void *arg);
 
 static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
@@ -1289,7 +1289,7 @@ static inline void mem_cgroup_iter_break(struct mem_cgroup *root,
 {
 }
 
-static inline void mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
+static inline void mem_cgroup_tree_scan_tasks(struct mem_cgroup *memcg,
 		int (*fn)(struct task_struct *, void *), void *arg)
 {
 }
diff --git a/mm/memcontrol.c b/mm/memcontrol.c
index 6bc6dade60d8..3baf0a4e0674 100644
--- a/mm/memcontrol.c
+++ b/mm/memcontrol.c
@@ -1164,7 +1164,7 @@ static void invalidate_reclaim_iterators(struct mem_cgroup *dead_memcg)
 }
 
 /**
- * mem_cgroup_scan_tasks - iterate over tasks of a memory cgroup hierarchy
+ * mem_cgroup_tree_scan_tasks - iterate over tasks of a memory cgroup hierarchy
  * @memcg: hierarchy root
  * @fn: function to call for each task
  * @arg: argument passed to @fn
@@ -1176,7 +1176,7 @@ static void invalidate_reclaim_iterators(struct mem_cgroup *dead_memcg)
  *
  * This function must not be called for the root memory cgroup.
  */
-void mem_cgroup_scan_tasks(struct mem_cgroup *memcg,
+void mem_cgroup_tree_scan_tasks(struct mem_cgroup *memcg,
 			   int (*fn)(struct task_struct *, void *), void *arg)
 {
 	struct mem_cgroup *iter;
diff --git a/mm/oom_kill.c b/mm/oom_kill.c
index 25923cfec9c6..af3b8407fb08 100644
--- a/mm/oom_kill.c
+++ b/mm/oom_kill.c
@@ -367,7 +367,7 @@ static void select_bad_process(struct oom_control *oc)
 	oc->chosen_points = LONG_MIN;
 
 	if (is_memcg_oom(oc))
-		mem_cgroup_scan_tasks(oc->memcg, oom_evaluate_task, oc);
+		mem_cgroup_tree_scan_tasks(oc->memcg, oom_evaluate_task, oc);
 	else {
 		struct task_struct *p;
 
@@ -428,7 +428,7 @@ static void dump_tasks(struct oom_control *oc)
 	pr_info("[  pid  ]   uid  tgid total_vm      rss rss_anon rss_file rss_shmem pgtables_bytes swapents oom_score_adj name\n");
 
 	if (is_memcg_oom(oc))
-		mem_cgroup_scan_tasks(oc->memcg, dump_task, oc);
+		mem_cgroup_tree_scan_tasks(oc->memcg, dump_task, oc);
 	else {
 		struct task_struct *p;
 		int i = 0;
@@ -1056,7 +1056,7 @@ static void oom_kill_process(struct oom_control *oc, const char *message)
 	if (oom_group) {
 		memcg_memory_event(oom_group, MEMCG_OOM_GROUP_KILL);
 		mem_cgroup_print_oom_group(oom_group);
-		mem_cgroup_scan_tasks(oom_group, oom_kill_memcg_member,
+		mem_cgroup_tree_scan_tasks(oom_group, oom_kill_memcg_member,
 				      (void *)message);
 		mem_cgroup_put(oom_group);
 	}
-- 
2.39.3
Re: [PATCH RESEND 1/6] memcontrol: rename mem_cgroup_scan_tasks()
Posted by Andrew Morton 9 months, 3 weeks ago
On Tue, 22 Apr 2025 11:19:19 +0000 xu xin <xu.xin.sc@gmail.com> wrote:

> From: xu xin <xu.xin.sc@gmail.com>
> To: xu.xin16@zte.com.cn
> Cc: akpm@linux-foundation.org, david@redhat.com, linux-fsdevel@vger.kernel.org, linux-kernel@vger.kernel.org, linux-mm@kvack.org, wang.yaxin@zte.com.cn, yang.yang29@zte.com.cn
> Subject: [PATCH RESEND 1/6] memcontrol: rename mem_cgroup_scan_tasks()
> Date: Tue, 22 Apr 2025 11:19:19 +0000
> 
> ...
>
> Signed-off-by: xu xin <xu.xin16@zte.com.cn>

It's clear what was meant, but please include the explicit

	From: xu xin <xu.xin16@zte.com.cn>

within each changelog, rather than only in the [0/N].

Patchset looks nice to me, thanks.  I'll await reviewer feedback before
proceeding.
Re: [PATCH RESEND 1/6] memcontrol: rename mem_cgroup_scan_tasks()
Posted by Matthew Wilcox 9 months, 3 weeks ago
On Tue, Apr 22, 2025 at 04:29:52PM -0700, Andrew Morton wrote:
> Patchset looks nice to me, thanks.  I'll await reviewer feedback before
> proceeding.

I thought we had a policy against adding new features to memcg-v1?
Certainly adding the feature to memcg-v2 would be a requirement before
it could be added to v1.
Re: [PATCH RESEND 1/6] memcontrol: rename mem_cgroup_scan_tasks()
Posted by Michal Hocko 9 months, 2 weeks ago
On Wed 23-04-25 13:22:07, Matthew Wilcox wrote:
> On Tue, Apr 22, 2025 at 04:29:52PM -0700, Andrew Morton wrote:
> > Patchset looks nice to me, thanks.  I'll await reviewer feedback before
> > proceeding.
> 
> I thought we had a policy against adding new features to memcg-v1?
> Certainly adding the feature to memcg-v2 would be a requirement before
> it could be added to v1.

Completely agreed! Is there any reason why v2 was not the first choice?
-- 
Michal Hocko
SUSE Labs