This patchset was called: "Create sched_select_cpu() and use it for workqueues"
for the first three versions.
Earlier discussions over v3, v2 and v1 can be found here:
https://lkml.org/lkml/2013/3/18/364
http://lists.linaro.org/pipermail/linaro-dev/2012-November/014344.html
http://www.mail-archive.com/[email protected]/msg13342.html
For power saving it is better to schedule work on cpus that aren't idle, as
bringing a cpu/cluster from idle state can be very costly (both performance and
power wise). Earlier we tried to use timer infrastructure to take this decision
but we found out later that scheduler gives even better results and so we should
use scheduler for choosing cpu for scheduling work.
In workqueue subsystem workqueues with flag WQ_UNBOUND are the ones which uses
cpu to select target cpu.
Here we are migrating few users of workqueues to WQ_UNBOUND. These drivers are
found to be very much active on idle or lightly busy system and using WQ_UNBOUND
for these gave impressive results.
Setup:
-----
- ARM Vexpress TC2 - big.LITTLE CPU
- Core 0-1: A15, 2-4: A7
- rootfs: linaro-ubuntu-devel
This patchset has been tested on a big LITTLE system (heterogeneous) but is
useful for all other homogeneous systems as well. During these tests audio was
played in background using aplay.
Results:
-------
Cluster A15 Energy Cluster A7 Energy Total
------------------------- ----------------------- ------
Without this patchset (Energy in Joules):
---------------------------------------------------
0.151162 2.183545 2.334707
0.223730 2.687067 2.910797
0.289687 2.732702 3.022389
0.454198 2.745908 3.200106
0.495552 2.746465 3.242017
Average:
0.322866 2.619137 2.942003
With this patchset (Energy in Joules):
-----------------------------------------------
0.226421 2.283658 2.510079
0.151361 2.236656 2.388017
0.197726 2.249849 2.447575
0.221915 2.229446 2.451361
0.347098 2.257707 2.604805
Average:
0.2289042 2.2514632 2.4803674
Above tests are repeated multiple times and events are tracked using trace-cmd
and analysed using kernelshark. And it was easily noticeable that idle time for
many cpus has increased considerably, which eventually saved some power.
PS: All the earlier Acks we got for drivers are reverted here as patches have
been updated significantly.
V3->V4:
-------
- Dropped changes to kernel/sched directory and hence
sched_select_non_idle_cpu().
- Dropped queue_work_on_any_cpu()
- Created system_freezable_unbound_wq
- Changed all patches accordingly.
V2->V3:
-------
- Dropped changes into core queue_work() API, rather create *_on_any_cpu()
APIs
- Dropped running timers migration patch as that was broken
- Migrated few users of workqueues to use *_on_any_cpu() APIs.
Viresh Kumar (4):
workqueue: Add system wide system_freezable_unbound_wq
PHYLIB: queue work on unbound wq
block: queue work on unbound wq
fbcon: queue work on unbound wq
block/blk-core.c | 3 ++-
block/blk-ioc.c | 2 +-
block/genhd.c | 10 ++++++----
drivers/net/phy/phy.c | 9 +++++----
drivers/video/console/fbcon.c | 2 +-
include/linux/workqueue.h | 4 ++++
kernel/workqueue.c | 7 ++++++-
7 files changed, 25 insertions(+), 12 deletions(-)
--
1.7.12.rc2.18.g61b472e
This patch adds system wide system_freezable_unbound_wq which will be used by
code that currently uses system_freezable_wq and can be moved to unbound
workqueues.
Signed-off-by: Viresh Kumar <[email protected]>
---
include/linux/workqueue.h | 4 ++++
kernel/workqueue.c | 7 ++++++-
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 835d12b..ab7597b 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -325,11 +325,15 @@ enum {
*
* system_freezable_wq is equivalent to system_wq except that it's
* freezable.
+ *
+ * system_freezable_unbound_wq is equivalent to system_unbound_wq except that
+ * it's freezable.
*/
extern struct workqueue_struct *system_wq;
extern struct workqueue_struct *system_long_wq;
extern struct workqueue_struct *system_unbound_wq;
extern struct workqueue_struct *system_freezable_wq;
+extern struct workqueue_struct *system_freezable_unbound_wq;
static inline struct workqueue_struct * __deprecated __system_nrt_wq(void)
{
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index df2f6c4..771a5cc 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -281,6 +281,8 @@ struct workqueue_struct *system_unbound_wq __read_mostly;
EXPORT_SYMBOL_GPL(system_unbound_wq);
struct workqueue_struct *system_freezable_wq __read_mostly;
EXPORT_SYMBOL_GPL(system_freezable_wq);
+struct workqueue_struct *system_freezable_unbound_wq __read_mostly;
+EXPORT_SYMBOL_GPL(system_freezable_unbound_wq);
static int worker_thread(void *__worker);
static void copy_workqueue_attrs(struct workqueue_attrs *to,
@@ -4467,8 +4469,11 @@ static int __init init_workqueues(void)
WQ_UNBOUND_MAX_ACTIVE);
system_freezable_wq = alloc_workqueue("events_freezable",
WQ_FREEZABLE, 0);
+ system_freezable_unbound_wq = alloc_workqueue("events_unbound_freezable",
+ WQ_FREEZABLE | WQ_UNBOUND, 0);
BUG_ON(!system_wq || !system_highpri_wq || !system_long_wq ||
- !system_unbound_wq || !system_freezable_wq);
+ !system_unbound_wq || !system_freezable_wq ||
+ !system_freezable_unbound_wq);
return 0;
}
early_initcall(init_workqueues);
--
1.7.12.rc2.18.g61b472e
Phylib uses workqueues for multiple purposes. There is no real dependency of
scheduling these on the cpu which scheduled them.
On a idle system, it is observed that and idle cpu wakes up many times just to
service this work. It would be better if we can schedule it on a cpu which the
scheduler believes to be the most appropriate one.
This patch replaces system_wq with system_unbound_wq for PHYLIB.
Cc: David S. Miller <[email protected]>
Cc: [email protected]
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/net/phy/phy.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index c14f147..b2fe180 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -439,7 +439,7 @@ void phy_start_machine(struct phy_device *phydev,
{
phydev->adjust_state = handler;
- schedule_delayed_work(&phydev->state_queue, HZ);
+ queue_delayed_work(system_unbound_wq, &phydev->state_queue, HZ);
}
/**
@@ -500,7 +500,7 @@ static irqreturn_t phy_interrupt(int irq, void *phy_dat)
disable_irq_nosync(irq);
atomic_inc(&phydev->irq_disable);
- schedule_work(&phydev->phy_queue);
+ queue_work(system_unbound_wq, &phydev->phy_queue);
return IRQ_HANDLED;
}
@@ -655,7 +655,7 @@ static void phy_change(struct work_struct *work)
/* reschedule state queue work to run as soon as possible */
cancel_delayed_work_sync(&phydev->state_queue);
- schedule_delayed_work(&phydev->state_queue, 0);
+ queue_delayed_work(system_unbound_wq, &phydev->state_queue, 0);
return;
@@ -918,7 +918,8 @@ void phy_state_machine(struct work_struct *work)
if (err < 0)
phy_error(phydev);
- schedule_delayed_work(&phydev->state_queue, PHY_STATE_TIME * HZ);
+ queue_delayed_work(system_unbound_wq, &phydev->state_queue,
+ PHY_STATE_TIME * HZ);
}
static inline void mmd_phy_indirect(struct mii_bus *bus, int prtad, int devad,
--
1.7.12.rc2.18.g61b472e
Block layer uses workqueues for multiple purposes. There is no real dependency
of scheduling these on the cpu which scheduled them.
On a idle system, it is observed that and idle cpu wakes up many times just to
service this work. It would be better if we can schedule it on a cpu which the
scheduler believes to be the most appropriate one.
This patch replaces normal workqueues with UNBOUND versions.
Cc: Jens Axboe <[email protected]>
Signed-off-by: Viresh Kumar <[email protected]>
---
block/blk-core.c | 3 ++-
block/blk-ioc.c | 2 +-
block/genhd.c | 10 ++++++----
3 files changed, 9 insertions(+), 6 deletions(-)
diff --git a/block/blk-core.c b/block/blk-core.c
index 492242f..91cd486 100644
--- a/block/blk-core.c
+++ b/block/blk-core.c
@@ -3186,7 +3186,8 @@ int __init blk_dev_init(void)
/* used for unplugging and affects IO latency/throughput - HIGHPRI */
kblockd_workqueue = alloc_workqueue("kblockd",
- WQ_MEM_RECLAIM | WQ_HIGHPRI, 0);
+ WQ_MEM_RECLAIM | WQ_HIGHPRI |
+ WQ_UNBOUND, 0);
if (!kblockd_workqueue)
panic("Failed to create kblockd\n");
diff --git a/block/blk-ioc.c b/block/blk-ioc.c
index 9c4bb82..5dd576d 100644
--- a/block/blk-ioc.c
+++ b/block/blk-ioc.c
@@ -144,7 +144,7 @@ void put_io_context(struct io_context *ioc)
if (atomic_long_dec_and_test(&ioc->refcount)) {
spin_lock_irqsave(&ioc->lock, flags);
if (!hlist_empty(&ioc->icq_list))
- schedule_work(&ioc->release_work);
+ queue_work(system_unbound_wq, &ioc->release_work);
else
free_ioc = true;
spin_unlock_irqrestore(&ioc->lock, flags);
diff --git a/block/genhd.c b/block/genhd.c
index a1ed52a..0f4470a 100644
--- a/block/genhd.c
+++ b/block/genhd.c
@@ -1488,9 +1488,10 @@ static void __disk_unblock_events(struct gendisk *disk, bool check_now)
intv = disk_events_poll_jiffies(disk);
set_timer_slack(&ev->dwork.timer, intv / 4);
if (check_now)
- queue_delayed_work(system_freezable_wq, &ev->dwork, 0);
+ queue_delayed_work(system_freezable_unbound_wq, &ev->dwork, 0);
else if (intv)
- queue_delayed_work(system_freezable_wq, &ev->dwork, intv);
+ queue_delayed_work(system_freezable_unbound_wq, &ev->dwork,
+ intv);
out_unlock:
spin_unlock_irqrestore(&ev->lock, flags);
}
@@ -1533,7 +1534,7 @@ void disk_flush_events(struct gendisk *disk, unsigned int mask)
spin_lock_irq(&ev->lock);
ev->clearing |= mask;
if (!ev->block)
- mod_delayed_work(system_freezable_wq, &ev->dwork, 0);
+ mod_delayed_work(system_freezable_unbound_wq, &ev->dwork, 0);
spin_unlock_irq(&ev->lock);
}
@@ -1626,7 +1627,8 @@ static void disk_check_events(struct disk_events *ev,
intv = disk_events_poll_jiffies(disk);
if (!ev->block && intv)
- queue_delayed_work(system_freezable_wq, &ev->dwork, intv);
+ queue_delayed_work(system_freezable_unbound_wq, &ev->dwork,
+ intv);
spin_unlock_irq(&ev->lock);
--
1.7.12.rc2.18.g61b472e
fbcon uses workqueues and it has no real dependency of scheduling these on the
cpu which scheduled them.
On a idle system, it is observed that and idle cpu wakes up many times just to
service this work. It would be better if we can schedule it on a cpu which the
scheduler believes to be the most appropriate one.
This patch replaces system_wq with system_unbound_wq.
Cc: Dave Airlie <[email protected]>
Cc: [email protected]
Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/video/console/fbcon.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/console/fbcon.c b/drivers/video/console/fbcon.c
index 3cd6759..6c1f7c3 100644
--- a/drivers/video/console/fbcon.c
+++ b/drivers/video/console/fbcon.c
@@ -404,7 +404,7 @@ static void cursor_timer_handler(unsigned long dev_addr)
struct fb_info *info = (struct fb_info *) dev_addr;
struct fbcon_ops *ops = info->fbcon_par;
- schedule_work(&info->queue);
+ queue_work(system_unbound_wq, &info->queue);
mod_timer(&ops->cursor_timer, jiffies + HZ/5);
}
--
1.7.12.rc2.18.g61b472e
From: Viresh Kumar <[email protected]>
Date: Sun, 31 Mar 2013 20:01:45 +0530
> Phylib uses workqueues for multiple purposes. There is no real dependency of
> scheduling these on the cpu which scheduled them.
>
> On a idle system, it is observed that and idle cpu wakes up many times just to
> service this work. It would be better if we can schedule it on a cpu which the
> scheduler believes to be the most appropriate one.
>
> This patch replaces system_wq with system_unbound_wq for PHYLIB.
>
> Cc: David S. Miller <[email protected]>
> Cc: [email protected]
> Signed-off-by: Viresh Kumar <[email protected]>
Acked-by: David S. Miller <[email protected]>
Hello, Viresh.
On Sun, Mar 31, 2013 at 08:01:46PM +0530, Viresh Kumar wrote:
> Block layer uses workqueues for multiple purposes. There is no real dependency
> of scheduling these on the cpu which scheduled them.
>
> On a idle system, it is observed that and idle cpu wakes up many times just to
> service this work. It would be better if we can schedule it on a cpu which the
> scheduler believes to be the most appropriate one.
>
> This patch replaces normal workqueues with UNBOUND versions.
Hmm.... so, we really don't want to unconditionally convert workqueues
to unbound. Especially not kblockd. On configurations with multiple
high iops devices with IRQ routing, having request completion runinng
on the same CPU has significant performance advantages. We can't
simply switch it to an unbound wokrqueue because it saves power on
small arm configuration.
Plus, I'd much prefer to be clearly marking the workqueues which would
contribute to powersaving when converted to unbound at least until we
can come up with a no-compromise solution which doesn't need to trade
off between cache locality and powersaving.
So, let's please introduce a new flag to mark these workqueues, say,
WQ_UNBOUND_FOR_POWER_SAVE or whatever (please come up with a better
name) and provide a compile time switch with boot time override.
Thanks.
--
tejun
On Sun, Mar 31, 2013 at 8:01 PM, Viresh Kumar <[email protected]> wrote:
> This patch adds system wide system_freezable_unbound_wq which will be used by
> code that currently uses system_freezable_wq and can be moved to unbound
> workqueues.
_Why_ do i need this change?
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> include/linux/workqueue.h | 4 ++++
> kernel/workqueue.c | 7 ++++++-
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index 835d12b..ab7597b 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -325,11 +325,15 @@ enum {
> *
> * system_freezable_wq is equivalent to system_wq except that it's
> * freezable.
> + *
> + * system_freezable_unbound_wq is equivalent to system_unbound_wq except that
> + * it's freezable.
> */
> extern struct workqueue_struct *system_wq;
> extern struct workqueue_struct *system_long_wq;
> extern struct workqueue_struct *system_unbound_wq;
> extern struct workqueue_struct *system_freezable_wq;
> +extern struct workqueue_struct *system_freezable_unbound_wq;
>
> static inline struct workqueue_struct * __deprecated __system_nrt_wq(void)
> {
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index df2f6c4..771a5cc 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -281,6 +281,8 @@ struct workqueue_struct *system_unbound_wq __read_mostly;
> EXPORT_SYMBOL_GPL(system_unbound_wq);
> struct workqueue_struct *system_freezable_wq __read_mostly;
> EXPORT_SYMBOL_GPL(system_freezable_wq);
> +struct workqueue_struct *system_freezable_unbound_wq __read_mostly;
> +EXPORT_SYMBOL_GPL(system_freezable_unbound_wq);
>
> static int worker_thread(void *__worker);
> static void copy_workqueue_attrs(struct workqueue_attrs *to,
> @@ -4467,8 +4469,11 @@ static int __init init_workqueues(void)
> WQ_UNBOUND_MAX_ACTIVE);
> system_freezable_wq = alloc_workqueue("events_freezable",
> WQ_FREEZABLE, 0);
> + system_freezable_unbound_wq = alloc_workqueue("events_unbound_freezable",
> + WQ_FREEZABLE | WQ_UNBOUND, 0);
> BUG_ON(!system_wq || !system_highpri_wq || !system_long_wq ||
> - !system_unbound_wq || !system_freezable_wq);
> + !system_unbound_wq || !system_freezable_wq ||
> + !system_freezable_unbound_wq);
> return 0;
> }
> early_initcall(init_workqueues);
> --
> 1.7.12.rc2.18.g61b472e
>
>
> _______________________________________________
> linaro-kernel mailing list
> [email protected]
> http://lists.linaro.org/mailman/listinfo/linaro-kernel
On 1 April 2013 10:50, Amit Kucheria <[email protected]> wrote:
> On Sun, Mar 31, 2013 at 8:01 PM, Viresh Kumar <[email protected]> wrote:
>> This patch adds system wide system_freezable_unbound_wq which will be used by
>> code that currently uses system_freezable_wq and can be moved to unbound
>> workqueues.
>
> _Why_ do i need this change?
Block layer uses system_freezable_wq for some work and to migrate them to a
UNBOUND wq it was required. Sorry if it is yet not clear.
Hi Tejun,
On 31 March 2013 23:49, Tejun Heo <[email protected]> wrote:
> So, let's please introduce a new flag to mark these workqueues, say,
> WQ_UNBOUND_FOR_POWER_SAVE or whatever (please come up with a better
> name) and provide a compile time switch with boot time override.
Just wanted to make this clear before writing it:
You want me to do something like (With better names):
int wq_unbound_for_power_save_enabled = 0;
#ifdef CONFIG_WQ_UNBOUND_FOR_POWER_SAVE
#define WQ_UNBOUND_FOR_POWER_SAVE wq_unbound_for_power_save_enabled
#else
#define WQ_UNBOUND_FOR_POWER_SAVE 0
And provide a call to enable/disable wq_unbound_for_power_save_enabled ??
Hello, Viresh.
Sorry about the delay. Lost this one somehow.
On Mon, Apr 01, 2013 at 12:01:05PM +0530, Viresh Kumar wrote:
> Just wanted to make this clear before writing it:
>
> You want me to do something like (With better names):
>
> int wq_unbound_for_power_save_enabled = 0;
>
> #ifdef CONFIG_WQ_UNBOUND_FOR_POWER_SAVE
> #define WQ_UNBOUND_FOR_POWER_SAVE wq_unbound_for_power_save_enabled
> #else
> #define WQ_UNBOUND_FOR_POWER_SAVE 0
>
> And provide a call to enable/disable wq_unbound_for_power_save_enabled ??
Not a call, probably a module_param() so that it can be switched
on/off during boot. You can make the param writable so that it can be
flipped run-time but updating existing workqueues would be non-trivial
and I don't think it's gonna be worthwhile.
Thanks!
--
tejun
On 4 April 2013 03:24, Tejun Heo <[email protected]> wrote:
> Not a call, probably a module_param() so that it can be switched
> on/off during boot. You can make the param writable so that it can be
> flipped run-time but updating existing workqueues would be non-trivial
> and I don't think it's gonna be worthwhile.
module_param()?? We can't compile kernel/workqueue.c as a module and
hence i went with #define + a variable with functions to set/reset it...
I am not looking to update all existing workqueues to use it but workqueues
which are affecting power for us... And if there are some very very
performance critical ones, then we must better use queue_work_on() for
them to make it more clear.
On Fri, Apr 5, 2013 at 2:47 AM, Viresh Kumar <[email protected]> wrote:
> module_param()?? We can't compile kernel/workqueue.c as a module and
> hence i went with #define + a variable with functions to set/reset it...
module_param works fine for kernel proper.
--
tejun
On 5 April 2013 17:52, Tejun Heo <[email protected]> wrote:
> On Fri, Apr 5, 2013 at 2:47 AM, Viresh Kumar <[email protected]> wrote:
>> module_param()?? We can't compile kernel/workqueue.c as a module and
>> hence i went with #define + a variable with functions to set/reset it...
>
> module_param works fine for kernel proper.
Got it!! Btw, to which Kconfig should i add WQ config option?
On Thu, Apr 4, 2013 at 3:24 AM, Tejun Heo <[email protected]> wrote:
> Hello, Viresh.
>
> Sorry about the delay. Lost this one somehow.
>
> On Mon, Apr 01, 2013 at 12:01:05PM +0530, Viresh Kumar wrote:
>> Just wanted to make this clear before writing it:
>>
>> You want me to do something like (With better names):
>>
>> int wq_unbound_for_power_save_enabled = 0;
>>
>> #ifdef CONFIG_WQ_UNBOUND_FOR_POWER_SAVE
>> #define WQ_UNBOUND_FOR_POWER_SAVE wq_unbound_for_power_save_enabled
>> #else
>> #define WQ_UNBOUND_FOR_POWER_SAVE 0
>>
>> And provide a call to enable/disable wq_unbound_for_power_save_enabled ??
>
> Not a call, probably a module_param() so that it can be switched
> on/off during boot. You can make the param writable so that it can be
> flipped run-time but updating existing workqueues would be non-trivial
> and I don't think it's gonna be worthwhile.
>
> Thanks!
Does it make sense to collect this sort of power optimisation under a
CONFIG_PM sub-item, say, CONFIG_PM_OPTIMISATIONS?
For people tuning for power, it could be single place to go enable
stuff and for people looking for performance regressions it would be a
single place to make sure nothing is enabled.
/Amit
On 4 April 2013 03:24, Tejun Heo <[email protected]> wrote:
> Not a call, probably a module_param() so that it can be switched
> on/off during boot. You can make the param writable so that it can be
> flipped run-time but updating existing workqueues would be non-trivial
> and I don't think it's gonna be worthwhile.
It might be better if we make it read only and so don't try ugly stuff on the
fly. Might be good for testing but not otherwise..
On Sun, Mar 31 2013, Tejun Heo wrote:
> Hello, Viresh.
>
> On Sun, Mar 31, 2013 at 08:01:46PM +0530, Viresh Kumar wrote:
> > Block layer uses workqueues for multiple purposes. There is no real dependency
> > of scheduling these on the cpu which scheduled them.
> >
> > On a idle system, it is observed that and idle cpu wakes up many times just to
> > service this work. It would be better if we can schedule it on a cpu which the
> > scheduler believes to be the most appropriate one.
> >
> > This patch replaces normal workqueues with UNBOUND versions.
>
> Hmm.... so, we really don't want to unconditionally convert workqueues
> to unbound. Especially not kblockd. On configurations with multiple
> high iops devices with IRQ routing, having request completion runinng
> on the same CPU has significant performance advantages. We can't
> simply switch it to an unbound wokrqueue because it saves power on
> small arm configuration.
I had the same complaint, when it was posted originally...
> Plus, I'd much prefer to be clearly marking the workqueues which would
> contribute to powersaving when converted to unbound at least until we
> can come up with a no-compromise solution which doesn't need to trade
> off between cache locality and powersaving.
>
> So, let's please introduce a new flag to mark these workqueues, say,
> WQ_UNBOUND_FOR_POWER_SAVE or whatever (please come up with a better
> name) and provide a compile time switch with boot time override.
And lets please have it off by default. The specialized configs / setups
can turn it on, but we should default to better performance.
--
Jens Axboe
On 8 April 2013 16:43, Jens Axboe <[email protected]> wrote:
> And lets please have it off by default. The specialized configs / setups
> can turn it on, but we should default to better performance.
Yes, we will.
On Mon, 2013-04-01 at 10:55 +0530, Viresh Kumar wrote:
> On 1 April 2013 10:50, Amit Kucheria <[email protected]> wrote:
> > On Sun, Mar 31, 2013 at 8:01 PM, Viresh Kumar <[email protected]> wrote:
> >> This patch adds system wide system_freezable_unbound_wq which will be used by
> >> code that currently uses system_freezable_wq and can be moved to unbound
> >> workqueues.
> >
> > _Why_ do i need this change?
>
> Block layer uses system_freezable_wq for some work and to migrate them to a
> UNBOUND wq it was required. Sorry if it is yet not clear.
Looks like Amit was left off the patch 0, and missed the power savings
explanation that you did there. Perhaps you should have included that in
each patch change log.
-- Steve
On 8 April 2013 22:29, Steven Rostedt <[email protected]> wrote:
> Looks like Amit was left off the patch 0, and missed the power savings
> explanation that you did there. Perhaps you should have included that in
> each patch change log.
He isn't supposed to miss any of my emails, he is my manager :)
Don't know if change log of all patches require those details..
On 31 March 2013 23:49, Tejun Heo <[email protected]> wrote:
> So, let's please introduce a new flag to mark these workqueues, say,
> WQ_UNBOUND_FOR_POWER_SAVE or whatever (please come up with a better
> name) and provide a compile time switch with boot time override.
Hi Tejun,
I have written a patch to get this done and want to get that a some review
before touching other drivers and spamming LKML with other patches :)
Here it is:
From: Viresh Kumar <[email protected]>
Date: Mon, 8 Apr 2013 16:45:40 +0530
Subject: [PATCH] workqueues: Introduce new flag WQ_POWER_EFFICIENT for power
oriented workqueues
Workqueues can be performance or power oriented. For performance we may want to
keep them running on a single cpu, so that it remains cache hot. For power we
can give scheduler the liberty to choose target cpu for running work handler.
Later one (Power oriented WQ) can be achieved if the workqueue is allocated with
WQ_UNBOUND flag. To make this compile time configurable with boot time override
this patch adds in another flag WQ_POWER_EFFICIENT. This will be converted to
WQ_UNBOUND (on wq allocation) if CONFIG_WQ_POWER_EFFICIENT is enabled and
bootargs contain workqueue.power_efficient=1. It is unused otherwise and normal
behavior of WQ without this flag is expected.
CONFIG option is currently added under kernel/power/Kconfig and is looking for
relevant Kconfig file.
Signed-off-by: Viresh Kumar <[email protected]>
---
Documentation/kernel-parameters.txt | 15 +++++++++++++++
include/linux/workqueue.h | 3 +++
kernel/power/Kconfig | 18 ++++++++++++++++++
kernel/workqueue.c | 14 ++++++++++++++
4 files changed, 50 insertions(+)
diff --git a/Documentation/kernel-parameters.txt
b/Documentation/kernel-parameters.txt
index 12c42a5..210e5fc 100644
--- a/Documentation/kernel-parameters.txt
+++ b/Documentation/kernel-parameters.txt
@@ -3249,6 +3249,21 @@ bytes respectively. Such letter suffixes can
also be entirely omitted.
that this also can be controlled per-workqueue for
workqueues visible under /sys/bus/workqueue/.
+ workqueue.power_efficient
+ Workqueues can be performance or power oriented. For
+ performance we may want to keep them running on a single
+ cpu, so that it remains cache hot. For power we can give
+ scheduler the liberty to choose target cpu for running
+ work handler.
+
+ Later one (Power oriented WQ) can be achieved if the
+ workqueue is allocated with WQ_UNBOUND flag. Enabling
+ power_efficient boot param will convert
+ WQ_POWER_EFFICIENT flag to WQ_UNBOUND on wq allocation.
+ This requires CONFIG_WQ_POWER_EFFICIENT to be enabled.
+ WQ_POWER_EFFICIENT is unused if power_efficient is not
+ set in boot params.
+
x2apic_phys [X86-64,APIC] Use x2apic physical mode instead of
default x2apic cluster mode on platforms
supporting x2apic.
diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
index 1a53816..168b5be 100644
--- a/include/linux/workqueue.h
+++ b/include/linux/workqueue.h
@@ -299,6 +299,9 @@ enum {
WQ_HIGHPRI = 1 << 4, /* high priority */
WQ_CPU_INTENSIVE = 1 << 5, /* cpu instensive workqueue */
WQ_SYSFS = 1 << 6, /* visible in sysfs, see wq_sysfs_register() */
+ WQ_POWER_EFFICIENT = 1 << 7, /* WQ_UNBOUND, for power
+ * saving, if wq_power_efficient is
+ * enabled. Unused otherwise. */
__WQ_DRAINING = 1 << 16, /* internal: workqueue is draining */
__WQ_ORDERED = 1 << 17, /* internal: workqueue is ordered */
diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
index 5dfdc9e..8d62400 100644
--- a/kernel/power/Kconfig
+++ b/kernel/power/Kconfig
@@ -263,6 +263,24 @@ config PM_GENERIC_DOMAINS
bool
depends on PM
+config WQ_POWER_EFFICIENT
+ bool "Workqueue allocated as UNBOUND for power efficiency"
+ depends on PM
+ help
+ Workqueues can be performance or power oriented. For performance we
+ may want to keep them running on a single cpu, so that it remains
+ cache hot. For power we can give scheduler the liberty to choose
+ target cpu for running work handler.
+
+ Later one (Power oriented WQ) can be achieved if the workqueue is
+ allocated with WQ_UNBOUND flag. Enabling power_efficient boot param
+ will convert WQ_POWER_EFFICIENT flag to WQ_UNBOUND on wq allocation.
+ This requires CONFIG_WQ_POWER_EFFICIENT to be enabled.
+ WQ_POWER_EFFICIENT is unused if power_efficient is not set in boot
+ params.
+
+ If in doubt, say N.
+
config PM_GENERIC_DOMAINS_SLEEP
def_bool y
depends on PM_SLEEP && PM_GENERIC_DOMAINS
diff --git a/kernel/workqueue.c b/kernel/workqueue.c
index 1228fd7..590e333 100644
--- a/kernel/workqueue.c
+++ b/kernel/workqueue.c
@@ -271,6 +271,11 @@ static cpumask_var_t *wq_numa_possible_cpumask;
static bool wq_disable_numa;
module_param_named(disable_numa, wq_disable_numa, bool, 0444);
+#ifdef CONFIG_WQ_POWER_EFFICIENT
+static bool wq_power_efficient = 0;
+module_param_named(power_efficient, wq_power_efficient, bool, 0444);
+#endif
+
static bool wq_numa_enabled; /* unbound NUMA affinity enabled */
/* buf for wq_update_unbound_numa_attrs(), protected by CPU hotplug
exclusion */
@@ -4085,6 +4090,15 @@ struct workqueue_struct
*__alloc_workqueue_key(const char *fmt,
struct workqueue_struct *wq;
struct pool_workqueue *pwq;
+ if (flags & WQ_POWER_EFFICIENT) {
+ flags &= ~WQ_POWER_EFFICIENT;
+
+#ifdef CONFIG_WQ_POWER_EFFICIENT
+ if (wq_power_efficient)
+ flags |= WQ_UNBOUND;
+#endif
+ }
+
/* allocate wq and format name */
if (flags & WQ_UNBOUND)
tbl_size = wq_numa_tbl_len * sizeof(wq->numa_pwq_tbl[0]);
On Tue, Apr 9, 2013 at 1:00 PM, Viresh Kumar <[email protected]> wrote:
> On 31 March 2013 23:49, Tejun Heo <[email protected]> wrote:
>> So, let's please introduce a new flag to mark these workqueues, say,
>> WQ_UNBOUND_FOR_POWER_SAVE or whatever (please come up with a better
>> name) and provide a compile time switch with boot time override.
>
> Hi Tejun,
>
> I have written a patch to get this done and want to get that a some review
> before touching other drivers and spamming LKML with other patches :)
>
> Here it is:
>
> From: Viresh Kumar <[email protected]>
> Date: Mon, 8 Apr 2013 16:45:40 +0530
> Subject: [PATCH] workqueues: Introduce new flag WQ_POWER_EFFICIENT for power
> oriented workqueues
>
> Workqueues can be performance or power oriented. For performance we may want to
> keep them running on a single cpu, so that it remains cache hot. For power we
> can give scheduler the liberty to choose target cpu for running work handler.
>
> Later one (Power oriented WQ) can be achieved if the workqueue is allocated with
> WQ_UNBOUND flag. To make this compile time configurable with boot time override
> this patch adds in another flag WQ_POWER_EFFICIENT. This will be converted to
> WQ_UNBOUND (on wq allocation) if CONFIG_WQ_POWER_EFFICIENT is enabled and
> bootargs contain workqueue.power_efficient=1. It is unused otherwise and normal
> behavior of WQ without this flag is expected.
>
> CONFIG option is currently added under kernel/power/Kconfig and is looking for
> relevant Kconfig file.
>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> Documentation/kernel-parameters.txt | 15 +++++++++++++++
> include/linux/workqueue.h | 3 +++
> kernel/power/Kconfig | 18 ++++++++++++++++++
> kernel/workqueue.c | 14 ++++++++++++++
> 4 files changed, 50 insertions(+)
>
> diff --git a/Documentation/kernel-parameters.txt
> b/Documentation/kernel-parameters.txt
> index 12c42a5..210e5fc 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -3249,6 +3249,21 @@ bytes respectively. Such letter suffixes can
> also be entirely omitted.
> that this also can be controlled per-workqueue for
> workqueues visible under /sys/bus/workqueue/.
>
> + workqueue.power_efficient
> + Workqueues can be performance or power oriented. For
> + performance we may want to keep them running on a single
> + cpu, so that it remains cache hot. For power we can give
> + scheduler the liberty to choose target cpu for running
> + work handler.
> +
> + Later one (Power oriented WQ) can be achieved if the
> + workqueue is allocated with WQ_UNBOUND flag. Enabling
> + power_efficient boot param will convert
> + WQ_POWER_EFFICIENT flag to WQ_UNBOUND on wq allocation.
> + This requires CONFIG_WQ_POWER_EFFICIENT to be enabled.
> + WQ_POWER_EFFICIENT is unused if power_efficient is not
> + set in boot params.
> +
> x2apic_phys [X86-64,APIC] Use x2apic physical mode instead of
> default x2apic cluster mode on platforms
> supporting x2apic.
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index 1a53816..168b5be 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -299,6 +299,9 @@ enum {
> WQ_HIGHPRI = 1 << 4, /* high priority */
> WQ_CPU_INTENSIVE = 1 << 5, /* cpu instensive workqueue */
> WQ_SYSFS = 1 << 6, /* visible in sysfs, see wq_sysfs_register() */
> + WQ_POWER_EFFICIENT = 1 << 7, /* WQ_UNBOUND, for power
> + * saving, if wq_power_efficient is
> + * enabled. Unused otherwise. */
>
> __WQ_DRAINING = 1 << 16, /* internal: workqueue is draining */
> __WQ_ORDERED = 1 << 17, /* internal: workqueue is ordered */
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 5dfdc9e..8d62400 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -263,6 +263,24 @@ config PM_GENERIC_DOMAINS
> bool
> depends on PM
>
> +config WQ_POWER_EFFICIENT
> + bool "Workqueue allocated as UNBOUND for power efficiency"
> + depends on PM
Should default to N so that existing configs don't get prompted for a choice.
> + help
> + Workqueues can be performance or power oriented. For performance we
> + may want to keep them running on a single cpu, so that it remains
> + cache hot. For power we can give scheduler the liberty to choose
> + target cpu for running work handler.
> +
> + Later one (Power oriented WQ) can be achieved if the workqueue is
> + allocated with WQ_UNBOUND flag. Enabling power_efficient boot param
> + will convert WQ_POWER_EFFICIENT flag to WQ_UNBOUND on wq allocation.
> + This requires CONFIG_WQ_POWER_EFFICIENT to be enabled.
> + WQ_POWER_EFFICIENT is unused if power_efficient is not set in boot
> + params.
> +
> + If in doubt, say N.
> +
> config PM_GENERIC_DOMAINS_SLEEP
> def_bool y
> depends on PM_SLEEP && PM_GENERIC_DOMAINS
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 1228fd7..590e333 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -271,6 +271,11 @@ static cpumask_var_t *wq_numa_possible_cpumask;
> static bool wq_disable_numa;
> module_param_named(disable_numa, wq_disable_numa, bool, 0444);
>
> +#ifdef CONFIG_WQ_POWER_EFFICIENT
> +static bool wq_power_efficient = 0;
1?
> +module_param_named(power_efficient, wq_power_efficient, bool, 0444);
> +#endif
> +
> static bool wq_numa_enabled; /* unbound NUMA affinity enabled */
>
> /* buf for wq_update_unbound_numa_attrs(), protected by CPU hotplug
> exclusion */
> @@ -4085,6 +4090,15 @@ struct workqueue_struct
> *__alloc_workqueue_key(const char *fmt,
> struct workqueue_struct *wq;
> struct pool_workqueue *pwq;
>
> + if (flags & WQ_POWER_EFFICIENT) {
> + flags &= ~WQ_POWER_EFFICIENT;
> +
> +#ifdef CONFIG_WQ_POWER_EFFICIENT
> + if (wq_power_efficient)
> + flags |= WQ_UNBOUND;
> +#endif
> + }
> +
> /* allocate wq and format name */
> if (flags & WQ_UNBOUND)
> tbl_size = wq_numa_tbl_len * sizeof(wq->numa_pwq_tbl[0]);
>
> _______________________________________________
> linaro-kernel mailing list
> [email protected]
> http://lists.linaro.org/mailman/listinfo/linaro-kernel
On 9 April 2013 14:48, Amit Kucheria <[email protected]> wrote:
> On Tue, Apr 9, 2013 at 1:00 PM, Viresh Kumar <[email protected]> wrote:
>> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
>> +config WQ_POWER_EFFICIENT
>> + bool "Workqueue allocated as UNBOUND for power efficiency"
>> + depends on PM
>
> Should default to N so that existing configs don't get prompted for a choice.
I thought defaults default is N if not present. I can add default n if
it is required.
>> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
>> +static bool wq_power_efficient = 0;
>
> 1?
It should be disabled by default even if CONFIG_WQ_POWER_EFFICIENT is
selected and so initialized it with zero.. Though i don't need to
initialize it as
default is 0.
On Tue, Apr 9, 2013 at 1:00 PM, Viresh Kumar <[email protected]> wrote:
> On 31 March 2013 23:49, Tejun Heo <[email protected]> wrote:
>> So, let's please introduce a new flag to mark these workqueues, say,
>> WQ_UNBOUND_FOR_POWER_SAVE or whatever (please come up with a better
>> name) and provide a compile time switch with boot time override.
>
> Hi Tejun,
>
> I have written a patch to get this done and want to get that a some review
> before touching other drivers and spamming LKML with other patches :)
>
> Here it is:
>
> From: Viresh Kumar <[email protected]>
> Date: Mon, 8 Apr 2013 16:45:40 +0530
> Subject: [PATCH] workqueues: Introduce new flag WQ_POWER_EFFICIENT for power
> oriented workqueues
>
> Workqueues can be performance or power oriented. For performance we may want to
> keep them running on a single cpu, so that it remains cache hot. For power we
> can give scheduler the liberty to choose target cpu for running work handler.
>
> Later one (Power oriented WQ) can be achieved if the workqueue is allocated with
Rephrase as "Power-efficient workqueues can be achieved..."
> WQ_UNBOUND flag. To make this compile time configurable with boot time override
> this patch adds in another flag WQ_POWER_EFFICIENT. This will be converted to
> WQ_UNBOUND (on wq allocation) if CONFIG_WQ_POWER_EFFICIENT is enabled and
> bootargs contain workqueue.power_efficient=1. It is unused otherwise and normal
> behavior of WQ without this flag is expected.
Addition of a new flag seems a bit excessive IMHO. Why can't we just
set the UNBOUND flag when the wq_power_efficient module param is set?
It would make the patch a lot simpler too.
> CONFIG option is currently added under kernel/power/Kconfig and is looking for
> relevant Kconfig file.
>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> Documentation/kernel-parameters.txt | 15 +++++++++++++++
> include/linux/workqueue.h | 3 +++
> kernel/power/Kconfig | 18 ++++++++++++++++++
> kernel/workqueue.c | 14 ++++++++++++++
> 4 files changed, 50 insertions(+)
>
> diff --git a/Documentation/kernel-parameters.txt
> b/Documentation/kernel-parameters.txt
> index 12c42a5..210e5fc 100644
> --- a/Documentation/kernel-parameters.txt
> +++ b/Documentation/kernel-parameters.txt
> @@ -3249,6 +3249,21 @@ bytes respectively. Such letter suffixes can
> also be entirely omitted.
> that this also can be controlled per-workqueue for
> workqueues visible under /sys/bus/workqueue/.
>
> + workqueue.power_efficient
> + Workqueues can be performance or power oriented. For
> + performance we may want to keep them running on a single
> + cpu, so that it remains cache hot. For power we can give
> + scheduler the liberty to choose target cpu for running
> + work handler.
> +
> + Later one (Power oriented WQ) can be achieved if the
> + workqueue is allocated with WQ_UNBOUND flag. Enabling
> + power_efficient boot param will convert
> + WQ_POWER_EFFICIENT flag to WQ_UNBOUND on wq allocation.
> + This requires CONFIG_WQ_POWER_EFFICIENT to be enabled.
> + WQ_POWER_EFFICIENT is unused if power_efficient is not
> + set in boot params.
> +
> x2apic_phys [X86-64,APIC] Use x2apic physical mode instead of
> default x2apic cluster mode on platforms
> supporting x2apic.
> diff --git a/include/linux/workqueue.h b/include/linux/workqueue.h
> index 1a53816..168b5be 100644
> --- a/include/linux/workqueue.h
> +++ b/include/linux/workqueue.h
> @@ -299,6 +299,9 @@ enum {
> WQ_HIGHPRI = 1 << 4, /* high priority */
> WQ_CPU_INTENSIVE = 1 << 5, /* cpu instensive workqueue */
> WQ_SYSFS = 1 << 6, /* visible in sysfs, see wq_sysfs_register() */
> + WQ_POWER_EFFICIENT = 1 << 7, /* WQ_UNBOUND, for power
> + * saving, if wq_power_efficient is
> + * enabled. Unused otherwise. */
>
> __WQ_DRAINING = 1 << 16, /* internal: workqueue is draining */
> __WQ_ORDERED = 1 << 17, /* internal: workqueue is ordered */
> diff --git a/kernel/power/Kconfig b/kernel/power/Kconfig
> index 5dfdc9e..8d62400 100644
> --- a/kernel/power/Kconfig
> +++ b/kernel/power/Kconfig
> @@ -263,6 +263,24 @@ config PM_GENERIC_DOMAINS
> bool
> depends on PM
>
> +config WQ_POWER_EFFICIENT
> + bool "Workqueue allocated as UNBOUND for power efficiency"
> + depends on PM
> + help
> + Workqueues can be performance or power oriented. For performance we
> + may want to keep them running on a single cpu, so that it remains
> + cache hot. For power we can give scheduler the liberty to choose
> + target cpu for running work handler.
> +
> + Later one (Power oriented WQ) can be achieved if the workqueue is
> + allocated with WQ_UNBOUND flag. Enabling power_efficient boot param
> + will convert WQ_POWER_EFFICIENT flag to WQ_UNBOUND on wq allocation.
> + This requires CONFIG_WQ_POWER_EFFICIENT to be enabled.
> + WQ_POWER_EFFICIENT is unused if power_efficient is not set in boot
> + params.
> +
> + If in doubt, say N.
> +
> config PM_GENERIC_DOMAINS_SLEEP
> def_bool y
> depends on PM_SLEEP && PM_GENERIC_DOMAINS
> diff --git a/kernel/workqueue.c b/kernel/workqueue.c
> index 1228fd7..590e333 100644
> --- a/kernel/workqueue.c
> +++ b/kernel/workqueue.c
> @@ -271,6 +271,11 @@ static cpumask_var_t *wq_numa_possible_cpumask;
> static bool wq_disable_numa;
> module_param_named(disable_numa, wq_disable_numa, bool, 0444);
>
> +#ifdef CONFIG_WQ_POWER_EFFICIENT
> +static bool wq_power_efficient = 0;
> +module_param_named(power_efficient, wq_power_efficient, bool, 0444);
> +#endif
> +
> static bool wq_numa_enabled; /* unbound NUMA affinity enabled */
>
> /* buf for wq_update_unbound_numa_attrs(), protected by CPU hotplug
> exclusion */
> @@ -4085,6 +4090,15 @@ struct workqueue_struct
> *__alloc_workqueue_key(const char *fmt,
> struct workqueue_struct *wq;
> struct pool_workqueue *pwq;
>
> + if (flags & WQ_POWER_EFFICIENT) {
> + flags &= ~WQ_POWER_EFFICIENT;
> +
> +#ifdef CONFIG_WQ_POWER_EFFICIENT
> + if (wq_power_efficient)
> + flags |= WQ_UNBOUND;
> +#endif
> + }
> +
> /* allocate wq and format name */
> if (flags & WQ_UNBOUND)
> tbl_size = wq_numa_tbl_len * sizeof(wq->numa_pwq_tbl[0]);
>
> _______________________________________________
> linaro-kernel mailing list
> [email protected]
> http://lists.linaro.org/mailman/listinfo/linaro-kernel
On 9 April 2013 15:23, Amit Kucheria <[email protected]> wrote:
> On Tue, Apr 9, 2013 at 1:00 PM, Viresh Kumar <[email protected]> wrote:
>> Later one (Power oriented WQ) can be achieved if the workqueue is allocated with
>
> Rephrase as "Power-efficient workqueues can be achieved..."
Ok.
>> WQ_UNBOUND flag. To make this compile time configurable with boot time override
>> this patch adds in another flag WQ_POWER_EFFICIENT. This will be converted to
>> WQ_UNBOUND (on wq allocation) if CONFIG_WQ_POWER_EFFICIENT is enabled and
>> bootargs contain workqueue.power_efficient=1. It is unused otherwise and normal
>> behavior of WQ without this flag is expected.
>
> Addition of a new flag seems a bit excessive IMHO. Why can't we just
> set the UNBOUND flag when the wq_power_efficient module param is set?
>
> It would make the patch a lot simpler too.
But how will we know if the user of wq wants to save power or not? He must
give some flag which is only used when power saving is enabled. We can't
set WQ_UNBOUND for all wqs.
On Tue, Apr 9, 2013 at 3:25 PM, Viresh Kumar <[email protected]> wrote:
> On 9 April 2013 15:23, Amit Kucheria <[email protected]> wrote:
>> On Tue, Apr 9, 2013 at 1:00 PM, Viresh Kumar <[email protected]> wrote:
>>> Later one (Power oriented WQ) can be achieved if the workqueue is allocated with
>>
>> Rephrase as "Power-efficient workqueues can be achieved..."
>
> Ok.
>
>>> WQ_UNBOUND flag. To make this compile time configurable with boot time override
>>> this patch adds in another flag WQ_POWER_EFFICIENT. This will be converted to
>>> WQ_UNBOUND (on wq allocation) if CONFIG_WQ_POWER_EFFICIENT is enabled and
>>> bootargs contain workqueue.power_efficient=1. It is unused otherwise and normal
>>> behavior of WQ without this flag is expected.
>>
>> Addition of a new flag seems a bit excessive IMHO. Why can't we just
>> set the UNBOUND flag when the wq_power_efficient module param is set?
>>
>> It would make the patch a lot simpler too.
>
> But how will we know if the user of wq wants to save power or not? He must
> give some flag which is only used when power saving is enabled. We can't
> set WQ_UNBOUND for all wqs.
You have the Kconfig option + the module param for that. You don't need a flag.
On 9 April 2013 15:28, Amit Kucheria <[email protected]> wrote:
> On Tue, Apr 9, 2013 at 3:25 PM, Viresh Kumar <[email protected]> wrote:
>> But how will we know if the user of wq wants to save power or not? He must
>> give some flag which is only used when power saving is enabled. We can't
>> set WQ_UNBOUND for all wqs.
>
> You have the Kconfig option + the module param for that. You don't need a flag.
I still don't agree.. Suppose there are three users A, B and C. A wants to use
WQ_UNBOUND when POWER_EFFICIENT Kconfig option and module param is
enabled. And B always want to enable WQ_UNBOUND and C doesn't want to
set WQ_UNBOUND at all..
Now, how will each of these behave differently based on module param settings.
B and C are quite clear but A needs a flag for indicating its usage.
Yo!
On Tue, Apr 09, 2013 at 01:00:59PM +0530, Viresh Kumar wrote:
> + workqueue.power_efficient
> + Workqueues can be performance or power oriented. For
> + performance we may want to keep them running on a single
> + cpu, so that it remains cache hot. For power we can give
> + scheduler the liberty to choose target cpu for running
> + work handler.
> +
> + Later one (Power oriented WQ) can be achieved if the
> + workqueue is allocated with WQ_UNBOUND flag. Enabling
> + power_efficient boot param will convert
> + WQ_POWER_EFFICIENT flag to WQ_UNBOUND on wq allocation.
> + This requires CONFIG_WQ_POWER_EFFICIENT to be enabled.
> + WQ_POWER_EFFICIENT is unused if power_efficient is not
> + set in boot params.
Looks mostly good to me but I think it'd be better if the parameter
name is a bit more specific.
> @@ -299,6 +299,9 @@ enum {
> WQ_HIGHPRI = 1 << 4, /* high priority */
> WQ_CPU_INTENSIVE = 1 << 5, /* cpu instensive workqueue */
> WQ_SYSFS = 1 << 6, /* visible in sysfs, see wq_sysfs_register() */
> + WQ_POWER_EFFICIENT = 1 << 7, /* WQ_UNBOUND, for power
> + * saving, if wq_power_efficient is
> + * enabled. Unused otherwise. */
Ditto for the flag name. It's not a requirement tho. If you can
think of something which is a bit more specific to what it does while
not being unreasonably long, great. If not, we'll live.
> @@ -271,6 +271,11 @@ static cpumask_var_t *wq_numa_possible_cpumask;
> static bool wq_disable_numa;
> module_param_named(disable_numa, wq_disable_numa, bool, 0444);
>
> +#ifdef CONFIG_WQ_POWER_EFFICIENT
> +static bool wq_power_efficient = 0;
> +module_param_named(power_efficient, wq_power_efficient, bool, 0444);
> +#endif
I don't think we need to make the whole thing configurable. Turning
it off isn't gonna save much - my gut tells me it's gonna be four
instructions. :)
What I meant was that the default value for the parameter would
probably be need to be configurable so that mobile people don't have
to include the kernel param all the time or patch the kernel
themselves.
> + if (flags & WQ_POWER_EFFICIENT) {
> + flags &= ~WQ_POWER_EFFICIENT;
No need to clear the flag.
> +#ifdef CONFIG_WQ_POWER_EFFICIENT
> + if (wq_power_efficient)
> + flags |= WQ_UNBOUND;
> +#endif
Thanks!
--
tejun
On 10 April 2013 00:00, Tejun Heo <[email protected]> wrote:
> On Tue, Apr 09, 2013 at 01:00:59PM +0530, Viresh Kumar wrote:
>> +#ifdef CONFIG_WQ_POWER_EFFICIENT
>> +static bool wq_power_efficient = 0;
>> +module_param_named(power_efficient, wq_power_efficient, bool, 0444);
>> +#endif
>
> I don't think we need to make the whole thing configurable. Turning
> it off isn't gonna save much - my gut tells me it's gonna be four
> instructions. :)
>
> What I meant was that the default value for the parameter would
> probably be need to be configurable so that mobile people don't have
> to include the kernel param all the time or patch the kernel
> themselves.
I didn't get it completely.. Are you asking to set the default value
to 1 instead
to keep it enabled by default if config option is selected?
Hello, Viresh.
On Mon, Apr 22, 2013 at 11:50:04AM +0530, Viresh Kumar wrote:
> On 10 April 2013 00:00, Tejun Heo <[email protected]> wrote:
> > On Tue, Apr 09, 2013 at 01:00:59PM +0530, Viresh Kumar wrote:
>
> >> +#ifdef CONFIG_WQ_POWER_EFFICIENT
> >> +static bool wq_power_efficient = 0;
> >> +module_param_named(power_efficient, wq_power_efficient, bool, 0444);
> >> +#endif
> >
> > I don't think we need to make the whole thing configurable. Turning
> > it off isn't gonna save much - my gut tells me it's gonna be four
> > instructions. :)
> >
> > What I meant was that the default value for the parameter would
> > probably be need to be configurable so that mobile people don't have
> > to include the kernel param all the time or patch the kernel
> > themselves.
>
> I didn't get it completely.. Are you asking to set the default value
> to 1 instead
> to keep it enabled by default if config option is selected?
Oh, sorry about that. I meant something like this.
#ifdef CONFIG_WQ_POWER_EFFICIENT_BY_DEFAULT // or something prettier
static bool wq_power_efficient = true;
#else
static bool wq_power_efficient = false;
#endif
module_param....
And its Kconfig entry
config WQ_POWER_EFFICIENT_BY_DEFAULT
bool "Blah Blah Viresh is awesome"
default n
Thanks!
--
tejun