Performance of del_timer_sync

I started this patch based on profiling an enterprise database application
on a 32p IA64 NUMA machine, where del_timer_sync was one of the top few functions taking CPU time in the kernel.

Quoted 4 lines · Geoff Gustafson

I started this patch based on profiling an enterprise database
application on a 32p IA64 NUMA machine, where del_timer_sync was one of the top few functions taking CPU time in the kernel.

Do you know where it's being called from?

Quoted 6 lines · Andrew Morton

Quoted 4 lines · Geoff Gustafson

I started this patch based on profiling an enterprise database
application on a 32p IA64 NUMA machine, where del_timer_sync was one of the top few functions taking CPU time in the kernel.

Quoted 1 line

Do you know where it's being called from?

OK, the main sources were:

sys_semtimedop() -> schedule_timeout()

sys_io_getevents() -> read_events() -> clear_timeout()

Quoted 7 lines

Quoted 7 lines

Quoted 1 line

Andrew Morton wrote on Tuesday, May 11, 2004 12:11 PM

Quoted 4 lines · Geoff Gustafson

I started this patch based on profiling an enterprise database
application on a 32p IA64 NUMA machine, where del_timer_sync was one of the top few functions taking CPU time in the kernel.

Quoted 8 lines

Quoted 1 line

Do you know where it's being called from?

OK, the main sources were:

sys_semtimedop() -> schedule_timeout()

sys_io_getevents() -> read_events() -> clear_timeout()

OK, thanks. schedule_timeout()! Ow.

Ingo, why is this not sufficient?

diff -puN kernel/timer.c~a kernel/timer.c

Quoted 6 lines

del_again:

kernel/timer.c renamed from kernel/timer.c~a +2 -0

 	ret += del_timer(timer);
+	if (!ret)
+		return 0;

kernel/timer.c int del_timer_sync(struct timer_list *ti · line 338

Quoted 2 lines

kernel/timer.c renamed from kernel/timer.c~a +0 -0

 	for_each_cpu(i) {
 		base = &per_cpu(tvec_bases, i);

Hehe ... , first thing we tried :-). But has problem that if timeout function is running while del_timer_sync() is called, it returns without waiting.

Quoted 10 lines · Kenneth W

Quoted 8 lines

del_again:

kernel/timer.c renamed from kernel/timer.c~a +2 -0

 	ret += del_timer(timer);
+	if (!ret)
+		return 0;
 
 	for_each_cpu(i) {
 		base = &per_cpu(tvec_bases, i);

Quoted 3 lines

Hehe ... , first thing we tried :-). But has problem that if timeout function is running while del_timer_sync() is called, it returns without waiting.

eh? If del_timer() returns non-zero, that means that the timer was successfully deleted before it triggered.

Quoted 9 lines · Andrew Morton

Ingo, why is this not sufficient?

(quoted diff) +2 -0

@@ -331,6 +331,8 @@ int del_timer_sync(struct timer_list *ti
 
 del_again:
 	ret += del_timer(timer);
+	if (!ret)
+		return 0;
 

Review int del_timer_sync(struct timer_list *ti · line 338

Quoted 2 lines

(quoted diff) +0 -0

 	for_each_cpu(i) {
 		base = &per_cpu(tvec_bases, i);

it's not sufficient because a timer might be running on another CPU even if it has not been deleted. We delete a timer prior running it (so that the timer fn can re-activate the timer). So del_timer_sync() has to synchronize independently of whether the timer was removed or not.

Ah, OK, the timer handler may re-add itself. Really, that's a bug in the caller: once they've decided to shoot down the timer the caller should have made state changes which prevent the handler from re-adding the timer.

Still, too late to change that.

Neither AIO nor schedule_timeout() actually re-add the timer so they don't need the full treatment, yes?

kernel/timer.c del_again: · line 357

Quoted 5 lines · Andrew Morton

kernel/timer.c renamed from kernel/timer.c~del-timer-speedup +5 -0

+int del_single_shot_timer(struct timer_struct *timer)
+{
+	if (del_timer(timer))
+		del_timer_sync(timer);
+}

it's probably better named del_timer_singleshot given the name we gave to the other timer functions.

Quoted 8 lines

Quoted 2 lines

Quoted 1 line

Quoted 1 line

Andrew Morton wrote on Tuesday, May 11, 2004 1:12 PM

Ingo, why is this not sufficient?

it's not sufficient because a timer might be running on another CPU even if it has not been deleted. We delete a timer prior running it (so that the timer fn can re-activate the timer). So del_timer_sync() has to synchronize independently of whether the timer was removed or not.

Ah, OK, the timer handler may re-add itself. Really, that's a bug in the
caller: once they've decided to shoot down the timer the caller should have made state changes which prevent the handler from re-adding the timer.

Still, too late to change that.

Neither AIO nor schedule_timeout() actually re-add the timer so they don't need the full treatment, yes?

diff -puN kernel/timer.c~del-timer-speedup kernel/timer.c

kernel/timer.c renamed from kernel/timer.c~del-timer-speedup +0 -0

@@ -348,8 +348,13 @@ del_again:
 
 	return ret;

Nah, that's ungrammatical. del_timer_singleshot means "delete a timer in a single-shot manner".

We have:

"add a timer"
"modify a timer"
"delete a timer"
"delete a timer synchronously"
"delete a single-shot timer"

kernel/timer.c del_again: · line 358

Quoted 8 lines

kernel/timer.c renamed from kernel/timer.c~del-timer-speedup +6 -0

+
+int del_single_shot_timer(struct timer_struct *timer)
+{
+	if (del_timer(timer))
+		del_timer_sync(timer);
+}
 #endif

I'm confused, isn't the polarity of del_timer() need to be reversed?

Also propagate the return value of del_timer_sync()?

kernel/timer.c del_again: · line 358

Quoted 6 lines

kernel/timer.c renamed from kernel/timer.c~del-timer-speedup +5 -0

+int del_single_shot_timer(struct timer_struct *timer)
+{
+	if (del_timer(timer))
+		del_timer_sync(timer);
+}
 #endif

I'm confused, isn't the polarity of del_timer() need to be reversed? Also propagate the return value of del_timer_sync()?

Quoted 9 lines

kernel/timer.c del_again: · line 358

Quoted 7 lines

Quoted 1 line

Quoted 1 line

Andrew Morton wrote on Tuesday, May 11, 2004 1:31 PM

kernel/timer.c renamed from kernel/timer.c~del-timer-speedup +5 -0

+int del_single_shot_timer(struct timer_struct *timer)
+{
+	if (del_timer(timer))
+		del_timer_sync(timer);
+}
 #endif

I'm confused, isn't the polarity of del_timer() need to be reversed?

Hey, I didn't compile it, let alone test it!

Quoted 1 line

Also propagate the return value of del_timer_sync()?

yup.

If it looks OK, please fix it up, kerneldocify the function and prepare a real patch?

Quoted 5 lines · Andrew Morton

Nah, that's ungrammatical. del_timer_singleshot means "delete a timer in a single-shot manner".

We have:

Quoted 5 lines

"add a timer"
"modify a timer"
"delete a timer"
"delete a timer synchronously"
"delete a single-shot timer"

hm, indeed. Miraculously, the existing timer API names are correct grammatically, so we might as well go for del_single_shot_timer() ...

Quoted 11 lines

Quoted 10 lines

Nah, that's ungrammatical. del_timer_singleshot means "delete a timer in a single-shot manner".

We have:

"add a timer"
"modify a timer"
"delete a timer"
"delete a timer synchronously"
"delete a single-shot timer"

Quoted 3 lines

hm, indeed. Miraculously, the existing timer API names are correct grammatically, so we might as well go for del_single_shot_timer() ...

<anal>del_singleshot_timer_sync</anal>

I vote we leave it up to Ken. But please, not del_timer_kenneth().

Quoted 1 line

Quoted 1 line

Quoted 1 line

Ingo Molnar wrote on Tuesday, May 11, 2004 1:57 PM

Quoted 15 lines · Andrew Morton

Quoted 14 lines

Quoted 10 lines

Nah, that's ungrammatical. del_timer_singleshot means "delete a timer in a single-shot manner".

We have:

"add a timer"
"modify a timer"
"delete a timer"
"delete a timer synchronously"
"delete a single-shot timer"

hm, indeed. Miraculously, the existing timer API names are correct grammatically, so we might as well go for del_single_shot_timer() ...

Quoted 3 lines

<anal>del_singleshot_timer_sync</anal>

I vote we leave it up to Ken. But please, not del_timer_kenneth().

yeah. Ken's got a license to name ;)