| Message ID | 1356141435-17340-17-git-send-email-tj@kernel.org (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
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); > } > > /** >
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.
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
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.
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); } /**
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(-)