All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v1 0/2] cpuidle: teo: Fix selection when idle state 0  is disabled
@ 2021-07-30 14:35 Rafael J. Wysocki
  2021-07-30 14:38 ` [PATCH v1 1/2] cpuidle: teo: Fix alternative idle state lookup Rafael J. Wysocki
                   ` (2 more replies)
  0 siblings, 3 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2021-07-30 14:35 UTC (permalink / raw)
  To: Linux PM, Doug Smythies; +Cc: LKML, Rafael J. Wysocki

Hi,

Patch [1/2] fixes idle state selection in the teo governor when idle state 0
is disabled (which is broken after recent changes in that governor) and patch
[2/2] renames two local variables on top of that change.

Please see the patch changelogs for details.

Thanks!




^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v1 1/2] cpuidle: teo: Fix alternative idle state lookup
  2021-07-30 14:35 [PATCH v1 0/2] cpuidle: teo: Fix selection when idle state 0 is disabled Rafael J. Wysocki
@ 2021-07-30 14:38 ` Rafael J. Wysocki
  2021-07-30 14:38 ` [PATCH v1 2/2] cpuidle: teo: Rename two local variables in teo_select() Rafael J. Wysocki
  2021-07-31  4:04 ` [PATCH v1 0/2] cpuidle: teo: Fix selection when idle state 0 is disabled Doug Smythies
  2 siblings, 0 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2021-07-30 14:38 UTC (permalink / raw)
  To: Linux PM, Doug Smythies; +Cc: LKML, Rafael J. Wysocki

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There are three mistakes in the loop in teo_select() that is looking
for an alternative candidate idle state.  First, it should walk all
of the idle states shallower than the current candidate one,
including all of the disabled ones, but it terminates after the first
enabled idle state.  Second, it should not terminate its last step
if idle state 0 is disabled (which is related to the first issue).
Finally, it may return the current alternative candidate idle state
prematurely if the time span criterion is not met by the idle state
under consideration at the moment.

To address the issues mentioned above, make the loop in question walk
all of the idle states shallower than the current candidate idle state
all the way down to idle state 0 and rearrange the checks in it.

Fixes: 77577558f25d ("cpuidle: teo: Rework most recent idle duration values treatment")
Reported-by: Doug Smythies <dsmythies@telus.net>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/governors/teo.c |   40 +++++++++++++++++++++++++++-------------
 1 file changed, 27 insertions(+), 13 deletions(-)

Index: linux-pm/drivers/cpuidle/governors/teo.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/teo.c
+++ linux-pm/drivers/cpuidle/governors/teo.c
@@ -397,32 +397,46 @@ static int teo_select(struct cpuidle_dri
 		intercept_sum = 0;
 		recent_sum = 0;
 
-		for (i = idx - 1; i >= idx0; i--) {
+		for (i = idx - 1; i >= 0; i--) {
 			struct teo_bin *bin = &cpu_data->state_bins[i];
 			s64 span_ns;
 
 			intercept_sum += bin->intercepts;
 			recent_sum += bin->recent;
 
+			span_ns = teo_middle_of_bin(i, drv);
+
+			if ((!alt_recent || 2 * recent_sum > idx_recent_sum) &&
+			    (!alt_intercepts ||
+			     2 * intercept_sum > idx_intercept_sum)) {
+				if (teo_time_ok(span_ns) &&
+				    !dev->states_usage[i].disable) {
+					idx = i;
+					duration_ns = span_ns;
+				} else {
+					/*
+					 * The current state is too shallow or
+					 * disabled, so take the first enabled
+					 * deeper state with suitable time span.
+					 */
+					idx = last_enabled_idx;
+					duration_ns = last_enabled_span_ns;
+				}
+				break;
+			}
+
 			if (dev->states_usage[i].disable)
 				continue;
 
-			span_ns = teo_middle_of_bin(i, drv);
 			if (!teo_time_ok(span_ns)) {
 				/*
-				 * The current state is too shallow, so select
-				 * the first enabled deeper state.
+				 * The current state is too shallow, but if an
+				 * alternative candidate state has been found,
+				 * it may still turn out to be a better choice.
 				 */
-				duration_ns = last_enabled_span_ns;
-				idx = last_enabled_idx;
-				break;
-			}
+				if (last_enabled_idx != idx)
+					continue;
 
-			if ((!alt_recent || 2 * recent_sum > idx_recent_sum) &&
-			    (!alt_intercepts ||
-			     2 * intercept_sum > idx_intercept_sum)) {
-				idx = i;
-				duration_ns = span_ns;
 				break;
 			}
 




^ permalink raw reply	[flat|nested] 5+ messages in thread

* [PATCH v1 2/2] cpuidle: teo: Rename two local variables in teo_select()
  2021-07-30 14:35 [PATCH v1 0/2] cpuidle: teo: Fix selection when idle state 0 is disabled Rafael J. Wysocki
  2021-07-30 14:38 ` [PATCH v1 1/2] cpuidle: teo: Fix alternative idle state lookup Rafael J. Wysocki
@ 2021-07-30 14:38 ` Rafael J. Wysocki
  2021-07-31  4:04 ` [PATCH v1 0/2] cpuidle: teo: Fix selection when idle state 0 is disabled Doug Smythies
  2 siblings, 0 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2021-07-30 14:38 UTC (permalink / raw)
  To: Linux PM, Doug Smythies; +Cc: LKML, Rafael J. Wysocki

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Rename two local variables in teo_select() so that their names better
reflect their purpose.

No functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpuidle/governors/teo.c |   14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/cpuidle/governors/teo.c
===================================================================
--- linux-pm.orig/drivers/cpuidle/governors/teo.c
+++ linux-pm/drivers/cpuidle/governors/teo.c
@@ -382,8 +382,8 @@ static int teo_select(struct cpuidle_dri
 	alt_intercepts = 2 * idx_intercept_sum > cpu_data->total - idx_hit_sum;
 	alt_recent = idx_recent_sum > NR_RECENT / 2;
 	if (alt_recent || alt_intercepts) {
-		s64 last_enabled_span_ns = duration_ns;
-		int last_enabled_idx = idx;
+		s64 first_suitable_span_ns = duration_ns;
+		int first_suitable_idx = idx;
 
 		/*
 		 * Look for the deepest idle state whose target residency had
@@ -419,8 +419,8 @@ static int teo_select(struct cpuidle_dri
 					 * disabled, so take the first enabled
 					 * deeper state with suitable time span.
 					 */
-					idx = last_enabled_idx;
-					duration_ns = last_enabled_span_ns;
+					idx = first_suitable_idx;
+					duration_ns = first_suitable_span_ns;
 				}
 				break;
 			}
@@ -434,14 +434,14 @@ static int teo_select(struct cpuidle_dri
 				 * alternative candidate state has been found,
 				 * it may still turn out to be a better choice.
 				 */
-				if (last_enabled_idx != idx)
+				if (first_suitable_idx != idx)
 					continue;
 
 				break;
 			}
 
-			last_enabled_span_ns = span_ns;
-			last_enabled_idx = i;
+			first_suitable_span_ns = span_ns;
+			first_suitable_idx = i;
 		}
 	}
 




^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v1 0/2] cpuidle: teo: Fix selection when idle state 0 is disabled
  2021-07-30 14:35 [PATCH v1 0/2] cpuidle: teo: Fix selection when idle state 0 is disabled Rafael J. Wysocki
  2021-07-30 14:38 ` [PATCH v1 1/2] cpuidle: teo: Fix alternative idle state lookup Rafael J. Wysocki
  2021-07-30 14:38 ` [PATCH v1 2/2] cpuidle: teo: Rename two local variables in teo_select() Rafael J. Wysocki
@ 2021-07-31  4:04 ` Doug Smythies
  2021-08-03 13:18   ` Rafael J. Wysocki
  2 siblings, 1 reply; 5+ messages in thread
From: Doug Smythies @ 2021-07-31  4:04 UTC (permalink / raw)
  To: Rafael J. Wysocki; +Cc: Linux PM, LKML, Rafael J. Wysocki, dsmythies

On Fri, Jul 30, 2021 at 7:39 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> Hi,
>
> Patch [1/2] fixes idle state selection in the teo governor when idle state 0
> is disabled (which is broken after recent changes in that governor) and patch
> [2/2] renames two local variables on top of that change.
>
> Please see the patch changelogs for details.

Hi Rafael,

I tested this version of the patches, and the idle state 0
disabled issue has been fixed.

... Doug

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v1 0/2] cpuidle: teo: Fix selection when idle state 0 is disabled
  2021-07-31  4:04 ` [PATCH v1 0/2] cpuidle: teo: Fix selection when idle state 0 is disabled Doug Smythies
@ 2021-08-03 13:18   ` Rafael J. Wysocki
  0 siblings, 0 replies; 5+ messages in thread
From: Rafael J. Wysocki @ 2021-08-03 13:18 UTC (permalink / raw)
  To: Doug Smythies; +Cc: Rafael J. Wysocki, Linux PM, LKML, Rafael J. Wysocki

On Sat, Jul 31, 2021 at 6:04 AM Doug Smythies <dsmythies@telus.net> wrote:
>
> On Fri, Jul 30, 2021 at 7:39 AM Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > Hi,
> >
> > Patch [1/2] fixes idle state selection in the teo governor when idle state 0
> > is disabled (which is broken after recent changes in that governor) and patch
> > [2/2] renames two local variables on top of that change.
> >
> > Please see the patch changelogs for details.
>
> Hi Rafael,
>
> I tested this version of the patches, and the idle state 0
> disabled issue has been fixed.

Thanks for the confirmation!

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-08-03 13:18 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-07-30 14:35 [PATCH v1 0/2] cpuidle: teo: Fix selection when idle state 0 is disabled Rafael J. Wysocki
2021-07-30 14:38 ` [PATCH v1 1/2] cpuidle: teo: Fix alternative idle state lookup Rafael J. Wysocki
2021-07-30 14:38 ` [PATCH v1 2/2] cpuidle: teo: Rename two local variables in teo_select() Rafael J. Wysocki
2021-07-31  4:04 ` [PATCH v1 0/2] cpuidle: teo: Fix selection when idle state 0 is disabled Doug Smythies
2021-08-03 13:18   ` Rafael J. Wysocki

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.