* [PATCH] cpufreq: conservative: Fix requested_freq handling @ 2018-10-08 15:09 Waldemar Rymarkiewicz 2018-10-09 7:47 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Waldemar Rymarkiewicz @ 2018-10-08 15:09 UTC (permalink / raw) To: Rafael J. Wysocki, Viresh Kumar Cc: Waldemar Rymarkiewicz, linux-pm, linux-kernel 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 ^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: conservative: Fix requested_freq handling 2018-10-08 15:09 [PATCH] cpufreq: conservative: Fix requested_freq handling Waldemar Rymarkiewicz @ 2018-10-09 7:47 ` Rafael J. Wysocki 2018-10-09 16:06 ` Waldemar Rymarkiewicz 0 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2018-10-09 7:47 UTC (permalink / raw) To: Waldemar Rymarkiewicz Cc: Rafael J. Wysocki, Viresh Kumar, Waldemar Rymarkiewicz, Linux PM, Linux Kernel Mailing List 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; > } ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: conservative: Fix requested_freq handling 2018-10-09 7:47 ` Rafael J. Wysocki @ 2018-10-09 16:06 ` Waldemar Rymarkiewicz 2018-10-11 21:06 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Waldemar Rymarkiewicz @ 2018-10-09 16:06 UTC (permalink / raw) To: rafael Cc: Rafael J. Wysocki, Viresh Kumar, Waldemar Rymarkiewicz, linux-pm, linux-kernel, Bartholomae, Thomas 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: conservative: Fix requested_freq handling 2018-10-09 16:06 ` Waldemar Rymarkiewicz @ 2018-10-11 21:06 ` Rafael J. Wysocki 2018-10-15 9:34 ` Waldemar Rymarkiewicz 0 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2018-10-11 21:06 UTC (permalink / raw) To: Waldemar Rymarkiewicz Cc: Viresh Kumar, Waldemar Rymarkiewicz, linux-pm, linux-kernel, Bartholomae, Thomas 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); ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: conservative: Fix requested_freq handling 2018-10-11 21:06 ` Rafael J. Wysocki @ 2018-10-15 9:34 ` Waldemar Rymarkiewicz 2018-10-15 11:31 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Waldemar Rymarkiewicz @ 2018-10-15 9:34 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Viresh Kumar, Waldemar Rymarkiewicz, linux-pm, linux-kernel, Bartholomae, Thomas 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: conservative: Fix requested_freq handling 2018-10-15 9:34 ` Waldemar Rymarkiewicz @ 2018-10-15 11:31 ` Rafael J. Wysocki 2018-10-15 12:50 ` Waldemar Rymarkiewicz 0 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2018-10-15 11:31 UTC (permalink / raw) To: Waldemar Rymarkiewicz Cc: Viresh Kumar, Waldemar Rymarkiewicz, linux-pm, linux-kernel, Bartholomae, Thomas 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; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: conservative: Fix requested_freq handling 2018-10-15 11:31 ` Rafael J. Wysocki @ 2018-10-15 12:50 ` Waldemar Rymarkiewicz 2018-10-15 21:03 ` Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Waldemar Rymarkiewicz @ 2018-10-15 12:50 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Viresh Kumar, Waldemar Rymarkiewicz, linux-pm, linux-kernel, Bartholomae, Thomas 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? ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: conservative: Fix requested_freq handling 2018-10-15 12:50 ` Waldemar Rymarkiewicz @ 2018-10-15 21:03 ` Rafael J. Wysocki 2018-10-15 21:21 ` [PATCH] cpufreq: conservative: Take limits changes into account properly Rafael J. Wysocki 0 siblings, 1 reply; 11+ messages in thread From: Rafael J. Wysocki @ 2018-10-15 21:03 UTC (permalink / raw) To: Waldemar Rymarkiewicz Cc: Rafael J. Wysocki, Viresh Kumar, Waldemar Rymarkiewicz, Linux PM, Linux Kernel Mailing List, t.bartholomae 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH] cpufreq: conservative: Take limits changes into account properly 2018-10-15 21:03 ` Rafael J. Wysocki @ 2018-10-15 21:21 ` Rafael J. Wysocki 2018-10-16 6:38 ` Waldemar Rymarkiewicz 2018-10-16 9:52 ` Viresh Kumar 0 siblings, 2 replies; 11+ messages in thread From: Rafael J. Wysocki @ 2018-10-15 21:21 UTC (permalink / raw) To: Linux PM Cc: Waldemar Rymarkiewicz, Viresh Kumar, Waldemar Rymarkiewicz, Linux Kernel Mailing List, t.bartholomae 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; ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: conservative: Take limits changes into account properly 2018-10-15 21:21 ` [PATCH] cpufreq: conservative: Take limits changes into account properly Rafael J. Wysocki @ 2018-10-16 6:38 ` Waldemar Rymarkiewicz 2018-10-16 9:52 ` Viresh Kumar 1 sibling, 0 replies; 11+ messages in thread From: Waldemar Rymarkiewicz @ 2018-10-16 6:38 UTC (permalink / raw) To: Rafael J. Wysocki Cc: linux-pm, Viresh Kumar, Waldemar Rymarkiewicz, linux-kernel, Bartholomae, Thomas 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> ^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH] cpufreq: conservative: Take limits changes into account properly 2018-10-15 21:21 ` [PATCH] cpufreq: conservative: Take limits changes into account properly Rafael J. Wysocki 2018-10-16 6:38 ` Waldemar Rymarkiewicz @ 2018-10-16 9:52 ` Viresh Kumar 1 sibling, 0 replies; 11+ messages in thread From: Viresh Kumar @ 2018-10-16 9:52 UTC (permalink / raw) To: Rafael J. Wysocki Cc: Linux PM, Waldemar Rymarkiewicz, Waldemar Rymarkiewicz, Linux Kernel Mailing List, t.bartholomae 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 ^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2018-10-16 9:52 UTC | newest] Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2018-10-08 15:09 [PATCH] cpufreq: conservative: Fix requested_freq handling Waldemar Rymarkiewicz 2018-10-09 7:47 ` Rafael J. Wysocki 2018-10-09 16:06 ` Waldemar Rymarkiewicz 2018-10-11 21:06 ` Rafael J. Wysocki 2018-10-15 9:34 ` Waldemar Rymarkiewicz 2018-10-15 11:31 ` Rafael J. Wysocki 2018-10-15 12:50 ` Waldemar Rymarkiewicz 2018-10-15 21:03 ` Rafael J. Wysocki 2018-10-15 21:21 ` [PATCH] cpufreq: conservative: Take limits changes into account properly Rafael J. Wysocki 2018-10-16 6:38 ` Waldemar Rymarkiewicz 2018-10-16 9:52 ` Viresh Kumar
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).