Run timers as softirqs, not tasklets

PATCH In review Matthew Wilcox 4 participants 6 messages Nov 17, 2002 – Nov 18, 2002

Share & tools

Matthew Wilcox

author

2002-11-17 17:09 UTC #0 lore raw

PATCH 1/1 Run timers as softirqs, not tasklets

Seems to me that the timer code is attempting to replicate the softirq characteristics at the tasklet level, which is a little pointless. This patch converts timers to be a first-class softirq citizen.

Ingo, was there a reason you didn't do it this way to begin with?

include/linux/interrupt.h +1 -0

diff -u linux-2.5.47-pci/include/linux/interrupt.h linux-2.5.47-pci/include/linux/interrupt.h
@@ -45,6 +45,7 @@
 enum
 {
 	HI_SOFTIRQ=0,
+	TIMER_SOFTIRQ,
 	NET_TX_SOFTIRQ,
 	NET_RX_SOFTIRQ,
 	SCSI_SOFTIRQ,

kernel/timer.c +4 -7

@@ -66,9 +66,6 @@ typedef struct tvec_t_base_s tvec_base_t
 /* Fake initialization */
 static DEFINE_PER_CPU(tvec_base_t, tvec_bases) = { SPIN_LOCK_UNLOCKED };
 
-/* Fake initialization needed to avoid compiler breakage */
-static DEFINE_PER_CPU(struct tasklet_struct, timer_tasklet) = { NULL };
-
 static void check_timer_failed(timer_t *timer)
 {
 	static int whine_count;

@@ -766,9 +763,9 @@ rwlock_t xtime_lock __cacheline_aligned_
 unsigned long last_time_offset;
 
 /*
- * This function runs timers and the timer-tq in softirq context.
+ * This function runs timers and the timer-tq in bottom half context.
  */
-static void run_timer_tasklet(unsigned long data)
+static void run_timer_softirq(struct softirq_action *h)
 {
 	tvec_base_t *base = &per_cpu(tvec_bases, smp_processor_id());

@@ -781,7 +778,7 @@ static void run_timer_tasklet(unsigned l
  */
 void run_local_timers(void)
 {
-	tasklet_hi_schedule(&per_cpu(timer_tasklet, smp_processor_id()));
+	raise_softirq(TIMER_SOFTIRQ);
 }
 
 /*

@@ -1140,7 +1137,6 @@ static void __devinit init_timers_cpu(in
 	}
 	for (j = 0; j < TVR_SIZE; j++)
 		INIT_LIST_HEAD(base->tv1.vec + j);
-	tasklet_init(&per_cpu(timer_tasklet, cpu), run_timer_tasklet, 0UL);
 }
 	
 static int __devinit timer_cpu_notify(struct notifier_block *self,

@@ -1167,4 +1163,5 @@ void __init init_timers(void)
 	timer_cpu_notify(&timers_nb, (unsigned long)CPU_UP_PREPARE,
 				(void *)(long)smp_processor_id());
 	register_cpu_notifier(&timers_nb);
+	open_softirq(TIMER_SOFTIRQ, run_timer_softirq, NULL);
 }

Signature · 1 line

Revolutions do not require corporate support.

Ingo Molnar

2002-11-17 17:59 UTC #1 lore raw

Quoted 3 lines · Matthew Wilcox

Seems to me that the timer code is attempting to replicate the softirq characteristics at the tasklet level, which is a little pointless. This patch converts timers to be a first-class softirq citizen.

i agree with your patch.

Quoted 1 line

Ingo, was there a reason you didn't do it this way to begin with?

because there was an interim state of the timer code in where we still had a global timer context (ie. a timer tasklet). Only later did it get converted to completely unsynchronized per-CPU tasklets. Which indeed is what softirqs are :-)

Ingo

Dipankar Sarma

2002-11-17 18:43 UTC #2 lore raw

In reply to Matthew Wilcox

Quoted 7 lines · Matthew Wilcox

Seems to me that the timer code is attempting to replicate the softirq characteristics at the tasklet level, which is a little pointless. This patch converts timers to be a first-class softirq citizen.
Ingo, was there a reason you didn't do it this way to begin with?

I wrote that part of smptimers to run the per-CPU lists from per-CPU tasklets while porting Ingo's code to 2.5 and Ingo just included it. At that time, it didn't seem necessary to use up a softirq vector when it could be easily done using tasklets.

However it should work fine with softirqs too.

Thanks
Dipankar

Ingo Molnar

2002-11-17 18:45 UTC #3 lore raw

Quoted 4 lines · Dipankar Sarma

I wrote that part of smptimers to run the per-CPU lists from per-CPU tasklets while porting Ingo's code to 2.5 and Ingo just included it. At that time, it didn't seem necessary to use up a softirq vector when it could be easily done using tasklets.

i think a separate timer softirq is justified, timers are important enough.

Ingo

george anzinger

2002-11-18 22:24 UTC #4 lore raw

Quoted 8 lines · Ingo Molnar

Quoted 4 lines · Dipankar Sarma
I wrote that part of smptimers to run the per-CPU lists from per-CPU tasklets while porting Ingo's code to 2.5 and Ingo just included it. At that time, it didn't seem necessary to use up a softirq vector when it could be easily done using tasklets.

Quoted 5 lines

i think a separate timer softirq is justified, timers are important enough.
Ingo

So then, is there any reason to not put them ahead of HI_SOFTIRQ? I.e.:

enum
{
TIMER_SOFTIRQ=0,
HI_SOFTIRQ
NET_TX_SOFTIRQ,
NET_RX_SOFTIRQ,
SCSI_SOFTIRQ,

Signature · 5 lines

George Anzinger   [email protected]
High-res-timers: 
http://sourceforge.net/projects/high-res-timers/
Preemption patch:
http://www.kernel.org/pub/linux/kernel/people/rml

Matthew Wilcox

author

2002-11-18 22:29 UTC #5 lore raw

Quoted 10 lines · george anzinger

So then, is there any reason to not put them ahead of HI_SOFTIRQ? I.e.:
enum
{
TIMER_SOFTIRQ=0,
HI_SOFTIRQ
NET_TX_SOFTIRQ,
NET_RX_SOFTIRQ,
SCSI_SOFTIRQ,

because then there would be no way to make a tasklet run before the timers? turn it around. convince us that timers should run first.

Signature · 1 line

Revolutions do not require corporate support.