Re: [PATCH V5 1/5] workqueues: Introduce new flag WQ_POWER_EFFICIENT for power oriented workqueues

From: Tejun Heo <tj@kernel.org>
To: Viresh Kumar <viresh.kumar@linaro.org>
Cc: Amit Kucheria <amit.kucheria@linaro.org>,
	davem@davemloft.net, linux-rt-users@vger.kernel.org,
	linux-kernel@vger.kernel.org,
	Robin Randhawa <robin.randhawa@arm.com>,
	Charles Garcia-Tobin <charles.garcia-tobin@arm.com>,
	Steve Bannister <Steve.Bannister@arm.com>,
	Peter Zijlstra <peterz@infradead.org>,
	Steven Rostedt <rostedt@goodmis.org>,
	Arvind Chauhan <arvind.chauhan@arm.com>,
	Patch Tracking <patches@linaro.org>,
	airlied@redhat.com, mingo@redhat.com,
	Jens Axboe <axboe@kernel.dk>, Liviu Dudau <Liviu.Dudau@arm.com>,
	Lists linaro-kernel <linaro-kernel@lists.linaro.org>
Subject: Re: [PATCH V5 1/5] workqueues: Introduce new flag WQ_POWER_EFFICIENT for power oriented workqueues
Date: Mon, 29 Apr 2013 09:19:17 -0700	[thread overview]
Message-ID: <20130429161917.GB19814@mtj.dyndns.org> (raw)
In-Reply-To: <CAKohpokJSwi24goSG=dTKkH0p+-YgsVURa_4giyU-u2TuK1bdA@mail.gmail.com>

Hello,

On Mon, Apr 29, 2013 at 12:06:28PM +0530, Viresh Kumar wrote:
> Whatever you wrote above confused me even more :)

Heh heh, confumageddon!!!!

> This is what i had in my mind until now. Its not about per-cpu workqueue.
> 
> Lets take example of system_wq. It doesn't have WQ_UNBOUND flag set.
> Now if we call queue_work_on() with cpu x and sytem_wq, then work
> will execute on cpu x. If we call queue_work() then it will queue the work
> on local cpu.

Yeap, !WQ_UNBOUND workqueues == per-cpu workqueues.

> At this time local cpu may be busy or idle (Atleast according to scheduler).
> We don't want a idle cpu (From schedulers perspective) to be used for
> running this work's handler due to two reasons.
> - idle cpu may be in WFI or deeper idle states and so we can avoid waking
>   it up.

I have no idea what WFI is but the physical CPU is already awake at
that time.  It can't be idle - it's running queue_work().  It could be
running in lower freq tho, which each code piece doesn't really have
much control over.

> - We will make idle cpu look busy and so other kernel stuff may be scheduled
>   on it now. But we could have kept it idle for a long time.

Hmmm... yeah, about the same thing I wrote, it's not really about not
waking up the CPU right now physically but avoiding forcing the
scheduler scheduling a pinned task on an otherwise quiescent CPU.
This effectively allows the scheduler to migrate such work items
towards a CPU which the scheduler considers to be better (in power or
whatever) leading to noticeable powersave.

> And what timer are you talking about? I am not talking about deffered work only,
> but normal work too.

Deferred work item == timer + work item.

> I might have wrongly phrased some part of my patch (maybe used workqueue
> instead of work), will fix that up.

I think it'd be necessary to distinguish the physical CPU being idle
and the scheduler considers it to be idle (no task to schedule on it)
and explain how increasing the latter can lead to powersave.  As it's
currently written, it seemingly, to me anyway, suggests that the
proposed change somehow avoids waking up actually idle CPU, which
isn't the case as queue_work() *always* schedules on the local CPU.
The local CPU can't be idle by definition.

Thanks.

-- 
tejun