| Message ID | 495ffb1175175b0180ca3da96eb5ed72a8280364.1379779777.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
On 09/22/2013 03:21 AM, Viresh Kumar wrote: > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> The optimization sounds good but IMHO if we can move this state out of the cpuidle common framework that would be nicer. The poll_idle is only applicable for x86 (acpi_driver and intel_idle), hence I suggest we move this state to these drivers, that will cleanup the framework code and will remove index shift macro CPUIDLE_DRIVER_STATE_START which IMHO is weid and prone-to-error. > --- > drivers/cpuidle/cpuidle.c | 41 ----------------------------------------- > drivers/cpuidle/driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 42 insertions(+), 41 deletions(-) > > diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c > index 43d5836..bf80236 100644 > --- a/drivers/cpuidle/cpuidle.c > +++ b/drivers/cpuidle/cpuidle.c > @@ -226,45 +226,6 @@ void cpuidle_resume(void) > mutex_unlock(&cpuidle_lock); > } > > -#ifdef CONFIG_ARCH_HAS_CPU_RELAX > -static int poll_idle(struct cpuidle_device *dev, > - struct cpuidle_driver *drv, int index) > -{ > - ktime_t t1, t2; > - s64 diff; > - > - t1 = ktime_get(); > - local_irq_enable(); > - while (!need_resched()) > - cpu_relax(); > - > - t2 = ktime_get(); > - diff = ktime_to_us(ktime_sub(t2, t1)); > - if (diff > INT_MAX) > - diff = INT_MAX; > - > - dev->last_residency = (int) diff; > - > - return index; > -} > - > -static void poll_idle_init(struct cpuidle_driver *drv) > -{ > - struct cpuidle_state *state = &drv->states[0]; > - > - snprintf(state->name, CPUIDLE_NAME_LEN, "POLL"); > - snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE"); > - state->exit_latency = 0; > - state->target_residency = 0; > - state->power_usage = -1; > - state->flags = 0; > - state->enter = poll_idle; > - state->disabled = false; > -} > -#else > -static void poll_idle_init(struct cpuidle_driver *drv) {} > -#endif /* CONFIG_ARCH_HAS_CPU_RELAX */ > - > /** > * cpuidle_enable_device - enables idle PM for a CPU > * @dev: the CPU > @@ -294,8 +255,6 @@ int cpuidle_enable_device(struct cpuidle_device *dev) > if (!dev->state_count) > dev->state_count = drv->state_count; > > - poll_idle_init(drv); > - > ret = cpuidle_add_device_sysfs(dev); > if (ret) > return ret; > diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c > index 7b2510a..a4a93b4 100644 > --- a/drivers/cpuidle/driver.c > +++ b/drivers/cpuidle/driver.c > @@ -10,6 +10,7 @@ > > #include <linux/mutex.h> > #include <linux/module.h> > +#include <linux/sched.h> > #include <linux/cpuidle.h> > #include <linux/cpumask.h> > #include <linux/clockchips.h> > @@ -179,6 +180,45 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) > } > } > > +#ifdef CONFIG_ARCH_HAS_CPU_RELAX > +static int poll_idle(struct cpuidle_device *dev, > + struct cpuidle_driver *drv, int index) > +{ > + ktime_t t1, t2; > + s64 diff; > + > + t1 = ktime_get(); > + local_irq_enable(); > + while (!need_resched()) > + cpu_relax(); > + > + t2 = ktime_get(); > + diff = ktime_to_us(ktime_sub(t2, t1)); > + if (diff > INT_MAX) > + diff = INT_MAX; > + > + dev->last_residency = (int) diff; > + > + return index; > +} > + > +static void poll_idle_init(struct cpuidle_driver *drv) > +{ > + struct cpuidle_state *state = &drv->states[0]; > + > + snprintf(state->name, CPUIDLE_NAME_LEN, "POLL"); > + snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE"); > + state->exit_latency = 0; > + state->target_residency = 0; > + state->power_usage = -1; > + state->flags = 0; > + state->enter = poll_idle; > + state->disabled = false; > +} > +#else > +static void poll_idle_init(struct cpuidle_driver *drv) {} > +#endif /* !CONFIG_ARCH_HAS_CPU_RELAX */ > + > /** > * __cpuidle_register_driver: register the driver > * @drv: a valid pointer to a struct cpuidle_driver > @@ -212,6 +252,8 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv) > on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer, > (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1); > > + poll_idle_init(drv); > + > return 0; > } > >
On 26 September 2013 03:52, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > On 09/22/2013 03:21 AM, Viresh Kumar wrote: >> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> This deserved a log, sorry for missing that :( > The optimization sounds good but IMHO if we can move this state out of > the cpuidle common framework that would be nicer. > > The poll_idle is only applicable for x86 (acpi_driver and intel_idle), > hence I suggest we move this state to these drivers, that will cleanup > the framework code and will remove index shift macro > CPUIDLE_DRIVER_STATE_START which IMHO is weid and prone-to-error. Lets see what X86 folks have to say about it and then we can do it.. Btw, wouldn't that add some code duplication in those two drivers?
On 09/26/2013 08:09 AM, Viresh Kumar wrote: > On 26 September 2013 03:52, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> On 09/22/2013 03:21 AM, Viresh Kumar wrote: >>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > > This deserved a log, sorry for missing that :( > >> The optimization sounds good but IMHO if we can move this state out of >> the cpuidle common framework that would be nicer. >> >> The poll_idle is only applicable for x86 (acpi_driver and intel_idle), >> hence I suggest we move this state to these drivers, that will cleanup >> the framework code and will remove index shift macro >> CPUIDLE_DRIVER_STATE_START which IMHO is weid and prone-to-error. > > Lets see what X86 folks have to say about it and then we can do it.. > Btw, wouldn't that add some code duplication in those two drivers? Yes, certainly and that will impact also the menu select governor function: ... /* * We want to default to C1 (hlt), not to busy polling * unless the timer is happening really really soon. */ if (data->expected_us > 5 && !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) data->last_state_idx = CPUIDLE_DRIVER_STATE_START; /* * Find the idle state with the lowest power while satisfying * our constraints. */ for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { struct cpuidle_state *s = &drv->states[i]; struct cpuidle_state_usage *su = &dev->states_usage[i]; if (s->disabled || su->disable) continue; if (s->target_residency > data->predicted_us) continue; if (s->exit_latency > latency_req) continue; if (s->exit_latency * multiplier > data->predicted_us) continue; data->last_state_idx = i; data->exit_us = s->exit_latency; } ....
On 26 September 2013 13:58, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: > Yes, certainly and that will impact also the menu select governor function: > > ... > > /* > * We want to default to C1 (hlt), not to busy polling > * unless the timer is happening really really soon. > */ > if (data->expected_us > 5 && > !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && > dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) > data->last_state_idx = CPUIDLE_DRIVER_STATE_START; > > /* > * Find the idle state with the lowest power while satisfying > * our constraints. > */ > for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { > struct cpuidle_state *s = &drv->states[i]; > struct cpuidle_state_usage *su = &dev->states_usage[i]; > > if (s->disabled || su->disable) > continue; > if (s->target_residency > data->predicted_us) > continue; > if (s->exit_latency > latency_req) > continue; > if (s->exit_latency * multiplier > data->predicted_us) > continue; > > data->last_state_idx = i; > data->exit_us = s->exit_latency; > } Hmm.. For now I will repost this patch as is and then you can go ahead for this bigger change.. I wouldn't be able to do this change now, as I would be rushing for my 2 weeks vacations :) If this patch looked okay to you, can you please Ack it ?
On 10/03/2013 12:33 PM, Viresh Kumar wrote: > On 26 September 2013 13:58, Daniel Lezcano <daniel.lezcano@linaro.org> wrote: >> Yes, certainly and that will impact also the menu select governor function: >> >> ... >> >> /* >> * We want to default to C1 (hlt), not to busy polling >> * unless the timer is happening really really soon. >> */ >> if (data->expected_us > 5 && >> !drv->states[CPUIDLE_DRIVER_STATE_START].disabled && >> dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable == 0) >> data->last_state_idx = CPUIDLE_DRIVER_STATE_START; >> >> /* >> * Find the idle state with the lowest power while satisfying >> * our constraints. >> */ >> for (i = CPUIDLE_DRIVER_STATE_START; i < drv->state_count; i++) { >> struct cpuidle_state *s = &drv->states[i]; >> struct cpuidle_state_usage *su = &dev->states_usage[i]; >> >> if (s->disabled || su->disable) >> continue; >> if (s->target_residency > data->predicted_us) >> continue; >> if (s->exit_latency > latency_req) >> continue; >> if (s->exit_latency * multiplier > data->predicted_us) >> continue; >> >> data->last_state_idx = i; >> data->exit_us = s->exit_latency; >> } > > Hmm.. For now I will repost this patch as is and then you can go ahead > for this bigger change.. I wouldn't be able to do this change now, as I > would be rushing for my 2 weeks vacations :) > > If this patch looked okay to you, can you please Ack it ? Acked-by: Daniel Lezcano <daniel.lezcano@linaro.org>
diff --git a/drivers/cpuidle/cpuidle.c b/drivers/cpuidle/cpuidle.c index 43d5836..bf80236 100644 --- a/drivers/cpuidle/cpuidle.c +++ b/drivers/cpuidle/cpuidle.c @@ -226,45 +226,6 @@ void cpuidle_resume(void) mutex_unlock(&cpuidle_lock); } -#ifdef CONFIG_ARCH_HAS_CPU_RELAX -static int poll_idle(struct cpuidle_device *dev, - struct cpuidle_driver *drv, int index) -{ - ktime_t t1, t2; - s64 diff; - - t1 = ktime_get(); - local_irq_enable(); - while (!need_resched()) - cpu_relax(); - - t2 = ktime_get(); - diff = ktime_to_us(ktime_sub(t2, t1)); - if (diff > INT_MAX) - diff = INT_MAX; - - dev->last_residency = (int) diff; - - return index; -} - -static void poll_idle_init(struct cpuidle_driver *drv) -{ - struct cpuidle_state *state = &drv->states[0]; - - snprintf(state->name, CPUIDLE_NAME_LEN, "POLL"); - snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE"); - state->exit_latency = 0; - state->target_residency = 0; - state->power_usage = -1; - state->flags = 0; - state->enter = poll_idle; - state->disabled = false; -} -#else -static void poll_idle_init(struct cpuidle_driver *drv) {} -#endif /* CONFIG_ARCH_HAS_CPU_RELAX */ - /** * cpuidle_enable_device - enables idle PM for a CPU * @dev: the CPU @@ -294,8 +255,6 @@ int cpuidle_enable_device(struct cpuidle_device *dev) if (!dev->state_count) dev->state_count = drv->state_count; - poll_idle_init(drv); - ret = cpuidle_add_device_sysfs(dev); if (ret) return ret; diff --git a/drivers/cpuidle/driver.c b/drivers/cpuidle/driver.c index 7b2510a..a4a93b4 100644 --- a/drivers/cpuidle/driver.c +++ b/drivers/cpuidle/driver.c @@ -10,6 +10,7 @@ #include <linux/mutex.h> #include <linux/module.h> +#include <linux/sched.h> #include <linux/cpuidle.h> #include <linux/cpumask.h> #include <linux/clockchips.h> @@ -179,6 +180,45 @@ static void __cpuidle_driver_init(struct cpuidle_driver *drv) } } +#ifdef CONFIG_ARCH_HAS_CPU_RELAX +static int poll_idle(struct cpuidle_device *dev, + struct cpuidle_driver *drv, int index) +{ + ktime_t t1, t2; + s64 diff; + + t1 = ktime_get(); + local_irq_enable(); + while (!need_resched()) + cpu_relax(); + + t2 = ktime_get(); + diff = ktime_to_us(ktime_sub(t2, t1)); + if (diff > INT_MAX) + diff = INT_MAX; + + dev->last_residency = (int) diff; + + return index; +} + +static void poll_idle_init(struct cpuidle_driver *drv) +{ + struct cpuidle_state *state = &drv->states[0]; + + snprintf(state->name, CPUIDLE_NAME_LEN, "POLL"); + snprintf(state->desc, CPUIDLE_DESC_LEN, "CPUIDLE CORE POLL IDLE"); + state->exit_latency = 0; + state->target_residency = 0; + state->power_usage = -1; + state->flags = 0; + state->enter = poll_idle; + state->disabled = false; +} +#else +static void poll_idle_init(struct cpuidle_driver *drv) {} +#endif /* !CONFIG_ARCH_HAS_CPU_RELAX */ + /** * __cpuidle_register_driver: register the driver * @drv: a valid pointer to a struct cpuidle_driver @@ -212,6 +252,8 @@ static int __cpuidle_register_driver(struct cpuidle_driver *drv) on_each_cpu_mask(drv->cpumask, cpuidle_setup_broadcast_timer, (void *)CLOCK_EVT_NOTIFY_BROADCAST_ON, 1); + poll_idle_init(drv); + return 0; }
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- drivers/cpuidle/cpuidle.c | 41 ----------------------------------------- drivers/cpuidle/driver.c | 42 ++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 42 insertions(+), 41 deletions(-)