Re: [PATCH 1/2] CPUFREQ: Remove unneeded dbs_mutexes from ondemand and conservative governors

[Thomas Renninger <trenn@suse.de>]

On Friday 26 June 2009 12:17:09 am Thomas Renninger wrote:
> On Thursday 25 June 2009 04:25:52 pm Mathieu Desnoyers wrote:
> > * Thomas Renninger (trenn@suse.de) wrote:
> > > Comment from Venkatesh:
> > > ...
> > > This mutex is just serializing the changes to those variables. I
> > > could't think of any functionality issues of not having the lock as
> > > such.
> > >
> > > -> rip it out.
> > >
> > > CC: Venkatesh Pallipadi <venkatesh.pallipadi@intel.com>
> > > Signed-off-by: Thomas Renninger <trenn@suse.de>
> > > ---
> > >  drivers/cpufreq/cpufreq_conservative.c |   61
> > > +++----------------------------- drivers/cpufreq/cpufreq_ondemand.c    
> > > | 48 +++---------------------- 2 files changed, 10 insertions(+), 99
> > > deletions(-)
> > >
> > > diff --git a/drivers/cpufreq/cpufreq_conservative.c
> > > b/drivers/cpufreq/cpufreq_conservative.c index 7a74d17..6303379 100644
> > > --- a/drivers/cpufreq/cpufreq_conservative.c
> > > +++ b/drivers/cpufreq/cpufreq_conservative.c
> > > @@ -18,7 +18,6 @@
> > >  #include <linux/cpu.h>
> > >  #include <linux/jiffies.h>
> > >  #include <linux/kernel_stat.h>
> > > -#include <linux/mutex.h>
> > >  #include <linux/hrtimer.h>
> > >  #include <linux/tick.h>
> > >  #include <linux/ktime.h>
> > > @@ -84,19 +83,6 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s,
> > > cpu_dbs_info);
> > >
> > >  static unsigned int dbs_enable;	/* number of CPUs using this policy */
> > >
> > > -/*
> > > - * DEADLOCK ALERT! There is a ordering requirement between cpu_hotplug
> > > - * lock and dbs_mutex. cpu_hotplug lock should always be held before
> > > - * dbs_mutex. If any function that can potentially take cpu_hotplug
> > > lock - * (like __cpufreq_driver_target()) is being called with
> > > dbs_mutex taken, then - * cpu_hotplug lock should be taken before that.
> > > Note that cpu_hotplug lock - * is recursive for the same process.
> > > -Venki - * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the
> > > dbs_mutex, because it - * would deadlock with
> > > cancel_delayed_work_sync(), which is needed for proper - * raceless
> > > workqueue teardown.
> > > - */
> > > -static DEFINE_MUTEX(dbs_mutex);
> > > -
> > >  static struct workqueue_struct	*kconservative_wq;
> > >
> > >  static struct dbs_tuners {
> > > @@ -236,10 +222,7 @@ static ssize_t store_sampling_down_factor(struct
> > > cpufreq_policy *unused, if (ret != 1 || input >
> > > MAX_SAMPLING_DOWN_FACTOR
> > >
> > > || input < 1) return -EINVAL;
> > >
> > > -	mutex_lock(&dbs_mutex);
> > >  	dbs_tuners_ins.sampling_down_factor = input;
> > > -	mutex_unlock(&dbs_mutex);
> > > -
> > >  	return count;
> > >  }
> > >
> > > @@ -253,10 +236,7 @@ static ssize_t store_sampling_rate(struct
> > > cpufreq_policy *unused, if (ret != 1)
> > >  		return -EINVAL;
> > >
> > > -	mutex_lock(&dbs_mutex);
> > >  	dbs_tuners_ins.sampling_rate = max(input, minimum_sampling_rate());
> > > -	mutex_unlock(&dbs_mutex);
> > > -
> > >  	return count;
> > >  }
> > >
> > > @@ -267,16 +247,11 @@ static ssize_t store_up_threshold(struct
> > > cpufreq_policy *unused, int ret;
> > >  	ret = sscanf(buf, "%u", &input);
> > >
> > > -	mutex_lock(&dbs_mutex);
> > >  	if (ret != 1 || input > 100 ||
> > > -			input <= dbs_tuners_ins.down_threshold) {
> > > -		mutex_unlock(&dbs_mutex);
> > > +			input <= dbs_tuners_ins.down_threshold)
> > >  		return -EINVAL;
> > > -	}
> > >
> > >  	dbs_tuners_ins.up_threshold = input;
> > > -	mutex_unlock(&dbs_mutex);
> >
> > Here, for instance, there might be a problem if down_threshold is
> > changed concurrently with a store_up_threshold() call. See that there is
> > a test before the modification, and we need the mutex there for it to be
> > consistent.
>
> Thanks, I was rather quick with the conservative changes..., but
> it should still be ok.
>
> It should be assured that if userspace is doing:
> echo x > down_threshold
> echo y > up_threshold
> that the first one will be served/finished first?
>
> If userspace is writing different values for each core to the global
> conservative/ondemand tunables, or you have concurent userspace tools
> trying to configure ondemand/conservative, it's a userspace bug.
> It's confusing that ondemand/conservative allows per core reads/writes to
> global variables and I hope to be able to provide something to change that
> in some days, maybe weeks.
>
> If you still can think of a possible issue, a userspace scenario would
> help.
>
> > > -
> > >  	return count;
> > >  }
> > >
> > > @@ -287,17 +262,12 @@ static ssize_t store_down_threshold(struct
> > > cpufreq_policy *unused, int ret;
> > >  	ret = sscanf(buf, "%u", &input);
> > >
> > > -	mutex_lock(&dbs_mutex);
> > >  	/* cannot be lower than 11 otherwise freq will not fall */
> > >  	if (ret != 1 || input < 11 || input > 100 ||
> > > -			input >= dbs_tuners_ins.up_threshold) {
> > > -		mutex_unlock(&dbs_mutex);
> > > +			input >= dbs_tuners_ins.up_threshold)
> > >  		return -EINVAL;
> > > -	}
> > >
> > >  	dbs_tuners_ins.down_threshold = input;
> > > -	mutex_unlock(&dbs_mutex);
> > > -
> > >  	return count;
> > >  }
> > >
> > > @@ -316,11 +286,9 @@ static ssize_t store_ignore_nice_load(struct
> > > cpufreq_policy *policy, if (input > 1)
> > >  		input = 1;
> > >
> > > -	mutex_lock(&dbs_mutex);
> > > -	if (input == dbs_tuners_ins.ignore_nice) { /* nothing to do */
> > > -		mutex_unlock(&dbs_mutex);
> > > +	if (input == dbs_tuners_ins.ignore_nice) /* nothing to do */
> > >  		return count;
> > > -	}
> > > +
> > >  	dbs_tuners_ins.ignore_nice = input;
> > >
> > >  	/* we need to re-evaluate prev_cpu_idle */
> > > @@ -332,8 +300,6 @@ static ssize_t store_ignore_nice_load(struct
> > > cpufreq_policy *policy, if (dbs_tuners_ins.ignore_nice)
> > >  			dbs_info->prev_cpu_nice = kstat_cpu(j).cpustat.nice;
> > >  	}
> > > -	mutex_unlock(&dbs_mutex);
> > > -
> > >  	return count;
> > >  }
> > >
> > > @@ -352,10 +318,7 @@ static ssize_t store_freq_step(struct
> > > cpufreq_policy *policy,
> > >
> > >  	/* no need to test here if freq_step is zero as the user might
> > > actually * want this, they would be crazy though :) */
> > > -	mutex_lock(&dbs_mutex);
> > >  	dbs_tuners_ins.freq_step = input;
> > > -	mutex_unlock(&dbs_mutex);
> > > -
> > >  	return count;
> > >  }
> > >
> > > @@ -566,13 +529,9 @@ static int cpufreq_governor_dbs(struct
> > > cpufreq_policy *policy,
> >
> > Hrm, this is where we want the mutexes removed, but I fear this is
> > creating a race between sysfs_create_group (sysfs file creation) and
> > policy initialization...
>
> This can be solved by moving this_dbs_info->enable incremenation
> after sysfs_create_group.
Forget this sentence, don't think about it, it's crap.
I better go to bed now...

       Thomas

> But yes, I forgot that in my patch, thanks!
>
> > I'm not convinced this mutex is not needed.
>
> I am. Maybe it still takes some more thinking or step by step rework.
> Finding an unintrusive, riskless short term solution for .30 is a
> challenge, though.
>
>     Thomas
>
> > Mathieu
> >
> > >  		if (this_dbs_info->enable) /* Already enabled */
> > >  			break;
> > >
> > > -		mutex_lock(&dbs_mutex);
> > > -
> > >  		rc = sysfs_create_group(&policy->kobj, &dbs_attr_group);
> > > -		if (rc) {
> > > -			mutex_unlock(&dbs_mutex);
> > > +		if (rc)
> > >  			return rc;
> > > -		}
> > >
> > >  		for_each_cpu(j, policy->cpus) {
> > >  			struct cpu_dbs_info_s *j_dbs_info;
> > > @@ -612,13 +571,9 @@ static int cpufreq_governor_dbs(struct
> > > cpufreq_policy *policy, CPUFREQ_TRANSITION_NOTIFIER);
> > >  		}
> > >  		dbs_timer_init(this_dbs_info);
> > > -
> > > -		mutex_unlock(&dbs_mutex);
> > > -
> > >  		break;
> > >
> > >  	case CPUFREQ_GOV_STOP:
> > > -		mutex_lock(&dbs_mutex);
> > >  		dbs_timer_exit(this_dbs_info);
> > >  		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
> > >  		dbs_enable--;
> > > @@ -631,13 +586,9 @@ static int cpufreq_governor_dbs(struct
> > > cpufreq_policy *policy, cpufreq_unregister_notifier(
> > >  					&dbs_cpufreq_notifier_block,
> > >  					CPUFREQ_TRANSITION_NOTIFIER);
> > > -
> > > -		mutex_unlock(&dbs_mutex);
> > > -
> > >  		break;
> > >
> > >  	case CPUFREQ_GOV_LIMITS:
> > > -		mutex_lock(&dbs_mutex);
> > >  		if (policy->max < this_dbs_info->cur_policy->cur)
> > >  			__cpufreq_driver_target(
> > >  					this_dbs_info->cur_policy,
> > > @@ -646,8 +597,6 @@ static int cpufreq_governor_dbs(struct
> > > cpufreq_policy *policy, __cpufreq_driver_target(
> > >  					this_dbs_info->cur_policy,
> > >  					policy->min, CPUFREQ_RELATION_L);
> > > -		mutex_unlock(&dbs_mutex);
> > > -
> > >  		break;
> > >  	}
> > >  	return 0;
> > > diff --git a/drivers/cpufreq/cpufreq_ondemand.c
> > > b/drivers/cpufreq/cpufreq_ondemand.c index e741c33..d080a48 100644
> > > --- a/drivers/cpufreq/cpufreq_ondemand.c
> > > +++ b/drivers/cpufreq/cpufreq_ondemand.c
> > > @@ -17,7 +17,6 @@
> > >  #include <linux/cpu.h>
> > >  #include <linux/jiffies.h>
> > >  #include <linux/kernel_stat.h>
> > > -#include <linux/mutex.h>
> > >  #include <linux/hrtimer.h>
> > >  #include <linux/tick.h>
> > >  #include <linux/ktime.h>
> > > @@ -91,19 +90,6 @@ static DEFINE_PER_CPU(struct cpu_dbs_info_s,
> > > cpu_dbs_info);
> > >
> > >  static unsigned int dbs_enable;	/* number of CPUs using this policy */
> > >
> > > -/*
> > > - * DEADLOCK ALERT! There is a ordering requirement between cpu_hotplug
> > > - * lock and dbs_mutex. cpu_hotplug lock should always be held before
> > > - * dbs_mutex. If any function that can potentially take cpu_hotplug
> > > lock - * (like __cpufreq_driver_target()) is being called with
> > > dbs_mutex taken, then - * cpu_hotplug lock should be taken before that.
> > > Note that cpu_hotplug lock - * is recursive for the same process.
> > > -Venki - * DEADLOCK ALERT! (2) : do_dbs_timer() must not take the
> > > dbs_mutex, because it - * would deadlock with
> > > cancel_delayed_work_sync(), which is needed for proper - * raceless
> > > workqueue teardown.
> > > - */
> > > -static DEFINE_MUTEX(dbs_mutex);
> > > -
> > >  static struct workqueue_struct	*kondemand_wq;
> > >
> > >  static struct dbs_tuners {
> > > @@ -269,14 +255,10 @@ static ssize_t store_sampling_rate(struct
> > > cpufreq_policy *unused, int ret;
> > >  	ret = sscanf(buf, "%u", &input);
> > >
> > > -	mutex_lock(&dbs_mutex);
> > > -	if (ret != 1) {
> > > -		mutex_unlock(&dbs_mutex);
> > > +	if (ret != 1)
> > >  		return -EINVAL;
> > > -	}
> > > -	dbs_tuners_ins.sampling_rate = max(input, minimum_sampling_rate());
> > > -	mutex_unlock(&dbs_mutex);
> > >
> > > +	dbs_tuners_ins.sampling_rate = max(input, minimum_sampling_rate());
> > >  	return count;
> > >  }
> > >
> > > @@ -287,16 +269,11 @@ static ssize_t store_up_threshold(struct
> > > cpufreq_policy *unused, int ret;
> > >  	ret = sscanf(buf, "%u", &input);
> > >
> > > -	mutex_lock(&dbs_mutex);
> > >  	if (ret != 1 || input > MAX_FREQUENCY_UP_THRESHOLD ||
> > > -			input < MIN_FREQUENCY_UP_THRESHOLD) {
> > > -		mutex_unlock(&dbs_mutex);
> > > +			input < MIN_FREQUENCY_UP_THRESHOLD)
> > >  		return -EINVAL;
> > > -	}
> > >
> > >  	dbs_tuners_ins.up_threshold = input;
> > > -	mutex_unlock(&dbs_mutex);
> > > -
> > >  	return count;
> > >  }
> > >
> > > @@ -315,11 +292,9 @@ static ssize_t store_ignore_nice_load(struct
> > > cpufreq_policy *policy, if (input > 1)
> > >  		input = 1;
> > >
> > > -	mutex_lock(&dbs_mutex);
> > > -	if (input == dbs_tuners_ins.ignore_nice) { /* nothing to do */
> > > -		mutex_unlock(&dbs_mutex);
> > > +	if (input == dbs_tuners_ins.ignore_nice) /* nothing to do */
> > >  		return count;
> > > -	}
> > > +
> > >  	dbs_tuners_ins.ignore_nice = input;
> > >
> > >  	/* we need to re-evaluate prev_cpu_idle */
> > > @@ -332,8 +307,6 @@ static ssize_t store_ignore_nice_load(struct
> > > cpufreq_policy *policy, dbs_info->prev_cpu_nice =
> > > kstat_cpu(j).cpustat.nice;
> > >
> > >  	}
> > > -	mutex_unlock(&dbs_mutex);
> > > -
> > >  	return count;
> > >  }
> > >
> > > @@ -350,10 +323,8 @@ static ssize_t store_powersave_bias(struct
> > > cpufreq_policy *unused, if (input > 1000)
> > >  		input = 1000;
> > >
> > > -	mutex_lock(&dbs_mutex);
> > >  	dbs_tuners_ins.powersave_bias = input;
> > >  	ondemand_powersave_bias_init();
> > > -	mutex_unlock(&dbs_mutex);
> > >
> > >  	return count;
> > >  }
> > > @@ -586,13 +557,11 @@ static int cpufreq_governor_dbs(struct
> > > cpufreq_policy *policy, if (this_dbs_info->enable) /* Already enabled
> > > */ break;
> > >
> > > -		mutex_lock(&dbs_mutex);
> > >  		dbs_enable++;
> > >
> > >  		rc = sysfs_create_group(&policy->kobj, &dbs_attr_group);
> > >  		if (rc) {
> > >  			dbs_enable--;
> > > -			mutex_unlock(&dbs_mutex);
> > >  			return rc;
> > >  		}
> > >
> > > @@ -627,28 +596,21 @@ static int cpufreq_governor_dbs(struct
> > > cpufreq_policy *policy, dbs_tuners_ins.sampling_rate =
> > > def_sampling_rate; }
> > >  		dbs_timer_init(this_dbs_info);
> > > -
> > > -		mutex_unlock(&dbs_mutex);
> > >  		break;
> > >
> > >  	case CPUFREQ_GOV_STOP:
> > > -		mutex_lock(&dbs_mutex);
> > >  		dbs_timer_exit(this_dbs_info);
> > >  		sysfs_remove_group(&policy->kobj, &dbs_attr_group);
> > >  		dbs_enable--;
> > > -		mutex_unlock(&dbs_mutex);
> > > -
> > >  		break;
> > >
> > >  	case CPUFREQ_GOV_LIMITS:
> > > -		mutex_lock(&dbs_mutex);
> > >  		if (policy->max < this_dbs_info->cur_policy->cur)
> > >  			__cpufreq_driver_target(this_dbs_info->cur_policy,
> > >  				policy->max, CPUFREQ_RELATION_H);
> > >  		else if (policy->min > this_dbs_info->cur_policy->cur)
> > >  			__cpufreq_driver_target(this_dbs_info->cur_policy,
> > >  				policy->min, CPUFREQ_RELATION_L);
> > > -		mutex_unlock(&dbs_mutex);
> > >  		break;
> > >  	}
> > >  	return 0;
> > > --
> > > 1.6.0.2