All of lore.kernel.org
 help / color / mirror / Atom feed
From: Xiaotian Feng <xtfeng@gmail.com>
To: akpm@linux-foundation.org, linux-kernel@vger.kernel.org
Cc: Xiaotian Feng <xtfeng@gmail.com>, Xiaotian Feng <dannyfeng@tencent.com>
Subject: [RFC PATCH] tasklet: ignore disabled tasklet in tasklet_action
Date: Fri,  2 Nov 2012 10:48:54 +0800	[thread overview]
Message-ID: <1351824534-2861-1-git-send-email-xtfeng@gmail.com> (raw)

We met a ksoftirqd 100% issue, the perf top shows kernel is busy
with tasklet_action(), but no actual action is shown. From dumped
kernel, there's only one disabled tasklet on the tasklet_vec.

tasklet_action might be handled after tasklet is disabled, this will
make disabled tasklet stayed on tasklet_vec. tasklet_action will not
handle disabled tasklet, but place it on the tail of tasklet_vec,
still raise softirq for this tasklet. Things will become worse if
device driver uses tasklet_disable on its device remove/close code.
The disabled tasklet will stay on the vec, frequently __raise_softirq_off()
and make ksoftirqd wakeup even if no tasklets need to be handled.

This patch introduced a new TASKLET_STATE_HI bit to indicate HI_SOFTIRQ,
in tasklet_action(), simply ignore the disabled tasklet and don't raise
the softirq nr. In my previous patch, I remove tasklet_hi_enable() since
it is the same as tasklet_enable(). So only tasklet_enable() needs to be
modified, if tasklet state is changed from disable to enable, use
__tasklet_schedule() to put it on the right vec.

Signed-off-by: Xiaotian Feng <dannyfeng@tencent.com>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
 include/linux/interrupt.h |   12 ++++++++++--
 kernel/softirq.c          |   10 +++++-----
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/include/linux/interrupt.h b/include/linux/interrupt.h
index 5e4e617..7e5bb00 100644
--- a/include/linux/interrupt.h
+++ b/include/linux/interrupt.h
@@ -521,7 +521,8 @@ struct tasklet_struct name = { NULL, 0, ATOMIC_INIT(1), func, data }
 enum
 {
 	TASKLET_STATE_SCHED,	/* Tasklet is scheduled for execution */
-	TASKLET_STATE_RUN	/* Tasklet is running (SMP only) */
+	TASKLET_STATE_RUN,	/* Tasklet is running (SMP only) */
+	TASKLET_STATE_HI	/* Tasklet is HI_SOFTIRQ */
 };
 
 #ifdef CONFIG_SMP
@@ -593,7 +594,14 @@ static inline void tasklet_disable(struct tasklet_struct *t)
 static inline void tasklet_enable(struct tasklet_struct *t)
 {
 	smp_mb__before_atomic_dec();
-	atomic_dec(&t->count);
+	if (atomic_dec_and_test(&t->count)) {
+		if (!test_bit(TASKLET_STATE_SCHED, &t->state))
+			return;
+		if (test_bit(TASKLET_STATE_HI, &t->state))
+			__tasklet_hi_schedule(t);
+		else
+			__tasklet_schedule(t);
+	}
 }
 
 static inline void tasklet_hi_enable(struct tasklet_struct *t)
diff --git a/kernel/softirq.c b/kernel/softirq.c
index cc96bdc..6d77aef 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -431,6 +431,7 @@ void __tasklet_hi_schedule(struct tasklet_struct *t)
 	*__this_cpu_read(tasklet_hi_vec.tail) = t;
 	__this_cpu_write(tasklet_hi_vec.tail,  &(t->next));
 	raise_softirq_irqoff(HI_SOFTIRQ);
+	set_bit(TASKLET_STATE_HI, &t->state);
 	local_irq_restore(flags);
 }
 
@@ -442,6 +443,7 @@ void __tasklet_hi_schedule_first(struct tasklet_struct *t)
 
 	t->next = __this_cpu_read(tasklet_hi_vec.head);
 	__this_cpu_write(tasklet_hi_vec.head, t);
+	set_bit(TASKLET_STATE_HI, &t->state);
 	__raise_softirq_irqoff(HI_SOFTIRQ);
 }
 
@@ -467,10 +469,9 @@ static void tasklet_action(struct softirq_action *a)
 				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
 					BUG();
 				t->func(t->data);
-				tasklet_unlock(t);
-				continue;
-			}
+			} 
 			tasklet_unlock(t);
+			continue;
 		}
 
 		local_irq_disable();
@@ -502,10 +503,9 @@ static void tasklet_hi_action(struct softirq_action *a)
 				if (!test_and_clear_bit(TASKLET_STATE_SCHED, &t->state))
 					BUG();
 				t->func(t->data);
-				tasklet_unlock(t);
-				continue;
 			}
 			tasklet_unlock(t);
+			continue;
 		}
 
 		local_irq_disable();
-- 
1.7.9.5


             reply	other threads:[~2012-11-02  2:48 UTC|newest]

Thread overview: 6+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-11-02  2:48 Xiaotian Feng [this message]
2012-11-05 22:52 ` [RFC PATCH] tasklet: ignore disabled tasklet in tasklet_action Andrew Morton
2012-11-06  1:22   ` Xiaotian Feng
2012-11-06  1:37     ` Andrew Morton
2012-11-28 18:00       ` [BUG -next-20121127] kernel BUG at kernel/softirq.c:471! Peter Hurley
2012-11-28 22:12         ` Peter Hurley

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1351824534-2861-1-git-send-email-xtfeng@gmail.com \
    --to=xtfeng@gmail.com \
    --cc=akpm@linux-foundation.org \
    --cc=dannyfeng@tencent.com \
    --cc=linux-kernel@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.