Skip to contentSkip to conversation

Fix TASKLET_STATE_SCHED WARN_ON()

PATCH In review john stultz 3 participants 5 messages Jun 6, 2007 – Jun 14, 2007

lore
Share & tools
lore

john stultz

author
#0 lore raw

PATCH 1/1 Fix TASKLET_STATE_SCHED WARN_ON()

Hey Ingo,
So we've been seeing the following trace fairly frequently on our SMP
boxes when running kernbench:

BUG: at kernel/softirq.c:639 __tasklet_action()

Call Trace:
 [<ffffffff8106d5da>] dump_trace+0xaa/0x32a
 [<ffffffff8106d89b>] show_trace+0x41/0x5c
 [<ffffffff8106d8cb>] dump_stack+0x15/0x17
 [<ffffffff81094a97>] __tasklet_action+0xdf/0x12e
 [<ffffffff81094f76>] tasklet_action+0x27/0x29
 [<ffffffff8109530a>] ksoftirqd+0x16c/0x271
 [<ffffffff81033d4d>] kthread+0xf5/0x128
 [<ffffffff8105ff68>] child_rip+0xa/0x12

Paul also pointed this out awhile back: http://lkml.org/lkml/2007/2/25/1

Anyway, I think I finally found the issue. Its a bit hard to explain, but the idea is while __tasklet_action is running the tasklet function on CPU1, if a call to tasklet_schedule() on CPU2 is made, and if right after we mark the TASKLET_STATE_SCHED bit we are preempted, __tasklet_action on CPU1 might be able to re-run the function, clear the bit and unlock the tasklet before CPU2 enters __tasklet_common_schedule. Once __tasklet_common_schedule locks the tasklet, we will add the tasklet to the list with the TASKLET_STATE_SCHED *unset*.

I've verified this race occurs w/ a WARN_ON in
__tasklet_common_schedule().

This fix avoids this race by making sure *after* we've locked the tasklet that the STATE_SCHED bit is set before adding it to the list.

Does it look ok to you?

thanks
-john

Signed-off-by John Stultz <[email protected]>
kernel/softirq.c +11 -4
Index: 2.6-rt/kernel/softirq.c
===================================================================
@@ -544,10 +544,17 @@ static void inline
 __tasklet_common_schedule(struct tasklet_struct *t, struct tasklet_head *head, unsigned int nr)
 {
 	if (tasklet_trylock(t)) {
-		WARN_ON(t->next != NULL);
-		t->next = head->list;
-		head->list = t;
-		raise_softirq_irqoff(nr);
+		/* We may have been preempted before tasklet_trylock
+		 * and __tasklet_action may have already run.
+		 * So double check the sched bit while the takslet
+		 * is locked before adding it to the list.
+		 */
+		if (test_bit(TASKLET_STATE_SCHED, &t->state)) {
+			WARN_ON(t->next != NULL);
+			t->next = head->list;
+			head->list = t;
+			raise_softirq_irqoff(nr);
+		}
 		tasklet_unlock(t);
 	}
 }
#1 lore raw
Quoted 4 lines · john stultz

This fix avoids this race by making sure *after* we've locked the tasklet that the STATE_SCHED bit is set before adding it to the list.

Does it look ok to you?

ah - nice!! What would be the worst-case effect of this bug? (besides the WARN_ON()?) In any case, this fix will be in -rt10. Great work!

Ingo

#3 lore raw
In reply to john stultz

kernel/softirq.c 2007-06-05 18:36:44.000000000 -0700 static void inline · line 549

Quoted 13 lines · john stultz
kernel/softirq.c 2007-06-05 18:36:44.000000000 -0700 renamed from kernel/softirq.c 2007-06-05 18:30:54.000000000 -0700 +3 -4
@@ -544,10 +544,17 @@ static void inline
 __tasklet_common_schedule(struct tasklet_struct *t, struct tasklet_head *head, unsigned int nr)
 {
        if (tasklet_trylock(t)) {
-               WARN_ON(t->next != NULL);
-               t->next = head->list;
-               head->list = t;
-               raise_softirq_irqoff(nr);
+               /* We may have been preempted before tasklet_trylock
+                * and __tasklet_action may have already run.
+                * So double check the sched bit while the takslet

s/takslet/tasklet/

Signature · 3 lines
Jesper Juhl <[email protected]>
Don't top-post  http://www.catb.org/~esr/jargon/html/T/top-post.html
Plain text mails only, please      http://www.expita.com/nomime.html
author
#2 lore raw
In reply to Ingo Molnar
Quoted 9 lines · Ingo Molnar
Quoted 4 lines · john stultz

This fix avoids this race by making sure *after* we've locked the tasklet that the STATE_SCHED bit is set before adding it to the list.

Does it look ok to you?

ah - nice!! What would be the worst-case effect of this bug? (besides the WARN_ON()?) In any case, this fix will be in -rt10. Great work!

The only side effect I can come up w/ is you might run a tasklet 3 times if it was only scheduled twice.

thanks
-john

author
#4 lore raw
In reply to john stultz
Quoted 59 lines · john stultz

Hey Ingo,
So we've been seeing the following trace fairly frequently on our SMP
boxes when running kernbench:

BUG: at kernel/softirq.c:639 __tasklet_action()

Call Trace:
 [<ffffffff8106d5da>] dump_trace+0xaa/0x32a
 [<ffffffff8106d89b>] show_trace+0x41/0x5c
 [<ffffffff8106d8cb>] dump_stack+0x15/0x17
 [<ffffffff81094a97>] __tasklet_action+0xdf/0x12e
 [<ffffffff81094f76>] tasklet_action+0x27/0x29
 [<ffffffff8109530a>] ksoftirqd+0x16c/0x271
 [<ffffffff81033d4d>] kthread+0xf5/0x128
 [<ffffffff8105ff68>] child_rip+0xa/0x12

Paul also pointed this out awhile back: http://lkml.org/lkml/2007/2/25/1

Anyway, I think I finally found the issue. Its a bit hard to explain, but the idea is while __tasklet_action is running the tasklet function on CPU1, if a call to tasklet_schedule() on CPU2 is made, and if right after we mark the TASKLET_STATE_SCHED bit we are preempted, __tasklet_action on CPU1 might be able to re-run the function, clear the bit and unlock the tasklet before CPU2 enters __tasklet_common_schedule. Once __tasklet_common_schedule locks the tasklet, we will add the tasklet to the list with the TASKLET_STATE_SCHED *unset*.

I've verified this race occurs w/ a WARN_ON in
__tasklet_common_schedule().

This fix avoids this race by making sure *after* we've locked the tasklet that the STATE_SCHED bit is set before adding it to the list.

Does it look ok to you?

thanks
-john

Signed-off-by John Stultz <[email protected]>

Index: 2.6-rt/kernel/softirq.c
===================================================================

kernel/softirq.c +4 -4
@@ -544,10 +544,17 @@ static void inline
 __tasklet_common_schedule(struct tasklet_struct *t, struct tasklet_head *head, unsigned int nr)
 {
 	if (tasklet_trylock(t)) {
-		WARN_ON(t->next != NULL);
-		t->next = head->list;
-		head->list = t;
-		raise_softirq_irqoff(nr);
+		/* We may have been preempted before tasklet_trylock
+		 * and __tasklet_action may have already run.
+		 * So double check the sched bit while the takslet
+		 * is locked before adding it to the list.

kernel/softirq.c static void inline · line 560

Quoted 10 lines
kernel/softirq.c +7 -0
+		 */
+		if (test_bit(TASKLET_STATE_SCHED, &t->state)) {
+			WARN_ON(t->next != NULL);
+			t->next = head->list;
+			head->list = t;
+			raise_softirq_irqoff(nr);
+		}
 		tasklet_unlock(t);
 	}
 }

So while digging on a strange OOM issue we were seeing (which actually ended up being fixed by Steven's softirq patch), I noticed that the fix above is incomplete. With only the patch above, we may no longer have unscheduled tasklets added to the list, but we may end up with scheduled tasklets that are not on the list (and will stay that way!).

The following additional patch should correct this issue. Although since we weren't actually hitting it, the issue is a bit theoretical, so I've not been able to prove it really fixes anything.

thanks
-john

kernel/softirq.c +15 -1
Index: 2.6-rt/kernel/softirq.c
===================================================================
@@ -544,6 +544,7 @@
 __tasklet_common_schedule(struct tasklet_struct *t, struct tasklet_head *head, unsigned int nr)
 {
 	if (tasklet_trylock(t)) {
+again:
 		/* We may have been preempted before tasklet_trylock
 		 * and __tasklet_action may have already run.
 		 * So double check the sched bit while the takslet

@@ -554,8 +555,21 @@
 			t->next = head->list;
 			head->list = t;
 			raise_softirq_irqoff(nr);
+			tasklet_unlock(t);
+		} else {
+			/* This is subtle. If we hit the corner case above
+			 * It is possible that we get preempted right here,
+			 * and another task has successfully called
+			 * tasklet_schedule(), then this function, and
+			 * failed on the trylock. Thus we must be sure
+			 * before releasing the tasklet lock, that the
+			 * SCHED_BIT is clear. Otherwise the tasklet
+			 * may get its SCHED_BIT set, but not added to the
+			 * list
+			 */
+			if (!tasklet_tryunlock(t))
+				goto again;
 		}
-		tasklet_unlock(t);
 	}
 }