diff mbox

[RFC,1/2] cpuidle: Add idle enter/exit time stamp for notifying current idle state.

Message ID 1364804657-16590-2-git-send-email-jonghwa3.lee@samsung.com (mailing list archive)
State RFC, archived
Headers show

Commit Message

Jonghwa Lee April 1, 2013, 8:24 a.m. UTC
This patch adds idle state time stamp to cpuidle device structure to
notify its current idle state. If last enter time is newer than last
exit time, then it means that the core is in idle now.

Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
---
 drivers/cpuidle/cpuidle.c |    8 ++++----
 include/linux/cpuidle.h   |    4 ++++
 2 files changed, 8 insertions(+), 4 deletions(-)

Comments

Daniel Lezcano April 2, 2013, 5 a.m. UTC | #1
On 04/01/2013 10:24 AM, Jonghwa Lee wrote:
> This patch adds idle state time stamp to cpuidle device structure to
> notify its current idle state. If last enter time is newer than last
> exit time, then it means that the core is in idle now.
> 
> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
> ---

The patch description does not explain what problem you want to solve,
how to solve it and the patch itself shows nothing.

Could you elaborate ?

Thanks
  -- Daniel

>  drivers/cpuidle/cpuidle.c |    8 ++++----
>  include/linux/cpuidle.h   |    4 ++++
>  2 files changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
> index eba6929..1e830cc 100644
> --- a/drivers/cpuidle/cpuidle.c
> +++ b/drivers/cpuidle/cpuidle.c
> @@ -233,18 +233,18 @@ int cpuidle_wrap_enter(struct cpuidle_device *dev,
>  				int (*enter)(struct cpuidle_device *dev,
>  					struct cpuidle_driver *drv, int index))
>  {
> -	ktime_t time_start, time_end;
>  	s64 diff;
>  
> -	time_start = ktime_get();
> +	dev->last_idle_start = ktime_get();
>  
>  	index = enter(dev, drv, index);
>  
> -	time_end = ktime_get();
> +	dev->last_idle_end = ktime_get();
>  
>  	local_irq_enable();
>  
> -	diff = ktime_to_us(ktime_sub(time_end, time_start));
> +	diff = ktime_to_us(ktime_sub(dev->last_idle_end,
> +				dev->last_idle_start));
>  	if (diff > INT_MAX)
>  		diff = INT_MAX;
>  
> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
> index 480c14d..d1af05f 100644
> --- a/include/linux/cpuidle.h
> +++ b/include/linux/cpuidle.h
> @@ -16,6 +16,7 @@
>  #include <linux/kobject.h>
>  #include <linux/completion.h>
>  #include <linux/hrtimer.h>
> +#include <linux/ktime.h>
>  
>  #define CPUIDLE_STATE_MAX	8
>  #define CPUIDLE_NAME_LEN	16
> @@ -74,6 +75,9 @@ struct cpuidle_device {
>  	struct kobject		kobj;
>  	struct completion	kobj_unregister;
>  
> +	ktime_t			last_idle_start;
> +	ktime_t			last_idle_end;
> +
>  #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
>  	int			safe_state_index;
>  	cpumask_t		coupled_cpus;
>
Jonghwa Lee April 2, 2013, 6:17 a.m. UTC | #2
On 2013? 04? 02? 14:00, Daniel Lezcano wrote:

> On 04/01/2013 10:24 AM, Jonghwa Lee wrote:
>> This patch adds idle state time stamp to cpuidle device structure to
>> notify its current idle state. If last enter time is newer than last
>> exit time, then it means that the core is in idle now.
>>
>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>> ---
> 
> The patch description does not explain what problem you want to solve,
> how to solve it and the patch itself shows nothing.
> 
> Could you elaborate ?


I'm sorry for lacking description. I supplement more.

This patch does add time-stamp for idle enter/exit only nothing more.
The reason why I needed them is that I wanted to know current cpu idle
state. It is hard to know whether cpu is in idle or not now.
When I check the cpuidle state usage, sometimes the information is wrong.
Because it is updated only when the cpu exits the idle state. So while the
cpu is idling, the cpuidle state usage holds past one. Therefore I put
the time-stamp for cpuidle enter/exit for checking current idling and
calculating idle state usage correctly.

I just make this patch temporary for my cpufreq governor work. So, it just
use time-stamp for all idle state together. After RFC working, I have a plan
to update this patch to use timestamp for each idle state.

Thanks,


And it also can be used to calculate cpuidle state usage even the cpu is idling.
I knew the code looks worthless, but as this patchset is just
suggesting idea,


> 
> Thanks
>   -- Daniel
> 
>>  drivers/cpuidle/cpuidle.c |    8 ++++----
>>  include/linux/cpuidle.h   |    4 ++++
>>  2 files changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
>> index eba6929..1e830cc 100644
>> --- a/drivers/cpuidle/cpuidle.c
>> +++ b/drivers/cpuidle/cpuidle.c
>> @@ -233,18 +233,18 @@ int cpuidle_wrap_enter(struct cpuidle_device *dev,
>>  				int (*enter)(struct cpuidle_device *dev,
>>  					struct cpuidle_driver *drv, int index))
>>  {
>> -	ktime_t time_start, time_end;
>>  	s64 diff;
>>  
>> -	time_start = ktime_get();
>> +	dev->last_idle_start = ktime_get();
>>  
>>  	index = enter(dev, drv, index);
>>  
>> -	time_end = ktime_get();
>> +	dev->last_idle_end = ktime_get();
>>  
>>  	local_irq_enable();
>>  
>> -	diff = ktime_to_us(ktime_sub(time_end, time_start));
>> +	diff = ktime_to_us(ktime_sub(dev->last_idle_end,
>> +				dev->last_idle_start));
>>  	if (diff > INT_MAX)
>>  		diff = INT_MAX;
>>  
>> diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
>> index 480c14d..d1af05f 100644
>> --- a/include/linux/cpuidle.h
>> +++ b/include/linux/cpuidle.h
>> @@ -16,6 +16,7 @@
>>  #include <linux/kobject.h>
>>  #include <linux/completion.h>
>>  #include <linux/hrtimer.h>
>> +#include <linux/ktime.h>
>>  
>>  #define CPUIDLE_STATE_MAX	8
>>  #define CPUIDLE_NAME_LEN	16
>> @@ -74,6 +75,9 @@ struct cpuidle_device {
>>  	struct kobject		kobj;
>>  	struct completion	kobj_unregister;
>>  
>> +	ktime_t			last_idle_start;
>> +	ktime_t			last_idle_end;
>> +
>>  #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
>>  	int			safe_state_index;
>>  	cpumask_t		coupled_cpus;
>>
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Lezcano April 2, 2013, 7:34 a.m. UTC | #3
On 04/02/2013 08:17 AM, jonghwa3.lee@samsung.com wrote:
> On 2013? 04? 02? 14:00, Daniel Lezcano wrote:
> 
>> On 04/01/2013 10:24 AM, Jonghwa Lee wrote:
>>> This patch adds idle state time stamp to cpuidle device structure to
>>> notify its current idle state. If last enter time is newer than last
>>> exit time, then it means that the core is in idle now.
>>>
>>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>>> ---
>>
>> The patch description does not explain what problem you want to solve,
>> how to solve it and the patch itself shows nothing.
>>
>> Could you elaborate ?
> 
> 
> I'm sorry for lacking description. I supplement more.
> 
> This patch does add time-stamp for idle enter/exit only nothing more.
> The reason why I needed them is that I wanted to know current cpu idle
> state. It is hard to know whether cpu is in idle or not now.

Did you looked at:

include/linux/sched.h:extern int idle_cpu(int cpu);

?

> When I check the cpuidle state usage, sometimes the information is wrong.
> Because it is updated only when the cpu exits the idle state. So while the
> cpu is idling, the cpuidle state usage holds past one. Therefore I put
> the time-stamp for cpuidle enter/exit for checking current idling and
> calculating idle state usage correctly.
> 
> I just make this patch temporary for my cpufreq governor work. So, it just
> use time-stamp for all idle state together. After RFC working, I have a plan
> to update this patch to use timestamp for each idle state.

I suggest you look at the enter_idle / exit_idle function and make your
governor to subscribe to the IDLE_START/EXIT notifiers.

arch/x86/kernel/process.c

These are defined for the x86 architecture, maybe worth to add it to
another architecture.

Thanks
  -- Daniel
Jonghwa Lee April 2, 2013, 9:37 a.m. UTC | #4
On 2013? 04? 02? 16:34, Daniel Lezcano wrote:

> On 04/02/2013 08:17 AM, jonghwa3.lee@samsung.com wrote:
>> On 2013? 04? 02? 14:00, Daniel Lezcano wrote:
>>
>>> On 04/01/2013 10:24 AM, Jonghwa Lee wrote:
>>>> This patch adds idle state time stamp to cpuidle device structure to
>>>> notify its current idle state. If last enter time is newer than last
>>>> exit time, then it means that the core is in idle now.
>>>>
>>>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>>>> ---
>>>
>>> The patch description does not explain what problem you want to solve,
>>> how to solve it and the patch itself shows nothing.
>>>
>>> Could you elaborate ?
>>
>>
>> I'm sorry for lacking description. I supplement more.
>>
>> This patch does add time-stamp for idle enter/exit only nothing more.
>> The reason why I needed them is that I wanted to know current cpu idle
>> state. It is hard to know whether cpu is in idle or not now.
> 
> Did you looked at:
> 
> include/linux/sched.h:extern int idle_cpu(int cpu);
> 


Yes, I did.

> ?
> 
>> When I check the cpuidle state usage, sometimes the information is wrong.
>> Because it is updated only when the cpu exits the idle state. So while the
>> cpu is idling, the cpuidle state usage holds past one. Therefore I put
>> the time-stamp for cpuidle enter/exit for checking current idling and
>> calculating idle state usage correctly.
>>
>> I just make this patch temporary for my cpufreq governor work. So, it just
>> use time-stamp for all idle state together. After RFC working, I have a plan
>> to update this patch to use timestamp for each idle state.
> 
> I suggest you look at the enter_idle / exit_idle function and make your
> governor to subscribe to the IDLE_START/EXIT notifiers.
> 
> arch/x86/kernel/process.c
> 
> These are defined for the x86 architecture, maybe worth to add it to
> another architecture.
> 


Thanks for your opinion.

Actually, I work on ARM architecture and I knew that the attempt of applying
idle notifier was failed. You probably knew it, because the link you gave me
before is that attempt. (https://lkml.org/lkml/2012/2/7/504) :) Currently, there
is only notifying call which is for led in arch/arm/kernel/process.c and I think
it isn't for me to use. Anyway, Do you really think it is better way to use
notifier than my way? Because I think it is too heavy for me. On my board,
sometimes entering idle happened hundreds times during the 100ms. I don't want
to call notifier that much time. IMO, just moving local variable to per-cpu
variable for showing the enter/exit time looks better although it requires code
modification on cpudile side.  What do you think?

Thanks,
Jonghwa.

> Thanks
>   -- Daniel
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Lezcano April 2, 2013, 10:08 a.m. UTC | #5
On 04/02/2013 11:37 AM, jonghwa3.lee@samsung.com wrote:
> On 2013? 04? 02? 16:34, Daniel Lezcano wrote:
> 
>> On 04/02/2013 08:17 AM, jonghwa3.lee@samsung.com wrote:
>>> On 2013? 04? 02? 14:00, Daniel Lezcano wrote:
>>>
>>>> On 04/01/2013 10:24 AM, Jonghwa Lee wrote:
>>>>> This patch adds idle state time stamp to cpuidle device structure to
>>>>> notify its current idle state. If last enter time is newer than last
>>>>> exit time, then it means that the core is in idle now.
>>>>>
>>>>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>>>>> ---
>>>>
>>>> The patch description does not explain what problem you want to solve,
>>>> how to solve it and the patch itself shows nothing.
>>>>
>>>> Could you elaborate ?
>>>
>>>
>>> I'm sorry for lacking description. I supplement more.
>>>
>>> This patch does add time-stamp for idle enter/exit only nothing more.
>>> The reason why I needed them is that I wanted to know current cpu idle
>>> state. It is hard to know whether cpu is in idle or not now.
>>
>> Did you looked at:
>>
>> include/linux/sched.h:extern int idle_cpu(int cpu);
>>
> 
> 
> Yes, I did.
> 
>> ?
>>
>>> When I check the cpuidle state usage, sometimes the information is wrong.
>>> Because it is updated only when the cpu exits the idle state. So while the
>>> cpu is idling, the cpuidle state usage holds past one. Therefore I put
>>> the time-stamp for cpuidle enter/exit for checking current idling and
>>> calculating idle state usage correctly.
>>>
>>> I just make this patch temporary for my cpufreq governor work. So, it just
>>> use time-stamp for all idle state together. After RFC working, I have a plan
>>> to update this patch to use timestamp for each idle state.
>>
>> I suggest you look at the enter_idle / exit_idle function and make your
>> governor to subscribe to the IDLE_START/EXIT notifiers.
>>
>> arch/x86/kernel/process.c
>>
>> These are defined for the x86 architecture, maybe worth to add it to
>> another architecture.
>>
> 
> 
> Thanks for your opinion.
> 
> Actually, I work on ARM architecture and I knew that the attempt of applying
> idle notifier was failed. You probably knew it, because the link you gave me
> before is that attempt. (https://lkml.org/lkml/2012/2/7/504) :)

Yeah, now I recall this thread.

> Currently, there
> is only notifying call which is for led in arch/arm/kernel/process.c and I think
> it isn't for me to use. Anyway, Do you really think it is better way to use
> notifier than my way? Because I think it is too heavy for me. On my board,
> sometimes entering idle happened hundreds times during the 100ms. I don't want
> to call notifier that much time. IMO, just moving local variable to per-cpu
> variable for showing the enter/exit time looks better although it requires code
> modification on cpudile side.  What do you think?

Sorry, but it is hard to figure out what you are trying to achieve with
a single patch.

IIUC, you want to know how long the cpu is idle including the current
state, right ? So you need to know if the cpu is idle and when it
entered the idle state, correct ?

If the cpu is idle and the information is per cpu, how will you read
this value from another cpu without introducing a locking mechanism ?

Does it mean the cpufreq governor needs cpuidle ? I am wondering if
these informations shouldn't be retrieved from the scheduler, not from
cpuidle.
Jonghwa Lee April 2, 2013, 11:07 a.m. UTC | #6
On 2013? 04? 02? 19:08, Daniel Lezcano wrote:

> On 04/02/2013 11:37 AM, jonghwa3.lee@samsung.com wrote:
>> On 2013? 04? 02? 16:34, Daniel Lezcano wrote:
>>
>>> On 04/02/2013 08:17 AM, jonghwa3.lee@samsung.com wrote:
>>>> On 2013? 04? 02? 14:00, Daniel Lezcano wrote:
>>>>
>>>>> On 04/01/2013 10:24 AM, Jonghwa Lee wrote:
>>>>>> This patch adds idle state time stamp to cpuidle device structure to
>>>>>> notify its current idle state. If last enter time is newer than last
>>>>>> exit time, then it means that the core is in idle now.
>>>>>>
>>>>>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>>>>>> ---
>>>>>
>>>>> The patch description does not explain what problem you want to solve,
>>>>> how to solve it and the patch itself shows nothing.
>>>>>
>>>>> Could you elaborate ?
>>>>
>>>>
>>>> I'm sorry for lacking description. I supplement more.
>>>>
>>>> This patch does add time-stamp for idle enter/exit only nothing more.
>>>> The reason why I needed them is that I wanted to know current cpu idle
>>>> state. It is hard to know whether cpu is in idle or not now.
>>>
>>> Did you looked at:
>>>
>>> include/linux/sched.h:extern int idle_cpu(int cpu);
>>>
>>
>>
>> Yes, I did.
>>
>>> ?
>>>
>>>> When I check the cpuidle state usage, sometimes the information is wrong.
>>>> Because it is updated only when the cpu exits the idle state. So while the
>>>> cpu is idling, the cpuidle state usage holds past one. Therefore I put
>>>> the time-stamp for cpuidle enter/exit for checking current idling and
>>>> calculating idle state usage correctly.
>>>>
>>>> I just make this patch temporary for my cpufreq governor work. So, it just
>>>> use time-stamp for all idle state together. After RFC working, I have a plan
>>>> to update this patch to use timestamp for each idle state.
>>>
>>> I suggest you look at the enter_idle / exit_idle function and make your
>>> governor to subscribe to the IDLE_START/EXIT notifiers.
>>>
>>> arch/x86/kernel/process.c
>>>
>>> These are defined for the x86 architecture, maybe worth to add it to
>>> another architecture.
>>>
>>
>>
>> Thanks for your opinion.
>>
>> Actually, I work on ARM architecture and I knew that the attempt of applying
>> idle notifier was failed. You probably knew it, because the link you gave me
>> before is that attempt. (https://lkml.org/lkml/2012/2/7/504) :)
> 
> Yeah, now I recall this thread.
> 


Oh my, I thought you gave the link but you didn't. It was Viresh Kumar from
other patch of the patchset. Sorry.

>> Currently, there
>> is only notifying call which is for led in arch/arm/kernel/process.c and I think
>> it isn't for me to use. Anyway, Do you really think it is better way to use
>> notifier than my way? Because I think it is too heavy for me. On my board,
>> sometimes entering idle happened hundreds times during the 100ms. I don't want
>> to call notifier that much time. IMO, just moving local variable to per-cpu
>> variable for showing the enter/exit time looks better although it requires code
>> modification on cpudile side.  What do you think?
> 
> Sorry, but it is hard to figure out what you are trying to achieve with
> a single patch.
> 
> IIUC, you want to know how long the cpu is idle including the current
> state, right ? So you need to know if the cpu is idle and when it
> entered the idle state, correct ?
>


Exactly.

 
> If the cpu is idle and the information is per cpu, how will you read
> this value from another cpu without introducing a locking mechanism ?
> 


I think it might be tolerated for incoherency of that data. Governor reads the
data only, and if recoded start time or end time are different in few usec with
real one then it doesn't effect to the result much.


> Does it mean the cpufreq governor needs cpuidle ? I am wondering if
> these informations shouldn't be retrieved from the scheduler, not from
> cpuidle.
> 


Yes, tick_sched per-cpu variable has all information that I need. But it isn't
global variable. And I'm afraid to change static variable to global one as my
pleases.


Thanks,
Jonghwa

--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Daniel Lezcano April 2, 2013, 11:18 a.m. UTC | #7
On 04/02/2013 01:07 PM, jonghwa3.lee@samsung.com wrote:
> On 2013? 04? 02? 19:08, Daniel Lezcano wrote:
> 
>> On 04/02/2013 11:37 AM, jonghwa3.lee@samsung.com wrote:
>>> On 2013? 04? 02? 16:34, Daniel Lezcano wrote:
>>>
>>>> On 04/02/2013 08:17 AM, jonghwa3.lee@samsung.com wrote:
>>>>> On 2013? 04? 02? 14:00, Daniel Lezcano wrote:
>>>>>
>>>>>> On 04/01/2013 10:24 AM, Jonghwa Lee wrote:
>>>>>>> This patch adds idle state time stamp to cpuidle device structure to
>>>>>>> notify its current idle state. If last enter time is newer than last
>>>>>>> exit time, then it means that the core is in idle now.
>>>>>>>
>>>>>>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>>>>>>> ---
>>>>>>
>>>>>> The patch description does not explain what problem you want to solve,
>>>>>> how to solve it and the patch itself shows nothing.
>>>>>>
>>>>>> Could you elaborate ?
>>>>>
>>>>>
>>>>> I'm sorry for lacking description. I supplement more.
>>>>>
>>>>> This patch does add time-stamp for idle enter/exit only nothing more.
>>>>> The reason why I needed them is that I wanted to know current cpu idle
>>>>> state. It is hard to know whether cpu is in idle or not now.
>>>>
>>>> Did you looked at:
>>>>
>>>> include/linux/sched.h:extern int idle_cpu(int cpu);
>>>>
>>>
>>>
>>> Yes, I did.
>>>
>>>> ?
>>>>
>>>>> When I check the cpuidle state usage, sometimes the information is wrong.
>>>>> Because it is updated only when the cpu exits the idle state. So while the
>>>>> cpu is idling, the cpuidle state usage holds past one. Therefore I put
>>>>> the time-stamp for cpuidle enter/exit for checking current idling and
>>>>> calculating idle state usage correctly.
>>>>>
>>>>> I just make this patch temporary for my cpufreq governor work. So, it just
>>>>> use time-stamp for all idle state together. After RFC working, I have a plan
>>>>> to update this patch to use timestamp for each idle state.
>>>>
>>>> I suggest you look at the enter_idle / exit_idle function and make your
>>>> governor to subscribe to the IDLE_START/EXIT notifiers.
>>>>
>>>> arch/x86/kernel/process.c
>>>>
>>>> These are defined for the x86 architecture, maybe worth to add it to
>>>> another architecture.
>>>>
>>>
>>>
>>> Thanks for your opinion.
>>>
>>> Actually, I work on ARM architecture and I knew that the attempt of applying
>>> idle notifier was failed. You probably knew it, because the link you gave me
>>> before is that attempt. (https://lkml.org/lkml/2012/2/7/504) :)
>>
>> Yeah, now I recall this thread.
>>
> 
> 
> Oh my, I thought you gave the link but you didn't. It was Viresh Kumar from
> other patch of the patchset. Sorry.
> 
>>> Currently, there
>>> is only notifying call which is for led in arch/arm/kernel/process.c and I think
>>> it isn't for me to use. Anyway, Do you really think it is better way to use
>>> notifier than my way? Because I think it is too heavy for me. On my board,
>>> sometimes entering idle happened hundreds times during the 100ms. I don't want
>>> to call notifier that much time. IMO, just moving local variable to per-cpu
>>> variable for showing the enter/exit time looks better although it requires code
>>> modification on cpudile side.  What do you think?
>>
>> Sorry, but it is hard to figure out what you are trying to achieve with
>> a single patch.
>>
>> IIUC, you want to know how long the cpu is idle including the current
>> state, right ? So you need to know if the cpu is idle and when it
>> entered the idle state, correct ?
>>
> 
> 
> Exactly.
> 
>  
>> If the cpu is idle and the information is per cpu, how will you read
>> this value from another cpu without introducing a locking mechanism ?
>>
> 
> 
> I think it might be tolerated for incoherency of that data. Governor reads the
> data only, and if recoded start time or end time are different in few usec with
> real one then it doesn't effect to the result much.
> 
> 
>> Does it mean the cpufreq governor needs cpuidle ? I am wondering if
>> these informations shouldn't be retrieved from the scheduler, not from
>> cpuidle.
>>
> 
> 
> Yes, tick_sched per-cpu variable has all information that I need. But it isn't
> global variable. And I'm afraid to change static variable to global one as my
> pleases.

It is a global variable but there is a function to get access:

extern struct tick_sched *tick_get_tick_sched(int cpu);

Does it fit better for what you want to achieve ?

Thanks
  -- Daniel
Jonghwa Lee April 3, 2013, 6:10 a.m. UTC | #8
On 2013? 04? 02? 20:18, Daniel Lezcano wrote:

> On 04/02/2013 01:07 PM, jonghwa3.lee@samsung.com wrote:
>> On 2013? 04? 02? 19:08, Daniel Lezcano wrote:
>>
>>> On 04/02/2013 11:37 AM, jonghwa3.lee@samsung.com wrote:
>>>> On 2013? 04? 02? 16:34, Daniel Lezcano wrote:
>>>>
>>>>> On 04/02/2013 08:17 AM, jonghwa3.lee@samsung.com wrote:
>>>>>> On 2013? 04? 02? 14:00, Daniel Lezcano wrote:
>>>>>>
>>>>>>> On 04/01/2013 10:24 AM, Jonghwa Lee wrote:
>>>>>>>> This patch adds idle state time stamp to cpuidle device structure to
>>>>>>>> notify its current idle state. If last enter time is newer than last
>>>>>>>> exit time, then it means that the core is in idle now.
>>>>>>>>
>>>>>>>> Signed-off-by: Jonghwa Lee <jonghwa3.lee@samsung.com>
>>>>>>>> ---
>>>>>>>
>>>>>>> The patch description does not explain what problem you want to solve,
>>>>>>> how to solve it and the patch itself shows nothing.
>>>>>>>
>>>>>>> Could you elaborate ?
>>>>>>
>>>>>>
>>>>>> I'm sorry for lacking description. I supplement more.
>>>>>>
>>>>>> This patch does add time-stamp for idle enter/exit only nothing more.
>>>>>> The reason why I needed them is that I wanted to know current cpu idle
>>>>>> state. It is hard to know whether cpu is in idle or not now.
>>>>>
>>>>> Did you looked at:
>>>>>
>>>>> include/linux/sched.h:extern int idle_cpu(int cpu);
>>>>>
>>>>
>>>>
>>>> Yes, I did.
>>>>
>>>>> ?
>>>>>
>>>>>> When I check the cpuidle state usage, sometimes the information is wrong.
>>>>>> Because it is updated only when the cpu exits the idle state. So while the
>>>>>> cpu is idling, the cpuidle state usage holds past one. Therefore I put
>>>>>> the time-stamp for cpuidle enter/exit for checking current idling and
>>>>>> calculating idle state usage correctly.
>>>>>>
>>>>>> I just make this patch temporary for my cpufreq governor work. So, it just
>>>>>> use time-stamp for all idle state together. After RFC working, I have a plan
>>>>>> to update this patch to use timestamp for each idle state.
>>>>>
>>>>> I suggest you look at the enter_idle / exit_idle function and make your
>>>>> governor to subscribe to the IDLE_START/EXIT notifiers.
>>>>>
>>>>> arch/x86/kernel/process.c
>>>>>
>>>>> These are defined for the x86 architecture, maybe worth to add it to
>>>>> another architecture.
>>>>>
>>>>
>>>>
>>>> Thanks for your opinion.
>>>>
>>>> Actually, I work on ARM architecture and I knew that the attempt of applying
>>>> idle notifier was failed. You probably knew it, because the link you gave me
>>>> before is that attempt. (https://lkml.org/lkml/2012/2/7/504) :)
>>>
>>> Yeah, now I recall this thread.
>>>
>>
>>
>> Oh my, I thought you gave the link but you didn't. It was Viresh Kumar from
>> other patch of the patchset. Sorry.
>>
>>>> Currently, there
>>>> is only notifying call which is for led in arch/arm/kernel/process.c and I think
>>>> it isn't for me to use. Anyway, Do you really think it is better way to use
>>>> notifier than my way? Because I think it is too heavy for me. On my board,
>>>> sometimes entering idle happened hundreds times during the 100ms. I don't want
>>>> to call notifier that much time. IMO, just moving local variable to per-cpu
>>>> variable for showing the enter/exit time looks better although it requires code
>>>> modification on cpudile side.  What do you think?
>>>
>>> Sorry, but it is hard to figure out what you are trying to achieve with
>>> a single patch.
>>>
>>> IIUC, you want to know how long the cpu is idle including the current
>>> state, right ? So you need to know if the cpu is idle and when it
>>> entered the idle state, correct ?
>>>
>>
>>
>> Exactly.
>>
>>  
>>> If the cpu is idle and the information is per cpu, how will you read
>>> this value from another cpu without introducing a locking mechanism ?
>>>
>>
>>
>> I think it might be tolerated for incoherency of that data. Governor reads the
>> data only, and if recoded start time or end time are different in few usec with
>> real one then it doesn't effect to the result much.
>>
>>
>>> Does it mean the cpufreq governor needs cpuidle ? I am wondering if
>>> these informations shouldn't be retrieved from the scheduler, not from
>>> cpuidle.
>>>
>>
>>
>> Yes, tick_sched per-cpu variable has all information that I need. But it isn't
>> global variable. And I'm afraid to change static variable to global one as my
>> pleases.
> 
> It is a global variable but there is a function to get access:
> 
> extern struct tick_sched *tick_get_tick_sched(int cpu);
> 
> Does it fit better for what you want to achieve ?


Yes, it seems exactly what I needed. I'll check it.
Thanks for your advice :)

Thanks,
Jonghwa.

> 
> Thanks
>   -- Daniel
> 
> 


--
To unsubscribe from this list: send the line "unsubscribe linux-pm" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c
index eba6929..1e830cc 100644
--- a/drivers/cpuidle/cpuidle.c
+++ b/drivers/cpuidle/cpuidle.c
@@ -233,18 +233,18 @@  int cpuidle_wrap_enter(struct cpuidle_device *dev,
 				int (*enter)(struct cpuidle_device *dev,
 					struct cpuidle_driver *drv, int index))
 {
-	ktime_t time_start, time_end;
 	s64 diff;
 
-	time_start = ktime_get();
+	dev->last_idle_start = ktime_get();
 
 	index = enter(dev, drv, index);
 
-	time_end = ktime_get();
+	dev->last_idle_end = ktime_get();
 
 	local_irq_enable();
 
-	diff = ktime_to_us(ktime_sub(time_end, time_start));
+	diff = ktime_to_us(ktime_sub(dev->last_idle_end,
+				dev->last_idle_start));
 	if (diff > INT_MAX)
 		diff = INT_MAX;
 
diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index 480c14d..d1af05f 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h
@@ -16,6 +16,7 @@ 
 #include <linux/kobject.h>
 #include <linux/completion.h>
 #include <linux/hrtimer.h>
+#include <linux/ktime.h>
 
 #define CPUIDLE_STATE_MAX	8
 #define CPUIDLE_NAME_LEN	16
@@ -74,6 +75,9 @@  struct cpuidle_device {
 	struct kobject		kobj;
 	struct completion	kobj_unregister;
 
+	ktime_t			last_idle_start;
+	ktime_t			last_idle_end;
+
 #ifdef CONFIG_ARCH_NEEDS_CPU_IDLE_COUPLED
 	int			safe_state_index;
 	cpumask_t		coupled_cpus;