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(-)
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
On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
<[email protected]> wrote:
>
> 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
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 <[email protected]>
> ---
> 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;
> }
On Tue, 9 Oct 2018 at 09:47, Rafael J. Wysocki <[email protected]> wrote:
>
> On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> <[email protected]> wrote:
> >
> > 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
>
> 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
On Tuesday, October 9, 2018 6:06:08 PM CEST Waldemar Rymarkiewicz wrote:
> On Tue, 9 Oct 2018 at 09:47, Rafael J. Wysocki <[email protected]> wrote:
> >
> > On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> > <[email protected]> wrote:
> > >
> > > 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
> >
> > 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);
On Thu, 11 Oct 2018 at 23:10, Rafael J. Wysocki <[email protected]> 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 <[email protected]> wrote:
> > >
> > > On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> > > <[email protected]> wrote:
> > > >
> > > > 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
> > >
> > > 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
On Monday, October 15, 2018 11:34:33 AM CEST Waldemar Rymarkiewicz wrote:
> On Thu, 11 Oct 2018 at 23:10, Rafael J. Wysocki <[email protected]> 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 <[email protected]> wrote:
> > > >
> > > > On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> > > > <[email protected]> wrote:
> > > > >
> > > > > 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
> > > >
> > > > 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;
On Mon, 15 Oct 2018 at 13:34, Rafael J. Wysocki <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> > > > >
> > > > > On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> > > > > <[email protected]> wrote:
> > > > > >
> > > > > > 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
> > > > >
> > > > > 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?
On Mon, Oct 15, 2018 at 2:51 PM Waldemar Rymarkiewicz
<[email protected]> wrote:
>
> On Mon, 15 Oct 2018 at 13:34, Rafael J. Wysocki <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> > > > > >
> > > > > > On Mon, Oct 8, 2018 at 5:11 PM Waldemar Rymarkiewicz
> > > > > > <[email protected]> wrote:
> > > > > > >
> > > > > > > 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
> > > > > >
> > > > > > 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
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
===================================================================
--- 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;
On Mon, 15 Oct 2018 at 23:24, Rafael J. Wysocki <[email protected]> wrote:
>
> 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
> ===================================================================
> --- 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 <[email protected]>
On 15-10-18, 23:21, Rafael J. Wysocki wrote:
> 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(-)
Acked-by: Viresh Kumar <[email protected]>
--
viresh