| Message ID | 002501d375d2$54f942c0$feebc840$@net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Fri, Dec 15, 2017 at 7:27 PM, Doug Smythies <dsmythies@telus.net> wrote: > On 2017.12.15 07:54 Rafael J. Wysocki wrote: >> On Friday, December 15, 2017 8:37:38 AM Doug Smythies wrote: >> >>> Could you please try the below reversion patch. >>> It is on top of kernel 4.15-rc3. >>> >>> On my system, and for the intel_cpufrequ scaling driver, >>> the default sampling_rate goes back to 20000 (which works) >>> from 500 uSec (which doesn't), for both the conservative >>> and ondemand governors. >> >> OK >> >> So AFAICS the problem is that dbs_update() makes assumptions that are not >> me, quite obviously, if the sampling interval is less than the tick period. >> >> For example, the "time_elapsed > 2 * sampling_rate" condition in it may >> very well trigger every time in that case and that obviously affects the >> conservative governor. > > It does trigger every time and that is why the conservative gov. sticks at > low frequency. > >> What about the patch below, then? > > I am testing now, from previous work, and already reported on this thread, > I already know that the conservative governor will be O.K.. > It will take me awhile to thoroughly test the ondemand governor. At only 1.0 > ticks at best it will be quite noisy and more sensitive to work/sleep frequencies > for periodic workflows. OK Please also check a variant with 2 * TICK_NSEC / NSEC_PER_USEC on the right-hand side. If that's substantially less noisy, we can use it instead. The response latency of the governor obviously depends on that, but the factor of 2 shouldn't really matter that much in that respect. > By the way, I modified your proposed patch, which needed to divide by > NSEC_PER_USEC instead of multiply by. > i.e. I am testing this: > > diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c > index 58d4f4e..1c2fd1d 100644 > --- a/drivers/cpufreq/cpufreq_governor.c > +++ b/drivers/cpufreq/cpufreq_governor.c > @@ -430,7 +430,14 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy) > if (ret) > goto free_policy_dbs_info; > > + /* > + * The sampling interval should not be greater than the transition > + * latency of the CPU, but it also cannot be less than the tick period > + * for dbs_update() to work correctly. > + */ > dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy); > + if (dbs_data->sampling_rate < TICK_NSEC / NSEC_PER_USEC) > + dbs_data->sampling_rate = TICK_NSEC / NSEC_PER_USEC; > > if (!have_governor_per_policy()) > gov->gdbs_data = dbs_data; > Yeah, that's what I really meant. :-) Using the multiplication instead of the division was a mistake (again, quite obviously). Thanks, Rafael
On 2017.12.17 17:16 Rafael J. Wysocki wrote: > After commit aa7519af450d (cpufreq: Use transition_delay_us for legacy > governors as well) the sampling_rate field of struct dbs_data may be > less than the tick period which causes dbs_update() to produce > incorrect results, so make the code ensure that the value of that > field will always be sufficiently large. > > Fixes: aa7519af450d (cpufreq: Use transition_delay_us for legacy governors as well) > Reported-by: Andy Tang <andy.tang@nxp.com> > Reported-by: Doug Smythies <dsmythies@telus.net> > Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/cpufreq/cpufreq_governor.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > Index: linux-pm/drivers/cpufreq/cpufreq_governor.c > =================================================================== > --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c > +++ linux-pm/drivers/cpufreq/cpufreq_governor.c > @@ -22,6 +22,8 @@ > > #include "cpufreq_governor.h" > > +#define CPUFREQ_DBS_MIN_SAMPLING_INTERVAL (2 * TICK_NSEC / NSEC_PER_USEC) > + Left over from the other thread on Friday I was testing both the above and the 1 TICK version: +#define CPUFREQ_DBS_MIN_SAMPLING_INTERVAL (TICK_NSEC / NSEC_PER_USEC) I tested periodic workflows at around 35% average load with work/sleep frequencies from 100 to 2100 hertz, for both 250 Hertz kernels (4 millisecond TICK) and 1000 Hertz kernels (1 millisecond TICK) The 1 TICK version does have a "nosier" response than the 2 TICK version, but both seem to work fine. Neither are worse than the schedutil response for the same test. I'd be O.K. with either the 1 TICK or 2 TICK versions. ... Doug
On Mon, Dec 18, 2017 at 5:11 PM, Doug Smythies <dsmythies@telus.net> wrote: > On 2017.12.17 17:16 Rafael J. Wysocki wrote: > >> After commit aa7519af450d (cpufreq: Use transition_delay_us for legacy >> governors as well) the sampling_rate field of struct dbs_data may be >> less than the tick period which causes dbs_update() to produce >> incorrect results, so make the code ensure that the value of that >> field will always be sufficiently large. >> >> Fixes: aa7519af450d (cpufreq: Use transition_delay_us for legacy governors as well) >> Reported-by: Andy Tang <andy.tang@nxp.com> >> Reported-by: Doug Smythies <dsmythies@telus.net> >> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >> --- >> drivers/cpufreq/cpufreq_governor.c | 19 ++++++++++++++++--- >> 1 file changed, 16 insertions(+), 3 deletions(-) >> >> Index: linux-pm/drivers/cpufreq/cpufreq_governor.c >> =================================================================== >> --- linux-pm.orig/drivers/cpufreq/cpufreq_governor.c >> +++ linux-pm/drivers/cpufreq/cpufreq_governor.c >> @@ -22,6 +22,8 @@ >> >> #include "cpufreq_governor.h" >> >> +#define CPUFREQ_DBS_MIN_SAMPLING_INTERVAL (2 * TICK_NSEC / NSEC_PER_USEC) >> + > > Left over from the other thread on Friday I was testing both the above and > the 1 TICK version: > > +#define CPUFREQ_DBS_MIN_SAMPLING_INTERVAL (TICK_NSEC / NSEC_PER_USEC) > > I tested periodic workflows at around 35% average load with work/sleep > frequencies from 100 to 2100 hertz, for both 250 Hertz kernels > (4 millisecond TICK) and 1000 Hertz kernels (1 millisecond TICK) > > The 1 TICK version does have a "nosier" response than the 2 TICK version, > but both seem to work fine. Neither are worse than the schedutil response > for the same test. > > I'd be O.K. with either the 1 TICK or 2 TICK versions. OK, thanks! I prefer "less noisy", so I'll take the 2 TICK one (originally in this patch). It can still be changed to 1 TICK later if there are complaints (even though I'm not really expecting any). Thanks, Rafael
diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index 58d4f4e..1c2fd1d 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -430,7 +430,14 @@ int cpufreq_dbs_governor_init(struct cpufreq_policy *policy) if (ret) goto free_policy_dbs_info; + /* + * The sampling interval should not be greater than the transition + * latency of the CPU, but it also cannot be less than the tick period + * for dbs_update() to work correctly. + */ dbs_data->sampling_rate = cpufreq_policy_transition_delay_us(policy); + if (dbs_data->sampling_rate < TICK_NSEC / NSEC_PER_USEC) + dbs_data->sampling_rate = TICK_NSEC / NSEC_PER_USEC; if (!have_governor_per_policy()) gov->gdbs_data = dbs_data;