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

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