Skip to contentSkip to conversation

sched: reset sched_entity depth on changing parent

PATCH In review Burke Libbey 4 participants 12 messages Oct 24, 2014 – Oct 27, 2014 Scheduler

lore
Share & tools
lore

Burke Libbey

author
#0 lore raw

PATCH 1/1 sched: reset sched_entity depth on changing parent

From 2014-02-15: https://lkml.org/lkml/2014/2/15/217

This issue was reported and patched, but it still occurs in some situations on newer kernel versions.

[2249353.328452] BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
[2249353.336528] IP: [<ffffffff810b1cf7>] check_preempt_wakeup+0xe7/0x210

se.parent gets out of sync with se.depth, causing a panic when the algorithm in find_matching_se assumes they are correct. This patch forces se.depth to be updated every time se.parent is, so they can no longer become desync'd.

CC Ingo Molnar <[email protected]>
CC Peter Zijlstra <[email protected]>
Signed-off-by Burke Libbey <[email protected]>

---

I haven't been able to isolate the problem. Though I'm pretty confident this fixes the issue I've been having, I have not been able to prove it.

I kept a debugging journal if you want more context for whatever reason:
https://gist.github.com/burke/c60dc5b8f0ba9bfd9275

kernel/sched/sched.h +1 -0
index 24156c84..bcffba2 100644
@@ -844,6 +844,7 @@ static inline void set_task_rq(struct task_struct *p, unsigned int cpu)
 #ifdef CONFIG_FAIR_GROUP_SCHED
  p->se.cfs_rq = tg->cfs_rq[cpu];
  p->se.parent = tg->se[cpu];
+ p->se.depth = p->se.parent ? p->se.parent->depth + 1 : 0;
 #endif
 
 #ifdef CONFIG_RT_GROUP_SCHED
maintainer
#1 lore raw
Quoted 12 lines · Burke Libbey

From 2014-02-15: https://lkml.org/lkml/2014/2/15/217

This issue was reported and patched, but it still occurs in some situations on newer kernel versions.

[2249353.328452] BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
[2249353.336528] IP: [<ffffffff810b1cf7>] check_preempt_wakeup+0xe7/0x210

se.parent gets out of sync with se.depth, causing a panic when the algorithm in find_matching_se assumes they are correct. This patch forces se.depth to be updated every time se.parent is, so they can no longer become desync'd.

Quoted 7 lines
CC Ingo Molnar <[email protected]>
CC Peter Zijlstra <[email protected]>
Signed-off-by Burke Libbey <[email protected]>

---

I haven't been able to isolate the problem. Though I'm pretty confident this fixes the issue I've been having, I have not been able to prove it.

So this isn't correct, switching rq should not change depth. I suspect you're just papering over the issue by frequently resetting the value, which simply narrows the race window.

#2 lore raw

24.10.2014, 19:58, "Peter Zijlstra" <[email protected]>:

Quoted 18 lines · Burke Libbey
Quoted 17 lines

 From 2014-02-15: https://lkml.org/lkml/2014/2/15/217

 This issue was reported and patched, but it still occurs in some situations on newer kernel versions.

 [2249353.328452] BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
 [2249353.336528] IP: [<ffffffff810b1cf7>] check_preempt_wakeup+0xe7/0x210

 se.parent gets out of sync with se.depth, causing a panic when the algorithm in find_matching_se assumes they are correct. This patch forces se.depth to be updated every time se.parent is, so they can no longer become desync'd.

CC Ingo Molnar <[email protected]>
CC Peter Zijlstra <[email protected]>
Signed-off-by Burke Libbey <[email protected]>

 ---

Quoted 6 lines
Quoted 2 lines

 I haven't been able to isolate the problem. Though I'm pretty confident this fixes the issue I've been having, I have not been able to prove it.

So this isn't correct, switching rq should not change depth. I suspect you're just papering over the issue by frequently resetting the value, which simply narrows the race window.

Just a hypothesis.

I was seeking a places where task_group of a task may change. I can't understand how changing of parent's cgroup during fork() applies to a child.

Child's cgroup is the same as parent's after dup_task_struct(). The only function changing task_group is sched_move_task(), but we do not call it between dup_task_struct() and wake_up_new_task(). Shouldn't we do something like this?

(compile tested only)
---

kernel/sched/core.c +6 -0
index cc18694..0ccbbdb 100644
@@ -7833,6 +7833,11 @@ static void cpu_cgroup_css_offline(struct cgroup_subsys_state *css)
 	sched_offline_group(tg);
 }
 
+static void cpu_cgroup_fork(struct task_struct *task)
+{
+	sched_move_task(task);
+}
+
 static int cpu_cgroup_can_attach(struct cgroup_subsys_state *css,
 				 struct cgroup_taskset *tset)
 {

@@ -8205,6 +8210,7 @@ struct cgroup_subsys cpu_cgrp_subsys = {
 	.css_free	= cpu_cgroup_css_free,
 	.css_online	= cpu_cgroup_css_online,
 	.css_offline	= cpu_cgroup_css_offline,
+	.fork		= cpu_cgroup_fork,
 	.can_attach	= cpu_cgroup_can_attach,
 	.attach		= cpu_cgroup_attach,
 	.exit		= cpu_cgroup_exit,

Or we just should set tsk->sched_task_group?

#3 lore raw

I've dived into this and found, we are really need this. I'll send a patch with description soon.

24.10.2014, 22:19, "Kirill Tkhai" <[email protected]>:

Quoted 67 lines

24.10.2014, 19:58, "Peter Zijlstra" <[email protected]>:

Quoted 18 lines · Burke Libbey
Quoted 17 lines

  From 2014-02-15: https://lkml.org/lkml/2014/2/15/217

  This issue was reported and patched, but it still occurs in some situations on newer kernel versions.

  [2249353.328452] BUG: unable to handle kernel NULL pointer dereference at 0000000000000150
  [2249353.336528] IP: [<ffffffff810b1cf7>] check_preempt_wakeup+0xe7/0x210

  se.parent gets out of sync with se.depth, causing a panic when the algorithm in find_matching_se assumes they are correct. This patch forces se.depth to be updated every time se.parent is, so they can no longer become desync'd.

CC Ingo Molnar <[email protected]>
CC Peter Zijlstra <[email protected]>
Signed-off-by Burke Libbey <[email protected]>

  ---

Quoted 5 lines
Quoted 2 lines

  I haven't been able to isolate the problem. Though I'm pretty confident this fixes the issue I've been having, I have not been able to prove it.

 So this isn't correct, switching rq should not change depth. I suspect you're just papering over the issue by frequently resetting the value, which simply narrows the race window.

Just a hypothesis.

I was seeking a places where task_group of a task may change. I can't understand how changing of parent's cgroup during fork() applies to a child.

Child's cgroup is the same as parent's after dup_task_struct(). The only function changing task_group is sched_move_task(), but we do not call it between dup_task_struct() and wake_up_new_task(). Shouldn't we do something like this?

(compile tested only)
---

kernel/sched/core.c +6 -0
index cc18694..0ccbbdb 100644
@@ -7833,6 +7833,11 @@ static void cpu_cgroup_css_offline(struct cgroup_subsys_state *css)
          sched_offline_group(tg);
  }
 
+static void cpu_cgroup_fork(struct task_struct *task)
+{
+ sched_move_task(task);
+}
+
  static int cpu_cgroup_can_attach(struct cgroup_subsys_state *css,
                                   struct cgroup_taskset *tset)
  {

@@ -8205,6 +8210,7 @@ struct cgroup_subsys cpu_cgrp_subsys = {
          .css_free = cpu_cgroup_css_free,
          .css_online = cpu_cgroup_css_online,
          .css_offline = cpu_cgroup_css_offline,
+ .fork = cpu_cgroup_fork,
          .can_attach = cpu_cgroup_can_attach,
          .attach = cpu_cgroup_attach,
          .exit = cpu_cgroup_exit,

Or we just should set tsk->sched_task_group?
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

maintainer
#4 lore raw
In reply to Kirill Tkhai
Quoted 2 lines · Kirill Tkhai

I was seeking a places where task_group of a task may change. I can't understand how changing of parent's cgroup during fork() applies to a child.

I didn't know we could change cgroup on fork(), I though the idea was you always inherited your parents cgroup.

How can this be?

#5 lore raw
Quoted 8 lines · Peter Zijlstra
Quoted 2 lines · Kirill Tkhai

I was seeking a places where task_group of a task may change. I can't understand how changing of parent's cgroup during fork() applies to a child.

I didn't know we could change cgroup on fork(), I though the idea was you always inherited your parents cgroup.

How can this be?

Hmmm? -ENEEDMORECONTEXT but the inheriting happens at one point during fork in cgroup_post_fork(). The child inherits whatever the parent cgroup is at that point.

Thanks.

Signature · 1 line
tejun
maintainer
#6 lore raw
Quoted 5 lines · Tejun Heo
Quoted 4 lines · Peter Zijlstra
Quoted 2 lines · Kirill Tkhai

I was seeking a places where task_group of a task may change. I can't understand how changing of parent's cgroup during fork() applies to a child.

Quoted 8 lines
Quoted 4 lines

I didn't know we could change cgroup on fork(), I though the idea was you always inherited your parents cgroup.

How can this be?

Hmmm? -ENEEDMORECONTEXT but the inheriting happens at one point during fork in cgroup_post_fork(). The child inherits whatever the parent cgroup is at that point.

So Kirill is saying that there is a race between fork and attach such that a child can end up in a different cgroup than the parent and we need to use the cgroup_subsys::fork call to fix that up.

I was always under the impression that fork was an 'atomic' operation from the point of cgroups, an attach (or move) would happen either before the fork or after, not during. But I appear to be mistaken in that assumption, going by the comments around cgroup_post_fork().

#7 lore raw

27.10.2014, 16:28, "Peter Zijlstra" <[email protected]>:

Quoted 8 lines · Tejun Heo
Quoted 7 lines · Peter Zijlstra
Quoted 6 lines · Kirill Tkhai
Quoted 2 lines

 I was seeking a places where task_group of a task may change. I can't understand how changing of parent's cgroup during fork() applies to a child.

 I didn't know we could change cgroup on fork(), I though the idea was you always inherited your parents cgroup.

Quoted 8 lines
Quoted 4 lines
Quoted 1 line

 How can this be?

 Hmmm? -ENEEDMORECONTEXT but the inheriting happens at one point during fork in cgroup_post_fork().  The child inherits whatever the parent cgroup is at that point.

So Kirill is saying that there is a race between fork and attach such that a child can end up in a different cgroup than the parent and we need to use the cgroup_subsys::fork call to fix that up.

I mean cgroup is the same, but sched_task_group is other (sched_task_group is equal to parent's on the moment of dup_task_struct()).

Quoted 4 lines

I was always under the impression that fork was an 'atomic' operation from the point of cgroups, an attach (or move) would happen either before the fork or after, not during. But I appear to be mistaken in that assumption, going by the comments around cgroup_post_fork().

maintainer
#8 lore raw
Quoted 16 lines · Kirill Tkhai

27.10.2014, 16:28, "Peter Zijlstra" <[email protected]>:

Quoted 13 lines · Tejun Heo
Quoted 11 lines · Peter Zijlstra
Quoted 7 lines · Kirill Tkhai
Quoted 2 lines

 I was seeking a places where task_group of a task may change. I can't understand how changing of parent's cgroup during fork() applies to a child.

 I didn't know we could change cgroup on fork(), I though the idea was you always inherited your parents cgroup.

 How can this be?

 Hmmm? -ENEEDMORECONTEXT but the inheriting happens at one point during fork in cgroup_post_fork().  The child inherits whatever the parent cgroup is at that point.

Quoted 6 lines
Quoted 3 lines

So Kirill is saying that there is a race between fork and attach such that a child can end up in a different cgroup than the parent and we need to use the cgroup_subsys::fork call to fix that up.

I mean cgroup is the same, but sched_task_group is other (sched_task_group is equal to parent's on the moment of dup_task_struct()).

But that still means the parent changed cgroup during fork right? It started out in a different cgroup than it ended up with, and we need that .fork callback to fixup state.

#9 lore raw

27.10.2014, 16:45, "Peter Zijlstra" <[email protected]>:

Quoted 17 lines · Kirill Tkhai
Quoted 16 lines

 27.10.2014, 16:28, "Peter Zijlstra" <[email protected]>:

Quoted 15 lines · Tejun Heo
Quoted 11 lines · Peter Zijlstra
Quoted 7 lines · Kirill Tkhai
Quoted 2 lines

  I was seeking a places where task_group of a task may change. I can't understand how changing of parent's cgroup during fork() applies to a child.

  I didn't know we could change cgroup on fork(), I though the idea was you always inherited your parents cgroup.

  How can this be?

  Hmmm? -ENEEDMORECONTEXT but the inheriting happens at one point during fork in cgroup_post_fork().  The child inherits whatever the parent cgroup is at that point.

 So Kirill is saying that there is a race between fork and attach such that a child can end up in a different cgroup than the parent and we need to use the cgroup_subsys::fork call to fix that up.

Quoted 6 lines
Quoted 2 lines

 I mean cgroup is the same, but sched_task_group is other (sched_task_group is equal to parent's on the moment of dup_task_struct()).

But that still means the parent changed cgroup during fork right? It started out in a different cgroup than it ended up with, and we need that .fork callback to fixup state.

Yeah, I'm agree.

#10 lore raw
In reply to Peter Zijlstra

Hello, Peter, Kirill.

On Mon, Oct 27, 2014 at 02:28:12PM +0100, Peter Zijlstra wrote:
...

Quoted 8 lines

So Kirill is saying that there is a race between fork and attach such that a child can end up in a different cgroup than the parent and we need to use the cgroup_subsys::fork call to fix that up.

I was always under the impression that fork was an 'atomic' operation from the point of cgroups, an attach (or move) would happen either before the fork or after, not during. But I appear to be mistaken in that assumption, going by the comments around cgroup_post_fork().

cgroup migration is atomic against threadgroup changes - tasks joining or leaving the threadgroup. If a process is forking a child, the child inherits the parent's cgroup at one point while forking and the parent may have changed its cgroup while fork was in progress. So, yeah, it can't depend on the dup'd result to be correct.

Thanks.

Signature · 1 line
tejun
#11 lore raw

On Mon, Oct 27, 2014 at 09:47:35AM -0400, Tejun Heo wrote:
...

Quoted 2 lines

cgroup migration is atomic against threadgroup changes - tasks joining or leaving the threadgroup. If a process is forking a child, the

                                                              ^
							      process
Signature · 1 line
tejun