The code under "assign_new_owner" looks very ugly and suboptimal.
We do not really need get_task_struct/put_task_struct(), we can
simply recheck/change mm->owner under tasklist_lock. And we do not
want to restart from the very beginning if ->mm was changed by the
time we take task_lock(), we can simply continue (if we do not drop
tasklist_lock).
Just move this code into the new simple helper, assign_new_owner().
Signed-off-by: Oleg Nesterov <oleg@xxxxxxxxxx>
---
kernel/exit.c | 56 ++++++++++++++++++++++++++------------------------------
1 files changed, 26 insertions(+), 30 deletions(-)
diff --git a/kernel/exit.c b/kernel/exit.c
index 22fcc05..4d446ab 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -293,6 +293,23 @@ kill_orphaned_pgrp(struct task_struct *tsk, struct task_struct *parent)
}
#ifdef CONFIG_MEMCG
+static bool assign_new_owner(struct mm_struct *mm, struct task_struct *c)
+{
+ bool ret = false;
+
+ if (c->mm != mm)
+ return ret;
+
+ task_lock(c); /* protects c->mm from changing */
+ if (c->mm == mm) {
+ mm->owner = c;
+ ret = true;
+ }
+ task_unlock(c);
+
+ return ret;
+}
+
/*
* A task is exiting. If it owned this mm, find a new owner for the mm.
*/
@@ -300,7 +317,6 @@ void mm_update_next_owner(struct mm_struct *mm)
{
struct task_struct *c, *g, *p = current;
-retry:
/*
* If the exiting or execing task is not the owner, it's
* someone else's problem.
@@ -322,16 +338,16 @@ retry:
* Search in the children
*/
list_for_each_entry(c, &p->children, sibling) {
- if (c->mm == mm)
- goto assign_new_owner;
+ if (assign_new_owner(mm, c))
+ goto done;
}
/*
* Search in the siblings
*/
list_for_each_entry(c, &p->real_parent->children, sibling) {
- if (c->mm == mm)
- goto assign_new_owner;
+ if (assign_new_owner(mm, c))
+ goto done;
}
/*
@@ -341,42 +357,22 @@ retry:
if (g->flags & PF_KTHREAD)
continue;
for_each_thread(g, c) {
- if (c->mm == mm)
- goto assign_new_owner;
+ if (assign_new_owner(mm, c))
+ goto done;
if (c->mm)
break;
}
}
- read_unlock(&tasklist_lock);
+
/*
* We found no owner yet mm_users > 1: this implies that we are
* most likely racing with swapoff (try_to_unuse()) or /proc or
* ptrace or page migration (get_task_mm()). Mark owner as NULL.
*/
mm->owner = NULL;
- return;
-
-assign_new_owner:
- BUG_ON(c == p);
- get_task_struct(c);
- /*
- * The task_lock protects c->mm from changing.
- * We always want mm->owner->mm == mm
- */
- task_lock(c);
- /*
- * Delay read_unlock() till we have the task_lock()
- * to ensure that c does not slip away underneath us
- */
+done:
read_unlock(&tasklist_lock);
- if (c->mm != mm) {
- task_unlock(c);
- put_task_struct(c);
- goto retry;
- }
- mm->owner = c;
- task_unlock(c);
- put_task_struct(c);
+ return;
}
#endif /* CONFIG_MEMCG */
--
1.5.5.1
--
To unsubscribe from this list: send the line "unsubscribe cgroups" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
