On Tue, 2010-02-02 at 08:51 -0500, Yury Polyanskiy wrote:
> If hrtimer_tasklet interface functions properly, the
> xfrm_timer_handler should be called in softirq context (and thus is
> never in parallel with xfrm_input()). The deadlock isn't possible then.
>
> In this case it seems that for some reason xfrm_timer_handler() is
> called in the hardirq context. The relevant code in hrtimer_tasklet:
>
> static enum hrtimer_restart __hrtimer_tasklet_trampoline(struct hrtimer *timer)
> {
> struct tasklet_hrtimer *ttimer =
> container_of(timer, struct tasklet_hrtimer, timer);
>
> if (hrtimer_is_hres_active(timer)) {
> tasklet_hi_schedule(&ttimer->tasklet);
> return HRTIMER_NORESTART;
> }
> return ttimer->function(timer);
> }
>
> I am copying Peter on this. Peter, how is it possible that
> ttimer->function() is called in hardirq?
>
> Could it be that switch from hres_active happened after the call to
> trampoline and before the if() above?
The original email had more information:
> {IN-HARDIRQ-W} state was registered at:
> [<c04718dc>] __lock_acquire+0xa9c/0x1890
> [<c047274f>] lock_acquire+0x7f/0xf0
> [<c0762958>] _raw_spin_lock+0x38/0x50
> [<c072b5ca>] xfrm_timer_handler+0x3a/0x260
> [<c0447d9d>] __hrtimer_tasklet_trampoline+0xd/0x10
> [<c04634ce>] hrtimer_run_queues+0x15e/0x2a0
> [<c045146d>] run_local_timers+0xd/0x20
> [<c04514b4>] update_process_times+0x34/0x70
> [<c046ce8a>] tick_periodic+0x2a/0x80
> [<c046cefe>] tick_handle_periodic+0x1e/0x90
> [<c0768377>] smp_apic_timer_interrupt+0x57/0x8b
> [<c076382f>] apic_timer_interrupt+0x2f/0x34
> [<c0401d3b>] cpu_idle+0x4b/0x80
> [<c074e0d7>] rest_init+0x67/0x70
> [<c0956874>] start_kernel+0x30e/0x314
> [<c095609e>] i386_start_kernel+0x9e/0xa5
Which indicates we were called from hardirq context, it appears that
that hrtimer_is_hres_active() case is indeed faulty. Not sure if I made
a mistake when I wrote that or if we changed hrtimer behaviour
afterwards, but the hrtimer fallback is still from hardirq context.
Which would seem to suggest the following patch:
---
Subject: hrtimer, softirq: Fix hrtimer->softirq trampoline
hrtimers callbacks are always done from hardirq context, either the
jiffy tick interrupt or the hrtimer device interrupt.
Signed-off-by: Peter Zijlstra <[email protected]>
---
kernel/softirq.c | 13 +++----------
1 files changed, 3 insertions(+), 10 deletions(-)
diff --git a/kernel/softirq.c b/kernel/softirq.c
index a09502e..c1983b7 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -500,22 +500,15 @@ EXPORT_SYMBOL(tasklet_kill);
*/
/*
- * The trampoline is called when the hrtimer expires. If this is
- * called from the hrtimer interrupt then we schedule the tasklet as
- * the timer callback function expects to run in softirq context. If
- * it's called in softirq context anyway (i.e. high resolution timers
- * disabled) then the hrtimer callback is called right away.
+ * The trampoline is called when the hrtimer expires.
*/
static enum hrtimer_restart __hrtimer_tasklet_trampoline(struct hrtimer *timer)
{
struct tasklet_hrtimer *ttimer =
container_of(timer, struct tasklet_hrtimer, timer);
- if (hrtimer_is_hres_active(timer)) {
- tasklet_hi_schedule(&ttimer->tasklet);
- return HRTIMER_NORESTART;
- }
- return ttimer->function(timer);
+ tasklet_hi_schedule(&ttimer->tasklet);
+ return HRTIMER_NORESTART;
}
/*
On Tue, 2010-02-02 at 09:28 -0500, Yury Polyanskiy wrote:
> On Tue, 02 Feb 2010 15:20:01 +0100
> Peter Zijlstra <[email protected]> wrote:
> > static enum hrtimer_restart __hrtimer_tasklet_trampoline(struct hrtimer *timer)
> > {
> > struct tasklet_hrtimer *ttimer =
> > container_of(timer, struct tasklet_hrtimer, timer);
> >
> > - if (hrtimer_is_hres_active(timer)) {
> > - tasklet_hi_schedule(&ttimer->tasklet);
> > - return HRTIMER_NORESTART;
> > - }
> > - return ttimer->function(timer);
> > + tasklet_hi_schedule(&ttimer->tasklet);
> > + return HRTIMER_NORESTART;
> > }
> >
>
> Are you totally against if(in_irq())?
Yeah, things like that are an indication that you really don't know wtf
you're doing and are just patching up.
There is a single site where hrtimer callbacks can indeed be done from
softirq, but in that case the above still works correctly, and I've been
meaning to get rid of that anyway.
On Tue, 02 Feb 2010 15:20:01 +0100
Peter Zijlstra <[email protected]> wrote:
> static enum hrtimer_restart __hrtimer_tasklet_trampoline(struct hrtimer *timer)
> {
> struct tasklet_hrtimer *ttimer =
> container_of(timer, struct tasklet_hrtimer, timer);
>
> - if (hrtimer_is_hres_active(timer)) {
> - tasklet_hi_schedule(&ttimer->tasklet);
> - return HRTIMER_NORESTART;
> - }
> - return ttimer->function(timer);
> + tasklet_hi_schedule(&ttimer->tasklet);
> + return HRTIMER_NORESTART;
> }
>
Are you totally against if(in_irq())?
Yury
Peter Zijlstra wrote:
> On Tue, 2010-02-02 at 08:51 -0500, Yury Polyanskiy wrote:
>
> The original email had more information:
>
>
>> {IN-HARDIRQ-W} state was registered at:
>> [<c04718dc>] __lock_acquire+0xa9c/0x1890
>> [<c047274f>] lock_acquire+0x7f/0xf0
>> [<c0762958>] _raw_spin_lock+0x38/0x50
>> [<c072b5ca>] xfrm_timer_handler+0x3a/0x260
>> [<c0447d9d>] __hrtimer_tasklet_trampoline+0xd/0x10
>> [<c04634ce>] hrtimer_run_queues+0x15e/0x2a0
>> [<c045146d>] run_local_timers+0xd/0x20
>> [<c04514b4>] update_process_times+0x34/0x70
>> [<c046ce8a>] tick_periodic+0x2a/0x80
>> [<c046cefe>] tick_handle_periodic+0x1e/0x90
>> [<c0768377>] smp_apic_timer_interrupt+0x57/0x8b
>> [<c076382f>] apic_timer_interrupt+0x2f/0x34
>> [<c0401d3b>] cpu_idle+0x4b/0x80
>> [<c074e0d7>] rest_init+0x67/0x70
>> [<c0956874>] start_kernel+0x30e/0x314
>> [<c095609e>] i386_start_kernel+0x9e/0xa5
>>
>
> Which indicates we were called from hardirq context, it appears that
> that hrtimer_is_hres_active() case is indeed faulty. Not sure if I made
> a mistake when I wrote that or if we changed hrtimer behaviour
> afterwards, but the hrtimer fallback is still from hardirq context.
>
> Which would seem to suggest the following patch:
>
> ---
> Subject: hrtimer, softirq: Fix hrtimer->softirq trampoline
>
> hrtimers callbacks are always done from hardirq context, either the
> jiffy tick interrupt or the hrtimer device interrupt.
>
> Signed-off-by: Peter Zijlstra <[email protected]>
>
>
With this patch, the inconsistent lock state INFO is gone. Thanks.
Wei Yongjun
From: Peter Zijlstra <[email protected]>
Date: Tue, 02 Feb 2010 15:20:01 +0100
> Subject: hrtimer, softirq: Fix hrtimer->softirq trampoline
>
> hrtimers callbacks are always done from hardirq context, either the
> jiffy tick interrupt or the hrtimer device interrupt.
>
> Signed-off-by: Peter Zijlstra <[email protected]>
Acked-by: David S. Miller <[email protected]>
It would be nice to give mention of the bug reporter et al.
in the final commit message.
Thanks.
Commit-ID: b9c3032277f756e73f6c673419dc414155e04e46
Gitweb: http://git.kernel.org/tip/b9c3032277f756e73f6c673419dc414155e04e46
Author: Peter Zijlstra <[email protected]>
AuthorDate: Wed, 3 Feb 2010 18:08:52 +0100
Committer: Thomas Gleixner <[email protected]>
CommitDate: Wed, 3 Feb 2010 18:17:40 +0100
hrtimer, softirq: Fix hrtimer->softirq trampoline
hrtimers callbacks are always done from hardirq context, either the
jiffy tick interrupt or the hrtimer device interrupt.
[ there is currently one exception that can still call a hrtimer
callback from softirq, but even in that case this will still
work correctly. ]
Reported-by: Wei Yongjun <[email protected]>
Signed-off-by: Peter Zijlstra <[email protected]>
Cc: Yury Polyanskiy <[email protected]>
Tested-by: Wei Yongjun <[email protected]>
Acked-by: David S. Miller <[email protected]>
LKML-Reference: <1265120401.24455.306.camel@laptop>
Signed-off-by: Thomas Gleixner <[email protected]>
---
kernel/softirq.c | 15 +++++----------
1 files changed, 5 insertions(+), 10 deletions(-)
diff --git a/kernel/softirq.c b/kernel/softirq.c
index a09502e..7c1a67e 100644
--- a/kernel/softirq.c
+++ b/kernel/softirq.c
@@ -500,22 +500,17 @@ EXPORT_SYMBOL(tasklet_kill);
*/
/*
- * The trampoline is called when the hrtimer expires. If this is
- * called from the hrtimer interrupt then we schedule the tasklet as
- * the timer callback function expects to run in softirq context. If
- * it's called in softirq context anyway (i.e. high resolution timers
- * disabled) then the hrtimer callback is called right away.
+ * The trampoline is called when the hrtimer expires. It schedules a tasklet
+ * to run __tasklet_hrtimer_trampoline() which in turn will call the intended
+ * hrtimer callback, but from softirq context.
*/
static enum hrtimer_restart __hrtimer_tasklet_trampoline(struct hrtimer *timer)
{
struct tasklet_hrtimer *ttimer =
container_of(timer, struct tasklet_hrtimer, timer);
- if (hrtimer_is_hres_active(timer)) {
- tasklet_hi_schedule(&ttimer->tasklet);
- return HRTIMER_NORESTART;
- }
- return ttimer->function(timer);
+ tasklet_hi_schedule(&ttimer->tasklet);
+ return HRTIMER_NORESTART;
}
/*