del_timer_sync() and queue_delayed_work()
Subsystems (13)
cover del_timer_sync() and queue_delayed_work()
Due to funnies with del_timer_sync() I started looking at del_timer_sync() users and found that we could probably simplify the delayed work interface a bit.
PATCH 1/4 lockdep, timer: Fix del_timer_sync() annotation
Calling local_bh_enable() will want to actually start processing softirqs, which isn't a good idea since this can get called with IRQs disabled.
Cure this by using _local_bh_enable() which doesn't start processing softirqs, and use raw_local_irq_save() to avoid any softirqs from happending without letting lockdep think IRQs are in fact disabled.
LKML-Reference: <new-submission>
--- kernel/timer.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
kernel/timer.c +5 -1
@@ -969,10 +969,14 @@ EXPORT_SYMBOL(try_to_del_timer_sync);int del_timer_sync (struct timer_list *timer) {#ifdef CONFIG_LOCKDEP +unsigned long flags; + +raw_local_irq_save (flags);local_bh_disable ();lock_map_acquire (&timer->lockdep_map);lock_map_release (&timer->lockdep_map); -local_bh_enable (); +_local_bh_enable (); +raw_local_irq_restore (flags);#endif /* * don't use it in hardirq context, because it
Re: [PATCH 1/4] lockdep, timer: Fix del_timer_sync() annotation
Quoted 9 lines · Peter Zijlstra
Calling
local_bh_enable()will want to actually start processing softirqs, which isn't a good idea since this can get called with IRQs disabled.Cure this by using
_local_bh_enable()which doesn't start processing softirqs, and useraw_local_irq_save()to avoid any softirqs from happending without letting lockdep think IRQs are in fact disabled.Reported-by Nick Bowler <[email protected]>
Nick, can you please test ?
Thanks,
tglx
Quoted 28 lines
Signed-off-by Peter Zijlstra <[email protected]>LKML-Reference: <new-submission>
--- kernel/timer.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)Index:
linux-2.6/kernel/timer.c
===================================================================kernel/timer.c +5
-1@@ -969,10 +969,14 @@ EXPORT_SYMBOL(try_to_del_timer_sync);int del_timer_sync (struct timer_list *timer) {#ifdef CONFIG_LOCKDEP +unsigned long flags; + +raw_local_irq_save (flags);local_bh_disable ();lock_map_acquire (&timer->lockdep_map);lock_map_release (&timer->lockdep_map); -local_bh_enable (); +_local_bh_enable (); +raw_local_irq_restore (flags);#endif /* * don't use it in hardirq context, because it
Re: [PATCH 1/4] lockdep, timer: Fix del_timer_sync() annotation
Quoted 5 lines · Thomas Gleixner
Quoted 4 lines · Peter Zijlstra
Calling
local_bh_enable()will want to actually start processing softirqs, which isn't a good idea since this can get called with IRQs disabled.
Quoted 7 lines
Quoted 5 lines
Cure this by using
_local_bh_enable()which doesn't start processing softirqs, and useraw_local_irq_save()to avoid any softirqs from happending without letting lockdep think IRQs are in fact disabled.Reported-by Nick Bowler <[email protected]>Nick, can you please test ?
Yes, this patch seems to solve the issue.
Thanks,
Signature · 1 line
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)
Re: [PATCH 1/4] lockdep, timer: Fix del_timer_sync() annotation
In reply to Peter ZijlstraQuoted 22 lines · Peter Zijlstra
Calling
local_bh_enable()will want to actually start processing softirqs, which isn't a good idea since this can get called with IRQs disabled.Cure this by using
_local_bh_enable()which doesn't start processing softirqs, and useraw_local_irq_save()to avoid any softirqs from happending without letting lockdep think IRQs are in fact disabled.Reported-by Nick Bowler <[email protected]>Signed-off-by Peter Zijlstra <[email protected]>LKML-Reference: <new-submission>
--- kernel/timer.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)Index:
linux-2.6/kernel/timer.c
===================================================================kernel/timer.c +0
-0@@ -969,10 +969,14 @@ EXPORT_SYMBOL(try_to_del_timer_sync);int del_timer_sync (struct timer_list *timer) {
kernel/timer.c EXPORT_SYMBOL(try_to_del_timer_sync); · line 979
Quoted 10 lines
kernel/timer.c +5
-1#ifdef CONFIG_LOCKDEP +unsigned long flags; + +raw_local_irq_save (flags);local_bh_disable ();lock_map_acquire (&timer->lockdep_map);lock_map_release (&timer->lockdep_map); -local_bh_enable (); +_local_bh_enable (); +raw_local_irq_restore (flags);
This is more cheap than what I have done ;)
I think it will cure the issue reported by Nick.
Thanks you anyway.
[tip:timers/urgent] lockdep, timer: Fix del_timer_sync() annotation
In reply to Peter ZijlstraCommit-ID: f266a5110d453b7987194460ac7edd31f1a5426c Gitweb: http://git.kernel.org/tip/f266a5110d453b7987194460ac7edd31f1a5426c Author: Peter Zijlstra <[email protected]> AuthorDate: Thu, 3 Feb 2011 15:09:41 +0100 Committer: Thomas Gleixner <[email protected]> CommitDate: Fri, 4 Feb 2011 10:31:22 +0100
lockdep, timer: Fix del_timer_sync() annotation
Calling local_bh_enable() will want to actually start processing softirqs, which isn't a good idea since this can get called with IRQs disabled.
Cure this by using _local_bh_enable() which doesn't start processing softirqs, and use raw_local_irq_save() to avoid any softirqs from happening without letting lockdep think IRQs are in fact disabled.
LKML-Reference: <[email protected]>
--- kernel/timer.c | 6 +++++- 1 files changed, 5 insertions(+), 1 deletions(-)
kernel/timer.c +5 -1
@@ -969,10 +969,14 @@ EXPORT_SYMBOL(try_to_del_timer_sync);int del_timer_sync (struct timer_list *timer) {#ifdef CONFIG_LOCKDEP +unsigned long flags; + +raw_local_irq_save (flags);local_bh_disable ();lock_map_acquire (&timer->lockdep_map);lock_map_release (&timer->lockdep_map); -local_bh_enable (); +_local_bh_enable (); +raw_local_irq_restore (flags);#endif /* * don't use it in hardirq context, because it
PATCH 2/4 timer: Provide mod_timer_on()
In reply to Peter ZijlstraIn order to convert queue_work_on() to mod_timer (so that we can avoid calling cancel_delayed_work() every time we want to modify a delayed work) we need to have a mod_timer_on().
LKML-Reference: <new-submission>
--- include/linux/timer.h | 1 kernel/timer.c | 75 ++++++++++++++++++++++++++++++++------------------ 2 files changed, 50 insertions(+), 26 deletions(-)
include/linux/timer.h +1 -0
@@ -209,6 +209,7 @@ static inline int timer_pending(const stextern void add_timer_on (struct timer_list *timer,int cpu);extern int del_timer (struct timer_list * timer);extern int mod_timer (struct timer_list *timer,unsigned long expires); +extern int mod_timer_on (struct timer_list *timer,unsigned long expires,int cpu);extern int mod_timer_pending (struct timer_list *timer,unsigned long expires);extern int mod_timer_pinned (struct timer_list *timer,unsigned long expires);
kernel/timer.c +49 -26
115 lines not on this page · raw message
PATCH 3/4 workqueue: Use mod_timer for queue_delayed_work()
In reply to Peter ZijlstraUse mod_timer for queue_delayed_work(), this allows us to avoid calling cancel_delayed_work() prior to calling queue_delayed_work() when changing a work's expiration time.
LKML-Reference: <new-submission>
---
kernel/workqueue.c +10 -3
@@ -1107,8 +1107,15 @@ static void delayed_work_timer_fn(unsignint queue_delayed_work (struct workqueue_struct *wq,struct delayed_work *dwork,unsigned long delay) { -if (delay ==0 ) +if (delay ==0 ) { +/* +* If the timer callback is currently running the work +* is already being enqueued and we don't actually have +* to do anything to make it run now... +*/ +__cancel_delayed_work (dwork);return queue_work (wq, &dwork->work); + }return queue_delayed_work_on (-1 , wq, dwork, delay); }
@@ -1160,9 +1167,9 @@ int queue_delayed_work_on(int cpu, struc timer->function = delayed_work_timer_fn;if (unlikely (cpu >=0 )) -add_timer_on (timer, cpu); +mod_timer_on (timer, cpu);else -add_timer (timer); +mod_timer (timer); ret =1 ; }return ret;
PATCH 4/4 workqueue: Remove now superfluous cancel_delayed_work() calls
In reply to Peter ZijlstraSince queue_delayed_work() can now deal with existing timers, we don't need to explicitly call cancel_delayed_work() anymore.
LKML-Reference: <new-submission>
--- block/blk-throttle.c | 5 ----- drivers/infiniband/hw/nes/nes_hw.c | 2 -- drivers/infiniband/hw/nes/nes_nic.c | 2 -- drivers/input/keyboard/qt2160.c | 3 --- drivers/input/mouse/synaptics_i2c.c | 7 ------- drivers/misc/bh1770glc.c | 1 - drivers/net/phy/phy.c | 1 - drivers/power/jz4740-battery.c | 2 -- fs/nfs/nfs4renewd.c | 1 - fs/ocfs2/cluster/heartbeat.c | 1 - net/core/dst.c | 1 - net/rfkill/input.c | 1 - 12 files changed, 27 deletions(-)
block/blk-throttle.c +0 -5
@@ -814,11 +814,6 @@ void throtl_schedule_delayed_work(structstruct delayed_work *dwork = &td->throtl_work;if (total_nr_queued (td) >0 ) { -/* -* We might have a work scheduled to be executed in future. -* Cancel that and schedule a new one. -*/ -__cancel_delayed_work (dwork);kblockd_schedule_delayed_work (q, dwork, delay);throtl_log (td,"schedule work. delay=%lu jiffies=%lu" , delay, jiffies);
drivers/infiniband/hw/nes/nes_hw.c +0 -2
@@ -2651,8 +2651,6 @@ static void nes_process_mac_intr(struct } }if (nesadapter->phy_type[mac_index] ==NES_PHY_TYPE_SFP_D ) { -if (nesdev->link_recheck) -cancel_delayed_work (&nesdev->work); nesdev->link_recheck =1 ;schedule_delayed_work (&nesdev->work,NES_LINK_RECHECK_DELAY );
drivers/infiniband/hw/nes/nes_nic.c +0 -2
@@ -243,8 +243,6 @@ static int nes_netdev_open(struct net_despin_lock_irqsave (&nesdev->nesadapter->phy_lock, flags);if (nesdev->nesadapter->phy_type[nesdev->mac_index] ==NES_PHY_TYPE_SFP_D ) { -if (nesdev->link_recheck) -cancel_delayed_work (&nesdev->work); nesdev->link_recheck =1 ;schedule_delayed_work (&nesdev->work,NES_LINK_RECHECK_DELAY ); }
drivers/input/keyboard/qt2160.c +0 -3
@@ -155,10 +155,7 @@ static irqreturn_t qt2160_irq(int irq, vunsigned long flags;spin_lock_irqsave (&qt2160->lock, flags); - -__cancel_delayed_work (&qt2160->dwork);schedule_delayed_work (&qt2160->dwork,0 ); -spin_unlock_irqrestore (&qt2160->lock, flags);return IRQ_HANDLED ;
drivers/input/mouse/synaptics_i2c.c +0 -7
@@ -374,14 +374,7 @@ static void synaptics_i2c_reschedule_worunsigned long flags;spin_lock_irqsave (&touch->lock, flags); - -/* -* If work is already scheduled then subsequent schedules will not -* change the scheduled time that's why we have to cancel it first. -*/ -__cancel_delayed_work (&touch->dwork);schedule_delayed_work (&touch->dwork, delay); -spin_unlock_irqrestore (&touch->lock, flags); }
drivers/misc/bh1770glc.c +0 -1
@@ -636,7 +636,6 @@ static irqreturn_t bh1770_irq(int irq, v* Simulate missing no-proximity interrupt 50ms after the * next expected interrupt time. */ -cancel_delayed_work_sync (&chip->prox_work);schedule_delayed_work (&chip->prox_work,msecs_to_jiffies (rate +50 )); }
drivers/net/phy/phy.c +0 -1
@@ -677,7 +677,6 @@ static void phy_change(struct work_struc goto irq_enable_err;/* reschedule state queue work to run as soon as possible */ -cancel_delayed_work_sync (&phydev->state_queue);schedule_delayed_work (&phydev->state_queue,0 );return ;
drivers/power/jz4740-battery.c +0 -2
@@ -173,7 +173,6 @@ static void jz_battery_external_power_ch {struct jz_battery *jz_battery =psy_to_jz_battery (psy); -cancel_delayed_work (&jz_battery->work);schedule_delayed_work (&jz_battery->work,0 ); }
@@ -181,7 +180,6 @@ static irqreturn_t jz_battery_charge_irq {struct jz_battery *jz_battery = data; -cancel_delayed_work (&jz_battery->work);schedule_delayed_work (&jz_battery->work,0 );return IRQ_HANDLED ;
fs/nfs/nfs4renewd.c +0 -1
@@ -115,7 +115,6 @@ nfs4_schedule_state_renewal(struct nfs_c timeout =5 *HZ ;dprintk ("%s: requeueing work. Lease period = %ld\n" , __func__, (timeout +HZ -1 ) /HZ ); -cancel_delayed_work (&clp->cl_renewd);schedule_delayed_work (&clp->cl_renewd, timeout);set_bit (NFS_CS_RENEWD , &clp->cl_res_state);spin_unlock (&clp->cl_lock);
fs/ocfs2/cluster/heartbeat.c +0 -1
@@ -332,7 +332,6 @@ static void o2hb_arm_write_timeout(strucclear_bit (reg->hr_region_num, o2hb_failed_region_bitmap);spin_unlock (&o2hb_live_lock); } -cancel_delayed_work (®->hr_write_timeout_work); reg->hr_last_timeout_start = jiffies;schedule_delayed_work (®->hr_write_timeout_work,msecs_to_jiffies (O2HB_MAX_WRITE_TIMEOUT_MS ));
net/core/dst.c +0 -1
@@ -207,7 +207,6 @@ void __dst_free(struct dst_entry *dst)if (dst_garbage.timer_inc >DST_GC_INC ) { dst_garbage.timer_inc =DST_GC_INC ; dst_garbage.timer_expires =DST_GC_MIN ; -cancel_delayed_work (&dst_gc_work);schedule_delayed_work (&dst_gc_work, dst_garbage.timer_expires); }spin_unlock_bh (&dst_garbage.lock);
net/rfkill/input.c +0 -1
@@ -163,7 +163,6 @@ static void rfkill_schedule_global_op(en rfkill_op_pending = true;if (op ==RFKILL_GLOBAL_OP_EPO && !rfkill_is_epo_lock_active ()) {/* bypass the limiter for EPO */ -cancel_delayed_work (&rfkill_op_work);schedule_delayed_work (&rfkill_op_work,0 ); rfkill_last_scheduled = jiffies; }else
Tejun Heo
maintainerRe: [PATCH 4/4] workqueue: Remove now superfluous cancel_delayed_work() calls
Hello, Peter.
Quoted 2 lines · Peter Zijlstra
Since
queue_delayed_work()can now deal with existing timers, we don't need to explicitly callcancel_delayed_work()anymore.
This is nice but there's small complication with the way queue_delayed_work() behaves. If a delayed work item is already pending, another queue_delayed_work() doesn't modify the delay whether the new delay is longer or shorter than the current one. The previous patch doesn't really change the behavior as the whole thing is gated with WORK_STRUCT_PENDING_BIT.
So, cancel_delayed_work() followed by queue_delayed_work() schedules the work to be executed at the specified time regardless of the current pending state while queue_delayed_work() takes effect iff currently the work item is not pending.
The current behavior is weird and it often is easier to use explicit timer + work item if the timer needs to be modified, but it has been that way from the beginning so I don't think changing it would be a good idea. We can introduce a new interface (mod_delayed_work() maybe) for this tho.
Thanks.
Signature · 1 line
tejun
Dmitry Torokhov
maintainerRe: [PATCH 4/4] workqueue: Remove now superfluous cancel_delayed_work() calls
Quoted 18 lines · Tejun Heo
Hello, Peter.
Quoted 2 lines · Peter Zijlstra
Since
queue_delayed_work()can now deal with existing timers, we don't need to explicitly callcancel_delayed_work()anymore.This is nice but there's small complication with the way
queue_delayed_work()behaves. If a delayed work item is already pending, anotherqueue_delayed_work()doesn't modify the delay whether the new delay is longer or shorter than the current one. The previous patch doesn't really change the behavior as the whole thing is gated withWORK_STRUCT_PENDING_BIT.So,
cancel_delayed_work()followed byqueue_delayed_work()schedules the work to be executed at the specified time regardless of the current pending state whilequeue_delayed_work()takes effect iff currently the work item is not pending.
Quoted 6 lines
The current behavior is weird and it often is easier to use explicit timer + work item if the timer needs to be modified, but it has been that way from the beginning so I don't think changing it would be a good idea. We can introduce a new interface (
mod_delayed_work()maybe) for this tho.
I agree. If we were to change queue_delayed_work() we'd have to verify that all users that do not presently use cancel_delayed_work() in reschedule would be OK with the new behavior.
Thanks.
Signature · 1 line
Dmitry
Re: [PATCH 4/4] workqueue: Remove now superfluous cancel_delayed_work() calls
In reply to Tejun HeoQuoted 13 lines · Tejun Heo
Hello, Peter.
Quoted 2 lines · Peter Zijlstra
Since
queue_delayed_work()can now deal with existing timers, we don't need to explicitly callcancel_delayed_work()anymore.This is nice but there's small complication with the way
queue_delayed_work()behaves. If a delayed work item is already pending, anotherqueue_delayed_work()doesn't modify the delay whether the new delay is longer or shorter than the current one. The previous patch doesn't really change the behavior as the whole thing is gated withWORK_STRUCT_PENDING_BIT.
Quoted 4 lines
So,
cancel_delayed_work()followed byqueue_delayed_work()schedules the work to be executed at the specified time regardless of the current pending state whilequeue_delayed_work()takes effect iff currently the work item is not pending.
Right, I didn't think it would matter much, the difference is tiny. Only a small window between the timer triggering and the work getting scheduled has a different semantics, it used to be the same as before that window, now its like after that window.
Since its all timing the code needs to deal with those cases anyway, no?
Quoted 5 lines
The current behavior is weird and it often is easier to use explicit timer + work item if the timer needs to be modified, but it has been that way from the beginning so I don't think changing it would be a good idea. We can introduce a new interface (
mod_delayed_work()maybe) for this tho.
Well we could simply not apply patch 4 and things simply remain as they are. The mod_timer() semantics are identical to add_timer() if you call del_timer() like thing before, so patch 3 doesn't actually change any semantics if you keep using it the old way.
Tejun Heo
maintainerRe: [PATCH 4/4] workqueue: Remove now superfluous cancel_delayed_work() calls
Hello, Peter.
Quoted 17 lines · Peter Zijlstra
Quoted 11 lines · Tejun Heo
Quoted 2 lines · Peter Zijlstra
Since
queue_delayed_work()can now deal with existing timers, we don't need to explicitly callcancel_delayed_work()anymore.This is nice but there's small complication with the way
queue_delayed_work()behaves. If a delayed work item is already pending, anotherqueue_delayed_work()doesn't modify the delay whether the new delay is longer or shorter than the current one. The previous patch doesn't really change the behavior as the whole thing is gated withWORK_STRUCT_PENDING_BIT.Quoted 4 lines
So,
cancel_delayed_work()followed byqueue_delayed_work()schedules the work to be executed at the specified time regardless of the current pending state whilequeue_delayed_work()takes effect iff currently the work item is not pending.
Quoted 6 lines
Right, I didn't think it would matter much, the difference is tiny. Only a small window between the timer triggering and the work getting scheduled has a different semantics, it used to be the same as before that window, now its like after that window.
Since its all timing the code needs to deal with those cases anyway, no?
No, AFAICS the change from add_timer() to mod_timer() doesn't make any difference. The control never reaches there if the work item is already pending. Please consider the following two sequences.
seq1. queue_delayed_work(wq, dwork, 10*HZ); cancel_delayed_work(dwork); queue_delayed_work(wq, dwork, 5*HZ);
seq2. queue_delayed_work(wq, dwork, 10*HZ); queue_delayed_work(wq, dwork, 5*HZ);
With or without the patch, dwork in seq1 will execute in 5 seconds, and, again, with our without the patch dwork in seq2 will execute in 10 seconds, because queueing is gated by WORK_STRUCT_PENDING_BIT and if the bit is already set the timer isn't modified at all.
IOW, those cancel_delayed_work()'s are there not because queue_delayed_work() calls add_timer() instead of mod_timer(). They're there because queue_delayed_work() always uses the first timeout duration and the users want to change it to a new value.
As I wrote before, I'm not a fan of the current behavior but that's how it is currently. This patch series doesn't change the behavior because the timers are guaranteed to be offline when queue_delayed_work_on() calls add_timer(). To actually change the behavior, queue_delayed_work_on() needs to be restructured and all its users audited.
Thank you.
Signature · 1 line
tejun