Xen devel - Mar 2011 - Re: [RFC PATCH V4 5/5] cpuidle: cpuidle driver for apm

Hi Stephen,

Thanks for reviewing.

On 03/23/2011 06:44 AM, Stephen Rothwell wrote:
> Hi,
>
> On Tue, 22 Mar 2011 18:03:40 +0530 Trinabh
Gupta<trinabh@linux.vnet.ibm.com>  wrote:
>>
>> +static int apm_setup_cpuidle(int cpu)
>> +{
>> +	struct cpuidle_device *dev = kzalloc(sizeof(struct cpuidle_device),
>> +					GFP_KERNEL);
>
> Same as xen comments: no NULL check.
>
>> +	int count = CPUIDLE_DRIVER_STATE_START;
>> +	dev->cpu = cpu;
>> +	dev->drv =&apm_idle_driver;
>
> Also wondering why you would ever have a different idle routine on
> different cpus?
Yes, this is an ongoing debate. Apparently it is a possibility
because of ACPI bugs. CPU''s can have asymmetric C-states
and overall different idle routines on different cpus. Please
refer to http://lkml.org/lkml/2009/9/24/132 and
https://lkml.org/lkml/2011/2/10/37 for a discussion around this.

I have posted a patch series that does global registration
i.e same idle routines for each cpu. Please check
http://lkml.org/lkml/2011/3/22/161 . That series applies on
top of this series. Global registration significantly
simplifies the design, but still we are not sure about the
direction to take.
>
>> +
>> +	dev->states[count] = state_apm_idle;
>> +	count++;
>> +
>> +	dev->state_count = count;
>> +
>> +	if (cpuidle_register_device(dev))
>> +		return -EIO;
>
> And we leak dev.
>
>> +static void apm_idle_exit(void)
>> +{
>> +	cpuidle_unregister_driver(&apm_idle_driver);
>
> Do we leak the per cpu device structure here?
I will see how we can save
per cpu device structure pointers so that we can free them.
>
>> +	return;
>
> Unnecessary return statement.
>
Thanks,
-Trinabh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> > Also wondering why you would ever have a different idle routine on
> > different cpus?
> 
> Yes, this is an ongoing debate. Apparently it is a possibility
> because of ACPI bugs. CPU''s can have asymmetric C-states
> and overall different idle routines on different cpus. Please
> refer to http://lkml.org/lkml/2009/9/24/132 and
> https://lkml.org/lkml/2011/2/10/37 for a discussion around this.
Althought the ACPI specification allows the BIOS to tell the OS
about different C-states per-processor, I know of zero system
in the field and zero systems in development that require that
capability.  That isn''t a guarantee that capability will never
be used, but I''m not holding my breath.

If there are systems with broken tables that make them
appear asymetric, then we should have a workaround that handles
that case, rather than complicating the normal code for
the broken case.

So I recommend deleting the extra per-cpu registration stuff
unless there is some other architecture that requires it
and can''t hadle the asymmetry in another way.
> I have posted a patch series that does global registration
> i.e same idle routines for each cpu. Please check
> http://lkml.org/lkml/2011/3/22/161 . That series applies on
> top of this series. Global registration significantly
> simplifies the design, but still we are not sure about the
> direction to take.
I''ll review that.

thanks,
Len Brown, Intel Open Source Technology Center


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 03/24/2011 02:02 AM, Len Brown wrote:
>>> Also wondering why you would ever have a different idle routine on
>>> different cpus?
>>
>> Yes, this is an ongoing debate. Apparently it is a possibility
>> because of ACPI bugs. CPU''s can have asymmetric C-states
>> and overall different idle routines on different cpus. Please
>> refer to http://lkml.org/lkml/2009/9/24/132 and
>> https://lkml.org/lkml/2011/2/10/37 for a discussion around this.
>
> Althought the ACPI specification allows the BIOS to tell the OS
> about different C-states per-processor, I know of zero system
> in the field and zero systems in development that require that
> capability.  That isn''t a guarantee that capability will never
> be used, but I''m not holding my breath.
>
> If there are systems with broken tables that make them
> appear asymetric, then we should have a workaround that handles
> that case, rather than complicating the normal code for
> the broken case.
>
> So I recommend deleting the extra per-cpu registration stuff
> unless there is some other architecture that requires it
> and can''t hadle the asymmetry in another way.
Yes, lets go forward with removal of per-cpu registration
and handle rare case of asymmetry in some other may.

Using intersection or union of C-states for each cpu may
be a solution. Using intersection or lowest common C-state
has the corner case that we could have packages/cores
supporting a new lower C-state in case of thermal limit and
they would want OS to go to this state. Using intersection
or lowest common C-state may prevent this.

Another option is to use union of C-states;
but I am not sure what happens if a CPU uses a state that
is not reported for it???

Maybe there is some other way to handle asymmetry ??
>
>> I have posted a patch series that does global registration
>> i.e same idle routines for each cpu. Please check
>> http://lkml.org/lkml/2011/3/22/161 . That series applies on
>> top of this series. Global registration significantly
>> simplifies the design, but still we are not sure about the
>> direction to take.
>
> I''ll review that.
Thanks; please review especially the data structure changes
https://lkml.org/lkml/2011/3/22/162

-Trinabh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

* Trinabh Gupta <trinabh@linux.vnet.ibm.com> [2011-03-24 19:58:29]:
> 
> 
> On 03/24/2011 02:02 AM, Len Brown wrote:
> >>>Also wondering why you would ever have a different idle routine
on
> >>>different cpus?
> >>
> >>Yes, this is an ongoing debate. Apparently it is a possibility
> >>because of ACPI bugs. CPU''s can have asymmetric C-states
> >>and overall different idle routines on different cpus. Please
> >>refer to http://lkml.org/lkml/2009/9/24/132 and
> >>https://lkml.org/lkml/2011/2/10/37 for a discussion around this.
> >
> >Althought the ACPI specification allows the BIOS to tell the OS
> >about different C-states per-processor, I know of zero system
> >in the field and zero systems in development that require that
> >capability.  That isn''t a guarantee that capability will never
> >be used, but I''m not holding my breath.
> >
> >If there are systems with broken tables that make them
> >appear asymetric, then we should have a workaround that handles
> >that case, rather than complicating the normal code for
> >the broken case.
> >
> >So I recommend deleting the extra per-cpu registration stuff
> >unless there is some other architecture that requires it
> >and can''t hadle the asymmetry in another way.
Hi Len,

The fear of breaking legacy hardware is what is holding us.  Arjan
also pointed that there are systems that has asymmetric C-States
either intentionally or due to a bug.  I agree with you that we should
remove per-cpu registration at this point and move forward with
a single registration.  We can workaround the corner cases.
> Yes, lets go forward with removal of per-cpu registration
> and handle rare case of asymmetry in some other may.
Yes.  Lets discuss the design/problems on the other patch series.
(http://lkml.org/lkml/2011/3/22/161)
> Using intersection or union of C-states for each cpu may
> be a solution. Using intersection or lowest common C-state
> has the corner case that we could have packages/cores
> supporting a new lower C-state in case of thermal limit and
> they would want OS to go to this state. Using intersection
> or lowest common C-state may prevent this.
> 
> Another option is to use union of C-states;
> but I am not sure what happens if a CPU uses a state that
> is not reported for it???
> 
> Maybe there is some other way to handle asymmetry ??
The simplest method will be a union of all C-States.  Basically let
the CPU that reports the maximum C-States to register or override the
current setup.  During boot-up allow the first CPU to do the
registration, but later override if another CPU comes up with more
C-States.

This will work assuming that calling a new (deeper) C-State on CPUs
that did not report them will cause no harm.  It should degenerate to
closest supported C-State.
> >
> >>I have posted a patch series that does global registration
> >>i.e same idle routines for each cpu. Please check
> >>http://lkml.org/lkml/2011/3/22/161 . That series applies on
> >>top of this series. Global registration significantly
> >>simplifies the design, but still we are not sure about the
> >>direction to take.
> >
> >I''ll review that.
> Thanks; please review especially the data structure changes
> https://lkml.org/lkml/2011/3/22/162
Single registration will allow us to combine struct cpuidle_device and
struct cpuidle_driver, and simplify the framework for large systems.
AFAIK other archs like powerpc or ARM would not need per-cpu
definitions.

--Vaidy


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> Maybe there is some other way to handle asymmetry ??
When somebody invents a system that needs asymmetry on purpose,
that is the time to get fancy.  Until that day, simply print a
FW_BUG WARNING and fall back to default_idle().

cheers,
Len Brown, Intel Open Source Technology Center


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

* Len Brown <lenb@kernel.org> [2011-03-25 03:24:24]:
> 
> > Maybe there is some other way to handle asymmetry ??
> 
> When somebody invents a system that needs asymmetry on purpose,
> that is the time to get fancy.  Until that day, simply print a
> FW_BUG WARNING and fall back to default_idle().
We may print a warning but not switch to default_idle().  There could
be laptops that gets an extra C-State when run on battery.  So when
the ACPI _PPC event happens for the C-State change, we need to handle
the situation such that the states are refreshed for all CPUs in one step.

Basically while we are updating the new C-State, the framework should
not detect this as asymmetric and switch to default_idle().  Please
fell free to suggest a more elegant solution to handle the runtime
C-State change event, though symmetric across all CPUs.

--Vaidy


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> > > Maybe there is some other way to handle asymmetry ??
I mis-spoke on asymmetry.

Moorestown is already an example of an asymmetric system,
since its deepest c-state is available on cpu0, but not on cpu1.
So it needs different tables for each cpu.

I think what would work is a default c-state table for the system,
and the ability of a per-cpu override table.  I think that would
gracefully handle the case of many identical cpus, and also systems
with different tables per cpu.

The same goes for write-access to the tables.
In the typical case, a single table can be shared for the entire system
and nobody will be writing to it.  However, with the governor changes
to call dev->prepare and sift through all the states to find the
legal one with the lowest power_usage... There is software today
out of tree that updates that power_usage entry from prepare().

As I mentioned, I''m not fond of that mechanism - it looks racey
to me.  I''d rather see the capability of a drivers idle handler
to demote to another handler in the driver and for the accounting
to not get messed up when that happens.  I think the way to do that
is to let the driver do the accounting rather than doing it in
the cpuidle caller.

cheers,
-Len Brown, Intel Open Source Technology Center


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Wed, 2011-03-30 at 22:17 -0400, Len Brown wrote:
> 
> Moorestown is already an example of an asymmetric system,
> since its deepest c-state is available on cpu0, but not on cpu1.
> So it needs different tables for each cpu. 
wtf are these hardware guys smoking and how the heck are we supposed to
schedule on such a machine? Prefer to keep cpu1 busy while idling cpu0?

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

> > Moorestown is already an example of an asymmetric system,
> > since its deepest c-state is available on cpu0, but not on cpu1.
> > So it needs different tables for each cpu. 
> 
> wtf are these hardware guys smoking and how the heck are we supposed to
> schedule on such a machine? Prefer to keep cpu1 busy while idling cpu0?
they are smoking micro-amps:-)

S0i3 on cpu0 can be entered only after cpu1 is already off-line,
among other system hardware dependencies...

So it makes no sense to export S0i3 as a c-state on cpu1.

When cpu1 is online, the scheduler treats it as a normal SMP.

cheers,
-Len Brown, Intel Open Source Technology Center



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 03/31/2011 07:47 AM, Len Brown wrote:
>>>> Maybe there is some other way to handle asymmetry ??
>
> I mis-spoke on asymmetry.
>
> Moorestown is already an example of an asymmetric system,
> since its deepest c-state is available on cpu0, but not on cpu1.
> So it needs different tables for each cpu.
>
> I think what would work is a default c-state table for the system,
> and the ability of a per-cpu override table.  I think that would
> gracefully handle the case of many identical cpus, and also systems
> with different tables per cpu.
Hi Len,

What would happen if a cpu enters a state which wasn''t
reported for it? I am wondering if an approach like union
of states of each would work.

Can we handle asymmetry through checking and demotion inside the
routine itself; just like you are proposing as dev->prepare
alternative? But I guess this may not be efficient if this
happens often.

I am not sure if having a per-cpu override would be very tidy
(ideas ?); and much better than per-cpu stuff. So just want to check
what would be the best way forward?
>
> The same goes for write-access to the tables.
> In the typical case, a single table can be shared for the entire system
> and nobody will be writing to it.  However, with the governor changes
> to call dev->prepare and sift through all the states to find the
> legal one with the lowest power_usage... There is software today
> out of tree that updates that power_usage entry from prepare().
>
> As I mentioned, I''m not fond of that mechanism - it looks racey
> to me.  I''d rather see the capability of a drivers idle handler
> to demote to another handler in the driver and for the accounting
> to not get messed up when that happens.  I think the way to do that
> is to let the driver do the accounting rather than doing it in
> the cpuidle caller.
I agree with this; we should move update of statistics inside the
driver routines (enter routines). They already take device/stats
structure as parameter.

Thanks,
-Trinabh

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, Apr 01, 2011 at 12:09:25AM -0400, Len Brown
wrote:
> > > Moorestown is already an example of an asymmetric system,
> > > since its deepest c-state is available on cpu0, but not on cpu1.
> > > So it needs different tables for each cpu. 
> > 
> > wtf are these hardware guys smoking and how the heck are we supposed
to
> > schedule on such a machine? Prefer to keep cpu1 busy while idling
cpu0?
> 
> they are smoking micro-amps:-)
> 
> S0i3 on cpu0 can be entered only after cpu1 is already off-line,
> among other system hardware dependencies...
> 
> So it makes no sense to export S0i3 as a c-state on cpu1.
> 
> When cpu1 is online, the scheduler treats it as a normal SMP.
Isn''t S0i3 a "system" state, as opposed to cpu state ?
Perhaps
we can treat it as such and still keep the c-states symmetric.
The cpu can transition to S0i3 from any cpu as long as others are
offlined, no ? In that sense, really all the cpus would
be in S0i3, which would make it symmetric. If this isn''t how
mrst cpuidle works, then cpuidle accounting may be broken
in principle because of this.

Thanks
Dipankar

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, 2011-04-01 at 00:09 -0400, Len Brown wrote:
> > > Moorestown is already an example of an asymmetric system,
> > > since its deepest c-state is available on cpu0, but not on cpu1.
> > > So it needs different tables for each cpu. 
> > 
> > wtf are these hardware guys smoking and how the heck are we supposed
to
> > schedule on such a machine? Prefer to keep cpu1 busy while idling
cpu0?
> 
> they are smoking micro-amps:-)
Has anybody told them that pushing lots of logic into software generally
burns more amps because it keeps the thing running longer?
> S0i3 on cpu0 can be entered only after cpu1 is already off-line,
> among other system hardware dependencies...
> 
> So it makes no sense to export S0i3 as a c-state on cpu1.
> 
> When cpu1 is online, the scheduler treats it as a normal SMP.
Dipankar''s reply seems to address this issue well.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On 4/1/2011 1:15 AM, Dipankar Sarma wrote:
> On Fri, Apr 01, 2011 at 12:09:25AM -0400, Len Brown wrote:
>>>> Moorestown is already an example of an asymmetric system,
>>>> since its deepest c-state is available on cpu0, but not on
cpu1.
>>>> So it needs different tables for each cpu.
>>> wtf are these hardware guys smoking and how the heck are we
supposed to
>>> schedule on such a machine? Prefer to keep cpu1 busy while idling
cpu0?
>> they are smoking micro-amps:-)
>>
>> S0i3 on cpu0 can be entered only after cpu1 is already off-line,
>> among other system hardware dependencies...
>>
>> So it makes no sense to export S0i3 as a c-state on cpu1.
>>
>> When cpu1 is online, the scheduler treats it as a normal SMP.
> Isn''t S0i3 a "system" state, as opposed to cpu state ?
it''s misnamed. it''s a C state to the OS.



_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, Apr 01, 2011 at 07:38:23AM -0700, Arjan van de Ven
wrote:
> On 4/1/2011 1:15 AM, Dipankar Sarma wrote:
> >On Fri, Apr 01, 2011 at 12:09:25AM -0400, Len Brown wrote:
> >>>>Moorestown is already an example of an asymmetric system,
> >>>>since its deepest c-state is available on cpu0, but not on
cpu1.
> >>>>So it needs different tables for each cpu.
> >>>wtf are these hardware guys smoking and how the heck are we
supposed to
> >>>schedule on such a machine? Prefer to keep cpu1 busy while
idling cpu0?
> >>they are smoking micro-amps:-)
> >>
> >>S0i3 on cpu0 can be entered only after cpu1 is already off-line,
> >>among other system hardware dependencies...
> >>
> >>So it makes no sense to export S0i3 as a c-state on cpu1.
> >>
> >>When cpu1 is online, the scheduler treats it as a normal SMP.
> >Isn''t S0i3 a "system" state, as opposed to cpu state
?
> 
> it''s misnamed. it''s a C state to the OS.
I understand that it has been implemented as a C-state from this -
http://build.meego.com/package/view_file?file=linux-2.6.37-mrst-s0i3.patch&package=kernel-adaptation-mrst&project=home%3Adliu9&srcmd5=a0929a2863150f5c8454507d6cd8f09d

The key question is this -

+int mrst_check_state_availability(struct cpuidle_device *dev)  
+{  
+   int cpu = smp_processor_id();  
+ 
+   /*  
+    * If there is another CPU running, the GPU is active,  
+    * the PMU is uninitialized, or there is a still-unprocessed  
+    * PMU command, we cannot enter S0i3.  
+    */  
+   if (!pmu_reg || !cpumask_equal(cpu_online_mask, cpumask_of(cpu)) ||  
+       s0i3_pmu_command_pending)  
+       dev->states[5].flags |= CPUIDLE_FLAG_IGNORE;  
+   else  
+       dev->states[5].flags &= ~CPUIDLE_FLAG_IGNORE;  

Is this really asymetric ? Or is it that if the other
cpu(s) are in C6, the chip can enter S0i3 ? If that is the case,
then isn''t this equivalent to all the cpus in the chip
entering S0i3 and thus symmetrical ? Also, if going to S0i3
relies on other cpus offlined, then we don''t need to
worry about asymetry from the scheduler/cpuidle point of
view, no ?

Thanks
Dipankar

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Fri, Apr 01, 2011 at 04:02:36PM +0200, Peter Zijlstra
wrote:
> 
> > S0i3 on cpu0 can be entered only after cpu1 is already off-line,
> > among other system hardware dependencies...
> > 
> > So it makes no sense to export S0i3 as a c-state on cpu1.
> > 
> > When cpu1 is online, the scheduler treats it as a normal SMP.
> 
> Dipankar''s reply seems to address this issue well.
I can''t find any Moorestown documentation at the Intel site, but
thinking about Len''s inputs a bit more, it seems there may
be still a problem asymetry from the scheduler perspective.

If cpu0 or cpu1 either of them can be offlined, there is no
asymetry. If only cpu1 can be offlined, it would mean that
one cpu may be more efficient depending on how we do
cpu offlining for power savings. It gets a bit messy.

Len, what exacty is the significance of offlining here ?
Apart from going to C6, what else is needed in cpu1 for
the chip to go to S0i3 ? Why is idle C6 not enough ?

Thanks
Dipankar

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Mon, 2011-04-04 at 20:02 +0530, Dipankar Sarma wrote:
> On Fri, Apr 01, 2011 at 04:02:36PM +0200, Peter Zijlstra wrote:
> > 
> > > S0i3 on cpu0 can be entered only after cpu1 is already off-line,
> > > among other system hardware dependencies...
> > > 
> > > So it makes no sense to export S0i3 as a c-state on cpu1.
> > > 
> > > When cpu1 is online, the scheduler treats it as a normal SMP.
> > 
> > Dipankar''s reply seems to address this issue well.
> 
> I can''t find any Moorestown documentation at the Intel site, but
> thinking about Len''s inputs a bit more, it seems there may
> be still a problem asymetry from the scheduler perspective.
> 
> If cpu0 or cpu1 either of them can be offlined, there is no
> asymetry. If only cpu1 can be offlined, it would mean that
> one cpu may be more efficient depending on how we do
> cpu offlining for power savings. It gets a bit messy.
> 
> Len, what exacty is the significance of offlining here ?
> Apart from going to C6, what else is needed in cpu1 for
> the chip to go to S0i3 ? Why is idle C6 not enough ?
I don''t think offlining is relevant, anybody using that for power
management is doing it wrong, _very_ wrong.

_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel

On Tue, Apr 05, 2011 at 05:01:32PM +0200, Peter Zijlstra
wrote:
> On Mon, 2011-04-04 at 20:02 +0530, Dipankar Sarma wrote:
> > On Fri, Apr 01, 2011 at 04:02:36PM +0200, Peter Zijlstra wrote:
> > I can''t find any Moorestown documentation at the Intel site,
but
> > thinking about Len''s inputs a bit more, it seems there may
> > be still a problem asymetry from the scheduler perspective.
> > 
> > If cpu0 or cpu1 either of them can be offlined, there is no
> > asymetry. If only cpu1 can be offlined, it would mean that
> > one cpu may be more efficient depending on how we do
> > cpu offlining for power savings. It gets a bit messy.
> > 
> > Len, what exacty is the significance of offlining here ?
> > Apart from going to C6, what else is needed in cpu1 for
> > the chip to go to S0i3 ? Why is idle C6 not enough ?
> 
> I don''t think offlining is relevant, anybody using that for power
> management is doing it wrong, _very_ wrong.
I am suggesting that it depends on the offlining logic. If cpu1
is being used as an added co-processor for some specific apps
and mostly offline otherwise, it may not be an issue. If
offlining is being used as a meta-scheduler over the kernel
scheduler (like power savings or whatever logic) than it
will cause asymmetry problems dependent on the ooffline logic - e.g.
it may be more advantageous for the kernel scheduler to schedule
on cpu0 keeping cpu1 free leading to S0i3 more often. Not advocating
it when we are trying to run away from it on powerpc :)

For now, it seems we are OK in handling S0i3 through cpuidle, just that 
it would be nice to understand the overall offline logic and why it is 
needed. Similar questions have come up in my discussions with ARM guys 
in the recent times as well.

Thanks
Dipankar


_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xensource.com
http://lists.xensource.com/xen-devel