Re: [PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Daniel Lezcano]

On 11/07/2014 05:29 AM, Preeti U Murthy wrote:

On 11/06/2014 07:12 PM, Daniel Lezcano wrote:


Preeti,

I am wondering if we aren't going to a false debate.

If the latency_req is 0, we should just poll and not enter in any idle
state even if one has zero exit latency. With a zero latency req, we
want full reactivity on the system, not enter an idle state with all the
computation in the menu governor, no ?

I agree this patch changes the behavior on PowerPC, but only if the
latency_req is set to zero. I don't think we are worried about power
saving when setting this value.

Couldn't the patch accepted as it is for the sake of consistency on all
the platform and then we optimize cleanly for the special latency zero
case ?


Alright Daniel, you can go ahead. I was thinking this patch through and
now realize that, like you point out the logic will only get complicated
with all the additional hack.

But would it be possible to add the weak arch_cpu_idle_loop() call for
the cases where latency requirement is 0 like you had suggested earlier
? This would ensure the polling logic does not break on PowerPC and we
don't bother the governor even. I will add the function in the core
PowerPC code. If arch does not define this function it will fall back to
cpu_idle_loop(). Fair enough?


Yes, sounds good.

I will add the weak function as the first patch in the series.

Thanks for your reviews.

  -- Daniel

--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Preeti U Murthy]

On 11/06/2014 07:12 PM, Daniel Lezcano wrote:
> 
> Preeti,
> 
> I am wondering if we aren't going to a false debate.
> 
> If the latency_req is 0, we should just poll and not enter in any idle
> state even if one has zero exit latency. With a zero latency req, we
> want full reactivity on the system, not enter an idle state with all the
> computation in the menu governor, no ?
> 
> I agree this patch changes the behavior on PowerPC, but only if the
> latency_req is set to zero. I don't think we are worried about power
> saving when setting this value.
> 
> Couldn't the patch accepted as it is for the sake of consistency on all
> the platform and then we optimize cleanly for the special latency zero
> case ?

Alright Daniel, you can go ahead. I was thinking this patch through and
now realize that, like you point out the logic will only get complicated
with all the additional hack.

But would it be possible to add the weak arch_cpu_idle_loop() call for
the cases where latency requirement is 0 like you had suggested earlier
? This would ensure the polling logic does not break on PowerPC and we
don't bother the governor even. I will add the function in the core
PowerPC code. If arch does not define this function it will fall back to
cpu_idle_loop(). Fair enough?

Regards
Preeti U Murthy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Preeti U Murthy]

On 11/06/2014 05:57 PM, Daniel Lezcano wrote:
> On 11/06/2014 05:08 AM, Preeti U Murthy wrote:
>> On 11/05/2014 07:58 PM, Daniel Lezcano wrote:
>>> On 10/29/2014 03:01 AM, Preeti U Murthy wrote:
 On 10/29/2014 12:29 AM, Daniel Lezcano wrote:
> On 10/28/2014 04:51 AM, Preeti Murthy wrote:
>> Hi Daniel,
>>
>> On Thu, Oct 23, 2014 at 2:31 PM, Daniel Lezcano
>>  wrote:
>>> When the pmqos latency requirement is set to zero that means
>>> "poll in
>>> all the
>>> cases".
>>>
>>> That is correctly implemented on x86 but not on the other archs.
>>>
>>> As how is written the code, if the latency request is zero, the
>>> governor will
>>> return zero, so corresponding, for x86, to the poll function, but
>>> for
>>> the
>>> others arch the default idle function. For example, on ARM this is
>>> wait-for-
>>> interrupt with a latency of '1', so violating the constraint.
>>
>> This is not true actually. On PowerPC the idle state 0 has an
>> exit_latency of 0.
>>
>>>
>>> In order to fix that, do the latency requirement check *before*
>>> calling the
>>> cpuidle framework in order to jump to the poll function without
>>> entering
>>> cpuidle. That has several benefits:
>>
>> Doing so actually hurts on PowerPC. Because the idle loop defined for
>> idle state 0 is different from what cpu_relax() does in
>> cpu_idle_loop().
>> The spinning is more power efficient in the former case. Moreover we
>> also set
>> certain register values which indicate an idle cpu. The ppc_runlatch
>> bits
>> do precisely this. These register values are being read by some user
>> space
>> tools.  So we will end up breaking them with this patch
>>
>> My suggestion is very well keep the latency requirement check in
>> kernel/sched/idle.c
>> like your doing in this patch. But before jumping to cpu_idle_loop
>> verify if the
>> idle state 0 has an exit_latency > 0 in addition to your check on the
>> latency_req == 0.
>> If not, you can fall through to the regular path of calling into the
>> cpuidle driver.
>> The scheduler can query the cpuidle_driver structure anyway.
>>
>> What do you think?
>
> Thanks for reviewing the patch and spotting this.
>
> Wouldn't make sense to create:
>
> void __weak_cpu_idle_poll(void) ?
>
> and override it with your specific poll function ?
>

 No this would become ugly as far as I can see. A weak function has
 to be
 defined under arch/* code. We will either need to duplicate the idle
 loop that we already have in the drivers or point the weak function to
 the first idle state defined by our driver. Both of which is not
 desirable (calling into the driver from arch code is ugly). Another
 reason why I don't like the idea of a weak function is that if you have
 missed looking at a specific driver and they have an idle loop with
 features similar to on powerpc, you will have to spot it yourself and
 include the arch specific cpu_idle_poll() for them.
>>>
>>> Yes, I agree this is a fair point. But actually I don't see the interest
>>> of having the poll loop in the cpuidle driver. These cleanups are
>>
>> We can't do that simply because the idle poll loop has arch specific
>> bits on powerpc.
> 
> I am not sure.
> 
> Could you describe what is the difference between the arch_cpu_idle
> function in arch/arm/powerpc/kernel/idle.c and the 0th power PC idle
> state ?

arch_cpu_idle() is the arch specific idle routine. It goes into deeper
idle state. I am guessing you meant to ask the difference between
power pc 0th idle state and the polling logic in cpu_idle_poll().

The 0th idle state is also a polling loop. Additionally it sets a couple
of registers to indicate idleness.

> 
> Is it kind of duplicate ?
> 
> And for polling, do you really want to use while (...); cpu_relax(); as
> it is x86 specific ? instead of the powerpc's arch_idle ?
> 
> Today, if latency_req == 0, it returns the 0th idle state, so polling.
> 
> If we jump to the arch_cpu_idle_poll, the result will be the same for
> all architecture.


So you propose creating a weak arch_cpu_idle_poll()? Ok if it is going
to make the cleanup easier, go ahead. I can add arch_cpu_idle_poll() in
the core code on powerpc.

> 
>>> preparing the removal of the CPUIDLE_DRIVER_STATE_START macro which
>>> leads to a lot of mess in the cpuidle code.
>>
>> How is the suggestion to check the exit_latency of idle state 0 when
>> latency_req  == 0 going to hinder this removal?
> 
> It sounds a bit hackish. I prefer to sort out the current situation.
> 
> And by the way, what is the reasoning behind having a target_residency /
> exit_latency equal to zero for an idle state ?

Its a polling idle state, hence the exit_latency is 0.

Regards
Preeti U Murthy

--
To unsubscribe from this list: sen

Re: [PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Daniel Lezcano]

Preeti,

I am wondering if we aren't going to a false debate.

If the latency_req is 0, we should just poll and not enter in any idle 
state even if one has zero exit latency. With a zero latency req, we 
want full reactivity on the system, not enter an idle state with all the 
computation in the menu governor, no ?


I agree this patch changes the behavior on PowerPC, but only if the 
latency_req is set to zero. I don't think we are worried about power 
saving when setting this value.


Couldn't the patch accepted as it is for the sake of consistency on all 
the platform and then we optimize cleanly for the special latency zero 
case ?


On 11/06/2014 05:08 AM, Preeti U Murthy wrote:

On 11/05/2014 07:58 PM, Daniel Lezcano wrote:

On 10/29/2014 03:01 AM, Preeti U Murthy wrote:

On 10/29/2014 12:29 AM, Daniel Lezcano wrote:

On 10/28/2014 04:51 AM, Preeti Murthy wrote:

Hi Daniel,

On Thu, Oct 23, 2014 at 2:31 PM, Daniel Lezcano
 wrote:

When the pmqos latency requirement is set to zero that means "poll in
all the
cases".

That is correctly implemented on x86 but not on the other archs.

As how is written the code, if the latency request is zero, the
governor will
return zero, so corresponding, for x86, to the poll function, but for
the
others arch the default idle function. For example, on ARM this is
wait-for-
interrupt with a latency of '1', so violating the constraint.


This is not true actually. On PowerPC the idle state 0 has an
exit_latency of 0.



In order to fix that, do the latency requirement check *before*
calling the
cpuidle framework in order to jump to the poll function without
entering
cpuidle. That has several benefits:


Doing so actually hurts on PowerPC. Because the idle loop defined for
idle state 0 is different from what cpu_relax() does in
cpu_idle_loop().
The spinning is more power efficient in the former case. Moreover we
also set
certain register values which indicate an idle cpu. The ppc_runlatch
bits
do precisely this. These register values are being read by some user
space
tools.  So we will end up breaking them with this patch

My suggestion is very well keep the latency requirement check in
kernel/sched/idle.c
like your doing in this patch. But before jumping to cpu_idle_loop
verify if the
idle state 0 has an exit_latency > 0 in addition to your check on the
latency_req == 0.
If not, you can fall through to the regular path of calling into the
cpuidle driver.
The scheduler can query the cpuidle_driver structure anyway.

What do you think?


Thanks for reviewing the patch and spotting this.

Wouldn't make sense to create:

void __weak_cpu_idle_poll(void) ?

and override it with your specific poll function ?



No this would become ugly as far as I can see. A weak function has to be
defined under arch/* code. We will either need to duplicate the idle
loop that we already have in the drivers or point the weak function to
the first idle state defined by our driver. Both of which is not
desirable (calling into the driver from arch code is ugly). Another
reason why I don't like the idea of a weak function is that if you have
missed looking at a specific driver and they have an idle loop with
features similar to on powerpc, you will have to spot it yourself and
include the arch specific cpu_idle_poll() for them.


Yes, I agree this is a fair point. But actually I don't see the interest
of having the poll loop in the cpuidle driver. These cleanups are


We can't do that simply because the idle poll loop has arch specific
bits on powerpc.


preparing the removal of the CPUIDLE_DRIVER_STATE_START macro which
leads to a lot of mess in the cpuidle code.


How is the suggestion to check the exit_latency of idle state 0 when
latency_req  == 0 going to hinder this removal?



With the removal of this macro, we should be able to move the select
loop from the menu governor and use it everywhere else. Furthermore,
this state which is flagged with TIME_VALID, isn't because the local
interrupt are enabled so we are measuring the interrupt time processing.
Beside that the idle loop for x86 is mostly not used.

So the idea would be to extract those idle loop from the drivers and use
them directly when:
  1. the idle selection fails (use the poll loop under certain
circumstances we have to redefine)


This behavior will not change as per my suggestion.


  2. when the latency req is zero


Its only here that I suggested you also verify state 0's exit_latency.
For the reason that the arch may have a more optimized idle poll loop,
which we cannot override with the generic cpuidle poll loop.

Regards
Preeti U Murthy


That will result in a cleaner code in cpuidle and in the governor.

Do you agree with that ?


But by having a check on the exit_latency, you are claiming that since
the driver's 0th idle state is no better than the generic idle loop in
cases of 0 latency req, we are better off calling the latter, which
looks reasonable. That way you don't have to bother about worsening the
idle loop

Re: [PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Daniel Lezcano]

On 11/06/2014 05:08 AM, Preeti U Murthy wrote:

On 11/05/2014 07:58 PM, Daniel Lezcano wrote:

On 10/29/2014 03:01 AM, Preeti U Murthy wrote:

On 10/29/2014 12:29 AM, Daniel Lezcano wrote:

On 10/28/2014 04:51 AM, Preeti Murthy wrote:

Hi Daniel,

On Thu, Oct 23, 2014 at 2:31 PM, Daniel Lezcano
 wrote:

When the pmqos latency requirement is set to zero that means "poll in
all the
cases".

That is correctly implemented on x86 but not on the other archs.

As how is written the code, if the latency request is zero, the
governor will
return zero, so corresponding, for x86, to the poll function, but for
the
others arch the default idle function. For example, on ARM this is
wait-for-
interrupt with a latency of '1', so violating the constraint.


This is not true actually. On PowerPC the idle state 0 has an
exit_latency of 0.



In order to fix that, do the latency requirement check *before*
calling the
cpuidle framework in order to jump to the poll function without
entering
cpuidle. That has several benefits:


Doing so actually hurts on PowerPC. Because the idle loop defined for
idle state 0 is different from what cpu_relax() does in
cpu_idle_loop().
The spinning is more power efficient in the former case. Moreover we
also set
certain register values which indicate an idle cpu. The ppc_runlatch
bits
do precisely this. These register values are being read by some user
space
tools.  So we will end up breaking them with this patch

My suggestion is very well keep the latency requirement check in
kernel/sched/idle.c
like your doing in this patch. But before jumping to cpu_idle_loop
verify if the
idle state 0 has an exit_latency > 0 in addition to your check on the
latency_req == 0.
If not, you can fall through to the regular path of calling into the
cpuidle driver.
The scheduler can query the cpuidle_driver structure anyway.

What do you think?


Thanks for reviewing the patch and spotting this.

Wouldn't make sense to create:

void __weak_cpu_idle_poll(void) ?

and override it with your specific poll function ?



No this would become ugly as far as I can see. A weak function has to be
defined under arch/* code. We will either need to duplicate the idle
loop that we already have in the drivers or point the weak function to
the first idle state defined by our driver. Both of which is not
desirable (calling into the driver from arch code is ugly). Another
reason why I don't like the idea of a weak function is that if you have
missed looking at a specific driver and they have an idle loop with
features similar to on powerpc, you will have to spot it yourself and
include the arch specific cpu_idle_poll() for them.


Yes, I agree this is a fair point. But actually I don't see the interest
of having the poll loop in the cpuidle driver. These cleanups are


We can't do that simply because the idle poll loop has arch specific
bits on powerpc.


I am not sure.

Could you describe what is the difference between the arch_cpu_idle 
function in arch/arm/powerpc/kernel/idle.c and the 0th power PC idle state ?


Is it kind of duplicate ?

And for polling, do you really want to use while (...); cpu_relax(); as 
it is x86 specific ? instead of the powerpc's arch_idle ?


Today, if latency_req == 0, it returns the 0th idle state, so polling.

If we jump to the arch_cpu_idle_poll, the result will be the same for 
all architecture.



preparing the removal of the CPUIDLE_DRIVER_STATE_START macro which
leads to a lot of mess in the cpuidle code.


How is the suggestion to check the exit_latency of idle state 0 when
latency_req  == 0 going to hinder this removal?


It sounds a bit hackish. I prefer to sort out the current situation.

And by the way, what is the reasoning behind having a target_residency / 
exit_latency equal to zero for an idle state ?


All this sounds really fuzzy for me.


With the removal of this macro, we should be able to move the select
loop from the menu governor and use it everywhere else. Furthermore,
this state which is flagged with TIME_VALID, isn't because the local
interrupt are enabled so we are measuring the interrupt time processing.
Beside that the idle loop for x86 is mostly not used.

So the idea would be to extract those idle loop from the drivers and use
them directly when:
  1. the idle selection fails (use the poll loop under certain
circumstances we have to redefine)


This behavior will not change as per my suggestion.


  2. when the latency req is zero


Its only here that I suggested you also verify state 0's exit_latency.
For the reason that the arch may have a more optimized idle poll loop,
which we cannot override with the generic cpuidle poll loop.

Regards
Preeti U Murthy


That will result in a cleaner code in cpuidle and in the governor.

Do you agree with that ?


But by having a check on the exit_latency, you are claiming that since
the driver's 0th idle state is no better than the generic idle loop in
cases of 0 latency req, we are better off calling the latter, which
looks reas

Re: [PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Preeti U Murthy]

On 11/05/2014 07:58 PM, Daniel Lezcano wrote:
> On 10/29/2014 03:01 AM, Preeti U Murthy wrote:
>> On 10/29/2014 12:29 AM, Daniel Lezcano wrote:
>>> On 10/28/2014 04:51 AM, Preeti Murthy wrote:
 Hi Daniel,

 On Thu, Oct 23, 2014 at 2:31 PM, Daniel Lezcano
  wrote:
> When the pmqos latency requirement is set to zero that means "poll in
> all the
> cases".
>
> That is correctly implemented on x86 but not on the other archs.
>
> As how is written the code, if the latency request is zero, the
> governor will
> return zero, so corresponding, for x86, to the poll function, but for
> the
> others arch the default idle function. For example, on ARM this is
> wait-for-
> interrupt with a latency of '1', so violating the constraint.

 This is not true actually. On PowerPC the idle state 0 has an
 exit_latency of 0.

>
> In order to fix that, do the latency requirement check *before*
> calling the
> cpuidle framework in order to jump to the poll function without
> entering
> cpuidle. That has several benefits:

 Doing so actually hurts on PowerPC. Because the idle loop defined for
 idle state 0 is different from what cpu_relax() does in
 cpu_idle_loop().
 The spinning is more power efficient in the former case. Moreover we
 also set
 certain register values which indicate an idle cpu. The ppc_runlatch
 bits
 do precisely this. These register values are being read by some user
 space
 tools.  So we will end up breaking them with this patch

 My suggestion is very well keep the latency requirement check in
 kernel/sched/idle.c
 like your doing in this patch. But before jumping to cpu_idle_loop
 verify if the
 idle state 0 has an exit_latency > 0 in addition to your check on the
 latency_req == 0.
 If not, you can fall through to the regular path of calling into the
 cpuidle driver.
 The scheduler can query the cpuidle_driver structure anyway.

 What do you think?
>>>
>>> Thanks for reviewing the patch and spotting this.
>>>
>>> Wouldn't make sense to create:
>>>
>>> void __weak_cpu_idle_poll(void) ?
>>>
>>> and override it with your specific poll function ?
>>>
>>
>> No this would become ugly as far as I can see. A weak function has to be
>> defined under arch/* code. We will either need to duplicate the idle
>> loop that we already have in the drivers or point the weak function to
>> the first idle state defined by our driver. Both of which is not
>> desirable (calling into the driver from arch code is ugly). Another
>> reason why I don't like the idea of a weak function is that if you have
>> missed looking at a specific driver and they have an idle loop with
>> features similar to on powerpc, you will have to spot it yourself and
>> include the arch specific cpu_idle_poll() for them.
> 
> Yes, I agree this is a fair point. But actually I don't see the interest
> of having the poll loop in the cpuidle driver. These cleanups are

We can't do that simply because the idle poll loop has arch specific
bits on powerpc.

> preparing the removal of the CPUIDLE_DRIVER_STATE_START macro which
> leads to a lot of mess in the cpuidle code.

How is the suggestion to check the exit_latency of idle state 0 when
latency_req  == 0 going to hinder this removal?

> 
> With the removal of this macro, we should be able to move the select
> loop from the menu governor and use it everywhere else. Furthermore,
> this state which is flagged with TIME_VALID, isn't because the local
> interrupt are enabled so we are measuring the interrupt time processing.
> Beside that the idle loop for x86 is mostly not used.
> 
> So the idea would be to extract those idle loop from the drivers and use
> them directly when:
>  1. the idle selection fails (use the poll loop under certain
> circumstances we have to redefine)

This behavior will not change as per my suggestion.

>  2. when the latency req is zero

Its only here that I suggested you also verify state 0's exit_latency.
For the reason that the arch may have a more optimized idle poll loop,
which we cannot override with the generic cpuidle poll loop.

Regards
Preeti U Murthy
> 
> That will result in a cleaner code in cpuidle and in the governor.
> 
> Do you agree with that ?
> 
>> But by having a check on the exit_latency, you are claiming that since
>> the driver's 0th idle state is no better than the generic idle loop in
>> cases of 0 latency req, we are better off calling the latter, which
>> looks reasonable. That way you don't have to bother about worsening the
>> idle loop behavior on any other driver.
> 
> 
> 
> 
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Daniel Lezcano]

On 11/05/2014 10:57 PM, Rafael J. Wysocki wrote:

On Thursday, October 23, 2014 11:01:17 AM Daniel Lezcano wrote:

When the pmqos latency requirement is set to zero that means "poll in all the
cases".

That is correctly implemented on x86 but not on the other archs.

As how is written the code, if the latency request is zero, the governor will
return zero, so corresponding, for x86, to the poll function, but for the
others arch the default idle function. For example, on ARM this is wait-for-
interrupt with a latency of '1', so violating the constraint.

In order to fix that, do the latency requirement check *before* calling the
cpuidle framework in order to jump to the poll function without entering
cpuidle. That has several benefits:

  1. It clarifies and unifies the code
  2. It fixes x86 vs other archs behavior
  3. Factors out the call to the same function
  4. Prevent to enter the cpuidle framework with its expensive cost in
 calculation

As the latency_req is needed in all the cases, change the select API to take
the latency_req as parameter in case it is not equal to zero.

As a positive side effect, it introduces the latency constraint specified
externally, so one more step to the cpuidle/scheduler integration.


I'm expecting to see a new version of this patchset relatively soon.

Are you planning to send one?


I would like to find an agreement with Preeti. But, yes, I am on it.

  -- Daniel

--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Rafael J. Wysocki]

On Thursday, October 23, 2014 11:01:17 AM Daniel Lezcano wrote:
> When the pmqos latency requirement is set to zero that means "poll in all the
> cases".
> 
> That is correctly implemented on x86 but not on the other archs.
> 
> As how is written the code, if the latency request is zero, the governor will
> return zero, so corresponding, for x86, to the poll function, but for the
> others arch the default idle function. For example, on ARM this is wait-for-
> interrupt with a latency of '1', so violating the constraint.
> 
> In order to fix that, do the latency requirement check *before* calling the
> cpuidle framework in order to jump to the poll function without entering
> cpuidle. That has several benefits:
> 
>  1. It clarifies and unifies the code
>  2. It fixes x86 vs other archs behavior
>  3. Factors out the call to the same function
>  4. Prevent to enter the cpuidle framework with its expensive cost in
> calculation
> 
> As the latency_req is needed in all the cases, change the select API to take
> the latency_req as parameter in case it is not equal to zero.
> 
> As a positive side effect, it introduces the latency constraint specified
> externally, so one more step to the cpuidle/scheduler integration.

I'm expecting to see a new version of this patchset relatively soon.

Are you planning to send one?

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Daniel Lezcano]

On 10/29/2014 03:01 AM, Preeti U Murthy wrote:

On 10/29/2014 12:29 AM, Daniel Lezcano wrote:

On 10/28/2014 04:51 AM, Preeti Murthy wrote:

Hi Daniel,

On Thu, Oct 23, 2014 at 2:31 PM, Daniel Lezcano
 wrote:

When the pmqos latency requirement is set to zero that means "poll in
all the
cases".

That is correctly implemented on x86 but not on the other archs.

As how is written the code, if the latency request is zero, the
governor will
return zero, so corresponding, for x86, to the poll function, but for
the
others arch the default idle function. For example, on ARM this is
wait-for-
interrupt with a latency of '1', so violating the constraint.


This is not true actually. On PowerPC the idle state 0 has an
exit_latency of 0.



In order to fix that, do the latency requirement check *before*
calling the
cpuidle framework in order to jump to the poll function without entering
cpuidle. That has several benefits:


Doing so actually hurts on PowerPC. Because the idle loop defined for
idle state 0 is different from what cpu_relax() does in cpu_idle_loop().
The spinning is more power efficient in the former case. Moreover we
also set
certain register values which indicate an idle cpu. The ppc_runlatch bits
do precisely this. These register values are being read by some user
space
tools.  So we will end up breaking them with this patch

My suggestion is very well keep the latency requirement check in
kernel/sched/idle.c
like your doing in this patch. But before jumping to cpu_idle_loop
verify if the
idle state 0 has an exit_latency > 0 in addition to your check on the
latency_req == 0.
If not, you can fall through to the regular path of calling into the
cpuidle driver.
The scheduler can query the cpuidle_driver structure anyway.

What do you think?


Thanks for reviewing the patch and spotting this.

Wouldn't make sense to create:

void __weak_cpu_idle_poll(void) ?

and override it with your specific poll function ?



No this would become ugly as far as I can see. A weak function has to be
defined under arch/* code. We will either need to duplicate the idle
loop that we already have in the drivers or point the weak function to
the first idle state defined by our driver. Both of which is not
desirable (calling into the driver from arch code is ugly). Another
reason why I don't like the idea of a weak function is that if you have
missed looking at a specific driver and they have an idle loop with
features similar to on powerpc, you will have to spot it yourself and
include the arch specific cpu_idle_poll() for them.


Yes, I agree this is a fair point. But actually I don't see the interest 
of having the poll loop in the cpuidle driver. These cleanups are 
preparing the removal of the CPUIDLE_DRIVER_STATE_START macro which 
leads to a lot of mess in the cpuidle code.


With the removal of this macro, we should be able to move the select 
loop from the menu governor and use it everywhere else. Furthermore, 
this state which is flagged with TIME_VALID, isn't because the local 
interrupt are enabled so we are measuring the interrupt time processing.

Beside that the idle loop for x86 is mostly not used.

So the idea would be to extract those idle loop from the drivers and use 
them directly when:
 1. the idle selection fails (use the poll loop under certain 
circumstances we have to redefine)

 2. when the latency req is zero

That will result in a cleaner code in cpuidle and in the governor.

Do you agree with that ?


But by having a check on the exit_latency, you are claiming that since
the driver's 0th idle state is no better than the generic idle loop in
cases of 0 latency req, we are better off calling the latter, which
looks reasonable. That way you don't have to bother about worsening the
idle loop behavior on any other driver.






--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Preeti U Murthy]

On 10/29/2014 12:29 AM, Daniel Lezcano wrote:
> On 10/28/2014 04:51 AM, Preeti Murthy wrote:
>> Hi Daniel,
>>
>> On Thu, Oct 23, 2014 at 2:31 PM, Daniel Lezcano
>>  wrote:
>>> When the pmqos latency requirement is set to zero that means "poll in
>>> all the
>>> cases".
>>>
>>> That is correctly implemented on x86 but not on the other archs.
>>>
>>> As how is written the code, if the latency request is zero, the
>>> governor will
>>> return zero, so corresponding, for x86, to the poll function, but for
>>> the
>>> others arch the default idle function. For example, on ARM this is
>>> wait-for-
>>> interrupt with a latency of '1', so violating the constraint.
>>
>> This is not true actually. On PowerPC the idle state 0 has an
>> exit_latency of 0.
>>
>>>
>>> In order to fix that, do the latency requirement check *before*
>>> calling the
>>> cpuidle framework in order to jump to the poll function without entering
>>> cpuidle. That has several benefits:
>>
>> Doing so actually hurts on PowerPC. Because the idle loop defined for
>> idle state 0 is different from what cpu_relax() does in cpu_idle_loop().
>> The spinning is more power efficient in the former case. Moreover we
>> also set
>> certain register values which indicate an idle cpu. The ppc_runlatch bits
>> do precisely this. These register values are being read by some user
>> space
>> tools.  So we will end up breaking them with this patch
>>
>> My suggestion is very well keep the latency requirement check in
>> kernel/sched/idle.c
>> like your doing in this patch. But before jumping to cpu_idle_loop
>> verify if the
>> idle state 0 has an exit_latency > 0 in addition to your check on the
>> latency_req == 0.
>> If not, you can fall through to the regular path of calling into the
>> cpuidle driver.
>> The scheduler can query the cpuidle_driver structure anyway.
>>
>> What do you think?
> 
> Thanks for reviewing the patch and spotting this.
> 
> Wouldn't make sense to create:
> 
> void __weak_cpu_idle_poll(void) ?
> 
> and override it with your specific poll function ?
> 

No this would become ugly as far as I can see. A weak function has to be
defined under arch/* code. We will either need to duplicate the idle
loop that we already have in the drivers or point the weak function to
the first idle state defined by our driver. Both of which is not
desirable (calling into the driver from arch code is ugly). Another
reason why I don't like the idea of a weak function is that if you have
missed looking at a specific driver and they have an idle loop with
features similar to on powerpc, you will have to spot it yourself and
include the arch specific cpu_idle_poll() for them.

But by having a check on the exit_latency, you are claiming that since
the driver's 0th idle state is no better than the generic idle loop in
cases of 0 latency req, we are better off calling the latter, which
looks reasonable. That way you don't have to bother about worsening the
idle loop behavior on any other driver.

Regards
Preeti U Murthy

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Daniel Lezcano]

On 10/28/2014 04:51 AM, Preeti Murthy wrote:

Hi Daniel,

On Thu, Oct 23, 2014 at 2:31 PM, Daniel Lezcano
 wrote:

When the pmqos latency requirement is set to zero that means "poll in all the
cases".

That is correctly implemented on x86 but not on the other archs.

As how is written the code, if the latency request is zero, the governor will
return zero, so corresponding, for x86, to the poll function, but for the
others arch the default idle function. For example, on ARM this is wait-for-
interrupt with a latency of '1', so violating the constraint.


This is not true actually. On PowerPC the idle state 0 has an exit_latency of 0.



In order to fix that, do the latency requirement check *before* calling the
cpuidle framework in order to jump to the poll function without entering
cpuidle. That has several benefits:


Doing so actually hurts on PowerPC. Because the idle loop defined for
idle state 0 is different from what cpu_relax() does in cpu_idle_loop().
The spinning is more power efficient in the former case. Moreover we also set
certain register values which indicate an idle cpu. The ppc_runlatch bits
do precisely this. These register values are being read by some user space
tools.  So we will end up breaking them with this patch

My suggestion is very well keep the latency requirement check in
kernel/sched/idle.c
like your doing in this patch. But before jumping to cpu_idle_loop verify if the
idle state 0 has an exit_latency > 0 in addition to your check on the
latency_req == 0.
If not, you can fall through to the regular path of calling into the
cpuidle driver.
The scheduler can query the cpuidle_driver structure anyway.

What do you think?


Thanks for reviewing the patch and spotting this.

Wouldn't make sense to create:

void __weak_cpu_idle_poll(void) ?

and override it with your specific poll function ?

--
  Linaro.org │ Open source software for ARM SoCs

Follow Linaro:   Facebook |
 Twitter |
 Blog

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Re: [PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Preeti Murthy]

Hi Daniel,

On Thu, Oct 23, 2014 at 2:31 PM, Daniel Lezcano
 wrote:
> When the pmqos latency requirement is set to zero that means "poll in all the
> cases".
>
> That is correctly implemented on x86 but not on the other archs.
>
> As how is written the code, if the latency request is zero, the governor will
> return zero, so corresponding, for x86, to the poll function, but for the
> others arch the default idle function. For example, on ARM this is wait-for-
> interrupt with a latency of '1', so violating the constraint.

This is not true actually. On PowerPC the idle state 0 has an exit_latency of 0.

>
> In order to fix that, do the latency requirement check *before* calling the
> cpuidle framework in order to jump to the poll function without entering
> cpuidle. That has several benefits:

Doing so actually hurts on PowerPC. Because the idle loop defined for
idle state 0 is different from what cpu_relax() does in cpu_idle_loop().
The spinning is more power efficient in the former case. Moreover we also set
certain register values which indicate an idle cpu. The ppc_runlatch bits
do precisely this. These register values are being read by some user space
tools.  So we will end up breaking them with this patch

My suggestion is very well keep the latency requirement check in
kernel/sched/idle.c
like your doing in this patch. But before jumping to cpu_idle_loop verify if the
idle state 0 has an exit_latency > 0 in addition to your check on the
latency_req == 0.
If not, you can fall through to the regular path of calling into the
cpuidle driver.
The scheduler can query the cpuidle_driver structure anyway.

What do you think?

Regards
Preeti U Murthy
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[PATCH V2 1/5] sched: idle: cpuidle: Check the latency req before idle

[Daniel Lezcano]

When the pmqos latency requirement is set to zero that means "poll in all the
cases".

That is correctly implemented on x86 but not on the other archs.

As how is written the code, if the latency request is zero, the governor will
return zero, so corresponding, for x86, to the poll function, but for the
others arch the default idle function. For example, on ARM this is wait-for-
interrupt with a latency of '1', so violating the constraint.

In order to fix that, do the latency requirement check *before* calling the
cpuidle framework in order to jump to the poll function without entering
cpuidle. That has several benefits:

 1. It clarifies and unifies the code
 2. It fixes x86 vs other archs behavior
 3. Factors out the call to the same function
 4. Prevent to enter the cpuidle framework with its expensive cost in
calculation

As the latency_req is needed in all the cases, change the select API to take
the latency_req as parameter in case it is not equal to zero.

As a positive side effect, it introduces the latency constraint specified
externally, so one more step to the cpuidle/scheduler integration.

Signed-off-by: Daniel Lezcano 
Acked-by: Nicolas Pitre 
---
 drivers/cpuidle/cpuidle.c  |  5 +++--
 drivers/cpuidle/governors/ladder.c |  9 +
 drivers/cpuidle/governors/menu.c   |  8 ++--
 include/linux/cpuidle.h|  7 ---
 kernel/sched/idle.c| 18 ++
 5 files changed, 24 insertions(+), 23 deletions(-)

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index ee9df5e..372c36f 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -158,7 +158,8 @@ int cpuidle_enter_state(struct cpuidle_device *dev, struct 
cpuidle_driver *drv,
  *
  * Returns the index of the idle state.
  */
-int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
+int cpuidle_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
+  int latency_req)
 {
if (off || !initialized)
return -ENODEV;
@@ -169,7 +170,7 @@ int cpuidle_select(struct cpuidle_driver *drv, struct 
cpuidle_device *dev)
if (unlikely(use_deepest_state))
return cpuidle_find_deepest_state(drv, dev);
 
-   return cpuidle_curr_governor->select(drv, dev);
+   return cpuidle_curr_governor->select(drv, dev, latency_req);
 }
 
 /**
diff --git a/drivers/cpuidle/governors/ladder.c 
b/drivers/cpuidle/governors/ladder.c
index 044ee0d..18f0da9 100644
--- a/drivers/cpuidle/governors/ladder.c
+++ b/drivers/cpuidle/governors/ladder.c
@@ -64,18 +64,11 @@ static inline void ladder_do_selection(struct ladder_device 
*ldev,
  * @dev: the CPU
  */
 static int ladder_select_state(struct cpuidle_driver *drv,
-   struct cpuidle_device *dev)
+  struct cpuidle_device *dev, int latency_req)
 {
struct ladder_device *ldev = &__get_cpu_var(ladder_devices);
struct ladder_device_state *last_state;
int last_residency, last_idx = ldev->last_state_idx;
-   int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
-
-   /* Special case when user has set very strict latency requirement */
-   if (unlikely(latency_req == 0)) {
-   ladder_do_selection(ldev, last_idx, 0);
-   return 0;
-   }
 
last_state = &ldev->states[last_idx];
 
diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index 34db2fb..96f8fb0 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c
@@ -287,10 +287,10 @@ again:
  * @drv: cpuidle driver containing state data
  * @dev: the CPU
  */
-static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
+static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev,
+  int latency_req)
 {
struct menu_device *data = &__get_cpu_var(menu_devices);
-   int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
int i;
unsigned int interactivity_req;
unsigned long nr_iowaiters, cpu_load;
@@ -302,10 +302,6 @@ static int menu_select(struct cpuidle_driver *drv, struct 
cpuidle_device *dev)
 
data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1;
 
-   /* Special case when user has set very strict latency requirement */
-   if (unlikely(latency_req == 0))
-   return 0;
-
/* determine the expected residency time, round up */
data->next_timer_us = ktime_to_us(tick_nohz_get_sleep_length());
 
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 25e0df6..fb465c1 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -122,7 +122,7 @@ struct cpuidle_driver {
 extern void disable_cpuidle(void);
 
 extern int cpuidle_select(struct cpuidle_driver *drv,
- struct cpuidle_device *dev);
+ struct cpuidle_device *dev, int latency_re