[PATCHSET] workqueue: don't use [delayed_]work_pending()

[PATCHSET] workqueue: don't use [delayed_]work_pending()

[Tejun Heo]

Hello,

Given the current set of workqueue APIs, there are very few cases
where [delayed_]work_pending() are actually necessary; however, it's
seemingly somewhat popular for a few purposes including skipping
queue/flush/cancel depending on the current state for optimization.

work_pending() could be slightly cheaper than performing the actual
operation because it can skip atomic bitops assuming that the user is
synchronizing against other workqueue operations properly; however,
most paths with this type of optimization are siberia-cold for this
level of optimization to matter - e.g. driver detach path or parameter
update via sysfs - and it's easy to get it subtly wrong and introduce
difficult-to-trigger race conditions.  It just isn't worth it.

Other use cases include using work_pending() state to decide the state
of previously scheduled async action.  This too, unfortunately, seems
easy to get wrong.  Several users forgot that work_pending() becomes
false *before* the work item starts execution and fails to synchronize
with on-going execution.  Unless one is specifically looking for
those, they can be tricky to spot.

Overall, [delayed_]work_pending() seem to bring more troubles than
benefits and not using them usually results in better code.  This
patchset removes [delayed_]work_pending() usages from various
subsystems.  A lot are straight-forward removal of unnecessary
optimizations.  Some fix bugs around work item handling.  Others
restructure code so that [delayed_]work_pending() isn't necessary.

After this patchset, there remain a handful of
[delayed_]work_pending() users.  Some of them legit.  Some quite
broken.  Hopefully, they can be converted too and we can unexport
these easy-to-misuse interface.

This patchset contains the following 25 patches.

 0001-charger_manager-don-t-use-delayed_-work_pending.patch
 0002-ab8500_charger-don-t-use-delayed_-work_pending.patch
 0003-sja1000-don-t-use-delayed_-work_pending.patch
 0004-ipw2x00-simplify-scan_event-handling.patch
 0005-devfreq-don-t-use-delayed_-work_pending.patch
 0006-libertas-don-t-use-delayed_-work_pending.patch
 0007-mwifiex-don-t-use-delayed_-work_pending.patch
 0008-thinkpad_acpi-don-t-use-delayed_-work_pending.patch
 0009-wl1251-don-t-use-delayed_-work_pending.patch
 0010-kprobes-fix-wait_for_kprobe_optimizer.patch
 0011-pm-don-t-use-delayed_-work_pending.patch
 0012-bluetooth-l2cap-don-t-use-delayed_-work_pending.patch
 0013-sound-wm8350-don-t-use-delayed_-work_pending.patch
 0014-rfkill-don-t-use-delayed_-work_pending.patch
 0015-x86-mce-don-t-use-delayed_-work_pending.patch
 0016-PM-Domains-don-t-use-delayed_-work_pending.patch
 0017-wm97xx-don-t-use-delayed_-work_pending.patch
 0018-TMIO-MMC-don-t-use-delayed_-work_pending.patch
 0019-net-caif-don-t-use-delayed_-work_pending.patch
 0020-wimax-i2400m-fix-i2400m-wake_tx_skb-handling.patch
 0021-tty-max3100-don-t-use-delayed_-work_pending.patch
 0022-usb-at91_udc-don-t-use-delayed_-work_pending.patch
 0023-video-exynos-don-t-use-delayed_-work_pending.patch
 0024-debugobjects-don-t-use-delayed_-work_pending.patch
 0025-ipc-don-t-use-delayed_-work_pending.patch

And available in the following git branch.

 git://git.kernel.org/pub/scm/linux/kernel/git/tj/wq.git review-work_pending-cleanup

diffstat follows.

 arch/x86/kernel/cpu/mcheck/mce.c        |   10 ++--------
 drivers/base/power/domain.c             |    3 +--
 drivers/devfreq/devfreq.c               |    3 +--
 drivers/input/touchscreen/wm97xx-core.c |    4 +---
 drivers/mmc/host/tmio_mmc_pio.c         |    3 +--
 drivers/net/caif/caif_shmcore.c         |   15 +++++----------
 drivers/net/can/sja1000/peak_pci.c      |    3 +--
 drivers/net/wimax/i2400m/netdev.c       |   31 +++++++++++++++++--------------
 drivers/net/wireless/ipw2x00/ipw2100.c  |   31 ++++++++-----------------------
 drivers/net/wireless/ipw2x00/ipw2100.h  |    3 +--
 drivers/net/wireless/ipw2x00/ipw2200.c  |   13 +++----------
 drivers/net/wireless/libertas/cfg.c     |    2 +-
 drivers/net/wireless/libertas/if_sdio.c |    9 ++++-----
 drivers/net/wireless/mwifiex/sdio.c     |    9 ++++-----
 drivers/net/wireless/ti/wl1251/ps.c     |    3 +--
 drivers/platform/x86/thinkpad_acpi.c    |    3 +--
 drivers/power/ab8500_charger.c          |   13 ++++---------
 drivers/power/charger-manager.c         |   31 ++++++++++++++++---------------
 drivers/tty/serial/max3100.c            |    3 +--
 drivers/usb/gadget/at91_udc.c           |    3 +--
 drivers/video/exynos/exynos_dp_core.c   |    6 ++----
 include/net/bluetooth/l2cap.h           |   24 ++++++++++++++++--------
 ipc/util.c                              |    3 +--
 kernel/kprobes.c                        |   23 +++++++++++++++--------
 kernel/power/autosleep.c                |    2 +-
 kernel/power/qos.c                      |    9 +++------
 lib/debugobjects.c                      |    7 +++----
 net/bluetooth/l2cap_core.c              |    7 +++----
 net/rfkill/input.c                      |    8 +++-----
 sound/soc/codecs/wm8350.c               |   10 ++++------
 30 files changed, 125 insertions(+), 169 deletions(-)

Thanks.

--
tejun

[PATCH 01/25] charger_manager: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests and rewrite _setup_polling() so that
it uses mod_delayed_work() if the next polling interval is sooner than
currently scheduled.  queue_delayed_work() is used otherwise.

Only compile tested.  I noticed that two work items - setup_polling
and cm_monitor_work - schedule each other.  It's a very unusual
construct and I'm fairly sure it's racy.  You can't break such
circular dependency by calling cancel on each.  I strongly recommend
revising the mechanism.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Anton Vorontsov <cbou@mail.ru>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: Donggeun Kim <dg77.kim@samsung.com>
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/power/charger-manager.c | 31 ++++++++++++++++---------------
 1 file changed, 16 insertions(+), 15 deletions(-)

diff --git a/drivers/power/charger-manager.c b/drivers/power/charger-manager.c
index adb3a4b..9fd9776 100644
--- a/drivers/power/charger-manager.c
+++ b/drivers/power/charger-manager.c
@@ -675,15 +675,21 @@ static void _setup_polling(struct work_struct *work)
 	WARN(cm_wq == NULL, "charger-manager: workqueue not initialized"
 			    ". try it later. %s\n", __func__);
 
+	/*
+	 * Use mod_delayed_work() iff the next polling interval should
+	 * occur before the currently scheduled one.  If @cm_monitor_work
+	 * isn't active, the end result is the same, so no need to worry
+	 * about stale @next_polling.
+	 */
 	_next_polling = jiffies + polling_jiffy;
 
-	if (!delayed_work_pending(&cm_monitor_work) ||
-	    (delayed_work_pending(&cm_monitor_work) &&
-	     time_after(next_polling, _next_polling))) {
-		next_polling = jiffies + polling_jiffy;
+	if (time_before(_next_polling, next_polling)) {
 		mod_delayed_work(cm_wq, &cm_monitor_work, polling_jiffy);
+		next_polling = _next_polling;
+	} else {
+		if (queue_delayed_work(cm_wq, &cm_monitor_work, polling_jiffy))
+			next_polling = _next_polling;
 	}
-
 out:
 	mutex_unlock(&cm_list_mtx);
 }
@@ -757,8 +763,7 @@ static void misc_event_handler(struct charger_manager *cm,
 	if (cm_suspended)
 		device_set_wakeup_capable(cm->dev, true);
 
-	if (!delayed_work_pending(&cm_monitor_work) &&
-	    is_polling_required(cm) && cm->desc->polling_interval_ms)
+	if (is_polling_required(cm) && cm->desc->polling_interval_ms)
 		schedule_work(&setup_polling);
 	uevent_notify(cm, default_event_names[type]);
 }
@@ -1176,8 +1181,7 @@ static int charger_extcon_notifier(struct notifier_block *self,
 	 * when charger cable is attached.
 	 */
 	if (cable->attached && is_polling_required(cable->cm)) {
-		if (work_pending(&setup_polling))
-			cancel_work_sync(&setup_polling);
+		cancel_work_sync(&setup_polling);
 		schedule_work(&setup_polling);
 	}
 
@@ -1667,10 +1671,8 @@ static int charger_manager_remove(struct platform_device *pdev)
 	list_del(&cm->entry);
 	mutex_unlock(&cm_list_mtx);
 
-	if (work_pending(&setup_polling))
-		cancel_work_sync(&setup_polling);
-	if (delayed_work_pending(&cm_monitor_work))
-		cancel_delayed_work_sync(&cm_monitor_work);
+	cancel_work_sync(&setup_polling);
+	cancel_delayed_work_sync(&cm_monitor_work);
 
 	for (i = 0 ; i < desc->num_charger_regulators ; i++) {
 		struct charger_regulator *charger
@@ -1739,8 +1741,7 @@ static int cm_suspend_prepare(struct device *dev)
 		cm_suspended = true;
 	}
 
-	if (delayed_work_pending(&cm->fullbatt_vchk_work))
-		cancel_delayed_work(&cm->fullbatt_vchk_work);
+	cancel_delayed_work(&cm->fullbatt_vchk_work);
 	cm->status_save_ext_pwr_inserted = is_ext_pwr_online(cm);
 	cm->status_save_batt = is_batt_present(cm);
 
-- 
1.8.0.2

Re: [PATCH 01/25] charger_manager: don't use [delayed_]work_pending()

[Anton Vorontsov]

On Fri, Dec 21, 2012 at 05:56:51PM -0800, Tejun Heo wrote:
> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
> 
> Remove unnecessary pending tests and rewrite _setup_polling() so that
> it uses mod_delayed_work() if the next polling interval is sooner than
> currently scheduled.  queue_delayed_work() is used otherwise.
> 
> Only compile tested.  I noticed that two work items - setup_polling
> and cm_monitor_work - schedule each other.  It's a very unusual
> construct and I'm fairly sure it's racy.  You can't break such
> circular dependency by calling cancel on each.  I strongly recommend
> revising the mechanism.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Anton Vorontsov <cbou@mail.ru>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: Donggeun Kim <dg77.kim@samsung.com>
> Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.

Charger manager is a fast moving target, so it is prone to conflict; it is
better if I take it via battery-2.6.git tree. It is merged, thanks a lot!

Anton

[PATCH 02/25] ab8500_charger: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from ab8500_charger.  Only compile
tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
Cc: Linus Walleij <linus.walleij@linaro.org>
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/power/ab8500_charger.c | 13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

diff --git a/drivers/power/ab8500_charger.c b/drivers/power/ab8500_charger.c
index 3be9c0e..40de240 100644
--- a/drivers/power/ab8500_charger.c
+++ b/drivers/power/ab8500_charger.c
@@ -1319,8 +1319,7 @@ static int ab8500_charger_usb_en(struct ux500_charger *charger,
 		dev_dbg(di->dev, "%s Disabled USB charging\n", __func__);
 
 		/* Cancel any pending Vbat check work */
-		if (delayed_work_pending(&di->check_vbat_work))
-			cancel_delayed_work(&di->check_vbat_work);
+		cancel_delayed_work(&di->check_vbat_work);
 
 	}
 	ab8500_power_supply_changed(di, &di->usb_chg.psy);
@@ -2460,11 +2459,8 @@ static int ab8500_charger_resume(struct platform_device *pdev)
 			dev_err(di->dev, "Failed to kick WD!\n");
 
 		/* If not already pending start a new timer */
-		if (!delayed_work_pending(
-			&di->kick_wd_work)) {
-			queue_delayed_work(di->charger_wq, &di->kick_wd_work,
-				round_jiffies(WD_KICK_INTERVAL));
-		}
+		queue_delayed_work(di->charger_wq, &di->kick_wd_work,
+				   round_jiffies(WD_KICK_INTERVAL));
 	}
 
 	/* If we still have a HW failure, schedule a new check */
@@ -2482,8 +2478,7 @@ static int ab8500_charger_suspend(struct platform_device *pdev,
 	struct ab8500_charger *di = platform_get_drvdata(pdev);
 
 	/* Cancel any pending HW failure check */
-	if (delayed_work_pending(&di->check_hw_failure_work))
-		cancel_delayed_work(&di->check_hw_failure_work);
+	cancel_delayed_work(&di->check_hw_failure_work);
 
 	return 0;
 }
-- 
1.8.0.2

RE: [PATCH 02/25] ab8500_charger: don't use [delayed_]work_pending()

[Arun MURTHY]

> > There's no need to test whether a (delayed) work item in pending
> > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > and quite a few of them are buggy.
> >
> > Remove unnecessary pending tests from ab8500_charger.  Only compile
> > tested.
> >
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
> > Cc: Linus Walleij <linus.walleij@linaro.org>

Acked-By: Arun Murthy <arun.murthy@stericsson.com>

Thanks and Regards,
Arun R Murthy
------------------

Re: [PATCH 02/25] ab8500_charger: don't use [delayed_]work_pending()

[Linus Walleij]

On Sat, Dec 22, 2012 at 2:56 AM, Tejun Heo <tj@kernel.org> wrote:

> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
>
> Remove unnecessary pending tests from ab8500_charger.  Only compile
> tested.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Srinidhi Kasagar <srinidhi.kasagar@stericsson.com>
> Cc: Linus Walleij <linus.walleij@linaro.org>

Works like a charm for us, thanks!
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>
Tested-by: Marcus Cooper <marcus.xm.cooper@stericsson.com>

Yours,
Linus Walleij

[PATCH 03/25] sja1000: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from sja1000.  Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Wolfgang Grandegger <wg@grandegger.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/net/can/sja1000/peak_pci.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/can/sja1000/peak_pci.c b/drivers/net/can/sja1000/peak_pci.c
index d84888f..600ac72 100644
--- a/drivers/net/can/sja1000/peak_pci.c
+++ b/drivers/net/can/sja1000/peak_pci.c
@@ -339,8 +339,7 @@ static void peak_pciec_set_leds(struct peak_pciec_card *card, u8 led_mask, u8 s)
  */
 static void peak_pciec_start_led_work(struct peak_pciec_card *card)
 {
-	if (!delayed_work_pending(&card->led_work))
-		schedule_delayed_work(&card->led_work, HZ);
+	schedule_delayed_work(&card->led_work, HZ);
 }
 
 /*
-- 
1.8.0.2

Re: [PATCH 03/25] sja1000: don't use [delayed_]work_pending()

[David Miller]

From: Tejun Heo <tj@kernel.org>
Date: Fri, 21 Dec 2012 17:56:53 -0800

> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
> 
> Remove unnecessary pending tests from sja1000.  Only compile tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Wolfgang Grandegger <wg@grandegger.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.

I would suggest just taking it via the workqueue tree, thanks Tejun.

Re: [PATCH 03/25] sja1000: don't use [delayed_]work_pending()

[Tejun Heo]

On Sat, Dec 22, 2012 at 12:01:11AM -0800, David Miller wrote:
> From: Tejun Heo <tj@kernel.org>
> Date: Fri, 21 Dec 2012 17:56:53 -0800
> 
> > There's no need to test whether a (delayed) work item in pending
> > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > and quite a few of them are buggy.
> > 
> > Remove unnecessary pending tests from sja1000.  Only compile tested.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Wolfgang Grandegger <wg@grandegger.com>
> > Cc: "David S. Miller" <davem@davemloft.net>
> > Cc: netdev@vger.kernel.org
> > ---
> > Please let me know how this patch should be routed.  I can take it
> > through the workqueue tree if necessary.
> 
> I would suggest just taking it via the workqueue tree, thanks Tejun.

Applied to wq/for-3.9-cleanups w/ your Acked-by added.

Thanks.

-- 
tejun

[PATCH 04/25] ipw2x00: simplify scan_event handling

[Tejun Heo]

* Drop unnesssary delayd_work_pending() tests.

* Unify scan_event_{now|later} by using mod_delayed_work() w/ 0 delay
  for scan_event_now.

* Make ipw2200 scan_event handling match ipw2100 - use
  mod_delayed_work() w/ 0 delay for immediate scanning.

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Stanislav Yakovlev <stas.yakovlev@gmail.com>
Cc: linux-wireless@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/net/wireless/ipw2x00/ipw2100.c | 31 ++++++++-----------------------
 drivers/net/wireless/ipw2x00/ipw2100.h |  3 +--
 drivers/net/wireless/ipw2x00/ipw2200.c | 13 +++----------
 3 files changed, 12 insertions(+), 35 deletions(-)

diff --git a/drivers/net/wireless/ipw2x00/ipw2100.c b/drivers/net/wireless/ipw2x00/ipw2100.c
index d92b21a..b3ab7b7 100644
--- a/drivers/net/wireless/ipw2x00/ipw2100.c
+++ b/drivers/net/wireless/ipw2x00/ipw2100.c
@@ -2181,9 +2181,10 @@ static void isr_indicate_rf_kill(struct ipw2100_priv *priv, u32 status)
 	mod_delayed_work(system_wq, &priv->rf_kill, round_jiffies_relative(HZ));
 }
 
-static void send_scan_event(void *data)
+static void ipw2100_scan_event(struct work_struct *work)
 {
-	struct ipw2100_priv *priv = data;
+	struct ipw2100_priv *priv = container_of(work, struct ipw2100_priv,
+						 scan_event.work);
 	union iwreq_data wrqu;
 
 	wrqu.data.length = 0;
@@ -2191,18 +2192,6 @@ static void send_scan_event(void *data)
 	wireless_send_event(priv->net_dev, SIOCGIWSCAN, &wrqu, NULL);
 }
 
-static void ipw2100_scan_event_later(struct work_struct *work)
-{
-	send_scan_event(container_of(work, struct ipw2100_priv,
-					scan_event_later.work));
-}
-
-static void ipw2100_scan_event_now(struct work_struct *work)
-{
-	send_scan_event(container_of(work, struct ipw2100_priv,
-					scan_event_now));
-}
-
 static void isr_scan_complete(struct ipw2100_priv *priv, u32 status)
 {
 	IPW_DEBUG_SCAN("scan complete\n");
@@ -2212,13 +2201,11 @@ static void isr_scan_complete(struct ipw2100_priv *priv, u32 status)
 
 	/* Only userspace-requested scan completion events go out immediately */
 	if (!priv->user_requested_scan) {
-		if (!delayed_work_pending(&priv->scan_event_later))
-			schedule_delayed_work(&priv->scan_event_later,
-					      round_jiffies_relative(msecs_to_jiffies(4000)));
+		schedule_delayed_work(&priv->scan_event,
+				      round_jiffies_relative(msecs_to_jiffies(4000)));
 	} else {
 		priv->user_requested_scan = 0;
-		cancel_delayed_work(&priv->scan_event_later);
-		schedule_work(&priv->scan_event_now);
+		mod_delayed_work(system_wq, &priv->scan_event, 0);
 	}
 }
 
@@ -4459,8 +4446,7 @@ static void ipw2100_kill_works(struct ipw2100_priv *priv)
 	cancel_delayed_work_sync(&priv->wx_event_work);
 	cancel_delayed_work_sync(&priv->hang_check);
 	cancel_delayed_work_sync(&priv->rf_kill);
-	cancel_work_sync(&priv->scan_event_now);
-	cancel_delayed_work_sync(&priv->scan_event_later);
+	cancel_delayed_work_sync(&priv->scan_event);
 }
 
 static int ipw2100_tx_allocate(struct ipw2100_priv *priv)
@@ -6195,8 +6181,7 @@ static struct net_device *ipw2100_alloc_device(struct pci_dev *pci_dev,
 	INIT_DELAYED_WORK(&priv->wx_event_work, ipw2100_wx_event_work);
 	INIT_DELAYED_WORK(&priv->hang_check, ipw2100_hang_check);
 	INIT_DELAYED_WORK(&priv->rf_kill, ipw2100_rf_kill);
-	INIT_WORK(&priv->scan_event_now, ipw2100_scan_event_now);
-	INIT_DELAYED_WORK(&priv->scan_event_later, ipw2100_scan_event_later);
+	INIT_DELAYED_WORK(&priv->scan_event, ipw2100_scan_event);
 
 	tasklet_init(&priv->irq_tasklet, (void (*)(unsigned long))
 		     ipw2100_irq_tasklet, (unsigned long)priv);
diff --git a/drivers/net/wireless/ipw2x00/ipw2100.h b/drivers/net/wireless/ipw2x00/ipw2100.h
index 5fe17cb..c6d7879 100644
--- a/drivers/net/wireless/ipw2x00/ipw2100.h
+++ b/drivers/net/wireless/ipw2x00/ipw2100.h
@@ -577,8 +577,7 @@ struct ipw2100_priv {
 	struct delayed_work wx_event_work;
 	struct delayed_work hang_check;
 	struct delayed_work rf_kill;
-	struct work_struct scan_event_now;
-	struct delayed_work scan_event_later;
+	struct delayed_work scan_event;
 
 	int user_requested_scan;
 
diff --git a/drivers/net/wireless/ipw2x00/ipw2200.c b/drivers/net/wireless/ipw2x00/ipw2200.c
index 844f201..2c2d6db 100644
--- a/drivers/net/wireless/ipw2x00/ipw2200.c
+++ b/drivers/net/wireless/ipw2x00/ipw2200.c
@@ -4480,18 +4480,11 @@ static void handle_scan_event(struct ipw_priv *priv)
 {
 	/* Only userspace-requested scan completion events go out immediately */
 	if (!priv->user_requested_scan) {
-		if (!delayed_work_pending(&priv->scan_event))
-			schedule_delayed_work(&priv->scan_event,
-					      round_jiffies_relative(msecs_to_jiffies(4000)));
+		schedule_delayed_work(&priv->scan_event,
+				      round_jiffies_relative(msecs_to_jiffies(4000)));
 	} else {
-		union iwreq_data wrqu;
-
 		priv->user_requested_scan = 0;
-		cancel_delayed_work(&priv->scan_event);
-
-		wrqu.data.length = 0;
-		wrqu.data.flags = 0;
-		wireless_send_event(priv->net_dev, SIOCGIWSCAN, &wrqu, NULL);
+		mod_delayed_work(system_wq, &priv->scan_event, 0);
 	}
 }
 
-- 
1.8.0.2

Re: [PATCH 04/25] ipw2x00: simplify scan_event handling

[Stanislav Yakovlev]

Hello, Tejun,

On 22 December 2012 04:56, Tejun Heo <tj@kernel.org> wrote:
> * Drop unnesssary delayd_work_pending() tests.
>
> * Unify scan_event_{now|later} by using mod_delayed_work() w/ 0 delay
>   for scan_event_now.
>
> * Make ipw2200 scan_event handling match ipw2100 - use
>   mod_delayed_work() w/ 0 delay for immediate scanning.
>
> Only compile tested.
>
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Stanislav Yakovlev <stas.yakovlev@gmail.com>
> Cc: linux-wireless@vger.kernel.org
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.

Please, feel free to take it through your tree.

Acked-by: Stanislav Yakovlev <stas.yakovlev@gmail.com>

Thanks!
Stanislav.

Re: [PATCH 04/25] ipw2x00: simplify scan_event handling

[Tejun Heo]

On Mon, Jan 28, 2013 at 12:02:27AM +0300, Stanislav Yakovlev wrote:
> Hello, Tejun,
> 
> On 22 December 2012 04:56, Tejun Heo <tj@kernel.org> wrote:
> > * Drop unnesssary delayd_work_pending() tests.
> >
> > * Unify scan_event_{now|later} by using mod_delayed_work() w/ 0 delay
> >   for scan_event_now.
> >
> > * Make ipw2200 scan_event handling match ipw2100 - use
> >   mod_delayed_work() w/ 0 delay for immediate scanning.
> >
> > Only compile tested.
> >
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Stanislav Yakovlev <stas.yakovlev@gmail.com>
> > Cc: linux-wireless@vger.kernel.org
> > ---
> > Please let me know how this patch should be routed.  I can take it
> > through the workqueue tree if necessary.
> 
> Please, feel free to take it through your tree.
> 
> Acked-by: Stanislav Yakovlev <stas.yakovlev@gmail.com>

Queued to wq/for-3.9-cleanups.  Thanks!

-- 
tejun

[PATCH 05/25] devfreq: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from devfreq.  Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: MyungJoo Ham <myungjoo.ham@samsung.com>
Cc: Kyungmin Park <kyungmin.park@samsung.com>
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/devfreq/devfreq.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/devfreq/devfreq.c b/drivers/devfreq/devfreq.c
index 53766f3..fb4695e 100644
--- a/drivers/devfreq/devfreq.c
+++ b/drivers/devfreq/devfreq.c
@@ -291,8 +291,7 @@ void devfreq_monitor_resume(struct devfreq *devfreq)
 	if (!devfreq->stop_polling)
 		goto out;
 
-	if (!delayed_work_pending(&devfreq->work) &&
-			devfreq->profile->polling_ms)
+	if (devfreq->profile->polling_ms)
 		queue_delayed_work(devfreq_wq, &devfreq->work,
 			msecs_to_jiffies(devfreq->profile->polling_ms));
 	devfreq->stop_polling = false;
-- 
1.8.0.2

[PATCH 06/25] libertas: don't use [delayed_]work_pending()

[Tejun Heo]

* delayed_work_pending() test in lbs_cfg_scan() is spurious as
  priv->scan_req can't be NULL w/ scan_work pending; otherwise,
  lbs_scan_worker() will segfault.  Drop it.  BTW, the synchronization
  around scan_work seems racy.  There's nothing synchronizing accesses
  to scan related fields in lbs_private.

* Drop work_pending() test from if_sdio_reset_card().  As
  work_pending() becomes %false before if_sdio_reset_card_worker()
  starts executing, it doesn't really protect anything.  reset_host
  may change between mmc_remove_host() and mmc_add_host().  Make
  if_sdio_reset_card_worker() cache the target mmc_host so that it
  isn't affected by if_sdio_reset_card() racing with it.

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Dan Williams <dcbw@redhat.com>
Cc: libertas-dev@lists.infradead.org
Cc: linux-wireless@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/net/wireless/libertas/cfg.c     | 2 +-
 drivers/net/wireless/libertas/if_sdio.c | 9 ++++-----
 2 files changed, 5 insertions(+), 6 deletions(-)

diff --git a/drivers/net/wireless/libertas/cfg.c b/drivers/net/wireless/libertas/cfg.c
index ec6d5d6..ec30cd1 100644
--- a/drivers/net/wireless/libertas/cfg.c
+++ b/drivers/net/wireless/libertas/cfg.c
@@ -814,7 +814,7 @@ static int lbs_cfg_scan(struct wiphy *wiphy,
 
 	lbs_deb_enter(LBS_DEB_CFG80211);
 
-	if (priv->scan_req || delayed_work_pending(&priv->scan_work)) {
+	if (priv->scan_req) {
 		/* old scan request not yet processed */
 		ret = -EAGAIN;
 		goto out;
diff --git a/drivers/net/wireless/libertas/if_sdio.c b/drivers/net/wireless/libertas/if_sdio.c
index 739309e..8c53c17 100644
--- a/drivers/net/wireless/libertas/if_sdio.c
+++ b/drivers/net/wireless/libertas/if_sdio.c
@@ -1074,6 +1074,8 @@ static struct mmc_host *reset_host;
 
 static void if_sdio_reset_card_worker(struct work_struct *work)
 {
+	struct mmc_host *target = reset_host;
+
 	/*
 	 * The actual reset operation must be run outside of lbs_thread. This
 	 * is because mmc_remove_host() will cause the device to be instantly
@@ -1085,8 +1087,8 @@ static void if_sdio_reset_card_worker(struct work_struct *work)
 	 */
 
 	pr_info("Resetting card...");
-	mmc_remove_host(reset_host);
-	mmc_add_host(reset_host);
+	mmc_remove_host(target);
+	mmc_add_host(target);
 }
 static DECLARE_WORK(card_reset_work, if_sdio_reset_card_worker);
 
@@ -1094,9 +1096,6 @@ static void if_sdio_reset_card(struct lbs_private *priv)
 {
 	struct if_sdio_card *card = priv->card;
 
-	if (work_pending(&card_reset_work))
-		return;
-
 	reset_host = card->func->card->host;
 	schedule_work(&card_reset_work);
 }
-- 
1.8.0.2

[PATCH 07/25] mwifiex: don't use [delayed_]work_pending()

[Tejun Heo]

Drop work_pending() test from mwifiex_sdio_card_reset().  As
work_pending() becomes %false before sdio_card_reset_worker() starts
executing, it doesn't really protect anything.  reset_host may change
between mmc_remove_host() and mmc_add_host().  Make
sdio_card_reset_worker() cache the target mmc_host so that it isn't
affected by mwifiex_sdio_card_reset() racing with it.

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Bing Zhao <bzhao@marvell.com>
Cc: linux-wireless@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/net/wireless/mwifiex/sdio.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/mwifiex/sdio.c b/drivers/net/wireless/mwifiex/sdio.c
index 5a1c1d0..f2874c3 100644
--- a/drivers/net/wireless/mwifiex/sdio.c
+++ b/drivers/net/wireless/mwifiex/sdio.c
@@ -1752,6 +1752,8 @@ mwifiex_update_mp_end_port(struct mwifiex_adapter *adapter, u16 port)
 static struct mmc_host *reset_host;
 static void sdio_card_reset_worker(struct work_struct *work)
 {
+	struct mmc_host *target = reset_host;
+
 	/* The actual reset operation must be run outside of driver thread.
 	 * This is because mmc_remove_host() will cause the device to be
 	 * instantly destroyed, and the driver then needs to end its thread,
@@ -1761,10 +1763,10 @@ static void sdio_card_reset_worker(struct work_struct *work)
 	 */
 
 	pr_err("Resetting card...\n");
-	mmc_remove_host(reset_host);
+	mmc_remove_host(target);
 	/* 20ms delay is based on experiment with sdhci controller */
 	mdelay(20);
-	mmc_add_host(reset_host);
+	mmc_add_host(target);
 }
 static DECLARE_WORK(card_reset_work, sdio_card_reset_worker);
 
@@ -1773,9 +1775,6 @@ static void mwifiex_sdio_card_reset(struct mwifiex_adapter *adapter)
 {
 	struct sdio_mmc_card *card = adapter->card;
 
-	if (work_pending(&card_reset_work))
-		return;
-
 	reset_host = card->func->card->host;
 	schedule_work(&card_reset_work);
 }
-- 
1.8.0.2

RE: [PATCH 07/25] mwifiex: don't use [delayed_]work_pending()

[Bing Zhao]

Hi Tejun,

Thanks for the patch.

> Drop work_pending() test from mwifiex_sdio_card_reset().  As
> work_pending() becomes %false before sdio_card_reset_worker() starts
> executing, it doesn't really protect anything.  reset_host may change
> between mmc_remove_host() and mmc_add_host().  Make
> sdio_card_reset_worker() cache the target mmc_host so that it isn't
> affected by mwifiex_sdio_card_reset() racing with it.
> 
> Only compile tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Bing Zhao <bzhao@marvell.com>
> Cc: linux-wireless@vger.kernel.org

Acked-by: Bing Zhao <bzhao@marvell.com>

> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.

If you are taking other patches in this series through your tree, please take this one too.

Thanks,
Bing

> 
> Thanks.
> 
>  drivers/net/wireless/mwifiex/sdio.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/net/wireless/mwifiex/sdio.c b/drivers/net/wireless/mwifiex/sdio.c
> index 5a1c1d0..f2874c3 100644
> --- a/drivers/net/wireless/mwifiex/sdio.c
> +++ b/drivers/net/wireless/mwifiex/sdio.c
> @@ -1752,6 +1752,8 @@ mwifiex_update_mp_end_port(struct mwifiex_adapter *adapter, u16 port)
>  static struct mmc_host *reset_host;
>  static void sdio_card_reset_worker(struct work_struct *work)
>  {
> +	struct mmc_host *target = reset_host;
> +
>  	/* The actual reset operation must be run outside of driver thread.
>  	 * This is because mmc_remove_host() will cause the device to be
>  	 * instantly destroyed, and the driver then needs to end its thread,
> @@ -1761,10 +1763,10 @@ static void sdio_card_reset_worker(struct work_struct *work)
>  	 */
> 
>  	pr_err("Resetting card...\n");
> -	mmc_remove_host(reset_host);
> +	mmc_remove_host(target);
>  	/* 20ms delay is based on experiment with sdhci controller */
>  	mdelay(20);
> -	mmc_add_host(reset_host);
> +	mmc_add_host(target);
>  }
>  static DECLARE_WORK(card_reset_work, sdio_card_reset_worker);
> 
> @@ -1773,9 +1775,6 @@ static void mwifiex_sdio_card_reset(struct mwifiex_adapter *adapter)
>  {
>  	struct sdio_mmc_card *card = adapter->card;
> 
> -	if (work_pending(&card_reset_work))
> -		return;
> -
>  	reset_host = card->func->card->host;
>  	schedule_work(&card_reset_work);
>  }
> --
> 1.8.0.2

Re: [PATCH 07/25] mwifiex: don't use [delayed_]work_pending()

[Tejun Heo]

On Sat, Dec 22, 2012 at 02:29:57PM -0800, Bing Zhao wrote:
> Hi Tejun,
> 
> Thanks for the patch.
> 
> > Drop work_pending() test from mwifiex_sdio_card_reset().  As
> > work_pending() becomes %false before sdio_card_reset_worker() starts
> > executing, it doesn't really protect anything.  reset_host may change
> > between mmc_remove_host() and mmc_add_host().  Make
> > sdio_card_reset_worker() cache the target mmc_host so that it isn't
> > affected by mwifiex_sdio_card_reset() racing with it.
> > 
> > Only compile tested.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Bing Zhao <bzhao@marvell.com>
> > Cc: linux-wireless@vger.kernel.org
> 
> Acked-by: Bing Zhao <bzhao@marvell.com>
> 
> > ---
> > Please let me know how this patch should be routed.  I can take it
> > through the workqueue tree if necessary.
> 
> If you are taking other patches in this series through your tree, please take this one too.

Applied to wq/for-3.9-cleanups.

Thanks.

-- 
tejun

[PATCH 08/25] thinkpad_acpi: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from thinkpad_acpi.  Only compile
tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Henrique de Moraes Holschuh <ibm-acpi@hmh.eng.br>
Cc: ibm-acpi-devel@lists.sourceforge.net
Cc: platform-driver-x86@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/platform/x86/thinkpad_acpi.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/platform/x86/thinkpad_acpi.c b/drivers/platform/x86/thinkpad_acpi.c
index 75dd651..8421d1e 100644
--- a/drivers/platform/x86/thinkpad_acpi.c
+++ b/drivers/platform/x86/thinkpad_acpi.c
@@ -4877,8 +4877,7 @@ static int __init light_init(struct ibm_init_struct *iibm)
 static void light_exit(void)
 {
 	led_classdev_unregister(&tpacpi_led_thinklight.led_classdev);
-	if (work_pending(&tpacpi_led_thinklight.work))
-		flush_workqueue(tpacpi_wq);
+	flush_workqueue(tpacpi_wq);
 }
 
 static int light_read(struct seq_file *m)
-- 
1.8.0.2

Re: [PATCH 08/25] thinkpad_acpi: don't use [delayed_]work_pending()

[Henrique de Moraes Holschuh]

On Fri, 21 Dec 2012, Tejun Heo wrote:
> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
> 
> Remove unnecessary pending tests from thinkpad_acpi.  Only compile
> tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Henrique de Moraes Holschuh <ibm-acpi@hmh.eng.br>
> Cc: ibm-acpi-devel@lists.sourceforge.net
> Cc: platform-driver-x86@vger.kernel.org

Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>

> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.

It will not clash with anything I know of, so feel free to route it
through the workqueue tree.

-- 
  "One disk to rule them all, One disk to find them. One disk to bring
  them all and in the darkness grind them. In the Land of Redmond
  where the shadows lie." -- The Silicon Valley Tarot
  Henrique Holschuh

Re: [PATCH 08/25] thinkpad_acpi: don't use [delayed_]work_pending()

[Tejun Heo]

On Sat, Dec 22, 2012 at 09:55:04PM -0200, Henrique de Moraes Holschuh wrote:
> On Fri, 21 Dec 2012, Tejun Heo wrote:
> > There's no need to test whether a (delayed) work item in pending
> > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > and quite a few of them are buggy.
> > 
> > Remove unnecessary pending tests from thinkpad_acpi.  Only compile
> > tested.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Henrique de Moraes Holschuh <ibm-acpi@hmh.eng.br>
> > Cc: ibm-acpi-devel@lists.sourceforge.net
> > Cc: platform-driver-x86@vger.kernel.org
> 
> Acked-by: Henrique de Moraes Holschuh <hmh@hmh.eng.br>
> 
> > ---
> > Please let me know how this patch should be routed.  I can take it
> > through the workqueue tree if necessary.
> 
> It will not clash with anything I know of, so feel free to route it
> through the workqueue tree.

Applied to wq/for-3.9-cleanups.  Thanks.

-- 
tejun

[PATCH 09/25] wl1251: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from wl1251.  Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Luciano Coelho <coelho@ti.com>
Cc: linux-wireless@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/net/wireless/ti/wl1251/ps.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ti/wl1251/ps.c b/drivers/net/wireless/ti/wl1251/ps.c
index db719f7..b9e27b9 100644
--- a/drivers/net/wireless/ti/wl1251/ps.c
+++ b/drivers/net/wireless/ti/wl1251/ps.c
@@ -68,8 +68,7 @@ int wl1251_ps_elp_wakeup(struct wl1251 *wl)
 	unsigned long timeout, start;
 	u32 elp_reg;
 
-	if (delayed_work_pending(&wl->elp_work))
-		cancel_delayed_work(&wl->elp_work);
+	cancel_delayed_work(&wl->elp_work);
 
 	if (!wl->elp)
 		return 0;
-- 
1.8.0.2

Re: [PATCH 09/25] wl1251: don't use [delayed_]work_pending()

[Luciano Coelho]

On Fri, 2012-12-21 at 17:56 -0800, Tejun Heo wrote:
> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
> 
> Remove unnecessary pending tests from wl1251.  Only compile tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Luciano Coelho <coelho@ti.com>
> Cc: linux-wireless@vger.kernel.org
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.
> 
> Thanks.

It's probably easier if you take it via your tree.  This driver doesn't
get patches very often, so I doubt there will be any conflicts.

Thank you!

Acked-by: Luciano Coelho <coelho@ti.com>

--
Luca.

Re: [PATCH 09/25] wl1251: don't use [delayed_]work_pending()

[Tejun Heo]

On Sat, Dec 22, 2012 at 04:14:29PM +0200, Luciano Coelho wrote:
> On Fri, 2012-12-21 at 17:56 -0800, Tejun Heo wrote:
> > There's no need to test whether a (delayed) work item in pending
> > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > and quite a few of them are buggy.
> > 
> > Remove unnecessary pending tests from wl1251.  Only compile tested.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Luciano Coelho <coelho@ti.com>
> > Cc: linux-wireless@vger.kernel.org
> > ---
> > Please let me know how this patch should be routed.  I can take it
> > through the workqueue tree if necessary.
> > 
> > Thanks.
> 
> It's probably easier if you take it via your tree.  This driver doesn't
> get patches very often, so I doubt there will be any conflicts.
> 
> Thank you!
> 
> Acked-by: Luciano Coelho <coelho@ti.com>

Applied to wq/for-3.9-cleanups.  Thanks.

-- 
tejun

[PATCH 10/25] kprobes: fix wait_for_kprobe_optimizer()

[Tejun Heo]

wait_for_kprobe_optimizer() seems largely broken.  It uses
optimizer_comp which is never re-initialized, so
wait_for_kprobe_optimizer() will never wait for anything once
kprobe_optimizer() finishes all pending jobs for the first time.

Also, aside from completion, delayed_work_pending() is %false once
kprobe_optimizer() starts execution and wait_for_kprobe_optimizer()
won't wait for it.

Reimplement it so that it flushes optimizing_work until
[un]optimizing_lists are empty.  Note that this also makes
optimizing_work execute immediately if someone's waiting for it, which
is the nicer behavior.

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 kernel/kprobes.c | 23 +++++++++++++++--------
 1 file changed, 15 insertions(+), 8 deletions(-)

diff --git a/kernel/kprobes.c b/kernel/kprobes.c
index 098f396..f230e81 100644
--- a/kernel/kprobes.c
+++ b/kernel/kprobes.c
@@ -471,7 +471,6 @@ static LIST_HEAD(unoptimizing_list);
 
 static void kprobe_optimizer(struct work_struct *work);
 static DECLARE_DELAYED_WORK(optimizing_work, kprobe_optimizer);
-static DECLARE_COMPLETION(optimizer_comp);
 #define OPTIMIZE_DELAY 5
 
 /*
@@ -552,8 +551,7 @@ static __kprobes void do_free_cleaned_kprobes(struct list_head *free_list)
 /* Start optimizer after OPTIMIZE_DELAY passed */
 static __kprobes void kick_kprobe_optimizer(void)
 {
-	if (!delayed_work_pending(&optimizing_work))
-		schedule_delayed_work(&optimizing_work, OPTIMIZE_DELAY);
+	schedule_delayed_work(&optimizing_work, OPTIMIZE_DELAY);
 }
 
 /* Kprobe jump optimizer */
@@ -592,16 +590,25 @@ static __kprobes void kprobe_optimizer(struct work_struct *work)
 	/* Step 5: Kick optimizer again if needed */
 	if (!list_empty(&optimizing_list) || !list_empty(&unoptimizing_list))
 		kick_kprobe_optimizer();
-	else
-		/* Wake up all waiters */
-		complete_all(&optimizer_comp);
 }
 
 /* Wait for completing optimization and unoptimization */
 static __kprobes void wait_for_kprobe_optimizer(void)
 {
-	if (delayed_work_pending(&optimizing_work))
-		wait_for_completion(&optimizer_comp);
+	mutex_lock(&kprobe_mutex);
+
+	while (!list_empty(&optimizing_list) || !list_empty(&unoptimizing_list)) {
+		mutex_unlock(&kprobe_mutex);
+
+		/* this will also make optimizing_work execute immmediately */
+		flush_delayed_work(&optimizing_work);
+		/* @optimizing_work might not have been queued yet, relax */
+		cpu_relax();
+
+		mutex_lock(&kprobe_mutex);
+	}
+
+	mutex_unlock(&kprobe_mutex);
 }
 
 /* Optimize kprobe if p is ready to be optimized */
-- 
1.8.0.2

Re: [PATCH 10/25] kprobes: fix wait_for_kprobe_optimizer()

[Masami Hiramatsu]

(2012/12/22 10:57), Tejun Heo wrote:
> wait_for_kprobe_optimizer() seems largely broken.  It uses
> optimizer_comp which is never re-initialized, so
> wait_for_kprobe_optimizer() will never wait for anything once
> kprobe_optimizer() finishes all pending jobs for the first time.

Thank you for fixing that!
I must misunderstand that the DECLARE_COMPLETION() macro.

> Also, aside from completion, delayed_work_pending() is %false once
> kprobe_optimizer() starts execution and wait_for_kprobe_optimizer()
> won't wait for it.
>
> Reimplement it so that it flushes optimizing_work until
> [un]optimizing_lists are empty.  Note that this also makes
> optimizing_work execute immediately if someone's waiting for it, which
> is the nicer behavior.

I think your enhancement is reasonable and GOOD for me.

Thanks again!

Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>


> 
> Only compile tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Ananth N Mavinakayanahalli <ananth@in.ibm.com>
> Cc: Anil S Keshavamurthy <anil.s.keshavamurthy@intel.com>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.
> 
> Thanks.
> 
>  kernel/kprobes.c | 23 +++++++++++++++--------
>  1 file changed, 15 insertions(+), 8 deletions(-)
> 
> diff --git a/kernel/kprobes.c b/kernel/kprobes.c
> index 098f396..f230e81 100644
> --- a/kernel/kprobes.c
> +++ b/kernel/kprobes.c
> @@ -471,7 +471,6 @@ static LIST_HEAD(unoptimizing_list);
>  
>  static void kprobe_optimizer(struct work_struct *work);
>  static DECLARE_DELAYED_WORK(optimizing_work, kprobe_optimizer);
> -static DECLARE_COMPLETION(optimizer_comp);
>  #define OPTIMIZE_DELAY 5
>  
>  /*
> @@ -552,8 +551,7 @@ static __kprobes void do_free_cleaned_kprobes(struct list_head *free_list)
>  /* Start optimizer after OPTIMIZE_DELAY passed */
>  static __kprobes void kick_kprobe_optimizer(void)
>  {
> -	if (!delayed_work_pending(&optimizing_work))
> -		schedule_delayed_work(&optimizing_work, OPTIMIZE_DELAY);
> +	schedule_delayed_work(&optimizing_work, OPTIMIZE_DELAY);
>  }
>  
>  /* Kprobe jump optimizer */
> @@ -592,16 +590,25 @@ static __kprobes void kprobe_optimizer(struct work_struct *work)
>  	/* Step 5: Kick optimizer again if needed */
>  	if (!list_empty(&optimizing_list) || !list_empty(&unoptimizing_list))
>  		kick_kprobe_optimizer();
> -	else
> -		/* Wake up all waiters */
> -		complete_all(&optimizer_comp);
>  }
>  
>  /* Wait for completing optimization and unoptimization */
>  static __kprobes void wait_for_kprobe_optimizer(void)
>  {
> -	if (delayed_work_pending(&optimizing_work))
> -		wait_for_completion(&optimizer_comp);
> +	mutex_lock(&kprobe_mutex);
> +
> +	while (!list_empty(&optimizing_list) || !list_empty(&unoptimizing_list)) {
> +		mutex_unlock(&kprobe_mutex);
> +
> +		/* this will also make optimizing_work execute immmediately */
> +		flush_delayed_work(&optimizing_work);
> +		/* @optimizing_work might not have been queued yet, relax */
> +		cpu_relax();
> +
> +		mutex_lock(&kprobe_mutex);
> +	}
> +
> +	mutex_unlock(&kprobe_mutex);
>  }
>  
>  /* Optimize kprobe if p is ready to be optimized */
> 


-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: [PATCH 10/25] kprobes: fix wait_for_kprobe_optimizer()

[Tejun Heo]

On Tue, Dec 25, 2012 at 12:51:37PM +0900, Masami Hiramatsu wrote:
> (2012/12/22 10:57), Tejun Heo wrote:
> > wait_for_kprobe_optimizer() seems largely broken.  It uses
> > optimizer_comp which is never re-initialized, so
> > wait_for_kprobe_optimizer() will never wait for anything once
> > kprobe_optimizer() finishes all pending jobs for the first time.
> 
> Thank you for fixing that!
> I must misunderstand that the DECLARE_COMPLETION() macro.
> 
> > Also, aside from completion, delayed_work_pending() is %false once
> > kprobe_optimizer() starts execution and wait_for_kprobe_optimizer()
> > won't wait for it.
> >
> > Reimplement it so that it flushes optimizing_work until
> > [un]optimizing_lists are empty.  Note that this also makes
> > optimizing_work execute immediately if someone's waiting for it, which
> > is the nicer behavior.
> 
> I think your enhancement is reasonable and GOOD for me.
> 
> Thanks again!
> 
> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>

Can I take it through workqueue branch w/ other patches?

Thanks!

-- 
tejun

Re: Re: [PATCH 10/25] kprobes: fix wait_for_kprobe_optimizer()

[Masami Hiramatsu]

(2013/01/29 4:49), Tejun Heo wrote:
> On Tue, Dec 25, 2012 at 12:51:37PM +0900, Masami Hiramatsu wrote:
>> (2012/12/22 10:57), Tejun Heo wrote:
>>> wait_for_kprobe_optimizer() seems largely broken.  It uses
>>> optimizer_comp which is never re-initialized, so
>>> wait_for_kprobe_optimizer() will never wait for anything once
>>> kprobe_optimizer() finishes all pending jobs for the first time.
>>
>> Thank you for fixing that!
>> I must misunderstand that the DECLARE_COMPLETION() macro.
>>
>>> Also, aside from completion, delayed_work_pending() is %false once
>>> kprobe_optimizer() starts execution and wait_for_kprobe_optimizer()
>>> won't wait for it.
>>>
>>> Reimplement it so that it flushes optimizing_work until
>>> [un]optimizing_lists are empty.  Note that this also makes
>>> optimizing_work execute immediately if someone's waiting for it, which
>>> is the nicer behavior.
>>
>> I think your enhancement is reasonable and GOOD for me.
>>
>> Thanks again!
>>
>> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> 
> Can I take it through workqueue branch w/ other patches?

Yes, of course. I think it is not a critical bug, so I can
wait for other patches.

Thank you,

-- 
Masami HIRAMATSU
IT Management Research Dept. Linux Technology Center
Hitachi, Ltd., Yokohama Research Laboratory
E-mail: masami.hiramatsu.pt@hitachi.com

Re: Re: [PATCH 10/25] kprobes: fix wait_for_kprobe_optimizer()

[Tejun Heo]

On Tue, Jan 29, 2013 at 08:53:18PM +0900, Masami Hiramatsu wrote:
> >> Acked-by: Masami Hiramatsu <masami.hiramatsu.pt@hitachi.com>
> > 
> > Can I take it through workqueue branch w/ other patches?
> 
> Yes, of course. I think it is not a critical bug, so I can
> wait for other patches.

Applied to wq/for-3.9-cleanups.

Thanks!

-- 
tejun

[PATCH 11/25] pm: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from pm autosleep and qos.  Only
compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
Cc: linux-pm@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 kernel/power/autosleep.c | 2 +-
 kernel/power/qos.c       | 9 +++------
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
index ca304046..c6422ff 100644
--- a/kernel/power/autosleep.c
+++ b/kernel/power/autosleep.c
@@ -66,7 +66,7 @@ static DECLARE_WORK(suspend_work, try_to_suspend);
 
 void queue_up_suspend_work(void)
 {
-	if (!work_pending(&suspend_work) && autosleep_state > PM_SUSPEND_ON)
+	if (autosleep_state > PM_SUSPEND_ON)
 		queue_work(autosleep_wq, &suspend_work);
 }
 
diff --git a/kernel/power/qos.c b/kernel/power/qos.c
index 9322ff7..587ddde 100644
--- a/kernel/power/qos.c
+++ b/kernel/power/qos.c
@@ -359,8 +359,7 @@ void pm_qos_update_request(struct pm_qos_request *req,
 		return;
 	}
 
-	if (delayed_work_pending(&req->work))
-		cancel_delayed_work_sync(&req->work);
+	cancel_delayed_work_sync(&req->work);
 
 	if (new_value != req->node.prio)
 		pm_qos_update_target(
@@ -386,8 +385,7 @@ void pm_qos_update_request_timeout(struct pm_qos_request *req, s32 new_value,
 		 "%s called for unknown object.", __func__))
 		return;
 
-	if (delayed_work_pending(&req->work))
-		cancel_delayed_work_sync(&req->work);
+	cancel_delayed_work_sync(&req->work);
 
 	if (new_value != req->node.prio)
 		pm_qos_update_target(
@@ -416,8 +414,7 @@ void pm_qos_remove_request(struct pm_qos_request *req)
 		return;
 	}
 
-	if (delayed_work_pending(&req->work))
-		cancel_delayed_work_sync(&req->work);
+	cancel_delayed_work_sync(&req->work);
 
 	pm_qos_update_target(pm_qos_array[req->pm_qos_class]->constraints,
 			     &req->node, PM_QOS_REMOVE_REQ,
-- 
1.8.0.2

Re: [PATCH 11/25] pm: don't use [delayed_]work_pending()

[Rafael J. Wysocki]

On Friday, December 21, 2012 05:57:01 PM Tejun Heo wrote:
> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.

Can you please say why they are buggy?

> Remove unnecessary pending tests from pm autosleep and qos.  Only
> compile tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: "Rafael J. Wysocki" <rjw@sisk.pl>
> Cc: linux-pm@vger.kernel.org
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.

I can take it.  I will send a pull request with fixes later in the cycle
(maybe even before -rc2).

Thanks,
Rafael


>  kernel/power/autosleep.c | 2 +-
>  kernel/power/qos.c       | 9 +++------
>  2 files changed, 4 insertions(+), 7 deletions(-)
> 
> diff --git a/kernel/power/autosleep.c b/kernel/power/autosleep.c
> index ca304046..c6422ff 100644
> --- a/kernel/power/autosleep.c
> +++ b/kernel/power/autosleep.c
> @@ -66,7 +66,7 @@ static DECLARE_WORK(suspend_work, try_to_suspend);
>  
>  void queue_up_suspend_work(void)
>  {
> -	if (!work_pending(&suspend_work) && autosleep_state > PM_SUSPEND_ON)
> +	if (autosleep_state > PM_SUSPEND_ON)
>  		queue_work(autosleep_wq, &suspend_work);
>  }
>  
> diff --git a/kernel/power/qos.c b/kernel/power/qos.c
> index 9322ff7..587ddde 100644
> --- a/kernel/power/qos.c
> +++ b/kernel/power/qos.c
> @@ -359,8 +359,7 @@ void pm_qos_update_request(struct pm_qos_request *req,
>  		return;
>  	}
>  
> -	if (delayed_work_pending(&req->work))
> -		cancel_delayed_work_sync(&req->work);
> +	cancel_delayed_work_sync(&req->work);
>  
>  	if (new_value != req->node.prio)
>  		pm_qos_update_target(
> @@ -386,8 +385,7 @@ void pm_qos_update_request_timeout(struct pm_qos_request *req, s32 new_value,
>  		 "%s called for unknown object.", __func__))
>  		return;
>  
> -	if (delayed_work_pending(&req->work))
> -		cancel_delayed_work_sync(&req->work);
> +	cancel_delayed_work_sync(&req->work);
>  
>  	if (new_value != req->node.prio)
>  		pm_qos_update_target(
> @@ -416,8 +414,7 @@ void pm_qos_remove_request(struct pm_qos_request *req)
>  		return;
>  	}
>  
> -	if (delayed_work_pending(&req->work))
> -		cancel_delayed_work_sync(&req->work);
> +	cancel_delayed_work_sync(&req->work);
>  
>  	pm_qos_update_target(pm_qos_array[req->pm_qos_class]->constraints,
>  			     &req->node, PM_QOS_REMOVE_REQ,
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 11/25] pm: don't use [delayed_]work_pending()

[Tejun Heo]

Hello, Rafael.

On Sat, Dec 22, 2012 at 12:53:29PM +0100, Rafael J. Wysocki wrote:
> On Friday, December 21, 2012 05:57:01 PM Tejun Heo wrote:
> > There's no need to test whether a (delayed) work item in pending
> > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > and quite a few of them are buggy.
> 
> Can you please say why they are buggy?

Usually one of the following two reasons.

* The user gets confused and fails to handle !PENDING && currently
  executing properly.

* work_pending() doesn't have any memory barrier and the caller
  assumes work_pending() is somehow properly synchronized by itself.

Thanks.

-- 
tejun

[PATCH 12/25] bluetooth/l2cap: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Reimplement l2cap_set_timer() such that it uses mod_delayed_work() or
schedule_delayed_work() depending on a new param @override and let the
users specify whether to override or not instead of using
delayed_work_pending().

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: Gustavo Padovan <gustavo@padovan.org>
Cc: Johan Hedberg <johan.hedberg@gmail.com>
Cc: linux-bluetooth@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 include/net/bluetooth/l2cap.h | 24 ++++++++++++++++--------
 net/bluetooth/l2cap_core.c    |  7 +++----
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
index 7588ef4..f12cbeb 100644
--- a/include/net/bluetooth/l2cap.h
+++ b/include/net/bluetooth/l2cap.h
@@ -718,17 +718,25 @@ static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
 }
 
 static inline void l2cap_set_timer(struct l2cap_chan *chan,
-				   struct delayed_work *work, long timeout)
+				   struct delayed_work *work, long timeout,
+				   bool override)
 {
+	bool was_pending;
+
 	BT_DBG("chan %p state %s timeout %ld", chan,
 	       state_to_string(chan->state), timeout);
 
-	/* If delayed work cancelled do not hold(chan)
-	   since it is already done with previous set_timer */
-	if (!cancel_delayed_work(work))
-		l2cap_chan_hold(chan);
+	/* @work should hold a reference to @chan */
+	l2cap_chan_hold(chan);
+
+	if (override)
+		was_pending = mod_delayed_work(system_wq, work, timeout);
+	else
+		was_pending = !schedule_delayed_work(work, timeout);
 
-	schedule_delayed_work(work, timeout);
+	/* if @work was already pending, lose the extra ref */
+	if (was_pending)
+		l2cap_chan_put(chan);
 }
 
 static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
@@ -745,12 +753,12 @@ static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
 	return ret;
 }
 
-#define __set_chan_timer(c, t) l2cap_set_timer(c, &c->chan_timer, (t))
+#define __set_chan_timer(c, t) l2cap_set_timer(c, &c->chan_timer, (t), true)
 #define __clear_chan_timer(c) l2cap_clear_timer(c, &c->chan_timer)
 #define __clear_retrans_timer(c) l2cap_clear_timer(c, &c->retrans_timer)
 #define __clear_monitor_timer(c) l2cap_clear_timer(c, &c->monitor_timer)
 #define __set_ack_timer(c) l2cap_set_timer(c, &chan->ack_timer, \
-		msecs_to_jiffies(L2CAP_DEFAULT_ACK_TO));
+		msecs_to_jiffies(L2CAP_DEFAULT_ACK_TO), true);
 #define __clear_ack_timer(c) l2cap_clear_timer(c, &c->ack_timer)
 
 static inline int __seq_offset(struct l2cap_chan *chan, __u16 seq1, __u16 seq2)
diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
index 2c78208..91db91c 100644
--- a/net/bluetooth/l2cap_core.c
+++ b/net/bluetooth/l2cap_core.c
@@ -246,10 +246,9 @@ static inline void l2cap_chan_set_err(struct l2cap_chan *chan, int err)
 
 static void __set_retrans_timer(struct l2cap_chan *chan)
 {
-	if (!delayed_work_pending(&chan->monitor_timer) &&
-	    chan->retrans_timeout) {
+	if (chan->retrans_timeout) {
 		l2cap_set_timer(chan, &chan->retrans_timer,
-				msecs_to_jiffies(chan->retrans_timeout));
+				msecs_to_jiffies(chan->retrans_timeout), false);
 	}
 }
 
@@ -258,7 +257,7 @@ static void __set_monitor_timer(struct l2cap_chan *chan)
 	__clear_retrans_timer(chan);
 	if (chan->monitor_timeout) {
 		l2cap_set_timer(chan, &chan->monitor_timer,
-				msecs_to_jiffies(chan->monitor_timeout));
+				msecs_to_jiffies(chan->monitor_timeout), true);
 	}
 }
 
-- 
1.8.0.2

Re: [PATCH 12/25] bluetooth/l2cap: don't use [delayed_]work_pending()

[Gustavo Padovan]

Hi Tejun,

* Tejun Heo <tj@kernel.org> [2012-12-21 17:57:02 -0800]:

> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
> 
> Reimplement l2cap_set_timer() such that it uses mod_delayed_work() or
> schedule_delayed_work() depending on a new param @override and let the
> users specify whether to override or not instead of using
> delayed_work_pending().
> 
> Only compile tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Marcel Holtmann <marcel@holtmann.org>
> Cc: Gustavo Padovan <gustavo@padovan.org>
> Cc: Johan Hedberg <johan.hedberg@gmail.com>
> Cc: linux-bluetooth@vger.kernel.org
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.
> 
> Thanks.
> 
>  include/net/bluetooth/l2cap.h | 24 ++++++++++++++++--------
>  net/bluetooth/l2cap_core.c    |  7 +++----
>  2 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/include/net/bluetooth/l2cap.h b/include/net/bluetooth/l2cap.h
> index 7588ef4..f12cbeb 100644
> --- a/include/net/bluetooth/l2cap.h
> +++ b/include/net/bluetooth/l2cap.h
> @@ -718,17 +718,25 @@ static inline void l2cap_chan_unlock(struct l2cap_chan *chan)
>  }
>  
>  static inline void l2cap_set_timer(struct l2cap_chan *chan,
> -				   struct delayed_work *work, long timeout)
> +				   struct delayed_work *work, long timeout,
> +				   bool override)
>  {
> +	bool was_pending;
> +
>  	BT_DBG("chan %p state %s timeout %ld", chan,
>  	       state_to_string(chan->state), timeout);
>  
> -	/* If delayed work cancelled do not hold(chan)
> -	   since it is already done with previous set_timer */
> -	if (!cancel_delayed_work(work))
> -		l2cap_chan_hold(chan);
> +	/* @work should hold a reference to @chan */
> +	l2cap_chan_hold(chan);
> +
> +	if (override)
> +		was_pending = mod_delayed_work(system_wq, work, timeout);
> +	else
> +		was_pending = !schedule_delayed_work(work, timeout);
>  
> -	schedule_delayed_work(work, timeout);
> +	/* if @work was already pending, lose the extra ref */
> +	if (was_pending)
> +		l2cap_chan_put(chan);
>  }
>  
>  static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
> @@ -745,12 +753,12 @@ static inline bool l2cap_clear_timer(struct l2cap_chan *chan,
>  	return ret;
>  }
>  
> -#define __set_chan_timer(c, t) l2cap_set_timer(c, &c->chan_timer, (t))
> +#define __set_chan_timer(c, t) l2cap_set_timer(c, &c->chan_timer, (t), true)
>  #define __clear_chan_timer(c) l2cap_clear_timer(c, &c->chan_timer)
>  #define __clear_retrans_timer(c) l2cap_clear_timer(c, &c->retrans_timer)
>  #define __clear_monitor_timer(c) l2cap_clear_timer(c, &c->monitor_timer)
>  #define __set_ack_timer(c) l2cap_set_timer(c, &chan->ack_timer, \
> -		msecs_to_jiffies(L2CAP_DEFAULT_ACK_TO));
> +		msecs_to_jiffies(L2CAP_DEFAULT_ACK_TO), true);
>  #define __clear_ack_timer(c) l2cap_clear_timer(c, &c->ack_timer)
>  
>  static inline int __seq_offset(struct l2cap_chan *chan, __u16 seq1, __u16 seq2)
> diff --git a/net/bluetooth/l2cap_core.c b/net/bluetooth/l2cap_core.c
> index 2c78208..91db91c 100644
> --- a/net/bluetooth/l2cap_core.c
> +++ b/net/bluetooth/l2cap_core.c
> @@ -246,10 +246,9 @@ static inline void l2cap_chan_set_err(struct l2cap_chan *chan, int err)
>  
>  static void __set_retrans_timer(struct l2cap_chan *chan)
>  {
> -	if (!delayed_work_pending(&chan->monitor_timer) &&
> -	    chan->retrans_timeout) {
> +	if (chan->retrans_timeout) {
>  		l2cap_set_timer(chan, &chan->retrans_timer,
> -				msecs_to_jiffies(chan->retrans_timeout));
> +				msecs_to_jiffies(chan->retrans_timeout), false);

Did you notice we are talking about two different works here?
delayed_work_pending() is checking the monitor_timer while l2cap_set_timer is
setting the retrans_timer. We can't run one of them while the is running.
This mean you can just get rid of the override flag and simplify this patch a
lot.

	Gustavo

[PATCH 13/25] sound/wm8350: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from wm8350.  Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: patches@opensource.wolfsonmicro.com
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 sound/soc/codecs/wm8350.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/sound/soc/codecs/wm8350.c b/sound/soc/codecs/wm8350.c
index fb92fb4..ec0efc1 100644
--- a/sound/soc/codecs/wm8350.c
+++ b/sound/soc/codecs/wm8350.c
@@ -283,18 +283,16 @@ static int pga_event(struct snd_soc_dapm_widget *w,
 		out->ramp = WM8350_RAMP_UP;
 		out->active = 1;
 
-		if (!delayed_work_pending(&codec->dapm.delayed_work))
-			schedule_delayed_work(&codec->dapm.delayed_work,
-					      msecs_to_jiffies(1));
+		schedule_delayed_work(&codec->dapm.delayed_work,
+				      msecs_to_jiffies(1));
 		break;
 
 	case SND_SOC_DAPM_PRE_PMD:
 		out->ramp = WM8350_RAMP_DOWN;
 		out->active = 0;
 
-		if (!delayed_work_pending(&codec->dapm.delayed_work))
-			schedule_delayed_work(&codec->dapm.delayed_work,
-					      msecs_to_jiffies(1));
+		schedule_delayed_work(&codec->dapm.delayed_work,
+				      msecs_to_jiffies(1));
 		break;
 	}
 
-- 
1.8.0.2

Re: [PATCH 13/25] sound/wm8350: don't use [delayed_]work_pending()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 254 bytes --]

On Fri, Dec 21, 2012 at 05:57:03PM -0800, Tejun Heo wrote:
> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.

Applied, thanks.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

[PATCH 14/25] rfkill: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from rfkill.  Only compile
tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: "John W. Linville" <linville@tuxdriver.com>
Cc: linux-wireless@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 net/rfkill/input.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/net/rfkill/input.c b/net/rfkill/input.c
index c9d931e..b85107b 100644
--- a/net/rfkill/input.c
+++ b/net/rfkill/input.c
@@ -148,11 +148,9 @@ static unsigned long rfkill_ratelimit(const unsigned long last)
 
 static void rfkill_schedule_ratelimited(void)
 {
-	if (delayed_work_pending(&rfkill_op_work))
-		return;
-	schedule_delayed_work(&rfkill_op_work,
-			      rfkill_ratelimit(rfkill_last_scheduled));
-	rfkill_last_scheduled = jiffies;
+	if (schedule_delayed_work(&rfkill_op_work,
+				  rfkill_ratelimit(rfkill_last_scheduled)))
+		rfkill_last_scheduled = jiffies;
 }
 
 static void rfkill_schedule_global_op(enum rfkill_sched_op op)
-- 
1.8.0.2

Re: [PATCH 14/25] rfkill: don't use [delayed_]work_pending()

[Johannes Berg]

On Fri, 2012-12-21 at 17:57 -0800, Tejun Heo wrote:
> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
> 
> Remove unnecessary pending tests from rfkill.  Only compile
> tested.

Looks fine to me, feel free to route through your tree -- nobody changes
rfkill much (famous last words...) :-)

johannes

Re: [PATCH 14/25] rfkill: don't use [delayed_]work_pending()

[Tejun Heo]

On Sat, Dec 22, 2012 at 09:22:13PM +0100, Johannes Berg wrote:
> On Fri, 2012-12-21 at 17:57 -0800, Tejun Heo wrote:
> > There's no need to test whether a (delayed) work item in pending
> > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > and quite a few of them are buggy.
> > 
> > Remove unnecessary pending tests from rfkill.  Only compile
> > tested.
> 
> Looks fine to me, feel free to route through your tree -- nobody changes
> rfkill much (famous last words...) :-)

Applied to wq/for-3.9-cleanups w/ your Acked-by added.  Thanks.

-- 
tejun

[PATCH 15/25] x86/mce: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from x86/mce.  Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Tony Luck <tony.luck@intel.com>
Cc: Borislav Petkov <bp@alien8.de>
Cc: linux-edac@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 arch/x86/kernel/cpu/mcheck/mce.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
index 80dbda8..c06a736 100644
--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -514,8 +514,7 @@ static void mce_schedule_work(void)
 {
 	if (!mce_ring_empty()) {
 		struct work_struct *work = &__get_cpu_var(mce_work);
-		if (!work_pending(work))
-			schedule_work(work);
+		schedule_work(work);
 	}
 }
 
@@ -1351,12 +1350,7 @@ int mce_notify_irq(void)
 		/* wake processes polling /dev/mcelog */
 		wake_up_interruptible(&mce_chrdev_wait);
 
-		/*
-		 * There is no risk of missing notifications because
-		 * work_pending is always cleared before the function is
-		 * executed.
-		 */
-		if (mce_helper[0] && !work_pending(&mce_trigger_work))
+		if (mce_helper[0])
 			schedule_work(&mce_trigger_work);
 
 		if (__ratelimit(&ratelimit))
-- 
1.8.0.2

Re: [PATCH 15/25] x86/mce: don't use [delayed_]work_pending()

[Borislav Petkov]

On Fri, Dec 21, 2012 at 05:57:05PM -0800, Tejun Heo wrote:
> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
> 
> Remove unnecessary pending tests from x86/mce.  Only compile tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Tony Luck <tony.luck@intel.com>
> Cc: Borislav Petkov <bp@alien8.de>
> Cc: linux-edac@vger.kernel.org
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.
> 
> Thanks.
> 
>  arch/x86/kernel/cpu/mcheck/mce.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/arch/x86/kernel/cpu/mcheck/mce.c b/arch/x86/kernel/cpu/mcheck/mce.c
> index 80dbda8..c06a736 100644
> --- a/arch/x86/kernel/cpu/mcheck/mce.c
> +++ b/arch/x86/kernel/cpu/mcheck/mce.c
> @@ -514,8 +514,7 @@ static void mce_schedule_work(void)
>  {
>  	if (!mce_ring_empty()) {
>  		struct work_struct *work = &__get_cpu_var(mce_work);
> -		if (!work_pending(work))
> -			schedule_work(work);
> +		schedule_work(work);

Right, just a nit. You could remove the local variable and have the
function simply be:

static void mce_schedule_work(void)
{
        if (!mce_ring_empty())
                schedule_work(&__get_cpu_var(mce_work));
}

before you send it upstream.

Also, the workqueue stuff is used on Intel boxes to poke mcelog when
there's an MCE to be reported to userspace so if Tony wants to give it a
run...

I'm fine with this being routed through your tree:

Acked-by: Borislav Petkov <bp@alien8.de>

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

[PATCH v2 15/25] x86/mce: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from x86/mce.  Only compile tested.

v2: Local var work removed from mce_schedule_work() as suggested by
    Borislav.

Signed-off-by: Tejun Heo <tj@kernel.org>
Acked-by: Borislav Petkov <bp@alien8.de>
Cc: Tony Luck <tony.luck@intel.com>
Cc: linux-edac@vger.kernel.org
---
Applied to wq/for-3.9-cleanups.  Thanks.

Thanks.

 arch/x86/kernel/cpu/mcheck/mce.c |   14 +++-----------
 1 file changed, 3 insertions(+), 11 deletions(-)

--- a/arch/x86/kernel/cpu/mcheck/mce.c
+++ b/arch/x86/kernel/cpu/mcheck/mce.c
@@ -512,11 +512,8 @@ int mce_available(struct cpuinfo_x86 *c)
 
 static void mce_schedule_work(void)
 {
-	if (!mce_ring_empty()) {
-		struct work_struct *work = &__get_cpu_var(mce_work);
-		if (!work_pending(work))
-			schedule_work(work);
-	}
+	if (!mce_ring_empty())
+		schedule_work(&__get_cpu_var(mce_work));
 }
 
 DEFINE_PER_CPU(struct irq_work, mce_irq_work);
@@ -1351,12 +1348,7 @@ int mce_notify_irq(void)
 		/* wake processes polling /dev/mcelog */
 		wake_up_interruptible(&mce_chrdev_wait);
 
-		/*
-		 * There is no risk of missing notifications because
-		 * work_pending is always cleared before the function is
-		 * executed.
-		 */
-		if (mce_helper[0] && !work_pending(&mce_trigger_work))
+		if (mce_helper[0])
 			schedule_work(&mce_trigger_work);
 
 		if (__ratelimit(&ratelimit))

[PATCH 16/25] PM / Domains: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from power domains.  Only compile
tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Cc: linux-pm@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/base/power/domain.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
index acc3a8d..9a6b05a 100644
--- a/drivers/base/power/domain.c
+++ b/drivers/base/power/domain.c
@@ -433,8 +433,7 @@ static bool genpd_abort_poweroff(struct generic_pm_domain *genpd)
  */
 void genpd_queue_power_off_work(struct generic_pm_domain *genpd)
 {
-	if (!work_pending(&genpd->power_off_work))
-		queue_work(pm_wq, &genpd->power_off_work);
+	queue_work(pm_wq, &genpd->power_off_work);
 }
 
 /**
-- 
1.8.0.2

Re: [PATCH 16/25] PM / Domains: don't use [delayed_]work_pending()

[Rafael J. Wysocki]

On Friday, December 21, 2012 05:57:06 PM Tejun Heo wrote:
> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.

Is the particular one you're removing from domain.c buggy?

> Remove unnecessary pending tests from power domains.  Only compile
> tested.

I can take this one too.

Thanks,
Rafael


> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Cc: linux-pm@vger.kernel.org
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.
> 
> Thanks.
> 
>  drivers/base/power/domain.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/base/power/domain.c b/drivers/base/power/domain.c
> index acc3a8d..9a6b05a 100644
> --- a/drivers/base/power/domain.c
> +++ b/drivers/base/power/domain.c
> @@ -433,8 +433,7 @@ static bool genpd_abort_poweroff(struct generic_pm_domain *genpd)
>   */
>  void genpd_queue_power_off_work(struct generic_pm_domain *genpd)
>  {
> -	if (!work_pending(&genpd->power_off_work))
> -		queue_work(pm_wq, &genpd->power_off_work);
> +	queue_work(pm_wq, &genpd->power_off_work);
>  }
>  
>  /**
> 
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 16/25] PM / Domains: don't use [delayed_]work_pending()

[Tejun Heo]

Hello, Rafael.

On Sat, Dec 22, 2012 at 12:57:20PM +0100, Rafael J. Wysocki wrote:
> On Friday, December 21, 2012 05:57:06 PM Tejun Heo wrote:
> > There's no need to test whether a (delayed) work item in pending
> > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > and quite a few of them are buggy.
> 
> Is the particular one you're removing from domain.c buggy?

It's a bit difficult to tell without understanding the code base but
from quick glancing it looks like it could be.  The queueing and
actual excution don't grab the same lock, so there doesn't seem to be
anything work_pending() returning %true for a work item which already
started executing.  Even if the bug is there, it's likely to be very
difficult to trigger tho, so I wouldn't consider it an urgent fix.

Thanks.

-- 
tejun

Re: [PATCH 16/25] PM / Domains: don't use [delayed_]work_pending()

[Rafael J. Wysocki]

On Tuesday, December 25, 2012 09:03:28 AM Tejun Heo wrote:
> Hello, Rafael.
> 
> On Sat, Dec 22, 2012 at 12:57:20PM +0100, Rafael J. Wysocki wrote:
> > On Friday, December 21, 2012 05:57:06 PM Tejun Heo wrote:
> > > There's no need to test whether a (delayed) work item in pending
> > > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > > and quite a few of them are buggy.
> > 
> > Is the particular one you're removing from domain.c buggy?
> 
> It's a bit difficult to tell without understanding the code base but
> from quick glancing it looks like it could be.  The queueing and
> actual excution don't grab the same lock, so there doesn't seem to be
> anything work_pending() returning %true for a work item which already
> started executing.  Even if the bug is there, it's likely to be very
> difficult to trigger tho, so I wouldn't consider it an urgent fix.

OK, so I'd generally prefer changelogs like this:

"There's no need to test whether a (delayed) work item is pending
before queueing, flushing or cancelling it, so remove work_pending()
tests used in those cases."

If that's fine with you, I'll queue up [16/25] and [11/25] for v3.9
with the above as the changelog.

Thanks,
Rafael


-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH 16/25] PM / Domains: don't use [delayed_]work_pending()

[Tejun Heo]

Hello,

On Tue, Dec 25, 2012 at 09:33:07PM +0100, Rafael J. Wysocki wrote:
> OK, so I'd generally prefer changelogs like this:
> 
> "There's no need to test whether a (delayed) work item is pending
> before queueing, flushing or cancelling it, so remove work_pending()
> tests used in those cases."
> 
> If that's fine with you, I'll queue up [16/25] and [11/25] for v3.9
> with the above as the changelog.

Sure thing.  Please go ahead.

Thanks.

-- 
tejun

[PATCH 17/25] wm97xx: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from wm97xx.  Instead of testing
work_pending(), use the return value of queue_work() to decide whether
to disable IRQ or not.

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
Cc: Liam Girdwood <lrg@slimlogic.co.uk>
Cc: linux-input@vger.kernel.org
Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/input/touchscreen/wm97xx-core.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c
index 5dbe73a..fd16c63 100644
--- a/drivers/input/touchscreen/wm97xx-core.c
+++ b/drivers/input/touchscreen/wm97xx-core.c
@@ -363,10 +363,8 @@ static irqreturn_t wm97xx_pen_interrupt(int irq, void *dev_id)
 {
 	struct wm97xx *wm = dev_id;
 
-	if (!work_pending(&wm->pen_event_work)) {
+	if (queue_work(wm->ts_workq, &wm->pen_event_work))
 		wm->mach_ops->irq_enable(wm, 0);
-		queue_work(wm->ts_workq, &wm->pen_event_work);
-	}
 
 	return IRQ_HANDLED;
 }
-- 
1.8.0.2

Re: [PATCH 17/25] wm97xx: don't use [delayed_]work_pending()

[Dmitry Torokhov]

Hi Tejun,

On Fri, Dec 21, 2012 at 05:57:07PM -0800, Tejun Heo wrote:
> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
> 
> Remove unnecessary pending tests from wm97xx.  Instead of testing
> work_pending(), use the return value of queue_work() to decide whether
> to disable IRQ or not.
> 
> Only compile tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Mark Brown <broonie@opensource.wolfsonmicro.com>
> Cc: Liam Girdwood <lrg@slimlogic.co.uk>
> Cc: linux-input@vger.kernel.org
> Cc: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.
> 
> Thanks.
> 
>  drivers/input/touchscreen/wm97xx-core.c | 4 +---
>  1 file changed, 1 insertion(+), 3 deletions(-)
> 
> diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c
> index 5dbe73a..fd16c63 100644
> --- a/drivers/input/touchscreen/wm97xx-core.c
> +++ b/drivers/input/touchscreen/wm97xx-core.c
> @@ -363,10 +363,8 @@ static irqreturn_t wm97xx_pen_interrupt(int irq, void *dev_id)
>  {
>  	struct wm97xx *wm = dev_id;
>  
> -	if (!work_pending(&wm->pen_event_work)) {
> +	if (queue_work(wm->ts_workq, &wm->pen_event_work))
>  		wm->mach_ops->irq_enable(wm, 0);
> -		queue_work(wm->ts_workq, &wm->pen_event_work);
> -	}
>  

This is not 100% equivalent transformation as now we schedule first and
disable IRQ later... Anyway, I think the driver shoudl be converted to
threaded IRQ instead. Mark, does the patch below make any sense to you?

Thanks.

-- 
Dmitry


Input: wm97xx - switch to using threaded IRQ

Instead of manually disabling and enabling interrupts and scheduling work to
access the device, let's use threaded oneshot interrupt handler. It simplifies
things.

Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/touchscreen/wm97xx-core.c |   42 +++++--------------------------
 include/linux/wm97xx.h                  |    1 -
 2 files changed, 7 insertions(+), 36 deletions(-)

diff --git a/drivers/input/touchscreen/wm97xx-core.c b/drivers/input/touchscreen/wm97xx-core.c
index 5dbe73a..bf5eddf 100644
--- a/drivers/input/touchscreen/wm97xx-core.c
+++ b/drivers/input/touchscreen/wm97xx-core.c
@@ -289,11 +289,12 @@ void wm97xx_set_suspend_mode(struct wm97xx *wm, u16 mode)
 EXPORT_SYMBOL_GPL(wm97xx_set_suspend_mode);
 
 /*
- * Handle a pen down interrupt.
+ * Codec PENDOWN irq handler
+ *
  */
-static void wm97xx_pen_irq_worker(struct work_struct *work)
+static irqreturn_t wm97xx_pen_interrupt(int irq, void *dev_id)
 {
-	struct wm97xx *wm = container_of(work, struct wm97xx, pen_event_work);
+	struct wm97xx *wm = dev_id;
 	int pen_was_down = wm->pen_is_down;
 
 	/* do we need to enable the touch panel reader */
@@ -347,27 +348,6 @@ static void wm97xx_pen_irq_worker(struct work_struct *work)
 	if (!wm->pen_is_down && wm->mach_ops->acc_enabled)
 		wm->mach_ops->acc_pen_up(wm);
 
-	wm->mach_ops->irq_enable(wm, 1);
-}
-
-/*
- * Codec PENDOWN irq handler
- *
- * We have to disable the codec interrupt in the handler because it
- * can take up to 1ms to clear the interrupt source. We schedule a task
- * in a work queue to do the actual interaction with the chip.  The
- * interrupt is then enabled again in the slow handler when the source
- * has been cleared.
- */
-static irqreturn_t wm97xx_pen_interrupt(int irq, void *dev_id)
-{
-	struct wm97xx *wm = dev_id;
-
-	if (!work_pending(&wm->pen_event_work)) {
-		wm->mach_ops->irq_enable(wm, 0);
-		queue_work(wm->ts_workq, &wm->pen_event_work);
-	}
-
 	return IRQ_HANDLED;
 }
 
@@ -378,12 +358,9 @@ static int wm97xx_init_pen_irq(struct wm97xx *wm)
 {
 	u16 reg;
 
-	/* If an interrupt is supplied an IRQ enable operation must also be
-	 * provided. */
-	BUG_ON(!wm->mach_ops->irq_enable);
-
-	if (request_irq(wm->pen_irq, wm97xx_pen_interrupt, IRQF_SHARED,
-			"wm97xx-pen", wm)) {
+	if (request_threaded_irq(wm->pen_irq, NULL, wm97xx_pen_interrupt,
+				 IRQF_SHARED | IRQF_ONESHOT,
+				 "wm97xx-pen", wm)) {
 		dev_err(wm->dev,
 			"Failed to register pen down interrupt, polling");
 		wm->pen_irq = 0;
@@ -502,7 +479,6 @@ static int wm97xx_ts_input_open(struct input_dev *idev)
 	wm->codec->dig_enable(wm, 1);
 
 	INIT_DELAYED_WORK(&wm->ts_reader, wm97xx_ts_reader);
-	INIT_WORK(&wm->pen_event_work, wm97xx_pen_irq_worker);
 
 	wm->ts_reader_min_interval = HZ >= 100 ? HZ / 100 : 1;
 	if (wm->ts_reader_min_interval < 1)
@@ -553,10 +529,6 @@ static void wm97xx_ts_input_close(struct input_dev *idev)
 
 	wm->pen_is_down = 0;
 
-	/* Balance out interrupt disables/enables */
-	if (cancel_work_sync(&wm->pen_event_work))
-		wm->mach_ops->irq_enable(wm, 1);
-
 	/* ts_reader rearms itself so we need to explicitly stop it
 	 * before we destroy the workqueue.
 	 */
diff --git a/include/linux/wm97xx.h b/include/linux/wm97xx.h
index fd98bb9..12f793d 100644
--- a/include/linux/wm97xx.h
+++ b/include/linux/wm97xx.h
@@ -280,7 +280,6 @@ struct wm97xx {
 	unsigned long ts_reader_min_interval; /* Minimum interval */
 	unsigned int pen_irq;		/* Pen IRQ number in use */
 	struct workqueue_struct *ts_workq;
-	struct work_struct pen_event_work;
 	u16 acc_slot;			/* AC97 slot used for acc touch data */
 	u16 acc_rate;			/* acc touch data rate */
 	unsigned pen_is_down:1;		/* Pen is down */

Re: [PATCH 17/25] wm97xx: don't use [delayed_]work_pending()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 610 bytes --]

On Sun, Dec 23, 2012 at 01:54:50AM -0800, Dmitry Torokhov wrote:

> This is not 100% equivalent transformation as now we schedule first and
> disable IRQ later... Anyway, I think the driver shoudl be converted to
> threaded IRQ instead. Mark, does the patch below make any sense to you?

I'm a bit nervous about the fact that currently both the pen down IRQ
and the coordinate read are pushed through a single workqueue so are
serialised but after your patch they'll be split into the IRQ thread and
the workqueue.  It *should* be fine but I'd have to sit there and study
it to convince myself that it's safe.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 17/25] wm97xx: don't use [delayed_]work_pending()

[Dmitry Torokhov]

On Mon, Dec 24, 2012 at 04:18:27PM +0000, Mark Brown wrote:
> On Sun, Dec 23, 2012 at 01:54:50AM -0800, Dmitry Torokhov wrote:
> 
> > This is not 100% equivalent transformation as now we schedule first and
> > disable IRQ later... Anyway, I think the driver shoudl be converted to
> > threaded IRQ instead. Mark, does the patch below make any sense to you?
> 
> I'm a bit nervous about the fact that currently both the pen down IRQ
> and the coordinate read are pushed through a single workqueue so are
> serialised but after your patch they'll be split into the IRQ thread and
> the workqueue.  It *should* be fine but I'd have to sit there and study
> it to convince myself that it's safe.

Mark,

Did yo have a chance to review the patch?

Thanks!

-- 
Dmitry

Re: [PATCH 17/25] wm97xx: don't use [delayed_]work_pending()

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 655 bytes --]

On Sat, Mar 09, 2013 at 03:53:36PM -0800, Dmitry Torokhov wrote:
> On Mon, Dec 24, 2012 at 04:18:27PM +0000, Mark Brown wrote:

> > I'm a bit nervous about the fact that currently both the pen down IRQ
> > and the coordinate read are pushed through a single workqueue so are
> > serialised but after your patch they'll be split into the IRQ thread and
> > the workqueue.  It *should* be fine but I'd have to sit there and study
> > it to convince myself that it's safe.

> Mark,

> Did yo have a chance to review the patch?

> Thanks!

Sort of.  I'd be much happier keeping them serialised, it's too much
work on legacy code to convince myself otherwise.

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH 17/25] wm97xx: don't use [delayed_]work_pending()

[Tejun Heo]

Hello, Dmitry.

On Sun, Dec 23, 2012 at 01:54:50AM -0800, Dmitry Torokhov wrote:
> This is not 100% equivalent transformation as now we schedule first and
> disable IRQ later... Anyway, I think the driver shoudl be converted to
> threaded IRQ instead. Mark, does the patch below make any sense to you?

Yeah, I think the conversion is actually broken.  There isn't anything
which prevents work item execution racing against irq disabling.  I
agree the right thing to do is using threaded irq handler.

Thanks!

-- 
tejun

[PATCH 18/25] TMIO MMC: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from tmio mmc.  Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
Cc: Ian Molton <ian@mnementh.co.uk>
Cc: linux-mmc@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/mmc/host/tmio_mmc_pio.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
index 50bf495..f4f18b3 100644
--- a/drivers/mmc/host/tmio_mmc_pio.c
+++ b/drivers/mmc/host/tmio_mmc_pio.c
@@ -573,8 +573,7 @@ static bool __tmio_mmc_card_detect_irq(struct tmio_mmc_host *host,
 		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |
 			TMIO_STAT_CARD_REMOVE);
 		if ((((ireg & TMIO_STAT_CARD_REMOVE) && mmc->card) ||
-		     ((ireg & TMIO_STAT_CARD_INSERT) && !mmc->card)) &&
-		    !work_pending(&mmc->detect.work))
+		     ((ireg & TMIO_STAT_CARD_INSERT) && !mmc->card)))
 			mmc_detect_change(host->mmc, msecs_to_jiffies(100));
 		return true;
 	}
-- 
1.8.0.2

Re: [PATCH 18/25] TMIO MMC: don't use [delayed_]work_pending()

[Guennadi Liakhovetski]

Hi Tejun

On Fri, 21 Dec 2012, Tejun Heo wrote:

> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
> 
> Remove unnecessary pending tests from tmio mmc.  Only compile tested.

Thanks for the patch. It looks good to me. What happens below is upon mmc 
card detection IRQs, of which there are typically several due to pin 
debouncing, a delayed work is scheduled to verify a possibly new card 
state (plugged in or removed). So, the code below checks, that after the 
first of those interrupts the work is already scheduled and doesn't 
schedule it again. IIUC, this is indeed unneeded, since 
queue_delayed_work_on() only has any effect if the work isn't queued yet. 
Ah, there's a nitpick - you can remove one pair of parenthesis;-) 
Otherwise, I guess, this is an

Acked-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>

and Chris can take it (preferably, after the correction) via his queue. If 
you give me a bit more time I can also test this, but it really looks 
pretty obvious. Maybe Chris will prefer you to take this via your queue - 
that's up to you two to decide.

Thanks
Guennadi

> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> Cc: Ian Molton <ian@mnementh.co.uk>
> Cc: linux-mmc@vger.kernel.org
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.
> 
> Thanks.
> 
>  drivers/mmc/host/tmio_mmc_pio.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/mmc/host/tmio_mmc_pio.c b/drivers/mmc/host/tmio_mmc_pio.c
> index 50bf495..f4f18b3 100644
> --- a/drivers/mmc/host/tmio_mmc_pio.c
> +++ b/drivers/mmc/host/tmio_mmc_pio.c
> @@ -573,8 +573,7 @@ static bool __tmio_mmc_card_detect_irq(struct tmio_mmc_host *host,
>  		tmio_mmc_ack_mmc_irqs(host, TMIO_STAT_CARD_INSERT |
>  			TMIO_STAT_CARD_REMOVE);
>  		if ((((ireg & TMIO_STAT_CARD_REMOVE) && mmc->card) ||
> -		     ((ireg & TMIO_STAT_CARD_INSERT) && !mmc->card)) &&
> -		    !work_pending(&mmc->detect.work))
> +		     ((ireg & TMIO_STAT_CARD_INSERT) && !mmc->card)))
>  			mmc_detect_change(host->mmc, msecs_to_jiffies(100));
>  		return true;
>  	}
> -- 
> 1.8.0.2
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

[PATCH 19/25] net/caif: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from caif.  Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/net/caif/caif_shmcore.c | 15 +++++----------
 1 file changed, 5 insertions(+), 10 deletions(-)

diff --git a/drivers/net/caif/caif_shmcore.c b/drivers/net/caif/caif_shmcore.c
index bc497d7..16bd654 100644
--- a/drivers/net/caif/caif_shmcore.c
+++ b/drivers/net/caif/caif_shmcore.c
@@ -183,9 +183,7 @@ int caif_shmdrv_rx_cb(u32 mbx_msg, void *priv)
 		spin_unlock_irqrestore(&pshm_drv->lock, flags);
 
 		/* Schedule RX work queue. */
-		if (!work_pending(&pshm_drv->shm_rx_work))
-			queue_work(pshm_drv->pshm_rx_workqueue,
-						&pshm_drv->shm_rx_work);
+		queue_work(pshm_drv->pshm_rx_workqueue, &pshm_drv->shm_rx_work);
 	}
 
 	/* Check for emptied buffers. */
@@ -246,9 +244,8 @@ int caif_shmdrv_rx_cb(u32 mbx_msg, void *priv)
 
 
 			/* Schedule the work queue. if required */
-			if (!work_pending(&pshm_drv->shm_tx_work))
-				queue_work(pshm_drv->pshm_tx_workqueue,
-							&pshm_drv->shm_tx_work);
+			queue_work(pshm_drv->pshm_tx_workqueue,
+				   &pshm_drv->shm_tx_work);
 		} else
 			spin_unlock_irqrestore(&pshm_drv->lock, flags);
 	}
@@ -374,8 +371,7 @@ static void shm_rx_work_func(struct work_struct *rx_work)
 	}
 
 	/* Schedule the work queue. if required */
-	if (!work_pending(&pshm_drv->shm_tx_work))
-		queue_work(pshm_drv->pshm_tx_workqueue, &pshm_drv->shm_tx_work);
+	queue_work(pshm_drv->pshm_tx_workqueue, &pshm_drv->shm_tx_work);
 
 }
 
@@ -528,8 +524,7 @@ static int shm_netdev_tx(struct sk_buff *skb, struct net_device *shm_netdev)
 	skb_queue_tail(&pshm_drv->sk_qhead, skb);
 
 	/* Schedule Tx work queue. for deferred processing of skbs*/
-	if (!work_pending(&pshm_drv->shm_tx_work))
-		queue_work(pshm_drv->pshm_tx_workqueue, &pshm_drv->shm_tx_work);
+	queue_work(pshm_drv->pshm_tx_workqueue, &pshm_drv->shm_tx_work);
 
 	return 0;
 }
-- 
1.8.0.2

[PATCH 20/25] wimax/i2400m: fix i2400m->wake_tx_skb handling

[Tejun Heo]

i2400m_net_wake_tx() sets ->wake_tx_skb with the given skb if
->wake_tx_ws is not pending; however, i2400m_wake_tx_work() could have
just started execution and haven't fetched -><wake_tx_skb yet.  The
previous packet will be leaked.

Update ->wake_tx_skb handling.

* i2400m_net_wake_tx() now tests whether the previous ->wake_tx_skb
  has been consumed by ->wake_tx_ws instead of testing work_pending().

* i2400m_net_wake_stop() is simplified similarly.  It always puts
  ->wake_tx_skb if non-NULL.

* Spurious ->wake_tx_skb dereference outside critical section dropped
  from i2400m_wake_tx_work().

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
Cc: linux-wimax@intel.com
Cc: wimax@linuxwimax.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/net/wimax/i2400m/netdev.c | 31 +++++++++++++++++--------------
 1 file changed, 17 insertions(+), 14 deletions(-)

diff --git a/drivers/net/wimax/i2400m/netdev.c b/drivers/net/wimax/i2400m/netdev.c
index 1d76ae8..530581c 100644
--- a/drivers/net/wimax/i2400m/netdev.c
+++ b/drivers/net/wimax/i2400m/netdev.c
@@ -156,7 +156,7 @@ void i2400m_wake_tx_work(struct work_struct *ws)
 	struct i2400m *i2400m = container_of(ws, struct i2400m, wake_tx_ws);
 	struct net_device *net_dev = i2400m->wimax_dev.net_dev;
 	struct device *dev = i2400m_dev(i2400m);
-	struct sk_buff *skb = i2400m->wake_tx_skb;
+	struct sk_buff *skb;
 	unsigned long flags;
 
 	spin_lock_irqsave(&i2400m->tx_lock, flags);
@@ -236,23 +236,26 @@ void i2400m_tx_prep_header(struct sk_buff *skb)
 void i2400m_net_wake_stop(struct i2400m *i2400m)
 {
 	struct device *dev = i2400m_dev(i2400m);
+	struct sk_buff *wake_tx_skb;
+	unsigned long flags;
 
 	d_fnstart(3, dev, "(i2400m %p)\n", i2400m);
-	/* See i2400m_hard_start_xmit(), references are taken there
-	 * and here we release them if the work was still
-	 * pending. Note we can't differentiate work not pending vs
-	 * never scheduled, so the NULL check does that. */
-	if (cancel_work_sync(&i2400m->wake_tx_ws) == 0
-	    && i2400m->wake_tx_skb != NULL) {
-		unsigned long flags;
-		struct sk_buff *wake_tx_skb;
-		spin_lock_irqsave(&i2400m->tx_lock, flags);
-		wake_tx_skb = i2400m->wake_tx_skb;	/* compat help */
-		i2400m->wake_tx_skb = NULL;	/* compat help */
-		spin_unlock_irqrestore(&i2400m->tx_lock, flags);
+	/*
+	 * See i2400m_hard_start_xmit(), references are taken there and
+	 * here we release them if the packet was still pending.
+	 */
+	cancel_work_sync(&i2400m->wake_tx_ws);
+
+	spin_lock_irqsave(&i2400m->tx_lock, flags);
+	wake_tx_skb = i2400m->wake_tx_skb;
+	i2400m->wake_tx_skb = NULL;
+	spin_unlock_irqrestore(&i2400m->tx_lock, flags);
+
+	if (wake_tx_skb) {
 		i2400m_put(i2400m);
 		kfree_skb(wake_tx_skb);
 	}
+
 	d_fnend(3, dev, "(i2400m %p) = void\n", i2400m);
 }
 
@@ -288,7 +291,7 @@ int i2400m_net_wake_tx(struct i2400m *i2400m, struct net_device *net_dev,
 	 * and if pending, release those resources. */
 	result = 0;
 	spin_lock_irqsave(&i2400m->tx_lock, flags);
-	if (!work_pending(&i2400m->wake_tx_ws)) {
+	if (!i2400m->wake_tx_skb) {
 		netif_stop_queue(net_dev);
 		i2400m_get(i2400m);
 		i2400m->wake_tx_skb = skb_get(skb);	/* transfer ref count */
-- 
1.8.0.2

RE: [PATCH 20/25] wimax/i2400m: fix i2400m->wake_tx_skb handling

[Perez-Gonzalez, Inaky]

> From: Tejun Heo [mailto:htejun@gmail.com] On Behalf Of Tejun Heo
0m: fix i2400m->wake_tx_skb handling
> 
...
> 
> Only compile tested.

I don't have access to hardware to test this--might want to contact Dan Williams,
as he has been more actively using it.

Re: [PATCH 20/25] wimax/i2400m: fix i2400m->wake_tx_skb handling

[Dan Williams]

On Fri, 2012-12-21 at 17:57 -0800, Tejun Heo wrote:
> i2400m_net_wake_tx() sets ->wake_tx_skb with the given skb if
> ->wake_tx_ws is not pending; however, i2400m_wake_tx_work() could have
> just started execution and haven't fetched -><wake_tx_skb yet.  The
> previous packet will be leaked.
> 
> Update ->wake_tx_skb handling.
> 
> * i2400m_net_wake_tx() now tests whether the previous ->wake_tx_skb
>   has been consumed by ->wake_tx_ws instead of testing work_pending().
> 
> * i2400m_net_wake_stop() is simplified similarly.  It always puts
>   ->wake_tx_skb if non-NULL.
> 
> * Spurious ->wake_tx_skb dereference outside critical section dropped
>   from i2400m_wake_tx_work().
> 
> Only compile tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>

Acked-by: Dan Williams <dcbw@redhat.com>

No regressions in a quick connection check to Clear and browsing a bunch
of pages.

Dan

> Cc: Inaky Perez-Gonzalez <inaky.perez-gonzalez@intel.com>
> Cc: linux-wimax@intel.com
> Cc: wimax@linuxwimax.org
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.
> 
> Thanks.
> 
>  drivers/net/wimax/i2400m/netdev.c | 31 +++++++++++++++++--------------
>  1 file changed, 17 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/net/wimax/i2400m/netdev.c b/drivers/net/wimax/i2400m/netdev.c
> index 1d76ae8..530581c 100644
> --- a/drivers/net/wimax/i2400m/netdev.c
> +++ b/drivers/net/wimax/i2400m/netdev.c
> @@ -156,7 +156,7 @@ void i2400m_wake_tx_work(struct work_struct *ws)
>  	struct i2400m *i2400m = container_of(ws, struct i2400m, wake_tx_ws);
>  	struct net_device *net_dev = i2400m->wimax_dev.net_dev;
>  	struct device *dev = i2400m_dev(i2400m);
> -	struct sk_buff *skb = i2400m->wake_tx_skb;
> +	struct sk_buff *skb;
>  	unsigned long flags;
>  
>  	spin_lock_irqsave(&i2400m->tx_lock, flags);
> @@ -236,23 +236,26 @@ void i2400m_tx_prep_header(struct sk_buff *skb)
>  void i2400m_net_wake_stop(struct i2400m *i2400m)
>  {
>  	struct device *dev = i2400m_dev(i2400m);
> +	struct sk_buff *wake_tx_skb;
> +	unsigned long flags;
>  
>  	d_fnstart(3, dev, "(i2400m %p)\n", i2400m);
> -	/* See i2400m_hard_start_xmit(), references are taken there
> -	 * and here we release them if the work was still
> -	 * pending. Note we can't differentiate work not pending vs
> -	 * never scheduled, so the NULL check does that. */
> -	if (cancel_work_sync(&i2400m->wake_tx_ws) == 0
> -	    && i2400m->wake_tx_skb != NULL) {
> -		unsigned long flags;
> -		struct sk_buff *wake_tx_skb;
> -		spin_lock_irqsave(&i2400m->tx_lock, flags);
> -		wake_tx_skb = i2400m->wake_tx_skb;	/* compat help */
> -		i2400m->wake_tx_skb = NULL;	/* compat help */
> -		spin_unlock_irqrestore(&i2400m->tx_lock, flags);
> +	/*
> +	 * See i2400m_hard_start_xmit(), references are taken there and
> +	 * here we release them if the packet was still pending.
> +	 */
> +	cancel_work_sync(&i2400m->wake_tx_ws);
> +
> +	spin_lock_irqsave(&i2400m->tx_lock, flags);
> +	wake_tx_skb = i2400m->wake_tx_skb;
> +	i2400m->wake_tx_skb = NULL;
> +	spin_unlock_irqrestore(&i2400m->tx_lock, flags);
> +
> +	if (wake_tx_skb) {
>  		i2400m_put(i2400m);
>  		kfree_skb(wake_tx_skb);
>  	}
> +
>  	d_fnend(3, dev, "(i2400m %p) = void\n", i2400m);
>  }
>  
> @@ -288,7 +291,7 @@ int i2400m_net_wake_tx(struct i2400m *i2400m, struct net_device *net_dev,
>  	 * and if pending, release those resources. */
>  	result = 0;
>  	spin_lock_irqsave(&i2400m->tx_lock, flags);
> -	if (!work_pending(&i2400m->wake_tx_ws)) {
> +	if (!i2400m->wake_tx_skb) {
>  		netif_stop_queue(net_dev);
>  		i2400m_get(i2400m);
>  		i2400m->wake_tx_skb = skb_get(skb);	/* transfer ref count */

Re: [PATCH 20/25] wimax/i2400m: fix i2400m->wake_tx_skb handling

[Tejun Heo]

On Fri, Jan 04, 2013 at 03:19:55PM -0600, Dan Williams wrote:
> On Fri, 2012-12-21 at 17:57 -0800, Tejun Heo wrote:
> > i2400m_net_wake_tx() sets ->wake_tx_skb with the given skb if
> > ->wake_tx_ws is not pending; however, i2400m_wake_tx_work() could have
> > just started execution and haven't fetched -><wake_tx_skb yet.  The
> > previous packet will be leaked.
> > 
> > Update ->wake_tx_skb handling.
> > 
> > * i2400m_net_wake_tx() now tests whether the previous ->wake_tx_skb
> >   has been consumed by ->wake_tx_ws instead of testing work_pending().
> > 
> > * i2400m_net_wake_stop() is simplified similarly.  It always puts
> >   ->wake_tx_skb if non-NULL.
> > 
> > * Spurious ->wake_tx_skb dereference outside critical section dropped
> >   from i2400m_wake_tx_work().
> > 
> > Only compile tested.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> 
> Acked-by: Dan Williams <dcbw@redhat.com>
> 
> No regressions in a quick connection check to Clear and browsing a bunch
> of pages.

Applied to wq/for-3.9-cleanups.

Thanks!

-- 
tejun

[PATCH 21/25] tty/max3100: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from max3100.  Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.cz>
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/tty/serial/max3100.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/tty/serial/max3100.c b/drivers/tty/serial/max3100.c
index 7ce3197..dd6277e 100644
--- a/drivers/tty/serial/max3100.c
+++ b/drivers/tty/serial/max3100.c
@@ -179,8 +179,7 @@ static void max3100_work(struct work_struct *w);
 
 static void max3100_dowork(struct max3100_port *s)
 {
-	if (!s->force_end_work && !work_pending(&s->work) &&
-	    !freezing(current) && !s->suspending)
+	if (!s->force_end_work && !freezing(current) && !s->suspending)
 		queue_work(s->workqueue, &s->work);
 }
 
-- 
1.8.0.2

Re: [PATCH 21/25] tty/max3100: don't use [delayed_]work_pending()

[Greg Kroah-Hartman]

On Fri, Dec 21, 2012 at 05:57:11PM -0800, Tejun Heo wrote:
> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
> 
> Remove unnecessary pending tests from max3100.  Only compile tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.cz>
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.

Please, feel free to take it through your tree:

Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Re: [PATCH 21/25] tty/max3100: don't use [delayed_]work_pending()

[Tejun Heo]

On Fri, Dec 21, 2012 at 08:21:25PM -0800, Greg Kroah-Hartman wrote:
> On Fri, Dec 21, 2012 at 05:57:11PM -0800, Tejun Heo wrote:
> > There's no need to test whether a (delayed) work item in pending
> > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > and quite a few of them are buggy.
> > 
> > Remove unnecessary pending tests from max3100.  Only compile tested.
> > 
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> > Cc: Jiri Slaby <jslaby@suse.cz>
> > ---
> > Please let me know how this patch should be routed.  I can take it
> > through the workqueue tree if necessary.
> 
> Please, feel free to take it through your tree:
> 
> Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>

Applied to wq/for-3.9-cleanups.  Thanks.

-- 
tejun

[PATCH 22/25] usb/at91_udc: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from at91_udc.  Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Andrew Victor <linux@maxim.org.za>
Cc: Nicolas Ferre <nicolas.ferre@atmel.com>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Felipe Balbi <balbi@ti.com>
Cc: linux-usb@vger.kernel.org
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/usb/gadget/at91_udc.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/usb/gadget/at91_udc.c b/drivers/usb/gadget/at91_udc.c
index f4a21f6..e81d8a2 100644
--- a/drivers/usb/gadget/at91_udc.c
+++ b/drivers/usb/gadget/at91_udc.c
@@ -1621,8 +1621,7 @@ static void at91_vbus_timer(unsigned long data)
 	 * bus such as i2c or spi which may sleep, so schedule some work
 	 * to read the vbus gpio
 	 */
-	if (!work_pending(&udc->vbus_timer_work))
-		schedule_work(&udc->vbus_timer_work);
+	schedule_work(&udc->vbus_timer_work);
 }
 
 static int at91_start(struct usb_gadget *gadget,
-- 
1.8.0.2

Re: [PATCH 22/25] usb/at91_udc: don't use [delayed_]work_pending()

[Nicolas Ferre]

On 12/22/2012 02:57 AM, Tejun Heo :
> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
> 
> Remove unnecessary pending tests from at91_udc.  Only compile tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Andrew Victor <linux@maxim.org.za>
> Cc: Nicolas Ferre <nicolas.ferre@atmel.com>

Acked-by: Nicolas Ferre <nicolas.ferre@atmel.com>

> Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
> Cc: Felipe Balbi <balbi@ti.com>
> Cc: linux-usb@vger.kernel.org
> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.
> 
> Thanks.
> 
>  drivers/usb/gadget/at91_udc.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/usb/gadget/at91_udc.c b/drivers/usb/gadget/at91_udc.c
> index f4a21f6..e81d8a2 100644
> --- a/drivers/usb/gadget/at91_udc.c
> +++ b/drivers/usb/gadget/at91_udc.c
> @@ -1621,8 +1621,7 @@ static void at91_vbus_timer(unsigned long data)
>  	 * bus such as i2c or spi which may sleep, so schedule some work
>  	 * to read the vbus gpio
>  	 */
> -	if (!work_pending(&udc->vbus_timer_work))
> -		schedule_work(&udc->vbus_timer_work);
> +	schedule_work(&udc->vbus_timer_work);
>  }
>  
>  static int at91_start(struct usb_gadget *gadget,
> 


-- 
Nicolas Ferre

[PATCH 23/25] video/exynos: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from exynos_dp_core.  Only compile
tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Kukjin Kim <kgene.kim@samsung.com>
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 drivers/video/exynos/exynos_dp_core.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/video/exynos/exynos_dp_core.c b/drivers/video/exynos/exynos_dp_core.c
index 28fd686..3002a6a 100644
--- a/drivers/video/exynos/exynos_dp_core.c
+++ b/drivers/video/exynos/exynos_dp_core.c
@@ -1121,8 +1121,7 @@ static int __devexit exynos_dp_remove(struct platform_device *pdev)
 
 	disable_irq(dp->irq);
 
-	if (work_pending(&dp->hotplug_work))
-		flush_work(&dp->hotplug_work);
+	flush_work(&dp->hotplug_work);
 
 	if (pdev->dev.of_node) {
 		if (dp->phy_addr)
@@ -1144,8 +1143,7 @@ static int exynos_dp_suspend(struct device *dev)
 	struct exynos_dp_platdata *pdata = dev->platform_data;
 	struct exynos_dp_device *dp = dev_get_drvdata(dev);
 
-	if (work_pending(&dp->hotplug_work))
-		flush_work(&dp->hotplug_work);
+	flush_work(&dp->hotplug_work);
 
 	if (dev->of_node) {
 		if (dp->phy_addr)
-- 
1.8.0.2

RE: [PATCH 23/25] video/exynos: don't use [delayed_]work_pending()

[Kukjin Kim]

Tejun Heo wrote:
> 
> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.
> 
> Remove unnecessary pending tests from exynos_dp_core.  Only compile
> tested.
> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Cc: Kukjin Kim <kgene.kim@samsung.com>

Acked-by: Kukjin Kim <kgene.kim@samsung.com>

> ---
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.
> 
Cc'ed Jingoo and Florian.

Now the exynos dp driver are being handled by Jingoo so let's waiting for
his opinion to take this by himself or not. 

- Kukjin Kim

> Thanks.
> 
>  drivers/video/exynos/exynos_dp_core.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/video/exynos/exynos_dp_core.c
> b/drivers/video/exynos/exynos_dp_core.c
> index 28fd686..3002a6a 100644
> --- a/drivers/video/exynos/exynos_dp_core.c
> +++ b/drivers/video/exynos/exynos_dp_core.c
> @@ -1121,8 +1121,7 @@ static int __devexit exynos_dp_remove(struct
> platform_device *pdev)
> 
>  	disable_irq(dp->irq);
> 
> -	if (work_pending(&dp->hotplug_work))
> -		flush_work(&dp->hotplug_work);
> +	flush_work(&dp->hotplug_work);
> 
>  	if (pdev->dev.of_node) {
>  		if (dp->phy_addr)
> @@ -1144,8 +1143,7 @@ static int exynos_dp_suspend(struct device *dev)
>  	struct exynos_dp_platdata *pdata = dev->platform_data;
>  	struct exynos_dp_device *dp = dev_get_drvdata(dev);
> 
> -	if (work_pending(&dp->hotplug_work))
> -		flush_work(&dp->hotplug_work);
> +	flush_work(&dp->hotplug_work);
> 
>  	if (dev->of_node) {
>  		if (dp->phy_addr)
> --
> 1.8.0.2

Re: [PATCH 23/25] video/exynos: don't use [delayed_]work_pending()

[Jingoo Han]

On Saturday, December 22, 2012 12:06 PM, Kukjin Kim wrote 
> Tejun Heo wrote:
> >
> > There's no need to test whether a (delayed) work item in pending
> > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > and quite a few of them are buggy.
> >
> > Remove unnecessary pending tests from exynos_dp_core.  Only compile
> > tested.
> >
> > Signed-off-by: Tejun Heo <tj@kernel.org>
> > Cc: Kukjin Kim <kgene.kim@samsung.com>
> 
> Acked-by: Kukjin Kim <kgene.kim@samsung.com>

Acked-by: Jingoo Han <jg1.han@samsung.com>


I tested this patch with Exynos5250.
Also, taking it through the workqueue tree would be better.

Best regards,
Jingoo Han

> 
> > ---
> > Please let me know how this patch should be routed.  I can take it
> > through the workqueue tree if necessary.
> >
> Cc'ed Jingoo and Florian.
> 
> Now the exynos dp driver are being handled by Jingoo so let's waiting for
> his opinion to take this by himself or not.
> 
> - Kukjin Kim
> 
> > Thanks.
> >
> >  drivers/video/exynos/exynos_dp_core.c | 6 ++----
> >  1 file changed, 2 insertions(+), 4 deletions(-)
> >
> > diff --git a/drivers/video/exynos/exynos_dp_core.c
> > b/drivers/video/exynos/exynos_dp_core.c
> > index 28fd686..3002a6a 100644
> > --- a/drivers/video/exynos/exynos_dp_core.c
> > +++ b/drivers/video/exynos/exynos_dp_core.c
> > @@ -1121,8 +1121,7 @@ static int __devexit exynos_dp_remove(struct
> > platform_device *pdev)
> >
> >  	disable_irq(dp->irq);
> >
> > -	if (work_pending(&dp->hotplug_work))
> > -		flush_work(&dp->hotplug_work);
> > +	flush_work(&dp->hotplug_work);
> >
> >  	if (pdev->dev.of_node) {
> >  		if (dp->phy_addr)
> > @@ -1144,8 +1143,7 @@ static int exynos_dp_suspend(struct device *dev)
> >  	struct exynos_dp_platdata *pdata = dev->platform_data;
> >  	struct exynos_dp_device *dp = dev_get_drvdata(dev);
> >
> > -	if (work_pending(&dp->hotplug_work))
> > -		flush_work(&dp->hotplug_work);
> > +	flush_work(&dp->hotplug_work);
> >
> >  	if (dev->of_node) {
> >  		if (dp->phy_addr)
> > --
> > 1.8.0.2

Re: [PATCH 23/25] video/exynos: don't use [delayed_]work_pending()

['Tejun Heo']

On Wed, Dec 26, 2012 at 01:04:02PM +0900, Jingoo Han wrote:
> On Saturday, December 22, 2012 12:06 PM, Kukjin Kim wrote 
> > Tejun Heo wrote:
> > >
> > > There's no need to test whether a (delayed) work item in pending
> > > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > > and quite a few of them are buggy.
> > >
> > > Remove unnecessary pending tests from exynos_dp_core.  Only compile
> > > tested.
> > >
> > > Signed-off-by: Tejun Heo <tj@kernel.org>
> > > Cc: Kukjin Kim <kgene.kim@samsung.com>
> > 
> > Acked-by: Kukjin Kim <kgene.kim@samsung.com>
> 
> Acked-by: Jingoo Han <jg1.han@samsung.com>
> 
> I tested this patch with Exynos5250.
> Also, taking it through the workqueue tree would be better.

Applied to wq/for-3.9-cleanups.  Thanks!

-- 
tejun

[PATCH 24/25] debugobjects: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from debugobjects.  While at it,
change @sched to bool and move kevent_up() test to later for brevity.

Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 lib/debugobjects.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/lib/debugobjects.c b/lib/debugobjects.c
index d11808c..b9dbfdf 100644
--- a/lib/debugobjects.c
+++ b/lib/debugobjects.c
@@ -189,20 +189,19 @@ static void free_obj_work(struct work_struct *work)
 static void free_object(struct debug_obj *obj)
 {
 	unsigned long flags;
-	int sched = 0;
+	bool sched;
 
 	raw_spin_lock_irqsave(&pool_lock, flags);
 	/*
 	 * schedule work when the pool is filled and the cache is
 	 * initialized:
 	 */
-	if (obj_pool_free > ODEBUG_POOL_SIZE && obj_cache)
-		sched = keventd_up() && !work_pending(&debug_obj_work);
+	sched = obj_pool_free > ODEBUG_POOL_SIZE && obj_cache;
 	hlist_add_head(&obj->node, &obj_pool);
 	obj_pool_free++;
 	obj_pool_used--;
 	raw_spin_unlock_irqrestore(&pool_lock, flags);
-	if (sched)
+	if (sched && keventd_up())
 		schedule_work(&debug_obj_work);
 }
 
-- 
1.8.0.2

[PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Tejun Heo]

There's no need to test whether a (delayed) work item in pending
before queueing, flushing or cancelling it.  Most uses are unnecessary
and quite a few of them are buggy.

Remove unnecessary pending tests from ipc.  Only compile tested.

Signed-off-by: Tejun Heo <tj@kernel.org>
Cc: Andrew Morton <akpm@linux-foundation.org>
---
Please let me know how this patch should be routed.  I can take it
through the workqueue tree if necessary.

Thanks.

 ipc/util.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/ipc/util.c b/ipc/util.c
index 72fd078..add2776 100644
--- a/ipc/util.c
+++ b/ipc/util.c
@@ -71,8 +71,7 @@ static int ipc_memory_callback(struct notifier_block *self,
 		 * activate the ipcns notification chain.
 		 * No need to keep several ipc work items on the queue.
 		 */
-		if (!work_pending(&ipc_memory_wq))
-			schedule_work(&ipc_memory_wq);
+		schedule_work(&ipc_memory_wq);
 		break;
 	case MEM_GOING_ONLINE:
 	case MEM_GOING_OFFLINE:
-- 
1.8.0.2

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Andrew Morton]

On Fri, 21 Dec 2012 17:57:15 -0800 Tejun Heo <tj@kernel.org> wrote:

> There's no need to test whether a (delayed) work item in pending
> before queueing, flushing or cancelling it.  Most uses are unnecessary
> and quite a few of them are buggy.

> -		if (!work_pending(&ipc_memory_wq))
> -			schedule_work(&ipc_memory_wq);
> +		schedule_work(&ipc_memory_wq);

Well, the new code is a ton slower than the old code if the work is
frequently pending, so some care is needed with such a conversion.

That's not an issue for the IPC callsite - memory offlining isn't
frequent.

> ...
>
> Please let me know how this patch should be routed.  I can take it
> through the workqueue tree if necessary.
> 

Please merge this one yourself.

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Tejun Heo]

Hello, Andrew.

On Fri, Dec 21, 2012 at 06:15:23PM -0800, Andrew Morton wrote:
> On Fri, 21 Dec 2012 17:57:15 -0800 Tejun Heo <tj@kernel.org> wrote:
> 
> > There's no need to test whether a (delayed) work item in pending
> > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > and quite a few of them are buggy.
> 
> > -		if (!work_pending(&ipc_memory_wq))
> > -			schedule_work(&ipc_memory_wq);
> > +		schedule_work(&ipc_memory_wq);
> 
> Well, the new code is a ton slower than the old code if the work is
> frequently pending, so some care is needed with such a conversion.

Yeah, I mentioned it in the head message.  it comes down to
test_and_set_bit() vs. test_bit() and none of the current users seems
to be hot enough for that to matter at all.

In very hot paths, such optimization *could* be valid.  The problem is
that [delayed_]work_pending() seem to be abused much more than they
are put to any actual usefulness.  Maybe we should rename them to
something really ugly.  I don't know.

> That's not an issue for the IPC callsite - memory offlining isn't
> frequent.
> 
> > ...
> >
> > Please let me know how this patch should be routed.  I can take it
> > through the workqueue tree if necessary.
> > 
> 
> Please merge this one yourself.

Can I add your acked-by?

Thanks.

-- 
tejun

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Borislav Petkov]

On Fri, Dec 21, 2012 at 06:22:10PM -0800, Tejun Heo wrote:
> Hello, Andrew.
> 
> On Fri, Dec 21, 2012 at 06:15:23PM -0800, Andrew Morton wrote:
> > On Fri, 21 Dec 2012 17:57:15 -0800 Tejun Heo <tj@kernel.org> wrote:
> > 
> > > There's no need to test whether a (delayed) work item in pending
> > > before queueing, flushing or cancelling it.  Most uses are unnecessary
> > > and quite a few of them are buggy.
> > 
> > > -		if (!work_pending(&ipc_memory_wq))
> > > -			schedule_work(&ipc_memory_wq);
> > > +		schedule_work(&ipc_memory_wq);
> > 
> > Well, the new code is a ton slower than the old code if the work is
> > frequently pending, so some care is needed with such a conversion.
> 
> Yeah, I mentioned it in the head message.  it comes down to
> test_and_set_bit() vs. test_bit() and none of the current users seems
> to be hot enough for that to matter at all.
> 
> In very hot paths, such optimization *could* be valid.  The problem is
> that [delayed_]work_pending() seem to be abused much more than they
> are put to any actual usefulness.  Maybe we should rename them to
> something really ugly.  I don't know.

Hmm, we're also disabling local interrupts for no reason, if there's no
work pending (this is queue_work_on()):

    2d1a:       9c                      pushfq
    2d1b:       41 5c                   pop    %r12
    2d1d:       fa                      cli
    2d1e:       e8 00 00 00 00          callq  2d23 <queue_work_on+0x33>
    2d23:       f0 0f ba 2b 00          lock btsl $0x0,(%rbx)

so there's IRQ disable + locked operation in schedule_work vs a simple
test_bit which doesn't even require the LOCK prefix.

Now you say those paths are not fast paths, but the reverse of
this optimization is also true: what happens if people start using
schedule_work() in fast paths without checking whether work is pending?
A useless IRQ disable + locked operation + IRQ enable.

I don't know but this could hurt in some situations, I'm thinking of RT
folk especially here.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Tejun Heo]

Hello, Borislav.

On Sat, Dec 22, 2012 at 12:09:29PM +0100, Borislav Petkov wrote:
> Now you say those paths are not fast paths, but the reverse of
> this optimization is also true: what happens if people start using
> schedule_work() in fast paths without checking whether work is pending?
> A useless IRQ disable + locked operation + IRQ enable.

That's a really strange argument.  If we extend from that, we have to
optimize cold paths to prevent fast paths copy from them, which sounds
really silly to me.

If one looks at something happening in a path as cold as memory
hotplug and thinks about optimizing a coupld memory accesses, the
person's priorities need serious reconsideration.  I think approaches
like that are actively harmful.  They lead to unnecessary (and thus
difficult to comprehend) convolutions which don't really help anything
while deteoriorating code base.

I don't think we have cases where this actually matters but it could
be that we can add work_pending() tests to queue_work_on().  I *think*
that shouldn't break work scheduling semantics.  Not completely sure
tho.  Need to think about it more.

Thanks.

-- 
tejun

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Tejun Heo]

Hello, again.

On Mon, Dec 24, 2012 at 10:33:34AM -0800, Tejun Heo wrote:
> I don't think we have cases where this actually matters but it could
> be that we can add work_pending() tests to queue_work_on().  I *think*
> that shouldn't break work scheduling semantics.  Not completely sure
> tho.  Need to think about it more.

I was confused a bit there.  We can't.  Nothing guarantees that the
queuer sees the cleared PENDING before the work item starts execution,
and I think ipc memory hotplug could also be broken from that.  It's
highly unlikely to actually happen and there may be external locking
which prevents the race from actually happening, but there's nothing
synchronizing queueing and the execution of the work item.  Looking at
that part of code only, it's possible that it fails to queue the work
item after a memory hotplug event even though the previous queueing
already started execution and processed a couple notifiers.

And you can see why you don't want this type of tricky micro
optimizations unless it's absolutely necessary and carefully
considered.  Cold paths get much less attention and testing.  Adding
micro optimizations to them is just a bad idea.

Thanks.

-- 
tejun

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Borislav Petkov]

On Mon, Dec 24, 2012 at 10:45:20AM -0800, Tejun Heo wrote:
> I was confused a bit there. We can't. Nothing guarantees that the
> queuer sees the cleared PENDING before the work item starts execution,
> and I think ipc memory hotplug could also be broken from that.

Stupid question: why not clear PENDING after execution is done? I'm
looking at process_one_work() here.

> It's highly unlikely to actually happen and there may be external
> locking which prevents the race from actually happening, but there's
> nothing synchronizing queueing and the execution of the work item.
> Looking at that part of code only, it's possible that it fails to
> queue the work item after a memory hotplug event even though the
> previous queueing already started execution and processed a couple
> notifiers.

Maybe failure to queue could be signalled with a proper return value
from __queue_work()?

Btw, I'm afraid I don't understand the "memory hotplug event" aspect and
how that can influence the queueing - all it does it is list_add_tail,
basically.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Tejun Heo]

Hello,

On Mon, Dec 24, 2012 at 08:41:01PM +0100, Borislav Petkov wrote:
> On Mon, Dec 24, 2012 at 10:45:20AM -0800, Tejun Heo wrote:
> > I was confused a bit there. We can't. Nothing guarantees that the
> > queuer sees the cleared PENDING before the work item starts execution,
> > and I think ipc memory hotplug could also be broken from that.
> 
> Stupid question: why not clear PENDING after execution is done? I'm
> looking at process_one_work() here.

The behavior is primarily because we want to enable workqueue users to
guarantee that a full work item execution happens at least once after
certain event happens.  ie. Let's say there's a work item which
processes data generated by a device.  If it's IRQ handler calls
queue_work() after getting notified of new data segment by the device,
it would want to guarantee that whole work item execution would happen
afterwards.  If you clear PENDING after execution, the event may
overlap with the end of the previous execution and the new data won't
be processed.

> > It's highly unlikely to actually happen and there may be external
> > locking which prevents the race from actually happening, but there's
> > nothing synchronizing queueing and the execution of the work item.
> > Looking at that part of code only, it's possible that it fails to
> > queue the work item after a memory hotplug event even though the
> > previous queueing already started execution and processed a couple
> > notifiers.
> 
> Maybe failure to queue could be signalled with a proper return value
> from __queue_work()?

It already does that but it's not about the return value.  You simply
cannot know whether to proceed with queueing or not with test_bit()
alone.

> Btw, I'm afraid I don't understand the "memory hotplug event" aspect and
> how that can influence the queueing - all it does it is list_add_tail,
> basically.

It doesn't have anything to do with memory hotplug event itself.  It's
a generic memory access ordering / synchronization issue.  There just
isn't anything preventing test_bit() from seeing PENDING bit from
before clearing.

Thanks.

-- 
tejun

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Borislav Petkov]

On Mon, Dec 24, 2012 at 07:29:14PM -0800, Tejun Heo wrote:
> The behavior is primarily because we want to enable workqueue users
> to guarantee that a full work item execution happens at least once
> after certain event happens. ie. Let's say there's a work item which
> processes data generated by a device. If it's IRQ handler calls
> queue_work() after getting notified of new data segment by the device,
> it would want to guarantee that whole work item execution would happen
> afterwards. If you clear PENDING after execution, the event may
> overlap with the end of the previous execution and the new data won't
> be processed.

Oh, ok, I didn't realize we did some sort of a one-shot queueing - I
thought users of wq are supposed to be polling PENDING and when it is
cleared, they queue. But come to think of it, we can't be doing that in
an IRQ handler.

[ … ]

> It doesn't have anything to do with memory hotplug event itself.  It's
> a generic memory access ordering / synchronization issue.  There just
> isn't anything preventing test_bit() from seeing PENDING bit from
> before clearing.

That's true. So is it that the whole PENDING bit testing was actually
the wrong sync primitive to use in most cases and we actually really
needed to disable interrupts around testing of that bit?

Which would make your patches actually bugfixes.

/me goes and rereads 0/25 announcement mail.

Yes, it sounds like [delayed_]work_pending() needs to go or at least
needs a big fat comment over it explaining that it is a special, cheaper
alternative to be used *only* in conjunction with other synchronization.

Ok, thanks Tejun, for taking the time and explaining it again to me.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Tejun Heo]

Hello, Borislav.

On Tue, Dec 25, 2012 at 11:46:14AM +0100, Borislav Petkov wrote:
> > It doesn't have anything to do with memory hotplug event itself.  It's
> > a generic memory access ordering / synchronization issue.  There just
> > isn't anything preventing test_bit() from seeing PENDING bit from
> > before clearing.
> 
> That's true. So is it that the whole PENDING bit testing was actually
> the wrong sync primitive to use in most cases and we actually really
> needed to disable interrupts around testing of that bit?

The IRQ disabling is for synchronization with other workqueue
operations.  Once PENDING is set, other workqueue operations may
busy-wait for the work item to be put on the appropriate queue so that
further operations can happen (e.g. flush / cancel).  So, if you have
to be atomic locally w.r.t. IRQs.

Memory ordering issue is the one which makes using test_bit() in
itself inadequate for gating the queue operation.  It could be all we
need is throwing in a smp_mb() before test_bit() to guarantee that
either clear PENDING is visible or all updates upto that point is
visible to the coming work item execution.  Not sure whether that
would be worthwhile tho.  The only case that may help is, I think, if
an already pending work item is queued very frequently from multiple
CPUs, which is unlikely and, if exists, indicates larger problems.

> Which would make your patches actually bugfixes.

Yes, why I'm talking about making [delayed_]work_pending() go away or
at least make it very ugly to use.

> /me goes and rereads 0/25 announcement mail.
> 
> Yes, it sounds like [delayed_]work_pending() needs to go or at least
> needs a big fat comment over it explaining that it is a special, cheaper
> alternative to be used *only* in conjunction with other synchronization.
> 
> Ok, thanks Tejun, for taking the time and explaining it again to me.

Thanks.

-- 
tejun

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Borislav Petkov]

On Mon, Dec 24, 2012 at 10:33:34AM -0800, Tejun Heo wrote:
> If one looks at something happening in a path as cold as memory
> hotplug and thinks about optimizing a coupld memory accesses, the
> person's priorities need serious reconsideration. I think approaches
> like that are actively harmful. They lead to unnecessary (and thus
> difficult to comprehend) convolutions which don't really help anything
> while deteoriorating code base.

Maybe I'm not expressing myself clearly enough so let me try again: all
I'm saying is, I don't like the idea of needlessly toggling interrupts
and having a locked operation just to understand that there's work
pending.

Basically, with the amount of bloat we're adding to the kernel, the
couple of instructions we're adding here and there and think they won't
harm us, tends to crop up with time until we're too damn slow to do
anything.

Now, you're saying that optimizing cold paths is something that calls
for serious reconsideration of a person's priorities, and I'm saying
just don't do anything in code that's not necessary. Fullstop. If this
is leading to convolutions, then the design is suboptimal and needs
adjustment. If [delayed_]work_pending is being abused, then fix it or
add a new primitive which only checks for pending work and doesn't
unconditionally toggle interrupts.

> I don't think we have cases where this actually matters but it could
> be that we can add work_pending() tests to queue_work_on(). I *think*
> that shouldn't break work scheduling semantics. Not completely sure
> tho. Need to think about it more.

Yes, something like that.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Tejun Heo]

Hello, Borislav.

On Mon, Dec 24, 2012 at 07:55:55PM +0100, Borislav Petkov wrote:
> Basically, with the amount of bloat we're adding to the kernel, the
> couple of instructions we're adding here and there and think they won't
> harm us, tends to crop up with time until we're too damn slow to do
> anything.

Yeah, I think we need them for the operations to hold w/o outer
synchronization.  If you can think of ways to optimize them, please be
my guest.

> Now, you're saying that optimizing cold paths is something that calls
> for serious reconsideration of a person's priorities, and I'm saying
> just don't do anything in code that's not necessary. Fullstop. If this

Not doing anything that's not necessary is an extreme and not
necessary what you want in cold paths.  If doing something extra makes
your code much simpler and more maintainable and the path is cold
enough for the extra to not matter, then that's the right trade-off.

> is leading to convolutions, then the design is suboptimal and needs
> adjustment. If [delayed_]work_pending is being abused, then fix it or
> add a new primitive which only checks for pending work and doesn't
> unconditionally toggle interrupts.

"only checks for pending work" in a way which is race-free and usable
from any context is a tricky thing to do.

> > I don't think we have cases where this actually matters but it could
> > be that we can add work_pending() tests to queue_work_on(). I *think*
> > that shouldn't break work scheduling semantics. Not completely sure
> > tho. Need to think about it more.
> 
> Yes, something like that.

And that's broken.  It seems trivial but it really isn't and trying to
optimize things like that in cold paths is just a bad idea.  Not
enough people will pay attention to them and they will stay subtly
broken for a very long time.  So, having "not doing anything in code
which isn't necessary in code" as priority in cold paths is likely to
hurt you.  A better one would be "doing straight-forward and simple
thing with acceptable overhead".

Thanks.

-- 
tejun

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Borislav Petkov]

On Mon, Dec 24, 2012 at 11:07:23AM -0800, Tejun Heo wrote:
> And that's broken. It seems trivial but it really isn't and trying to
> optimize things like that in cold paths is just a bad idea. Not enough
> people will pay attention to them and they will stay subtly broken for
> a very long time. So, having "not doing anything in code which isn't
> necessary in code" as priority in cold paths is likely to hurt you.
> A better one would be "doing straight-forward and simple thing with
> acceptable overhead".

Ok, understood. I have only one question: how do you make sure
schedule_work() is used only in cold paths?

Btw, there's __cancel_delayed_work() which is not used anywhere and it
could be deleted AFAICT.

Thanks.

-- 
Regards/Gruss,
    Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Re: [PATCH 25/25] ipc: don't use [delayed_]work_pending()

[Tejun Heo]

Hello, Borislav.

On Mon, Dec 24, 2012 at 08:32:58PM +0100, Borislav Petkov wrote:
> Ok, understood. I have only one question: how do you make sure
> schedule_work() is used only in cold paths?

Hot and cold are relative terms and unless someone is doing things
like invoking queue_work() from high-frequency interrupt handler, the
level of overhead from queue_work() isn't likely to matter.  The best
way to deal with such hot paths would differ depending on the
specifics of each hot path - ie. bouncing to workqueue from IRQ
handler would be better handled by threaded IRQ handlers.

At this point, especially given how all of work_pending() users are
way too cold for any of this to matter, I don't think we need to worry
about this.

> Btw, there's __cancel_delayed_work() which is not used anywhere and it
> could be deleted AFAICT.

Yeah, there are several interfaces which are being deprecated.
They'll be gone in a few cycles.

Thanks.

-- 
tejun