diff mbox

[RFC] net: wireless: move regulatory timeout work to power efficient workqueue

Message ID 1391123310-6425-1-git-send-email-zoran.markovic@linaro.org
State Awaiting Upstream, archived
Delegated to: David Miller
Headers show

Commit Message

Zoran Markovic Jan. 30, 2014, 11:08 p.m. UTC 
From: Shaibal Dutta <shaibal.dutta@broadcom.com>

For better use of CPU idle time, allow the scheduler to select the CPU
on which the timeout work of regulatory settings would be executed.
This extends CPU idle residency time and saves power.

This functionality is enabled when CONFIG_WQ_POWER_EFFICIENT is selected.

Cc: Johannes Berg <johannes@sipsolutions.net>
Cc: "John W. Linville" <linville@tuxdriver.com>
Cc: "David S. Miller" <davem@davemloft.net>
Signed-off-by: Shaibal Dutta <shaibal.dutta@broadcom.com>
[zoran.markovic@linaro.org: Rebased to latest kernel. Added commit message.]
Signed-off-by: Zoran Markovic <zoran.markovic@linaro.org>
---
 net/wireless/reg.c |    9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

Comments

Johannes Berg Jan. 31, 2014, 9:21 a.m. UTC | #1 
On Thu, 2014-01-30 at 15:08 -0800, Zoran Markovic wrote:
> From: Shaibal Dutta <shaibal.dutta@broadcom.com>
> 
> For better use of CPU idle time, allow the scheduler to select the CPU
> on which the timeout work of regulatory settings would be executed.
> This extends CPU idle residency time and saves power.
> 
> This functionality is enabled when CONFIG_WQ_POWER_EFFICIENT is selected.

> -		schedule_delayed_work(&reg_timeout, msecs_to_jiffies(3142));
> +		queue_delayed_work(system_power_efficient_wq,
> +				   &reg_timeout, msecs_to_jiffies(3142));

I'm not sure if this is part of a larger patchset actually adding that
"system_power_efficient_wq", but maybe it'd be better to expose a
function as an API rather than the wq struct?

Something like

scheduled_delayed_work_pwr_efficient(...)?

?

johannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo Jan. 31, 2014, 9:35 a.m. UTC | #2 
Hello,

On Fri, Jan 31, 2014 at 10:21:24AM +0100, Johannes Berg wrote:
> I'm not sure if this is part of a larger patchset actually adding that
> "system_power_efficient_wq", but maybe it'd be better to expose a
> function as an API rather than the wq struct?
> 
> Something like
> 
> scheduled_delayed_work_pwr_efficient(...)?

While there are some benefits to using dedicated functions for
specific workqueues, I don't think it brings enough benefits to
justify adding dedicated API and am unlikely to add new ones.

Thanks.
Johannes Berg Jan. 31, 2014, 9:42 a.m. UTC | #3 
On Fri, 2014-01-31 at 04:35 -0500, Tejun Heo wrote:
> Hello,
> 
> On Fri, Jan 31, 2014 at 10:21:24AM +0100, Johannes Berg wrote:
> > I'm not sure if this is part of a larger patchset actually adding that
> > "system_power_efficient_wq", but maybe it'd be better to expose a
> > function as an API rather than the wq struct?
> > 
> > Something like
> > 
> > scheduled_delayed_work_pwr_efficient(...)?
> 
> While there are some benefits to using dedicated functions for
> specific workqueues, I don't think it brings enough benefits to
> justify adding dedicated API and am unlikely to add new ones.

Fair enough, I guess I'll take those patches in.

johannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Johannes Berg Jan. 31, 2014, 1:38 p.m. UTC | #4 
On Thu, 2014-01-30 at 15:08 -0800, Zoran Markovic wrote:
> From: Shaibal Dutta <shaibal.dutta@broadcom.com>
> 
> For better use of CPU idle time, allow the scheduler to select the CPU
> on which the timeout work of regulatory settings would be executed.
> This extends CPU idle residency time and saves power.
> 
> This functionality is enabled when CONFIG_WQ_POWER_EFFICIENT is selected.

Applied.

johannes

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stanislaw Gruszka Feb. 5, 2014, 9:17 a.m. UTC | #5 
Hi

On Fri, Jan 31, 2014 at 04:35:31AM -0500, Tejun Heo wrote:
> On Fri, Jan 31, 2014 at 10:21:24AM +0100, Johannes Berg wrote:
> > I'm not sure if this is part of a larger patchset actually adding that
> > "system_power_efficient_wq", but maybe it'd be better to expose a
> > function as an API rather than the wq struct?
> > 
> > Something like
> > 
> > scheduled_delayed_work_pwr_efficient(...)?
> 
> While there are some benefits to using dedicated functions for
> specific workqueues, I don't think it brings enough benefits to
> justify adding dedicated API and am unlikely to add new ones.

What are selection criteria when choosing between system_wq or
system_power_efficient_wq on drivers ? IOW if I would be writing
a new driver which workqueue should I use and when ?

I think that should be driver independent, at least for most of drivers.
If system have to run in low power mode, system_power_efficient_wq
should be chosen automatically by schedule_work(), otherwise when high
performance is more important schedule_work() should use system_wq.

Stanislaw
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tejun Heo Feb. 5, 2014, 9:25 a.m. UTC | #6 
Hello,

On Wed, Feb 05, 2014 at 10:17:42AM +0100, Stanislaw Gruszka wrote:
> What are selection criteria when choosing between system_wq or
> system_power_efficient_wq on drivers ? IOW if I would be writing
> a new driver which workqueue should I use and when ?

Yeah, it's a bit ad-hoc at the moment.  The original intention was
just marking the ones which can be shown to have noticeable power
impacts which weren't expected to be too many but we may now have an
self-feeding feedback loop growing new usages, likely somewhat
overzealously and be better off making things more generic.

> I think that should be driver independent, at least for most of drivers.
> If system have to run in low power mode, system_power_efficient_wq
> should be chosen automatically by schedule_work(), otherwise when high
> performance is more important schedule_work() should use system_wq.

The problem there is that system_wq has traditionally guaranteed
per-cpu execution.  It can't automatically be switched to unbound
behavior.  The best long term solution would be isolating the users
which depend on per-cpu behavior and mark them specially rather than
the other way around that we're doing now, making per-cpu guarantee
the special case rather than the norm.  That's gonna take a lot of
auditing tho.

Thanks.
diff mbox

Patch

diff --git a/net/wireless/reg.c b/net/wireless/reg.c
index 9b897fc..6e21011 100644
--- a/net/wireless/reg.c
+++ b/net/wireless/reg.c
@@ -1703,7 +1703,8 @@  static void reg_process_hint(struct regulatory_request *reg_request)
 		if (treatment == REG_REQ_OK ||
 		    treatment == REG_REQ_ALREADY_SET)
 			return;
-		schedule_delayed_work(&reg_timeout, msecs_to_jiffies(3142));
+		queue_delayed_work(system_power_efficient_wq,
+				   &reg_timeout, msecs_to_jiffies(3142));
 		return;
 	case NL80211_REGDOM_SET_BY_DRIVER:
 		treatment = reg_process_hint_driver(wiphy, reg_request);
@@ -2294,7 +2295,8 @@  static int reg_set_rd_driver(const struct ieee80211_regdomain *rd,
 
 	request_wiphy = wiphy_idx_to_wiphy(driver_request->wiphy_idx);
 	if (!request_wiphy) {
-		schedule_delayed_work(&reg_timeout, 0);
+		queue_delayed_work(system_power_efficient_wq,
+				   &reg_timeout, 0);
 		return -ENODEV;
 	}
 
@@ -2354,7 +2356,8 @@  static int reg_set_rd_country_ie(const struct ieee80211_regdomain *rd,
 
 	request_wiphy = wiphy_idx_to_wiphy(country_ie_request->wiphy_idx);
 	if (!request_wiphy) {
-		schedule_delayed_work(&reg_timeout, 0);
+		queue_delayed_work(system_power_efficient_wq,
+				   &reg_timeout, 0);
 		return -ENODEV;
 	}