"Rafael J. Wysocki" [off-list ref] writes:
[...]
Second, quite honestly, I don't see a connection to genpd here.
The connection with genpd is because the *reason* the timer was
shutdown/stopped is because it shares power with the CPU, which is why
the timer stops when the CPU hits ceratin low power states. IOW, it's
in the same power domain as the CPU.
What you seem to be saying is "maybe we can eliminate the need to check the
return value of tick_broadcast_enter() in the idle loop if we proactively
disable the TIMER_STOP idle states of a CPU when we start to use that CPU's
timer as a broadcast one".
So this seems to be about the timekeeping rather than power domains, because
that's where the broadcast thing is done. So the code setting up the CPU's
timer for broadcast would pretty much need to pause cpuidle, go through the
CPU's idle states and disable the TIMER_STOP ones. And do the reverse when the
timer is not going the be used for broadcast any more.
Or..., modify the timer subystem to use runtime PM on the timer devices,
create a genpd that includes the timer device, and use
pm_genpd_attach_cpuidle() to attach that genpd so that whenever that
timer is runtime PM active, the deeper C-states cannot be hit.
So question is whether or not this is actually really more
straightforward than checking the return value of
tick_broadcast_enter() in the idle loop after all.
Unfortunetly this problem doesn't only affect timers.
Daniel's broader point is that $SUBJECT series only handles this for the
timer, but there's actually a more general problem to solve for *any*
device that shares a power domain with a CPU (e.g. CPU-local
timers, interrupt controllers, performance monitoring units, floating
point units, etc. etc.)
If we keep adding checks to the idle loop for all those devices, we're
heading for a mess. (In fact, this is exactly what CPUidle drivers in
lots of vendor trees are doing, and it is indeed quite messy, and very
vendor specific.)
Also, solving this more general problem was the primary motivation for
adding the gnpd _attach_cpuidle() feature in the first place, so why not
use that?
Longer term, IMO, these dependencies between CPUs and all these "extras"
logic that share a power domain should be modeled by a genpd. If all
those devices are using runtime PM, including the CPUs, and they are
grouped into a genpd, then we we can very easily know at the genpd level
whether or not the CPU could be powered down, and to what level. This
longer-term solution is what I want to discuss at LPC this year in my
"Unifiy idle management of CPUs and IO devices" topic[1]. ( Also FYI,
using a genpd to model a CPU and connected logic is part of the
motivation behind the recent proposals to add support for multiple
states to genpd by Axel Haslam. )
Anyways I digress...
In the short term, while your patches look fine to me, the objection I
have is that it's only a band-aid fix that handles timers, but none of
the other "extras" that might share a power rail with the CPU. So,
until we have the long-term stuff sorted out, the better
short-term solution IMO is the _attach_cpuidle() one above.
Kevin
[1] http://wiki.linuxplumbersconf.org/2015:energy-aware_scheduling
On Wednesday, May 13, 2015 03:59:55 PM Kevin Hilman wrote:
"Rafael J. Wysocki" [off-list ref] writes:
[...]
quoted
Second, quite honestly, I don't see a connection to genpd here.
The connection with genpd is because the *reason* the timer was
shutdown/stopped is because it shares power with the CPU, which is why
the timer stops when the CPU hits ceratin low power states. IOW, it's
in the same power domain as the CPU.
Well, what if you don't have genpd on that system? Is the problem at hand not
relevant then magically?
Rafael
On Wed, May 13, 2015 at 5:16 PM, Rafael J. Wysocki [off-list ref] wrote:
On Wednesday, May 13, 2015 03:59:55 PM Kevin Hilman wrote:
quoted
"Rafael J. Wysocki" [off-list ref] writes:
[...]
quoted
Second, quite honestly, I don't see a connection to genpd here.
The connection with genpd is because the *reason* the timer was
shutdown/stopped is because it shares power with the CPU, which is why
the timer stops when the CPU hits ceratin low power states. IOW, it's
in the same power domain as the CPU.
Well, what if you don't have genpd on that system? Is the problem at hand not
relevant then magically?
Well, if you're not using genpd to model hardware power domain
dependencies, then yes you'll definitely need a different solution.
And, as we discussed on IRC. If you only care about timers, and genpd
is not in use, then $SUBJECT series is a fine approach, and I have no
objections. But for SoCs where there are several other things that
share power with CPU, we need a more generic, genpd based solution,
which it seems we're in agreement on. And since the two approaches
are not mutually exclusive, then I have real objections to applying
this series.
Acked-by: Kevin Hilman <redacted>
Kevin
On Wednesday, May 13, 2015 05:13:27 PM Kevin Hilman wrote:
On Wed, May 13, 2015 at 5:16 PM, Rafael J. Wysocki [off-list ref] wrote:
quoted
On Wednesday, May 13, 2015 03:59:55 PM Kevin Hilman wrote:
quoted
"Rafael J. Wysocki" [off-list ref] writes:
[...]
quoted
Second, quite honestly, I don't see a connection to genpd here.
The connection with genpd is because the *reason* the timer was
shutdown/stopped is because it shares power with the CPU, which is why
the timer stops when the CPU hits ceratin low power states. IOW, it's
in the same power domain as the CPU.
Well, what if you don't have genpd on that system? Is the problem at hand not
relevant then magically?
Well, if you're not using genpd to model hardware power domain
dependencies, then yes you'll definitely need a different solution.
And, as we discussed on IRC. If you only care about timers, and genpd
is not in use, then $SUBJECT series is a fine approach, and I have no
objections. But for SoCs where there are several other things that
share power with CPU, we need a more generic, genpd based solution,
which it seems we're in agreement on. And since the two approaches
are not mutually exclusive, then I have real objections to applying
this series.
I guess a "no" is missing in the last sentence. ;-)
Acked-by: Kevin Hilman <redacted>
Thanks!
Rafael
On 05/14/2015 04:29 AM, Kevin Hilman wrote:
"Rafael J. Wysocki" [off-list ref] writes:
[...]
quoted
Second, quite honestly, I don't see a connection to genpd here.
The connection with genpd is because the *reason* the timer was
shutdown/stopped is because it shares power with the CPU, which is why
the timer stops when the CPU hits ceratin low power states. IOW, it's
in the same power domain as the CPU.
quoted
What you seem to be saying is "maybe we can eliminate the need to check the
return value of tick_broadcast_enter() in the idle loop if we proactively
disable the TIMER_STOP idle states of a CPU when we start to use that CPU's
timer as a broadcast one".
So this seems to be about the timekeeping rather than power domains, because
that's where the broadcast thing is done. So the code setting up the CPU's
timer for broadcast would pretty much need to pause cpuidle, go through the
CPU's idle states and disable the TIMER_STOP ones. And do the reverse when the
timer is not going the be used for broadcast any more.
Or..., modify the timer subystem to use runtime PM on the timer devices,
create a genpd that includes the timer device, and use
pm_genpd_attach_cpuidle() to attach that genpd so that whenever that
timer is runtime PM active, the deeper C-states cannot be hit.
I think you are missing a point here. If such a solution were possible,
the tick broadcast framework would not have been designed to support
deep cpu idle states. One reason we cannot go this way of course, is not
all archs may support genpd as was pointed out. But the second reason
IMO is that a timer is runtime PM active as long as there is some
deferred work, either in the near or far future.
The point behind the broadcast framework is let these CPUs go to deeper
idle states when the timers are in the "far" future. We can potentially
save power by doing so and don't need to keep the entire power domain
active just because the timer is supposed to fire 5 minutes from now,
which is precisely what happens if we go the genpd way.
Hence I don't think we can trivially club timers with genpd unless we
have a way to power the timer PM domain down, depending on when it is
supposed to fire, in which case we will merely be replicating the
cpuidle governor code.
Regards
Preeti U Murthy
quoted
So question is whether or not this is actually really more
straightforward than checking the return value of
tick_broadcast_enter() in the idle loop after all.
Unfortunetly this problem doesn't only affect timers.
Daniel's broader point is that $SUBJECT series only handles this for the
timer, but there's actually a more general problem to solve for *any*
device that shares a power domain with a CPU (e.g. CPU-local
timers, interrupt controllers, performance monitoring units, floating
point units, etc. etc.)
If we keep adding checks to the idle loop for all those devices, we're
heading for a mess. (In fact, this is exactly what CPUidle drivers in
lots of vendor trees are doing, and it is indeed quite messy, and very
vendor specific.)
Also, solving this more general problem was the primary motivation for
adding the gnpd _attach_cpuidle() feature in the first place, so why not
use that?
Longer term, IMO, these dependencies between CPUs and all these "extras"
logic that share a power domain should be modeled by a genpd. If all
those devices are using runtime PM, including the CPUs, and they are
grouped into a genpd, then we we can very easily know at the genpd level
whether or not the CPU could be powered down, and to what level. This
longer-term solution is what I want to discuss at LPC this year in my
"Unifiy idle management of CPUs and IO devices" topic[1]. ( Also FYI,
using a genpd to model a CPU and connected logic is part of the
motivation behind the recent proposals to add support for multiple
states to genpd by Axel Haslam. )
Anyways I digress...
In the short term, while your patches look fine to me, the objection I
have is that it's only a band-aid fix that handles timers, but none of
the other "extras" that might share a power rail with the CPU. So,
until we have the long-term stuff sorted out, the better
short-term solution IMO is the _attach_cpuidle() one above.
Kevin
[1] http://wiki.linuxplumbersconf.org/2015:energy-aware_scheduling
_______________________________________________
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev