Re: [PATCH 3/7] CPUFREQ: ondemand/conservative: sanitize sampling_rate restrictions

[Thomas Renninger <trenn@suse.de>]

On Wednesday 04 February 2009 06:07:25 Andrew Morton wrote:
> On Tue,  3 Feb 2009 17:46:42 +0100 Thomas Renninger <trenn@suse.de> wrote:
> > Limit sampling rate to transition_latency * 100 or kernel limits.
> > If sampling_rate is tried to be set too low, set the lowest allowed
> > value.
> >
> > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > ---
> >  Documentation/cpu-freq/governors.txt   |   14 +++++++++++++-
> >  drivers/cpufreq/cpufreq_conservative.c |   19 ++++++++++++++++---
> >  drivers/cpufreq/cpufreq_ondemand.c     |   19 +++++++++++++++----
> >  3 files changed, 44 insertions(+), 8 deletions(-)
> >
> > diff --git a/Documentation/cpu-freq/governors.txt
> > b/Documentation/cpu-freq/governors.txt index 9b18512..ce73f3e 100644
> > --- a/Documentation/cpu-freq/governors.txt
> > +++ b/Documentation/cpu-freq/governors.txt
> > @@ -117,7 +117,19 @@ accessible parameters:
> >  sampling_rate: measured in uS (10^-6 seconds), this is how often you
> >  want the kernel to look at the CPU usage and to make decisions on
> >  what to do about the frequency.  Typically this is set to values of
> > -around '10000' or more.
> > +around '10000' or more. It's default value is (cmp. with
> > users-guide.txt): +transition_latency * 1000
> > +The lowest value you can set is:
> > +transition_latency * 100 or it may get restricted to a value where it
> > +makes not sense for the kernel anymore to poll that often which depends
> > +on your HZ config variable (HZ=1000: max=20000us, HZ=250: max=5000).
> > +Be aware that transition latency is in ns and sampling_rate is in us, so
> > you +get the same sysfs value by default.
> > +Sampling rate should always get adjusted considering the transition
> > latency +To set the sampling rate 750 times as high as the transition
> > latency +in the bash (as said, 1000 is default), do:
> > +echo `$(($(cat cpuinfo_transition_latency) * 750 / 1000)) \
> > +    >ondemand/sampling_rate
> >
> >  show_sampling_rate_(min|max): THIS INTERFACE IS DEPRECATED, DON'T USE
> > IT. You can use wider ranges now and the general
> > diff --git a/drivers/cpufreq/cpufreq_conservative.c
> > b/drivers/cpufreq/cpufreq_conservative.c index 5ba0a3f..b8bbd4a 100644
> > --- a/drivers/cpufreq/cpufreq_conservative.c
> > +++ b/drivers/cpufreq/cpufreq_conservative.c
> > @@ -54,7 +54,18 @@ static unsigned int def_sampling_rate;
> >  			(MIN_SAMPLING_RATE_RATIO * jiffies_to_usecs(10))
> >  #define MIN_SAMPLING_RATE			\
> >  			(def_sampling_rate / MIN_SAMPLING_RATE_RATIO)
This one will vanish after the deprecation time expired.

> > +/* Above MIN_SAMPLING_RATE will vanish with its sysfs file soon
> > + * Define the minimal settable sampling rate to the greater of:
> > + *   - "HW transition latency" * 100 (same as default sampling / 10)
> > + *   - MIN_STAT_SAMPLING_RATE
> > + * To avoid that userspace shoots itself.
> > +*/
> > +#define MINIMUM_SAMPLING_RATE			\
> > +			((def_sampling_rate / 10) < MIN_STAT_SAMPLING_RATE \
> > +			 ? MIN_STAT_SAMPLING_RATE : (def_sampling_rate / 10))
>
> Use
> 	max(def_sampling_rate / 10, MIN_STAT_SAMPLING_RATE)
> ?
Ok.

> But really, this should be a plain old C function.  Hiding a bunch of C
> code inside a macro which purports to be a compile-time constant is rude.
>
> > +/* This will also vanish soon with removing sampling_rate_max */
> >  #define MAX_SAMPLING_RATE			(500 * def_sampling_rate)
>
> Ditto, really.
This one will also be removed after the deprecation time expired.
Therefore I'd prefer to not touch it now and leave it as it is,
but I will remove these with finally removing sampling_rate_{min,max}.

I will also use max(..) where suggested and repost the three, modified
patches.
Thanks for your detailed, review. It's appreciated!

     Thomas

> > +
> >  #define DEF_SAMPLING_RATE_LATENCY_MULTIPLIER	(1000)
> >  #define DEF_SAMPLING_DOWN_FACTOR		(1)
> >  #define MAX_SAMPLING_DOWN_FACTOR		(10)
> > @@ -197,12 +208,14 @@ static ssize_t store_sampling_rate(struct
> > cpufreq_policy *unused, ret = sscanf (buf, "%u", &input);
> >
> >  	mutex_lock(&dbs_mutex);
> > -	if (ret != 1 || input > MAX_SAMPLING_RATE || input < MIN_SAMPLING_RATE)
> > { +	if (ret != 1) {
> >  		mutex_unlock(&dbs_mutex);
> >  		return -EINVAL;
> >  	}
> > -
> > -	dbs_tuners_ins.sampling_rate = input;
> > +	if  (input < MINIMUM_SAMPLING_RATE)
> > +		dbs_tuners_ins.sampling_rate = MINIMUM_SAMPLING_RATE;
> > +	else
> > +		dbs_tuners_ins.sampling_rate = input;
>
> 	dbs_tuners_ins.sampling_rate = max(MINIMUM_SAMPLING_RATE, input);
>
> >  	mutex_unlock(&dbs_mutex);
> >
> >  	return count;
> > diff --git a/drivers/cpufreq/cpufreq_ondemand.c
> > b/drivers/cpufreq/cpufreq_ondemand.c index 4b42e36..70e0841 100644
> > --- a/drivers/cpufreq/cpufreq_ondemand.c
> > +++ b/drivers/cpufreq/cpufreq_ondemand.c
> > @@ -52,6 +52,16 @@ static unsigned int def_sampling_rate;
> >  			(MIN_SAMPLING_RATE_RATIO * jiffies_to_usecs(10))
> >  #define MIN_SAMPLING_RATE			\
> >  			(def_sampling_rate / MIN_SAMPLING_RATE_RATIO)
> > +/* Above MIN_SAMPLING_RATE will vanish with its sysfs file soon
> > + * Define the minimal settable sampling rate to the greater of:
> > + *   - "HW transition latency" * 100 (same as default sampling / 10)
> > + *   - MIN_STAT_SAMPLING_RATE
> > + * To avoid that userspace shoots itself.
> > +*/
> > +#define MINIMUM_SAMPLING_RATE			\
> > +			((def_sampling_rate / 10) < MIN_STAT_SAMPLING_RATE \
> > +			 ? MIN_STAT_SAMPLING_RATE : (def_sampling_rate / 10))
> > +/* This will also vanish soon with removing sampling_rate_max */
> >  #define MAX_SAMPLING_RATE			(500 * def_sampling_rate)
> >  #define DEF_SAMPLING_RATE_LATENCY_MULTIPLIER	(1000)
> >  #define TRANSITION_LATENCY_LIMIT		(10 * 1000 * 1000)
> > @@ -243,13 +253,14 @@ static ssize_t store_sampling_rate(struct
> > cpufreq_policy *unused, ret = sscanf(buf, "%u", &input);
> >
> >  	mutex_lock(&dbs_mutex);
> > -	if (ret != 1 || input > MAX_SAMPLING_RATE
> > -		     || input < MIN_SAMPLING_RATE) {
> > +	if (ret != 1) {
> >  		mutex_unlock(&dbs_mutex);
> >  		return -EINVAL;
> >  	}
> > -
> > -	dbs_tuners_ins.sampling_rate = input;
> > +	if  (input < MINIMUM_SAMPLING_RATE)
> > +		dbs_tuners_ins.sampling_rate = MINIMUM_SAMPLING_RATE;
> > +	else
> > +		dbs_tuners_ins.sampling_rate = input;
> >  	mutex_unlock(&dbs_mutex);
>
> etceterata.
> --
> To unsubscribe from this list: send the line "unsubscribe cpufreq" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html