[PATCH 0/4] del_timer_sync() and queue_delayed_work()

[PATCH 0/4] del_timer_sync() and queue_delayed_work()

[Peter Zijlstra]

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

[Peter Zijlstra]

[-- Attachment #1: timer-del_timer_sync.patch --]
[-- Type: text/plain, Size: 1165 bytes --]

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.

Reported-by: Nick Bowler <nbowler@elliptictech.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <new-submission>
---
 kernel/timer.c |    6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Index: linux-2.6/kernel/timer.c
===================================================================
--- linux-2.6.orig/kernel/timer.c
+++ linux-2.6/kernel/timer.c
@@ -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()

[Peter Zijlstra]

[-- Attachment #1: timer-mod_timer_on.patch --]
[-- Type: text/plain, Size: 4987 bytes --]

In 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().

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <new-submission>
---
 include/linux/timer.h |    1 
 kernel/timer.c        |   75 ++++++++++++++++++++++++++++++++------------------
 2 files changed, 50 insertions(+), 26 deletions(-)

Index: linux-2.6/include/linux/timer.h
===================================================================
--- linux-2.6.orig/include/linux/timer.h
+++ linux-2.6/include/linux/timer.h
@@ -209,6 +209,7 @@ static inline int timer_pending(const st
 extern 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);
 
Index: linux-2.6/kernel/timer.c
===================================================================
--- linux-2.6.orig/kernel/timer.c
+++ linux-2.6/kernel/timer.c
@@ -648,8 +648,8 @@ static struct tvec_base *lock_timer_base
 }
 
 static inline int
-__mod_timer(struct timer_list *timer, unsigned long expires,
-						bool pending_only, int pinned)
+__mod_timer_on(struct timer_list *timer, unsigned long expires,
+	       bool pending_only, int pinned, int cpu)
 {
 	struct tvec_base *base, *new_base;
 	unsigned long flags;
@@ -673,12 +673,14 @@ __mod_timer(struct timer_list *timer, un
 
 	debug_activate(timer, expires);
 
-	cpu = smp_processor_id();
+	if (cpu == -1) {
+		cpu = smp_processor_id();
 
 #if defined(CONFIG_NO_HZ) && defined(CONFIG_SMP)
-	if (!pinned && get_sysctl_timer_migration() && idle_cpu(cpu))
-		cpu = get_nohz_timer_target();
+		if (!pinned && get_sysctl_timer_migration() && idle_cpu(cpu))
+			cpu = get_nohz_timer_target();
 #endif
+	}
 	new_base = per_cpu(tvec_bases, cpu);
 
 	if (base != new_base) {
@@ -705,12 +707,30 @@ __mod_timer(struct timer_list *timer, un
 		base->next_timer = timer->expires;
 	internal_add_timer(base, timer);
 
+	if (cpu != smp_processor_id()) {
+		/*
+		 * Check whether the other CPU is idle and needs to be
+		 * triggered to reevaluate the timer wheel when nohz is
+		 * active. We are protected against the other CPU fiddling
+		 * with the timer by holding the timer base lock. This also
+		 * makes sure that a CPU on the way to idle can not evaluate
+		 * the timer wheel.
+		 */
+		wake_up_idle_cpu(cpu);
+	}
 out_unlock:
 	spin_unlock_irqrestore(&base->lock, flags);
 
 	return ret;
 }
 
+static inline int
+__mod_timer(struct timer_list *timer, unsigned long expires,
+	    bool pending_only, int pinned)
+{
+	return __mod_timer_on(timer, expires, pending_only, pinned, -1);
+}
+
 /**
  * mod_timer_pending - modify a pending timer's timeout
  * @timer: the pending timer to be modified
@@ -826,6 +846,29 @@ int mod_timer_pinned(struct timer_list *
 EXPORT_SYMBOL(mod_timer_pinned);
 
 /**
+ * mod_timer_on - modify a timer's timeout and move it to a specific cpu
+ * @timer: the timer to be modified
+ * @expires: new timeout in jiffies
+ * @cpu: cpu to migrate the timer to
+ *
+ * See mod_timer(), equivalent to:
+ *
+ *   del_timer(timer); timer->expires = expires; add_timer_on(timer, cpu);
+ *
+ * The function returns whether it has modified a pending timer or not.
+ */
+int mod_timer_on(struct timer_list *timer, unsigned long expires, int cpu)
+{
+	if (timer_pending(timer) && timer->expires == expires)
+		return 1;
+
+	expires = apply_slack(timer, expires);
+
+	return __mod_timer_on(timer, expires, false, TIMER_NOT_PINNED, cpu);
+}
+EXPORT_SYMBOL(mod_timer_on);
+
+/**
  * add_timer - start a timer
  * @timer: the timer to be added
  *
@@ -855,28 +898,8 @@ EXPORT_SYMBOL(add_timer);
  */
 void add_timer_on(struct timer_list *timer, int cpu)
 {
-	struct tvec_base *base = per_cpu(tvec_bases, cpu);
-	unsigned long flags;
-
-	timer_stats_timer_set_start_info(timer);
 	BUG_ON(timer_pending(timer) || !timer->function);
-	spin_lock_irqsave(&base->lock, flags);
-	timer_set_base(timer, base);
-	debug_activate(timer, timer->expires);
-	if (time_before(timer->expires, base->next_timer) &&
-	    !tbase_get_deferrable(timer->base))
-		base->next_timer = timer->expires;
-	internal_add_timer(base, timer);
-	/*
-	 * Check whether the other CPU is idle and needs to be
-	 * triggered to reevaluate the timer wheel when nohz is
-	 * active. We are protected against the other CPU fiddling
-	 * with the timer by holding the timer base lock. This also
-	 * makes sure that a CPU on the way to idle can not evaluate
-	 * the timer wheel.
-	 */
-	wake_up_idle_cpu(cpu);
-	spin_unlock_irqrestore(&base->lock, flags);
+	mod_timer_on(timer, timer->expires, cpu);
 }
 EXPORT_SYMBOL_GPL(add_timer_on);
 

[PATCH 3/4] workqueue: Use mod_timer for queue_delayed_work()

[Peter Zijlstra]

[-- Attachment #1: workqueue-mod_timer_on.patch --]
[-- Type: text/plain, Size: 1240 bytes --]

Use 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.

Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
LKML-Reference: <new-submission>
---
Index: linux-2.6/kernel/workqueue.c
===================================================================
--- linux-2.6.orig/kernel/workqueue.c
+++ linux-2.6/kernel/workqueue.c
@@ -1107,8 +1107,15 @@ static void delayed_work_timer_fn(unsign
 int 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

[Peter Zijlstra]

[-- Attachment #1: workqueue-remove-cancel_delayed_work.patch --]
[-- Type: text/plain, Size: 7932 bytes --]

Since queue_delayed_work() can now deal with existing timers, we don't
need to explicitly call cancel_delayed_work() anymore.

Cc: Jens Axboe <axboe@kernel.dk>
Cc: Faisal Latif <faisal.latif@intel.com>
Cc: Roland Dreier <roland@kernel.org>
Cc: Sean Hefty <sean.hefty@intel.com>
Cc: Hal Rosenstock <hal.rosenstock@gmail.com>
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Cc: Alessandro Rubini <rubini@ipvvis.unipv.it>
Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: Mark Fasheh <mfasheh@suse.com>
Cc: Joel Becker <jlbec@evilplan.org>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: "John W. Linville" <linville@tuxdriver.com>
Cc: Johannes Berg <johannes@sipsolutions.net>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
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(-)

Index: linux-2.6/block/blk-throttle.c
===================================================================
--- linux-2.6.orig/block/blk-throttle.c
+++ linux-2.6/block/blk-throttle.c
@@ -814,11 +814,6 @@ void throtl_schedule_delayed_work(struct
 	struct 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);
Index: linux-2.6/drivers/infiniband/hw/nes/nes_hw.c
===================================================================
--- linux-2.6.orig/drivers/infiniband/hw/nes/nes_hw.c
+++ linux-2.6/drivers/infiniband/hw/nes/nes_hw.c
@@ -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);
Index: linux-2.6/drivers/infiniband/hw/nes/nes_nic.c
===================================================================
--- linux-2.6.orig/drivers/infiniband/hw/nes/nes_nic.c
+++ linux-2.6/drivers/infiniband/hw/nes/nes_nic.c
@@ -243,8 +243,6 @@ static int nes_netdev_open(struct net_de
 
 	spin_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);
 	}
Index: linux-2.6/drivers/input/keyboard/qt2160.c
===================================================================
--- linux-2.6.orig/drivers/input/keyboard/qt2160.c
+++ linux-2.6/drivers/input/keyboard/qt2160.c
@@ -155,10 +155,7 @@ static irqreturn_t qt2160_irq(int irq, v
 	unsigned 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;
Index: linux-2.6/drivers/input/mouse/synaptics_i2c.c
===================================================================
--- linux-2.6.orig/drivers/input/mouse/synaptics_i2c.c
+++ linux-2.6/drivers/input/mouse/synaptics_i2c.c
@@ -374,14 +374,7 @@ static void synaptics_i2c_reschedule_wor
 	unsigned 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);
 }
 
Index: linux-2.6/drivers/misc/bh1770glc.c
===================================================================
--- linux-2.6.orig/drivers/misc/bh1770glc.c
+++ linux-2.6/drivers/misc/bh1770glc.c
@@ -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));
 	}
Index: linux-2.6/drivers/net/phy/phy.c
===================================================================
--- linux-2.6.orig/drivers/net/phy/phy.c
+++ linux-2.6/drivers/net/phy/phy.c
@@ -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;
Index: linux-2.6/drivers/power/jz4740-battery.c
===================================================================
--- linux-2.6.orig/drivers/power/jz4740-battery.c
+++ linux-2.6/drivers/power/jz4740-battery.c
@@ -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;
Index: linux-2.6/fs/nfs/nfs4renewd.c
===================================================================
--- linux-2.6.orig/fs/nfs/nfs4renewd.c
+++ linux-2.6/fs/nfs/nfs4renewd.c
@@ -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);
Index: linux-2.6/fs/ocfs2/cluster/heartbeat.c
===================================================================
--- linux-2.6.orig/fs/ocfs2/cluster/heartbeat.c
+++ linux-2.6/fs/ocfs2/cluster/heartbeat.c
@@ -332,7 +332,6 @@ static void o2hb_arm_write_timeout(struc
 		clear_bit(reg->hr_region_num, o2hb_failed_region_bitmap);
 		spin_unlock(&o2hb_live_lock);
 	}
-	cancel_delayed_work(&reg->hr_write_timeout_work);
 	reg->hr_last_timeout_start = jiffies;
 	schedule_delayed_work(&reg->hr_write_timeout_work,
 			      msecs_to_jiffies(O2HB_MAX_WRITE_TIMEOUT_MS));
Index: linux-2.6/net/core/dst.c
===================================================================
--- linux-2.6.orig/net/core/dst.c
+++ linux-2.6/net/core/dst.c
@@ -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);
Index: linux-2.6/net/rfkill/input.c
===================================================================
--- linux-2.6.orig/net/rfkill/input.c
+++ linux-2.6/net/rfkill/input.c
@@ -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

Re: [PATCH 1/4] lockdep, timer: Fix del_timer_sync() annotation

[Thomas Gleixner]

On Thu, 3 Feb 2011, Peter Zijlstra wrote:

> 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.
> 
> Reported-by: Nick Bowler <nbowler@elliptictech.com>

Nick, can you please test ?

Thanks,

	tglx

> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> LKML-Reference: <new-submission>
> ---
>  kernel/timer.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/kernel/timer.c
> ===================================================================
> --- linux-2.6.orig/kernel/timer.c
> +++ linux-2.6/kernel/timer.c
> @@ -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 4/4] workqueue: Remove now superfluous cancel_delayed_work() calls

[Tejun Heo]

Hello, Peter.

On Thu, Feb 03, 2011 at 03:09:44PM +0100, Peter Zijlstra wrote:
> Since queue_delayed_work() can now deal with existing timers, we don't
> need to explicitly call cancel_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.

-- 
tejun

Re: [PATCH 4/4] workqueue: Remove now superfluous cancel_delayed_work() calls

[Dmitry Torokhov]

On Thu, Feb 03, 2011 at 05:19:06PM +0100, Tejun Heo wrote:
> Hello, Peter.
> 
> On Thu, Feb 03, 2011 at 03:09:44PM +0100, Peter Zijlstra wrote:
> > Since queue_delayed_work() can now deal with existing timers, we don't
> > need to explicitly call cancel_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.
> 

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.

-- 
Dmitry

Re: [PATCH 4/4] workqueue: Remove now superfluous cancel_delayed_work() calls

[Peter Zijlstra]

On Thu, 2011-02-03 at 17:19 +0100, Tejun Heo wrote:
> Hello, Peter.
> 
> On Thu, Feb 03, 2011 at 03:09:44PM +0100, Peter Zijlstra wrote:
> > Since queue_delayed_work() can now deal with existing timers, we don't
> > need to explicitly call cancel_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.

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?

> 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.

Re: [PATCH 1/4] lockdep, timer: Fix del_timer_sync() annotation

[Yong Zhang]

On Thu, Feb 03, 2011 at 03:09:41PM +0100, Peter Zijlstra wrote:
> 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.
> 
> Reported-by: Nick Bowler <nbowler@elliptictech.com>
> Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
> LKML-Reference: <new-submission>
> ---
>  kernel/timer.c |    6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> Index: linux-2.6/kernel/timer.c
> ===================================================================
> --- linux-2.6.orig/kernel/timer.c
> +++ linux-2.6/kernel/timer.c
> @@ -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);

This is more cheap than what I have done ;)
I think it will cure the issue reported by Nick.
Thanks you anyway.

Reviewed-by: Yong Zhang <yong.zhang0@gmail.com>

[tip:timers/urgent] lockdep, timer: Fix del_timer_sync() annotation

[tip-bot for Peter Zijlstra]

Commit-ID:  f266a5110d453b7987194460ac7edd31f1a5426c
Gitweb:     http://git.kernel.org/tip/f266a5110d453b7987194460ac7edd31f1a5426c
Author:     Peter Zijlstra <a.p.zijlstra@chello.nl>
AuthorDate: Thu, 3 Feb 2011 15:09:41 +0100
Committer:  Thomas Gleixner <tglx@linutronix.de>
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.

Reported-by: Nick Bowler <nbowler@elliptictech.com>
Signed-off-by: Peter Zijlstra <a.p.zijlstra@chello.nl>
Reviewed-by: Yong Zhang <yong.zhang0@gmail.com>
LKML-Reference: <20110203141548.039540914@chello.nl>
Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
---
 kernel/timer.c |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/kernel/timer.c b/kernel/timer.c
index 43ca993..d53ce66 100644
--- a/kernel/timer.c
+++ b/kernel/timer.c
@@ -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 4/4] workqueue: Remove now superfluous cancel_delayed_work() calls

[Tejun Heo]

Hello, Peter.

On Thu, Feb 03, 2011 at 06:45:41PM +0100, Peter Zijlstra wrote:
> On Thu, 2011-02-03 at 17:19 +0100, Tejun Heo wrote:
> > On Thu, Feb 03, 2011 at 03:09:44PM +0100, Peter Zijlstra wrote:
> > > Since queue_delayed_work() can now deal with existing timers, we don't
> > > need to explicitly call cancel_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.
> 
> 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.

-- 
tejun

Re: [PATCH 1/4] lockdep, timer: Fix del_timer_sync() annotation

[Nick Bowler]

On 2011-02-03 16:35 +0100, Thomas Gleixner wrote:
> On Thu, 3 Feb 2011, Peter Zijlstra wrote:
> > 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.
> > 
> > Reported-by: Nick Bowler <nbowler@elliptictech.com>
> 
> Nick, can you please test ?

Yes, this patch seems to solve the issue.

Thanks,
-- 
Nick Bowler, Elliptic Technologies (http://www.elliptictech.com/)