From: Lorenzo Pieralisi <lorenzo.pieralisi@arm.com>
To: Colin Cross <ccross@android.com>
Cc: Vincent Guittot <vincent.guittot@linaro.org>,
Daniel Lezcano <daniel.lezcano@linaro.org>,
Kevin Hilman <khilman@ti.com>, Len Brown <len.brown@intel.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Amit Kucheria <amit.kucheria@linaro.org>,
"linux-tegra@vger.kernel.org" <linux-tegra@vger.kernel.org>,
"linux-pm@lists.linux-foundation.org"
<linux-pm@lists.linux-foundation.org>,
"linux-omap@vger.kernel.org" <linux-omap@vger.kernel.org>,
Arjan van de Ven <arjan@linux.intel.com>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [linux-pm] [PATCH 0/3] coupled cpuidle state support
Date: Wed, 1 Feb 2012 18:07:05 +0000 [thread overview]
Message-ID: <20120201180705.GA20936@e102568-lin.cambridge.arm.com> (raw)
In-Reply-To: <CAMbhsRTFzc8aGsSGUs2==4FiqBb7zOit4cO=6U698k+RCE+kqQ@mail.gmail.com>
On Wed, Feb 01, 2012 at 05:30:15PM +0000, Colin Cross wrote:
> On Wed, Feb 1, 2012 at 6:59 AM, Lorenzo Pieralisi
> <lorenzo.pieralisi@arm.com> wrote:
> > On Wed, Feb 01, 2012 at 12:13:26PM +0000, Vincent Guittot wrote:
> >
> > [...]
> >
> >> >> In your patch, you put in safe state (WFI for most of platform) the
> >> >> cpus that become idle and these cpus are woken up each time a new cpu
> >> >> of the cluster becomes idle. Then, the cluster state is chosen and the
> >> >> cpus enter the selected C-state. On ux500, we are using another
> >> >> behavior for synchronizing the cpus. The cpus are prepared to enter
> >> >> the c-state that has been chosen by the governor and the last cpu,
> >> >> that enters idle, chooses the final cluster state (according to cpus'
> >> >> C-state). The main advantage of this solution is that you don't need
> >> >> to wake other cpus to enter the C-state of a cluster. This can be
> >> >> quite worth full when tasks mainly run on one cpu. Have you also think
> >> >> about such behavior when developing the coupled cpuidle driver ? It
> >> >> could be interesting to add such behavior.
> >> >
> >> > Waking up the cpus that are in the safe state is not done just to
> >> > choose the target state, it's done to allow the cpus to take
> >> > themselves to the target low power state. On ux500, are you saying
> >> > you take the cpus directly from the safe state to a lower power state
> >> > without ever going back to the active state? I once implemented Tegra
> >>
> >> yes it is
> >
> > But if there is a single power rail for the entire cluster, when a CPU
> > is "prepared" for shutdown this means that you have to save the context and
> > clean L1, maybe for nothing since if other CPUs are up and running the
> > CPU going idle can just enter a simple standby wfi (clock-gated but power on).
> >
> > With Colin's approach, context is saved and L1 cleaned only when it is
> > almost certain the cluster is powered off (so the CPUs).
> >
> > It is a trade-off, I am not saying one approach is better than the
> > other; we just have to make sure that preparing the CPU for "possible" shutdown
> > is better than sending IPIs to take CPUs out of wfi and synchronize
> > them (this happens if and only if CPUs enter coupled C-states).
> >
> > As usual this will depend on use cases (and silicon implementations :) )
> >
> > It is definitely worth benchmarking them.
> >
>
> I'm less worried about performance, and more worried about race
> conditions. How do you deal with the following situation:
> CPU0 goes to WFI, and saves its state
> CPU1 goes idle, and selects a deep idle state that powers down CPU0
> CPU1 saves is state, and is about to trigger the power down
> CPU0 gets an interrupt, restores its state, and modifies state (maybe
> takes a spinlock during boot)
> CPU1 cuts the power to CPU0
>
> On OMAP4, the race is handled in hardware. When CPU1 tries to cut the
> power to the blocks shared by CPU0 the hardware will ignore the
> request if CPU0 is not in WFI. On Tegra2, there is no hardware
> support and I had to handle it with a spinlock implemented in scratch
> registers because CPU0 is out of coherency when it starts booting and
> ldrex/strex don't work. I'm not convinced my implementation is
> correct, and I'd be curious to see any other implementations.
That's a problem you solved with coupled C-states (ie your example in
the cover letter), where the primary waits for other CPUs to be reset
before issuing the power down command, right ? At that point in time
secondaries cannot wake up (?) and if wfi (ie power down) aborts you just
take the secondaries out of reset and restart executing simultaneously,
correct ? It mirrors the suspend behaviour, which is easier to deal with
than completely random idle paths.
It is true that this should be managed by the PM HW; if HW is not
capable of managing these situations things get nasty as you highlighted.
And it is also true ldrex/strex on cacheable memory might not be available in
those early warm-boot stages. I came up with a locking algorithm on
strongly ordered memory to deal with that, but I am still not sure it is
something we really really need.
I will test coupled C-state code ASAP, and come back with feedback.
Thanks,
Lorenzo
next prev parent reply other threads:[~2012-02-01 18:07 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-12-21 0:09 [PATCH 0/3] coupled cpuidle state support Colin Cross
2011-12-21 0:09 ` [PATCH 1/3] cpuidle: refactor out cpuidle_enter_state Colin Cross
2012-01-04 14:08 ` [linux-pm] " Jean Pihet
2011-12-21 0:09 ` [PATCH 2/3] cpuidle: fix error handling in __cpuidle_register_device Colin Cross
2011-12-21 0:09 ` [PATCH 3/3] cpuidle: add support for states that affect multiple cpus Colin Cross
2011-12-21 9:02 ` [PATCH 0/3] coupled cpuidle state support Arjan van de Ven
2011-12-21 9:40 ` Colin Cross
2011-12-21 9:44 ` Arjan van de Ven
2011-12-21 9:55 ` Colin Cross
2011-12-21 12:12 ` Arjan van de Ven
2011-12-21 19:05 ` Colin Cross
2011-12-21 19:36 ` Arjan van de Ven
2011-12-21 19:42 ` [linux-pm] " Colin Cross
2011-12-22 8:35 ` Shilimkar, Santosh
2011-12-22 8:53 ` Arjan van de Ven
2011-12-22 9:30 ` Shilimkar, Santosh
2011-12-22 21:20 ` Colin Cross
2012-03-14 0:39 ` Colin Cross
2012-01-04 0:41 ` Kevin Hilman
2012-01-04 17:27 ` Shilimkar, Santosh
2012-01-20 8:46 ` Daniel Lezcano
2012-01-20 20:40 ` Colin Cross
2012-01-25 14:04 ` Daniel Lezcano
2012-01-31 14:13 ` Daniel Lezcano
2012-01-27 8:54 ` [linux-pm] " Vincent Guittot
2012-01-27 17:32 ` Colin Cross
2012-02-01 12:13 ` Vincent Guittot
2012-02-01 14:59 ` Lorenzo Pieralisi
2012-02-01 17:30 ` Colin Cross
2012-02-01 18:07 ` Lorenzo Pieralisi [this message]
2012-02-03 1:19 ` Colin Cross
[not found] ` <8762e8kqi6.fsf@ti.com>
2012-03-14 0:28 ` Colin Cross
2012-03-14 0:47 ` Colin Cross
2012-03-14 14:23 ` [linux-pm] " Kevin Hilman
2012-03-14 2:04 ` Arjan van de Ven
2012-03-14 2:21 ` Colin Cross
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20120201180705.GA20936@e102568-lin.cambridge.arm.com \
--to=lorenzo.pieralisi@arm.com \
--cc=amit.kucheria@linaro.org \
--cc=arjan@linux.intel.com \
--cc=ccross@android.com \
--cc=daniel.lezcano@linaro.org \
--cc=khilman@ti.com \
--cc=len.brown@intel.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-omap@vger.kernel.org \
--cc=linux-pm@lists.linux-foundation.org \
--cc=linux-tegra@vger.kernel.org \
--cc=vincent.guittot@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).