[PATCH] cpufreq: conservative: Fix requested_freq handling

[PATCH] cpufreq: conservative: Fix requested_freq handling

[Waldemar Rymarkiewicz]

From: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>

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 <waldemarx.rymarkiewicz@intel.com>
---
 drivers/cpufreq/cpufreq_conservative.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
index f20f20a..7f90f6e 100644
--- a/drivers/cpufreq/cpufreq_conservative.c
+++ b/drivers/cpufreq/cpufreq_conservative.c
@@ -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;
 	}
 
@@ -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;
 	}
 
  out:
+	dbs_info->requested_freq = requested_freq;
 	return dbs_data->sampling_rate;
 }
 
-- 
2.10.1

Re: [PATCH] cpufreq: conservative: Fix requested_freq handling

[Rafael J. Wysocki]

On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
<waldemar.rymarkiewicz@gmail.com> wrote:
>
> From: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>
>
> 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?

> = 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 <waldemarx.rymarkiewicz@intel.com>
> ---
>  drivers/cpufreq/cpufreq_conservative.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/cpufreq/cpufreq_conservative.c b/drivers/cpufreq/cpufreq_conservative.c
> index f20f20a..7f90f6e 100644
> --- a/drivers/cpufreq/cpufreq_conservative.c
> +++ b/drivers/cpufreq/cpufreq_conservative.c
> @@ -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;
>         }
>
> @@ -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;
>         }
>
>   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?

>         return dbs_data->sampling_rate;
>  }

Re: [PATCH] cpufreq: conservative: Fix requested_freq handling

[Waldemar Rymarkiewicz]

On Tue, 9 Oct 2018 at 09:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
>
> On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> <waldemar.rymarkiewicz@gmail.com> wrote:
> >
> > From: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>
> >
> > 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.

> > @@ -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;
> >         }
> >
> >   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;

+/*
+* 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;

Thanks,
/Waldek

Re: [PATCH] cpufreq: conservative: Fix requested_freq handling

[Rafael J. Wysocki]

On Tuesday, October 9, 2018 6:06:08 PM CEST Waldemar Rymarkiewicz wrote:
> On Tue, 9 Oct 2018 at 09:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
> >
> > On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> > <waldemar.rymarkiewicz@gmail.com> wrote:
> > >
> > > From: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>
> > >
> > > 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.
> 
> > > @@ -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;
> > >         }
> > >
> > >   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;
> 
> +/*
> +* 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).

Wouldn't that address the problem at hand?

---
 drivers/cpufreq/cpufreq_conservative.c |    4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -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);
 

Re: [PATCH] cpufreq: conservative: Fix requested_freq handling

[Waldemar Rymarkiewicz]

On Thu, 11 Oct 2018 at 23:10, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Tuesday, October 9, 2018 6:06:08 PM CEST Waldemar Rymarkiewicz wrote:
> > On Tue, 9 Oct 2018 at 09:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > >
> > > On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> > > <waldemar.rymarkiewicz@gmail.com> wrote:
> > > >
> > > > From: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>
> > > >
> > > > 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.
> >
> > > > @@ -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;
> > > >         }
> > > >
> > > >   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;
> >
> > +/*
> > +* 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.

Thanks,
/Waldek

Re: [PATCH] cpufreq: conservative: Fix requested_freq handling

[Rafael J. Wysocki]

On Monday, October 15, 2018 11:34:33 AM CEST Waldemar Rymarkiewicz wrote:
> On Thu, 11 Oct 2018 at 23:10, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Tuesday, October 9, 2018 6:06:08 PM CEST Waldemar Rymarkiewicz wrote:
> > > On Tue, 9 Oct 2018 at 09:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > >
> > > > On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> > > > <waldemar.rymarkiewicz@gmail.com> wrote:
> > > > >
> > > > > From: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>
> > > > >
> > > > > 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.
> > >
> > > > > @@ -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;
> > > > >         }
> > > > >
> > > > >   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;
> > >
> > > +/*
> > > +* 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.

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
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -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;
 
-		if (requested_freq > freq_steps)
+		if (requested_freq > policy->min + freq_steps)
 			requested_freq -= freq_steps;
 		else
 			requested_freq = policy->min;

Re: [PATCH] cpufreq: conservative: Fix requested_freq handling

[Waldemar Rymarkiewicz]

On Mon, 15 Oct 2018 at 13:34, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> On Monday, October 15, 2018 11:34:33 AM CEST Waldemar Rymarkiewicz wrote:
> > On Thu, 11 Oct 2018 at 23:10, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > >
> > > On Tuesday, October 9, 2018 6:06:08 PM CEST Waldemar Rymarkiewicz wrote:
> > > > On Tue, 9 Oct 2018 at 09:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > >
> > > > > On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> > > > > <waldemar.rymarkiewicz@gmail.com> wrote:
> > > > > >
> > > > > > From: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>
> > > > > >
> > > > > > 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.
> > > >
> > > > > > @@ -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;
> > > > > >         }
> > > > > >
> > > > > >   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;
> > > >
> > > > +/*
> > > > +* 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.
>
> 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
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
> +++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
> @@ -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;
>
> -               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?

Re: [PATCH] cpufreq: conservative: Fix requested_freq handling

[Rafael J. Wysocki]

On Mon, Oct 15, 2018 at 2:51 PM Waldemar Rymarkiewicz
<waldemar.rymarkiewicz@gmail.com> wrote:
>
> On Mon, 15 Oct 2018 at 13:34, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> >
> > On Monday, October 15, 2018 11:34:33 AM CEST Waldemar Rymarkiewicz wrote:
> > > On Thu, 11 Oct 2018 at 23:10, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > > >
> > > > On Tuesday, October 9, 2018 6:06:08 PM CEST Waldemar Rymarkiewicz wrote:
> > > > > On Tue, 9 Oct 2018 at 09:47, Rafael J. Wysocki <rafael@kernel.org> wrote:
> > > > > >
> > > > > > On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> > > > > > <waldemar.rymarkiewicz@gmail.com> wrote:
> > > > > > >
> > > > > > > From: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>
> > > > > > >
> > > > > > > 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.
> > > > >
> > > > > > > @@ -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;
> > > > > > >         }
> > > > > > >
> > > > > > >   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;
> > > > >
> > > > > +/*
> > > > > +* 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.
> >
> > 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
> > ===================================================================
> > --- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
> > +++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
> > @@ -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;
> >
> > -               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?

Yes, I will, but let me resend it with a proper changelog first.

Thanks,
Rafael

[PATCH] cpufreq: conservative: Take limits changes into account properly

[Rafael J. Wysocki]

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

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 <waldemarx.rymarkiewicz@intel.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq_conservative.c |    6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
+++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
@@ -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;
 
-		if (requested_freq > freq_steps)
+		if (requested_freq > policy->min + freq_steps)
 			requested_freq -= freq_steps;
 		else
 			requested_freq = policy->min;

Re: [PATCH] cpufreq: conservative: Take limits changes into account properly

[Waldemar Rymarkiewicz]

On Mon, 15 Oct 2018 at 23:24, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>
> 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 <waldemarx.rymarkiewicz@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq_conservative.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> Index: linux-pm/drivers/cpufreq/cpufreq_conservative.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/cpufreq_conservative.c
> +++ linux-pm/drivers/cpufreq/cpufreq_conservative.c
> @@ -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;
>
> -               if (requested_freq > freq_steps)
> +               if (requested_freq > policy->min + freq_steps)
>                         requested_freq -= freq_steps;
>                 else
>                         requested_freq = policy->min;

Looks good.

Acked-by: Waldemar Rymarkiewicz <waldemarx.rymarkiewicz@intel.com>

Re: [PATCH] cpufreq: conservative: Take limits changes into account properly

[Viresh Kumar]

On 15-10-18, 23:21, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> 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 <waldemarx.rymarkiewicz@intel.com>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/cpufreq_conservative.c |    6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)

Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

-- 
viresh