cpufreq: conservative: Fix requested_freq handling

From: Waldemar Rymarkiewicz <[email protected]>

The governor updates dbs_info->requested_freq only after increasing or decreasing frequency. There is, however, an use case when this is not sufficient.

Imagine, external module constraining cpufreq policy in a way that policy->max

= policy->min = max_available_freq (eg. 1Ghz). CPUfreq will set freq to max_freq and conservative gov will not try downscale/upscale due to the limits. It will just exit instead

    if (requested_freq > policy->max || requested_freq < policy->min)
            //max=min=1Ghz -> requested_freq=cur=1Ghz
            requested_freq = policy->cur;
    [...]
    if (requested_freq == policy->max)
             goto out;

In a result, dbs_info->requested_freq is not updated with newly calculated requested_freq=1Ghz. Next, execution of update routine will use again previously stored requested_freq (in my case it was min_available_freq)

    [...]
    unsigned int requested_freq = dbs_info->requested_freq;
    [....]

Now, when external module returns to previous policy limits that is policy->min = min_available_freq and policy->max = max_available_freq, conservative governor is not able to decrease frequency because stored requested_freq is still or rather already set to min_available_freq so the check (for decreasing)

    [...]
    if (load < cs_tuners->down_threshold) {
    [....]
           if (requested_freq == policy->min)
                   goto out;
    [...]

returns from routine before it does any freq change. To fix that just update dbs_info->requested_freq every time we go out from the update routine.

Signed-off-by Waldemar Rymarkiewicz <[email protected]>

---
 drivers/cpufreq/cpufreq_conservative.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

drivers/cpufreq/cpufreq_conservative.c +0 -1

index f20f20a..7f90f6e 100644
@@ -113,7 +113,6 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
                        requested_freq = policy->max;
 
                __cpufreq_driver_target(policy, requested_freq, CPUFREQ_RELATION_H);
-               dbs_info->requested_freq = requested_freq;
                goto out;
        }
 

drivers/cpufreq/cpufreq_conservative.c +0 -0

        return dbs_data->sampling_rate;
 }

Quoted 3 lines · Waldemar Rymarkiewicz

From: Waldemar Rymarkiewicz <[email protected]>

Quoted 5 lines

The governor updates dbs_info->requested_freq only after increasing or decreasing frequency. There is, however, an use case when this is not sufficient.

Imagine, external module constraining cpufreq policy in a way that policy->max

Is the "external module" here a utility or a demon running in user space?

Quoted 7 lines

drivers/cpufreq/cpufreq_conservative.c +0 -1

@@ -136,10 +135,10 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
                        requested_freq = policy->min;
 
                __cpufreq_driver_target(policy, requested_freq, CPUFREQ_RELATION_L);
-               dbs_info->requested_freq = requested_freq;
        }
 

drivers/cpufreq/cpufreq_conservative.c static unsigned int cs_dbs_update(struct cpufreq_policy *policy) · line 141

Quoted 2 lines

drivers/cpufreq/cpufreq_conservative.c +1 -0

  out:
+       dbs_info->requested_freq = requested_freq;

This will have a side effect when requested_freq is updated before the thresholds checks due to the policy_dbs->idle_periods < UINT_MAX check.

Shouldn't that be avoided?

Quoted 6 lines · Rafael J. Wysocki

Quoted 3 lines · Waldemar Rymarkiewicz

From: Waldemar Rymarkiewicz <[email protected]>

Quoted 7 lines

Quoted 5 lines

The governor updates dbs_info->requested_freq only after increasing or decreasing frequency. There is, however, an use case when this is not sufficient.

Imagine, external module constraining cpufreq policy in a way that policy->max

Is the "external module" here a utility or a demon running in user space?

No, this is a driver that communicates with a firmware and makes sure CPU is running at the highest rate in specific time.
It uses verify_within_limits and update_policy, a standard way to constraint cpufreq policy limits.

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -1

@@ -136,10 +135,10 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
                        requested_freq = policy->min;
 
                __cpufreq_driver_target(policy, requested_freq, CPUFREQ_RELATION_L);
-               dbs_info->requested_freq = requested_freq;
        }
 

Quoted 8 lines

Review static unsigned int cs_dbs_update(struct cpufreq_policy *policy) · line 141

Quoted 2 lines

(quoted diff) +1 -0

  out:
+       dbs_info->requested_freq = requested_freq;

This will have a side effect when requested_freq is updated before the thresholds checks due to the policy_dbs->idle_periods < UINT_MAX check.

Shouldn't that be avoided?

I would say we should.

A hardware design I use is running 4.9 kernel where the check does not exist yet, so there is not a problem.
Anyway, the check policy_dbs->idle_periods < UINT_MAX can change requested_freq either to requested_freq = policy->min or requested_freq -= freq_steps;. The first case will not change anything for us as policy->max=min=cur. The second, however, will force to update freq which is definitely not expected when limits are set to min=max. Simply it will not go out here:

if (load < cs_tuners->down_threshold) {
      if (requested_freq == policy->min)
           goto out;   <---

...
}

Am I right here? If so, shouldn't we check explicitly

/*
* If requested_freq is out of range, it is likely that the limits
* changed in the meantime, so fall back to current frequency in that
* case.
*/
if (requested_freq > policy->max || requested_freq < policy->min)

       requested_freq = policy->cur;

Quoted 63 lines · CEST Waldemar Rymarkiewicz

Quoted 6 lines · Rafael J. Wysocki

Quoted 3 lines · Waldemar Rymarkiewicz

From: Waldemar Rymarkiewicz <[email protected]>

Quoted 7 lines

Quoted 5 lines

The governor updates dbs_info->requested_freq only after increasing or decreasing frequency. There is, however, an use case when this is not sufficient.

Imagine, external module constraining cpufreq policy in a way that policy->max

Is the "external module" here a utility or a demon running in user space?

No, this is a driver that communicates with a firmware and makes sure CPU is running at the highest rate in specific time.
It uses verify_within_limits and update_policy, a standard way to constraint cpufreq policy limits.

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -1

@@ -136,10 +135,10 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
                        requested_freq = policy->min;
 
                __cpufreq_driver_target(policy, requested_freq, CPUFREQ_RELATION_L);
-               dbs_info->requested_freq = requested_freq;
        }
 

Quoted 8 lines

Review static unsigned int cs_dbs_update(struct cpufreq_policy *policy) · line 141

Quoted 2 lines

(quoted diff) +1 -0

  out:
+       dbs_info->requested_freq = requested_freq;

This will have a side effect when requested_freq is updated before the thresholds checks due to the policy_dbs->idle_periods < UINT_MAX check.

Shouldn't that be avoided?

I would say we should.

A hardware design I use is running 4.9 kernel where the check does not exist yet, so there is not a problem.
Anyway, the check policy_dbs->idle_periods < UINT_MAX can change requested_freq either to requested_freq = policy->min or requested_freq -= freq_steps;. The first case will not change anything for us as policy->max=min=cur. The second, however, will force to update freq which is definitely not expected when limits are set to min=max. Simply it will not go out here:

if (load < cs_tuners->down_threshold) {
      if (requested_freq == policy->min)
           goto out;   <---

...
}

Am I right here? If so, shouldn't we check explicitly

/*
* If requested_freq is out of range, it is likely that the limits
* changed in the meantime, so fall back to current frequency in that
* case.
*/
if (requested_freq > policy->max || requested_freq < policy->min)

       requested_freq = policy->cur;

Review

Quoted 6 lines

(quoted diff) +6 -0

+/*
+* If the the new limits min,max are equal, there is no point to process further
+*/
+
+if (requested_freq == policy->max  &&  requested_freq == policy->min)
+     goto out;

If my understanding of the problem is correct, it would be better to simply update dbs_info->requested_freq along with requested_freq when that is found to be out of range. IOW, something like the appended patch (untested).

Quoted 65 lines · Rafael J. Wysocki

Quoted 63 lines · CEST Waldemar Rymarkiewicz

Quoted 6 lines · Rafael J. Wysocki

Quoted 3 lines · Waldemar Rymarkiewicz

From: Waldemar Rymarkiewicz <[email protected]>

Quoted 7 lines

Quoted 5 lines

The governor updates dbs_info->requested_freq only after increasing or decreasing frequency. There is, however, an use case when this is not sufficient.

Imagine, external module constraining cpufreq policy in a way that policy->max

Is the "external module" here a utility or a demon running in user space?

No, this is a driver that communicates with a firmware and makes sure CPU is running at the highest rate in specific time.
It uses verify_within_limits and update_policy, a standard way to constraint cpufreq policy limits.

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -1

@@ -136,10 +135,10 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
                        requested_freq = policy->min;
 
                __cpufreq_driver_target(policy, requested_freq, CPUFREQ_RELATION_L);
-               dbs_info->requested_freq = requested_freq;
        }
 

Quoted 8 lines

Review static unsigned int cs_dbs_update(struct cpufreq_policy *policy) · line 141

Quoted 2 lines

(quoted diff) +1 -0

  out:
+       dbs_info->requested_freq = requested_freq;

This will have a side effect when requested_freq is updated before the thresholds checks due to the policy_dbs->idle_periods < UINT_MAX check.

Shouldn't that be avoided?

I would say we should.

A hardware design I use is running 4.9 kernel where the check does not exist yet, so there is not a problem.
Anyway, the check policy_dbs->idle_periods < UINT_MAX can change requested_freq either to requested_freq = policy->min or requested_freq -= freq_steps;. The first case will not change anything for us as policy->max=min=cur. The second, however, will force to update freq which is definitely not expected when limits are set to min=max. Simply it will not go out here:

if (load < cs_tuners->down_threshold) {
      if (requested_freq == policy->min)
           goto out;   <---

...
}

Am I right here? If so, shouldn't we check explicitly

/*
* If requested_freq is out of range, it is likely that the limits
* changed in the meantime, so fall back to current frequency in that
* case.
*/
if (requested_freq > policy->max || requested_freq < policy->min)

       requested_freq = policy->cur;

Quoted 10 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+/*
+* If the the new limits min,max are equal, there is no point to process further
+*/
+
+if (requested_freq == policy->max  &&  requested_freq == policy->min)
+     goto out;

If my understanding of the problem is correct, it would be better to simply update dbs_info->requested_freq along with requested_freq when that is found to be out of range. IOW, something like the appended patch (untested).

Yes, this will solve the original problem as well.

I think there could also be a problem with policy_dbs->idle_periods < UINT_MAX check. It it's true it can modify requested_freq ( requested_freq -= freq_steps) and further it can result in a change of the freq, requested_freq == policy->max is not anymore true. I would expect governor not to change freq (requested_freq) when policy->max=policy->min=policy->cur.

Quoted 77 lines · CEST Waldemar Rymarkiewicz

Quoted 65 lines · Rafael J. Wysocki

Quoted 63 lines · CEST Waldemar Rymarkiewicz

Quoted 6 lines · Rafael J. Wysocki

Quoted 3 lines · Waldemar Rymarkiewicz

From: Waldemar Rymarkiewicz <[email protected]>

Quoted 7 lines

Quoted 5 lines

The governor updates dbs_info->requested_freq only after increasing or decreasing frequency. There is, however, an use case when this is not sufficient.

Imagine, external module constraining cpufreq policy in a way that policy->max

Is the "external module" here a utility or a demon running in user space?

No, this is a driver that communicates with a firmware and makes sure CPU is running at the highest rate in specific time.
It uses verify_within_limits and update_policy, a standard way to constraint cpufreq policy limits.

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -1

@@ -136,10 +135,10 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
                        requested_freq = policy->min;
 
                __cpufreq_driver_target(policy, requested_freq, CPUFREQ_RELATION_L);
-               dbs_info->requested_freq = requested_freq;
        }
 

Quoted 8 lines

Review static unsigned int cs_dbs_update(struct cpufreq_policy *policy) · line 141

Quoted 2 lines

(quoted diff) +1 -0

  out:
+       dbs_info->requested_freq = requested_freq;

This will have a side effect when requested_freq is updated before the thresholds checks due to the policy_dbs->idle_periods < UINT_MAX check.

Shouldn't that be avoided?

I would say we should.

A hardware design I use is running 4.9 kernel where the check does not exist yet, so there is not a problem.
Anyway, the check policy_dbs->idle_periods < UINT_MAX can change requested_freq either to requested_freq = policy->min or requested_freq -= freq_steps;. The first case will not change anything for us as policy->max=min=cur. The second, however, will force to update freq which is definitely not expected when limits are set to min=max. Simply it will not go out here:

if (load < cs_tuners->down_threshold) {
      if (requested_freq == policy->min)
           goto out;   <---

...
}

Am I right here? If so, shouldn't we check explicitly

/*
* If requested_freq is out of range, it is likely that the limits
* changed in the meantime, so fall back to current frequency in that
* case.
*/
if (requested_freq > policy->max || requested_freq < policy->min)

       requested_freq = policy->cur;

Quoted 10 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+/*
+* If the the new limits min,max are equal, there is no point to process further
+*/
+
+if (requested_freq == policy->max  &&  requested_freq == policy->min)
+     goto out;

If my understanding of the problem is correct, it would be better to simply update dbs_info->requested_freq along with requested_freq when that is found to be out of range. IOW, something like the appended patch (untested).

Quoted 8 lines

Yes, this will solve the original problem as well.

I think there could also be a problem with policy_dbs->idle_periods < UINT_MAX check. It it's true it can modify requested_freq ( requested_freq -= freq_steps) and further it can result in a change of the freq, requested_freq == policy->max is not anymore true. I would expect governor not to change freq (requested_freq) when policy->max=policy->min=policy->cur.

Well, that's because there is a bug in that code IMO. It should never decrease requested_freq below policy->min in particular.

Please find a patch with that fixed below.

---
 drivers/cpufreq/cpufreq_conservative.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================

drivers/cpufreq/cpufreq_conservative.c +3 -1

@@ -80,8 +80,10 @@ static unsigned int cs_dbs_update(struct
         * changed in the meantime, so fall back to current frequency in that
         * case.
         */
-       if (requested_freq > policy->max || requested_freq < policy->min)
+       if (requested_freq > policy->max || requested_freq < policy->min) {
                requested_freq = policy->cur;
+               dbs_info->requested_freq = requested_freq;
+       }
 
        freq_step = get_freq_step(cs_tuners, policy);
 

Quoted 117 lines · Rafael J. Wysocki

Quoted 77 lines · CEST Waldemar Rymarkiewicz

Quoted 65 lines · Rafael J. Wysocki

Quoted 63 lines · CEST Waldemar Rymarkiewicz

Quoted 6 lines · Rafael J. Wysocki

Quoted 3 lines · Waldemar Rymarkiewicz

From: Waldemar Rymarkiewicz <[email protected]>

Quoted 7 lines

Quoted 5 lines

The governor updates dbs_info->requested_freq only after increasing or decreasing frequency. There is, however, an use case when this is not sufficient.

Imagine, external module constraining cpufreq policy in a way that policy->max

Is the "external module" here a utility or a demon running in user space?

No, this is a driver that communicates with a firmware and makes sure CPU is running at the highest rate in specific time.
It uses verify_within_limits and update_policy, a standard way to constraint cpufreq policy limits.

Quoted 7 lines

Quoted 7 lines

(quoted diff) +0 -1

@@ -136,10 +135,10 @@ static unsigned int cs_dbs_update(struct cpufreq_policy *policy)
                        requested_freq = policy->min;
 
                __cpufreq_driver_target(policy, requested_freq, CPUFREQ_RELATION_L);
-               dbs_info->requested_freq = requested_freq;
        }
 

Quoted 8 lines

Review static unsigned int cs_dbs_update(struct cpufreq_policy *policy) · line 141

Quoted 2 lines

(quoted diff) +1 -0

  out:
+       dbs_info->requested_freq = requested_freq;

This will have a side effect when requested_freq is updated before the thresholds checks due to the policy_dbs->idle_periods < UINT_MAX check.

Shouldn't that be avoided?

I would say we should.

A hardware design I use is running 4.9 kernel where the check does not exist yet, so there is not a problem.
Anyway, the check policy_dbs->idle_periods < UINT_MAX can change requested_freq either to requested_freq = policy->min or requested_freq -= freq_steps;. The first case will not change anything for us as policy->max=min=cur. The second, however, will force to update freq which is definitely not expected when limits are set to min=max. Simply it will not go out here:

if (load < cs_tuners->down_threshold) {
      if (requested_freq == policy->min)
           goto out;   <---

...
}

Am I right here? If so, shouldn't we check explicitly

/*
* If requested_freq is out of range, it is likely that the limits
* changed in the meantime, so fall back to current frequency in that
* case.
*/
if (requested_freq > policy->max || requested_freq < policy->min)

       requested_freq = policy->cur;

Quoted 10 lines

Review

Quoted 6 lines

(quoted diff) +6 -0

+/*
+* If the the new limits min,max are equal, there is no point to process further
+*/
+
+if (requested_freq == policy->max  &&  requested_freq == policy->min)
+     goto out;

If my understanding of the problem is correct, it would be better to simply update dbs_info->requested_freq along with requested_freq when that is found to be out of range. IOW, something like the appended patch (untested).

Quoted 8 lines

Yes, this will solve the original problem as well.

I think there could also be a problem with policy_dbs->idle_periods < UINT_MAX check. It it's true it can modify requested_freq ( requested_freq -= freq_steps) and further it can result in a change of the freq, requested_freq == policy->max is not anymore true. I would expect governor not to change freq (requested_freq) when policy->max=policy->min=policy->cur.

Well, that's because there is a bug in that code IMO. It should never decrease requested_freq below policy->min in particular.

Please find a patch with that fixed below.

---
 drivers/cpufreq/cpufreq_conservative.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================

drivers/cpufreq/cpufreq_conservative.c +3 -1

@@ -80,8 +80,10 @@ static unsigned int cs_dbs_update(struct
         * changed in the meantime, so fall back to current frequency in that
         * case.
         */
-       if (requested_freq > policy->max || requested_freq < policy->min)
+       if (requested_freq > policy->max || requested_freq < policy->min) {
                requested_freq = policy->cur;
+               dbs_info->requested_freq = requested_freq;
+       }
 
        freq_step = get_freq_step(cs_tuners, policy);
 

@@ -92,7 +94,7 @@ static unsigned int cs_dbs_update(struct
        if (policy_dbs->idle_periods < UINT_MAX) {
                unsigned int freq_steps = policy_dbs->idle_periods * freq_step;
 

drivers/cpufreq/cpufreq_conservative.c static unsigned int cs_dbs_update(struct · line 100

Quoted 5 lines

drivers/cpufreq/cpufreq_conservative.c +1 -1

-               if (requested_freq > freq_steps)
+               if (requested_freq > policy->min + freq_steps)
                        requested_freq -= freq_steps;
                else
                        requested_freq = policy->min;

Yes looks good now. Will you apply this patch?

From: Rafael J. Wysocki <[email protected]>

If the policy limits change between invocations of cs_dbs_update(), the requested frequency value stored in dbs_info may not be updated and the function may use a stale value of it next time. Moreover, if idle periods are takem into account by cs_dbs_update(), the requested frequency value stored in dbs_info may be below the min policy limit, which is incorrect.

To fix these problems, always update the requested frequency value in dbs_info along with the local copy of it when the previous requested frequency is beyond the policy limits and avoid decreasing the requested frequency below the min policy limit when taking idle periods into account.

Reported-by Waldemar Rymarkiewicz <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/cpufreq/cpufreq_conservative.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================

drivers/cpufreq/cpufreq_conservative.c +3 -1

@@ -80,8 +80,10 @@ static unsigned int cs_dbs_update(struct
         * changed in the meantime, so fall back to current frequency in that
         * case.
         */
-       if (requested_freq > policy->max || requested_freq < policy->min)
+       if (requested_freq > policy->max || requested_freq < policy->min) {
                requested_freq = policy->cur;
+               dbs_info->requested_freq = requested_freq;
+       }
 
        freq_step = get_freq_step(cs_tuners, policy);
 

From: Rafael J. Wysocki <[email protected]>

If the policy limits change between invocations of cs_dbs_update(), the requested frequency value stored in dbs_info may not be updated and the function may use a stale value of it next time. Moreover, if idle periods are takem into account by cs_dbs_update(), the requested frequency value stored in dbs_info may be below the min policy limit, which is incorrect.

To fix these problems, always update the requested frequency value in dbs_info along with the local copy of it when the previous requested frequency is beyond the policy limits and avoid decreasing the requested frequency below the min policy limit when taking idle periods into account.

Reported-by Waldemar Rymarkiewicz <[email protected]>

Signed-off-by Rafael J. Wysocki <[email protected]>

---
 drivers/cpufreq/cpufreq_conservative.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)