Re: [PATCH 2/2] cpufreq: ondemand: Replace ignore_nice_load with nice_max_freq

[Thomas Renninger <trenn@suse.de>]

Hi,

On Thursday 21 January 2010 04:15:37 Mike Chan wrote:
..
> **Note: ingore_nice_load has been removed since the same functionality
> can be achieved by setting nice_max_freq to scaling_min_freq.
This breaks userspace tools.
Why not keep ignore_nice_load and set nice_max_freq to min/max internally.
You could mark it deprecated by printk_once("Don't use this file it's
deprecated, use nice_max_freq instead").

> Signed-off-by: Mike Chan <mike@android.com>
> ---
>  drivers/cpufreq/cpufreq_ondemand.c |   96 +++++++++++++++++++++++-------------
>  1 files changed, 62 insertions(+), 34 deletions(-)
Whatabout cpufreq_conservative.c?
Does something similar make sense there, too?
...
> -static ssize_t store_ignore_nice_load(struct kobject *a, struct attribute *b,
> +static ssize_t store_nice_max_freq(struct kobject *a, struct attribute *b,
>  				      const char *buf, size_t count)
>  {
>  	unsigned int input;
> @@ -330,15 +330,12 @@ static ssize_t store_ignore_nice_load(struct kobject *a, struct attribute *b,
>  	if (ret != 1)
>  		return -EINVAL;
>  
> -	if (input > 1)
> -		input = 1;

> -
>  	mutex_lock(&dbs_mutex);
> -	if (input == dbs_tuners_ins.ignore_nice) { /* nothing to do */
> +	if (input == dbs_tuners_ins.nice_max_freq) { /* nothing to do */
>  		mutex_unlock(&dbs_mutex);
>  		return count;
>  	}
> -	dbs_tuners_ins.ignore_nice = input;
> +	dbs_tuners_ins.nice_max_freq = input;
What happens if userspace provides a wrong value?
Sanity checking is ugly at this place, though.
Only idea I have is to iterate over all cpuinfo.{min,max}_freq

...  
>  };
> @@ -412,7 +409,7 @@ static ssize_t store_##file_name##_old					\
>  }
>  write_one_old(sampling_rate);
>  write_one_old(up_threshold);
> -write_one_old(ignore_nice_load);
> +write_one_old(nice_max_freq);
This is the depracated
/sys/devices/system/cpu/cpuX/cpufreq/ondemand
interface. If this should be a global variable for all cores
here:
/sys/devices/system/cpu/cpufreq/ondemand
you don't need it. If this should be configurable per core
you don't need the other and should add this one not marked
old and place it outside this paragraph:
/*** delete after deprecation time ***/

>  write_one_old(powersave_bias);
>  
>  #define define_one_rw_old(object, _name)       \
> @@ -421,7 +418,7 @@ __ATTR(_name, 0644, show_##_name##_old, store_##_name##_old)
>  
>  define_one_rw_old(sampling_rate_old, sampling_rate);
>  define_one_rw_old(up_threshold_old, up_threshold);
> -define_one_rw_old(ignore_nice_load_old, ignore_nice_load);
> +define_one_rw_old(nice_max_freq_old, nice_max_freq);
same here.
>  define_one_rw_old(powersave_bias_old, powersave_bias);
>  
>  static struct attribute *dbs_attributes_old[] = {
> @@ -429,7 +426,7 @@ static struct attribute *dbs_attributes_old[] = {
>         &sampling_rate_min_old.attr,
>         &sampling_rate_old.attr,
>         &up_threshold_old.attr,
> -       &ignore_nice_load_old.attr,
> +       &nice_max_freq_old.attr,
and here.
>         &powersave_bias_old.attr,
>         NULL
>  };
...
> @@ -477,12 +473,13 @@ static void dbs_check_cpu(struct cpu_dbs_info_s *this_dbs_info)
>  	 */
>  
>  	/* Get Absolute Load - in terms of freq */
> -	max_load_freq = 0;
> +	max_load_freq = max_ignore_nice_load_freq = 0;
>  
>  	for_each_cpu(j, policy->cpus) {
>  		struct cpu_dbs_info_s *j_dbs_info;
>  		cputime64_t cur_wall_time, cur_idle_time;
> -		unsigned int idle_time, wall_time;
> +		unsigned int idle_time,  wall_time;
not needed whitespace.

> +		unsigned long cur_nice_jiffies;
>  		unsigned int load, load_freq;
>  		int freq_avg;
>  
...
> @@ -512,27 +513,47 @@ static void dbs_check_cpu(struct cpu_dbs_info_s *this_dbs_info)
>  					cputime64_to_jiffies64(cur_nice);
>  
>  			j_dbs_info->prev_cpu_nice = kstat_cpu(j).cpustat.nice;
> -			idle_time += jiffies_to_usecs(cur_nice_jiffies);
> +			nice_idle_time += jiffies_to_usecs(cur_nice_jiffies);
> +
> +			if (wall_time < nice_idle_time)
> +				continue;
Can wall_time and nice_idle_time ever be both zero?
> +			load = 100 * (wall_time - nice_idle_time) / wall_time;
Then you divide through zero here.
> +			load_freq = load * freq_avg;
> +			if (load_freq > max_ignore_nice_load_freq)
> +				max_ignore_nice_load_freq = load_freq;
>  		}
>  
> -		if (unlikely(!wall_time || wall_time < idle_time))
> +		if (unlikely(!wall_time || wall_time < idle_time +
> +					jiffies_to_usecs(cur_nice_jiffies)))
>  			continue;
>  
...