On Thu, Jan 19, 2017 at 05:25:37PM +0800, Alex Shi wrote:

quoted

That said, I have the feeling that is taking the wrong direction. Each time we
are entering idle, we check the latencies. Entering idle can be done thousand
of times per second. Wouldn't make sense to disable the states not fulfilling
the constraints at the moment the latencies are changed ? As the idle states
have increasing exit latencies, setting an idle state limit to disable all
states after that limit may be more efficient than checking again and again in
the idle path, no ?

You'r right. save some checking is good thing to do.

Hi Alex,

I think you missed the point.

What I am proposing is to change the current approach by disabling all the
states after a specific latency.

We add a specific internal function:

static int cpuidle_set_latency(struct cpuidle_driver *drv,
				struct cpuidle_device *dev,
				int latency)
{
	int i, idx;

	for (i = 0, idx = 0; i < drv->state_count; i++) {

		struct cpuidle_state *s = &drv->states[i];			

		if (s->latency > latency)
			break;

		idx = i;
	}

	dev->state_count = idx;

	return 0;
}

This function is called from the notifier callback:

static int cpuidle_latency_notify(struct notifier_block *b,
                unsigned long l, void *v)
 {
-       wake_up_all_idle_cpus();
+       struct cpuidle_device *dev;
+       struct cpuidle_driver *drv;
+
+       cpuidle_pause_and_lock();
+       for_each_possible_cpu(cpu) {
+               dev = &per_cpu(cpuidle_dev, cpu);
+               drv = = cpuidle_get_cpu_driver(dev);    
+               cpuidle_set_latency(drv, dev, l)
+       }
+       cpuidle_resume_and_unlock();
+
        return NOTIFY_OK;
 }

-----------------------------------------------------------------------------

The menu governor becomes:

diff --git a/drivers/cpuidle/governors/menu.c b/drivers/cpuidle/governors/menu.c
index bba3c2af..87e58e3 100644
--- a/drivers/cpuidle/governors/menu.c
+++ b/drivers/cpuidle/governors/menu.c

@@ -352,7 +352,7 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
         * Find the idle state with the lowest power while satisfying
         * our constraints.
         */
-       for (i = data->last_state_idx + 1; i < drv->state_count; i++) {
+       for (i = data->last_state_idx + 1; i < dev->state_count; i++) {
                struct cpuidle_state *s = &drv->states[i];
                struct cpuidle_state_usage *su = &dev->states_usage[i];

... with a cleanup around latency_req.

-----------------------------------------------------------------------------

And the cpuidle_device structure is changed to:

diff --git a/include/linux/cpuidle.h b/include/linux/cpuidle.h
index b923c32..2fc966cb 100644
--- a/include/linux/cpuidle.h
+++ b/include/linux/cpuidle.h

@@ -88,6 +88,7 @@ struct cpuidle_device {
        cpumask_t               coupled_cpus;
        struct cpuidle_coupled  *coupled;
 #endif
+       int state_count;
 };
 
 DECLARE_PER_CPU(struct cpuidle_device *, cpuidle_devices);

At init time, the drv->state_count and all cpu's dev->state_count are the same.

Well, that is the rough idea: instead of reading the latency when entering
idle, let's disable/enable the idle states when we set a new latency.

I did not check how that fits with the per cpu latency, but I think it will be
cleaner to change the approach rather than spreading latencies dances around.