[PATCH] cgroup: minor optimization around the usage of cur_tasks_head

[PATCH] cgroup: minor optimization around the usage of cur_tasks_head

[Yafang Shao]

Recently there was an issue occurred on our production envrionment with a
very old kernel version 4.19. That issue can be fixed by upstream
commit 9c974c772464 ("cgroup: Iterate tasks that did not finish do_exit()")

When I was trying to fix that issue on our production environment, I found
we can create a hotfix with a simplified version of the commit -

As the usage of cur_tasks_head is within the function
css_task_iter_advance(), we can make it as a local variable. That could
make it more clear and easier to understand. Another benefit is we don't
need to carry it in css_task_iter.

Signed-off-by: Yafang Shao <laoar.shao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Michal Koutn√Ω <mkoutny-IBi9RG/b67k@public.gmane.org>
---
 include/linux/cgroup.h |  1 -
 kernel/cgroup/cgroup.c | 29 ++++++++++++++++-------------
 2 files changed, 16 insertions(+), 14 deletions(-)

diff --git a/include/linux/cgroup.h b/include/linux/cgroup.h
index 75c151413fda..f619a92d0fa0 100644
--- a/include/linux/cgroup.h
+++ b/include/linux/cgroup.h
@@ -61,7 +61,6 @@ struct css_task_iter {
 
 	struct list_head		*task_pos;
 
-	struct list_head		*cur_tasks_head;
 	struct css_set			*cur_cset;
 	struct css_set			*cur_dcset;
 	struct task_struct		*cur_task;
diff --git a/kernel/cgroup/cgroup.c b/kernel/cgroup/cgroup.c
index 919194de39c8..ffb0c863b97a 100644
--- a/kernel/cgroup/cgroup.c
+++ b/kernel/cgroup/cgroup.c
@@ -4545,10 +4545,12 @@ static struct css_set *css_task_iter_next_css_set(struct css_task_iter *it)
 /**
  * css_task_iter_advance_css_set - advance a task iterator to the next css_set
  * @it: the iterator to advance
+ * @cur_tasks_head: head of current tasks
  *
  * Advance @it to the next css_set to walk.
  */
-static void css_task_iter_advance_css_set(struct css_task_iter *it)
+static void css_task_iter_advance_css_set(struct css_task_iter *it,
+				struct list_head **cur_tasks_head)
 {
 	struct css_set *cset;
 
@@ -4557,13 +4559,13 @@ static void css_task_iter_advance_css_set(struct css_task_iter *it)
 	/* Advance to the next non-empty css_set and find first non-empty tasks list*/
 	while ((cset = css_task_iter_next_css_set(it))) {
 		if (!list_empty(&cset->tasks)) {
-			it->cur_tasks_head = &cset->tasks;
+			*cur_tasks_head = &cset->tasks;
 			break;
 		} else if (!list_empty(&cset->mg_tasks)) {
-			it->cur_tasks_head = &cset->mg_tasks;
+			*cur_tasks_head = &cset->mg_tasks;
 			break;
 		} else if (!list_empty(&cset->dying_tasks)) {
-			it->cur_tasks_head = &cset->dying_tasks;
+			*cur_tasks_head = &cset->dying_tasks;
 			break;
 		}
 	}
@@ -4571,7 +4573,7 @@ static void css_task_iter_advance_css_set(struct css_task_iter *it)
 		it->task_pos = NULL;
 		return;
 	}
-	it->task_pos = it->cur_tasks_head->next;
+	it->task_pos = (*cur_tasks_head)->next;
 
 	/*
 	 * We don't keep css_sets locked across iteration steps and thus
@@ -4610,6 +4612,7 @@ static void css_task_iter_skip(struct css_task_iter *it,
 
 static void css_task_iter_advance(struct css_task_iter *it)
 {
+	struct list_head *cur_tasks_head = NULL;
 	struct task_struct *task;
 
 	lockdep_assert_held(&css_set_lock);
@@ -4626,18 +4629,18 @@ static void css_task_iter_advance(struct css_task_iter *it)
 			it->task_pos = it->task_pos->next;
 
 		if (it->task_pos == &it->cur_cset->tasks) {
-			it->cur_tasks_head = &it->cur_cset->mg_tasks;
-			it->task_pos = it->cur_tasks_head->next;
+			cur_tasks_head = &it->cur_cset->mg_tasks;
+			it->task_pos = cur_tasks_head->next;
 		}
 		if (it->task_pos == &it->cur_cset->mg_tasks) {
-			it->cur_tasks_head = &it->cur_cset->dying_tasks;
-			it->task_pos = it->cur_tasks_head->next;
+			cur_tasks_head = &it->cur_cset->dying_tasks;
+			it->task_pos = cur_tasks_head->next;
 		}
 		if (it->task_pos == &it->cur_cset->dying_tasks)
-			css_task_iter_advance_css_set(it);
+			css_task_iter_advance_css_set(it, &cur_tasks_head);
 	} else {
 		/* called from start, proceed to the first cset */
-		css_task_iter_advance_css_set(it);
+		css_task_iter_advance_css_set(it, &cur_tasks_head);
 	}
 
 	if (!it->task_pos)
@@ -4651,12 +4654,12 @@ static void css_task_iter_advance(struct css_task_iter *it)
 			goto repeat;
 
 		/* and dying leaders w/o live member threads */
-		if (it->cur_tasks_head == &it->cur_cset->dying_tasks &&
+		if (cur_tasks_head == &it->cur_cset->dying_tasks &&
 		    !atomic_read(&task->signal->live))
 			goto repeat;
 	} else {
 		/* skip all dying ones */
-		if (it->cur_tasks_head == &it->cur_cset->dying_tasks)
+		if (cur_tasks_head == &it->cur_cset->dying_tasks)
 			goto repeat;
 	}
 }
-- 
2.17.1

Re: [PATCH] cgroup: minor optimization around the usage of cur_tasks_head

[Michal Koutný]

Hello Yafang.

On Wed, Jan 26, 2022 at 02:17:05PM +0000, Yafang Shao <laoar.shao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org> wrote:
> As the usage of cur_tasks_head is within the function
> css_task_iter_advance(), we can make it as a local variable. That could
> make it more clear and easier to understand. Another benefit is we don't
> need to carry it in css_task_iter.

It looks correct. When refactoring in the sake of understandibility
(disputable :), wouldn't it be better to avoid the double-pointer arg
passed into css_task_iter_advance_css_set() and just return the new
cur_tasks_head (the input value doesn't seem relevant)?

Thanks,
Michal

Re: [PATCH] cgroup: minor optimization around the usage of cur_tasks_head

[Tejun Heo]

On Wed, Jan 26, 2022 at 02:17:05PM +0000, Yafang Shao wrote:
> Recently there was an issue occurred on our production envrionment with a
> very old kernel version 4.19. That issue can be fixed by upstream
> commit 9c974c772464 ("cgroup: Iterate tasks that did not finish do_exit()")
> 
> When I was trying to fix that issue on our production environment, I found
> we can create a hotfix with a simplified version of the commit -
> 
> As the usage of cur_tasks_head is within the function
> css_task_iter_advance(), we can make it as a local variable. That could
> make it more clear and easier to understand. Another benefit is we don't
> need to carry it in css_task_iter.
> 
> Signed-off-by: Yafang Shao <laoar.shao-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
> Cc: Michal Koutný <mkoutny-IBi9RG/b67k@public.gmane.org>

I can't tell whether this is better or not. Sure, it loses one pointer from
the struct but that doesn't really gain anything practical. On the other
hand, before, we could understand where the iteration was by just dumping
the struct. After, we can't. At best, maybe this change is a wash.

Thanks.

-- 
tejun