[V4,3/4] block: queue work on unbound wq

On 31 March 2013 23:49, Tejun Heo <tj@kernel.org> 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 <viresh.kumar@linaro.org>
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 <viresh.kumar@linaro.org>
---
 Documentation/kernel-parameters.txt | 15 +++++++++++++++
 include/linux/workqueue.h           |  3 +++
 kernel/power/Kconfig                | 18 ++++++++++++++++++
 kernel/workqueue.c                  | 14 ++++++++++++++
 4 files changed, 50 insertions(+)

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]);

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!

On 10 April 2013 00:00, Tejun Heo <tj@kernel.org> 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 <tj@kernel.org> 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!