| Message ID | 1265120401.24455.306.camel@laptop |
|---|---|
| State | Not Applicable, archived |
| Delegated to: | David Miller |
| Headers | show |
On Tue, 02 Feb 2010 15:20:01 +0100 Peter Zijlstra <peterz@infradead.org> 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
On Tue, 2010-02-02 at 09:28 -0500, Yury Polyanskiy wrote: > On Tue, 02 Feb 2010 15:20:01 +0100 > Peter Zijlstra <peterz@infradead.org> 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. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <a.p.zijlstra@chello.nl> > > With this patch, the inconsistent lock state INFO is gone. Thanks. Wei Yongjun -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Peter Zijlstra <peterz@infradead.org> 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 <a.p.zijlstra@chello.nl> Acked-by: David S. Miller <davem@davemloft.net> It would be nice to give mention of the bug reporter et al. in the final commit message. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; } /*