cpuidle: menu: nearby timer use lightest state; allow state 0 to be disabled

I've found the menu driver does not allow state0 to properly be disabled because of all this poll logic selecting the first state and then trying to iterate over subsequent states. Ripping most of that out and simplifying it solved that issue but raised more questions about polling logic.

Firstly polling logic is there only on architectures which define ARCH_HAS_CPU_RELAX, which is only x86. Seems like if we think a timer is so close that no powersave should be done, then surely just picking the lightest mode (whether that is polling or something else) would be best.

But looking further into it, it seems maybe like some x86 hack (as the comments and changelog in 7884084f3bcc and subsequent attempts to work around Atom and broken firmware suggests). I would have thought such broken hard/firmware should get workarounds applied to fix the state values rather than add such logic?

On the other hand, if (CPUIDLE_DRIVER_STATE_START > 0) is shorthand for if (x86 hacks), that's fine I'm happy to leave that alone and just work with the else parts...

This is just a draft, but it does what I want at least with regard to disabling state0 for testing.

Quoted 17 lines · On Wed, 31 May 2017 02:26:31 +1000 Nicholas Piggin <[email protected]> wrote:

I've found the menu driver does not allow state0 to properly be disabled because of all this poll logic selecting the first state and then trying to iterate over subsequent states. Ripping most of that out and simplifying it solved that issue but raised more questions about polling logic.

Firstly polling logic is there only on architectures which define ARCH_HAS_CPU_RELAX, which is only x86. Seems like if we think a timer is so close that no powersave should be done, then surely just picking the lightest mode (whether that is polling or something else) would be best.

But looking further into it, it seems maybe like some x86 hack (as the comments and changelog in 7884084f3bcc and subsequent attempts to work around Atom and broken firmware suggests). I would have thought such broken hard/firmware should get workarounds applied to fix the state values rather than add such logic?

Quoted 6 lines

On the other hand, if (CPUIDLE_DRIVER_STATE_START > 0) is shorthand for if (x86 hacks), that's fine I'm happy to leave that alone and just work with the else parts...

This is just a draft, but it does what I want at least with regard to disabling state0 for testing.

Ping, any thoughts on these issues?

I've found the menu driver does not allow state0 to properly be disabled because of all this poll logic selecting the first state and then trying to iterate over subsequent states. Ripping most of that out and simplifying it solved that issue but raised more questions about polling logic.

Firstly polling logic is there only on architectures which define ARCH_HAS_CPU_RELAX, which is only x86. Seems like if we think a timer is so close that no powersave should be done, then surely just picking the lightest mode (whether that is polling or something else) would be best.

But looking further into it, it seems maybe like some x86 hack (as the comments and changelog in 7884084f3bcc and subsequent attempts to work around Atom and broken firmware suggests). I would have thought such broken hard/firmware should get workarounds applied to fix the state values rather than add such logic?

On the other hand, if (CPUIDLE_DRIVER_STATE_START > 0) is shorthand for if (x86 hacks), that's fine I'm happy to leave that alone and just work with the else parts...

This is just a draft, but it does what I want at least with regard to disabling state0 for testing.

Thanks,
Nick

---
 drivers/cpuidle/governors/menu.c | 25 ++++++++++++++++---------
 1 file changed, 16 insertions(+), 9 deletions(-)

drivers/cpuidle/governors/menu.c +10 -8

index b2330fd69e34..05de5d345ac7 100644
@@ -286,6 +286,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 	struct device *device = get_cpu_device(dev->cpu);
 	int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
 	int i;
+	int first_idx;
+	int idx;
 	unsigned int interactivity_req;
 	unsigned int expected_interval;
 	unsigned long nr_iowaiters, cpu_load;

@@ -318,7 +320,6 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 	data->predicted_us = DIV_ROUND_CLOSEST_ULL((uint64_t)data->next_timer_us *
 					 data->correction_factor[data->bucket],
 					 RESOLUTION * DECAY);
-
 	expected_interval = get_typical_interval(data);
 	expected_interval = min(expected_interval, data->next_timer_us);
 

@@ -327,19 +328,19 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 		unsigned int polling_threshold;
 
 		/*
-		 * We want to default to C1 (hlt), not to busy polling
-		 * unless the timer is happening really really soon, or
-		 * C1's exit latency exceeds the user configured limit.
+		 * x86 wants to avoid busy polling unless the timer is
+		 * happening really really soon, or C1's exit latency exceeds
+		 * the uesr configured limit.
 		 */
 		polling_threshold = max_t(unsigned int, 20, s->target_residency);
 		if (data->next_timer_us > polling_threshold &&
 		    latency_req > s->exit_latency && !s->disabled &&
 		    !dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable)
-			data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
+			first_idx = CPUIDLE_DRIVER_STATE_START;
 		else
-			data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1;
+			first_idx = CPUIDLE_DRIVER_STATE_START - 1;
 	} else {
-		data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
+		first_idx = 0;
 	}
 
 	/*

@@ -359,7 +360,8 @@ 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++) {
+	idx = -1;
+	for (i = first_idx; i < drv->state_count; i++) {
 		struct cpuidle_state *s = &drv->states[i];
 		struct cpuidle_state_usage *su = &dev->states_usage[i];
 

Hello Nicholas,

Quoted 4 lines · Nicholas Piggin

I've found the menu driver does not allow state0 to properly be disabled because of all this poll logic selecting the first state and then trying to iterate over subsequent states. Ripping most of that out and simplifying it solved that issue but raised more questions about polling logic.

At one point menu governor did allow state0 to be disabled. However, in cases where the predicted residency is so small that none of the higher idle states are valid, the menu governor would return -1 (no suitable state). As a result, the history would never get populated. Thus, the menu governor would always predict state0 which was disabled thus resulting in a viscious cycle where none of the idle states were entered into on a completely idle system.

This was fixed in commit 9c4b2867ed7c8c8784dd417ffd16e705e81eb145 ("
cpuidle: menu: Fix menu_select() for CPUIDLE_DRIVER_STATE_START == 0") which had an unfortunate side-effect of not allowing state0 to be disabled.

Quoted 12 lines

Firstly polling logic is there only on architectures which define ARCH_HAS_CPU_RELAX, which is only x86. Seems like if we think a timer is so close that no powersave should be done, then surely just picking the lightest mode (whether that is polling or something else) would be best.

But looking further into it, it seems maybe like some x86 hack (as the comments and changelog in 7884084f3bcc and subsequent attempts to work around Atom and broken firmware suggests). I would have thought such broken hard/firmware should get workarounds applied to fix the state values rather than add such logic?

Quoted 3 lines

On the other hand, if (CPUIDLE_DRIVER_STATE_START > 0) is shorthand for if (x86 hacks), that's fine I'm happy to leave that alone and just work with the else parts...

At least on POWER, CPUIDLE_DRIVER_START == 0.

Review static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev) · line 379

Quoted 10 lines

(quoted diff) +3 -1

@@ -370,9 +372,14 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 		if (s->exit_latency > latency_req)
 			break;
 
-		data->last_state_idx = i;
+		idx = i;
 	}
 
+	if (idx == -1) /* no states */
+		idx = 0;  

So even if state0 is disabled, when no suitable states are found we will still fallback to state0. The additional thing this patch does is to check inside the loop if state0 is disabled or not. This patch improves the readability by making the fallback to state0 on no suitable states being found.

Are you able to observe any functional difference with this patch ?

Quoted 2 lines

Are you able to observe any functional difference with this patch ?

Yes for some tests, but it did have a bug where state0 was still being used despite other states being available. This patch really disables state0 (unless all states are disabled, then it falls back to 0 again).

---
 drivers/cpuidle/governors/menu.c | 20 +++++++++++++++-----
 1 file changed, 15 insertions(+), 5 deletions(-)

drivers/cpuidle/governors/menu.c +15 -5

index b2330fd69e34..61b64c2b2cb8 100644
@@ -286,6 +286,8 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 	struct device *device = get_cpu_device(dev->cpu);
 	int latency_req = pm_qos_request(PM_QOS_CPU_DMA_LATENCY);
 	int i;
+	int first_idx;
+	int idx;
 	unsigned int interactivity_req;
 	unsigned int expected_interval;
 	unsigned long nr_iowaiters, cpu_load;

@@ -335,11 +337,11 @@ static int menu_select(struct cpuidle_driver *drv, struct cpuidle_device *dev)
 		if (data->next_timer_us > polling_threshold &&
 		    latency_req > s->exit_latency && !s->disabled &&
 		    !dev->states_usage[CPUIDLE_DRIVER_STATE_START].disable)
-			data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
+			first_idx = CPUIDLE_DRIVER_STATE_START;
 		else
-			data->last_state_idx = CPUIDLE_DRIVER_STATE_START - 1;
+			first_idx = CPUIDLE_DRIVER_STATE_START - 1;
 	} else {
-		data->last_state_idx = CPUIDLE_DRIVER_STATE_START;
+		first_idx = 0;
 	}
 
 	/*

@@ -359,20 +361,28 @@ 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++) {
+	idx = -1;
+	for (i = first_idx; 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 (idx == -1)
+			idx = i; /* first enabled state */
 		if (s->target_residency > data->predicted_us)
 			break;
 		if (s->exit_latency > latency_req)
 			break;
 
-		data->last_state_idx = i;
+		idx = i;
 	}
 
+	if (idx == -1)
+		idx = 0; /* No states enabled. Must use 0. */
+
+	data->last_state_idx = idx;
+
 	return data->last_state_idx;
 }