Messages in this thread | | | From | "Rafael J. Wysocki" <> | Date | Fri, 6 Oct 2023 22:03:03 +0200 | Subject | Re: [PATCH 1/2] cpufreq: userspace: Use fine-grained mutex in userspace governor |
| |
On Thu, Oct 5, 2023 at 1:05 PM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 12-09-23, 06:10, Liao Chang wrote: > > The userspace governor currently uses a big global mutex to avoid the > > race condition on the governor_data field of cpufreq_policy structure. > > This leads to a low concurrency if multiple userspace applications are > > trying to set the speed of different policies at the same time. This > > patch introduces a per-policy mutex to allow the updating of different > > policies to be performed concurrently, improving overall concurrency. > > > > Signed-off-by: Liao Chang <liaochang1@huawei.com> > > --- > > drivers/cpufreq/cpufreq_userspace.c | 69 +++++++++++++++++------------ > > 1 file changed, 40 insertions(+), 29 deletions(-) > > > > diff --git a/drivers/cpufreq/cpufreq_userspace.c b/drivers/cpufreq/cpufreq_userspace.c > > index 50a4d7846580..442e31060d62 100644 > > --- a/drivers/cpufreq/cpufreq_userspace.c > > +++ b/drivers/cpufreq/cpufreq_userspace.c > > @@ -16,7 +16,11 @@ > > #include <linux/slab.h> > > > > static DEFINE_PER_CPU(unsigned int, cpu_is_managed); > > -static DEFINE_MUTEX(userspace_mutex); > > + > > +struct userspace_policy { > > + unsigned int setspeed; > > + struct mutex mutex; > > +}; > > > > /** > > * cpufreq_set - set the CPU frequency > > @@ -28,19 +32,19 @@ static DEFINE_MUTEX(userspace_mutex); > > static int cpufreq_set(struct cpufreq_policy *policy, unsigned int freq) > > { > > int ret = -EINVAL; > > - unsigned int *setspeed = policy->governor_data; > > + struct userspace_policy *userspace = policy->governor_data; > > > > pr_debug("cpufreq_set for cpu %u, freq %u kHz\n", policy->cpu, freq); > > > > - mutex_lock(&userspace_mutex); > > + mutex_lock(&userspace->mutex); > > if (!per_cpu(cpu_is_managed, policy->cpu)) > > goto err; > > > > - *setspeed = freq; > > + userspace->setspeed = freq; > > > > ret = __cpufreq_driver_target(policy, freq, CPUFREQ_RELATION_L); > > err: > > - mutex_unlock(&userspace_mutex); > > + mutex_unlock(&userspace->mutex); > > return ret; > > } > > > > @@ -51,67 +55,74 @@ static ssize_t show_speed(struct cpufreq_policy *policy, char *buf) > > > > static int cpufreq_userspace_policy_init(struct cpufreq_policy *policy) > > { > > - unsigned int *setspeed; > > + struct userspace_policy *userspace; > > > > - setspeed = kzalloc(sizeof(*setspeed), GFP_KERNEL); > > - if (!setspeed) > > + userspace = kzalloc(sizeof(*userspace), GFP_KERNEL); > > + if (!userspace) > > return -ENOMEM; > > > > - policy->governor_data = setspeed; > > + mutex_init(&userspace->mutex); > > + > > + policy->governor_data = userspace; > > return 0; > > } > > > > +/* > > + * Any routine that writes to the policy struct will hold the "rwsem" of > > + * policy struct that means it is free to free "governor_data" here. > > + */ > > static void cpufreq_userspace_policy_exit(struct cpufreq_policy *policy) > > { > > - mutex_lock(&userspace_mutex); > > kfree(policy->governor_data); > > policy->governor_data = NULL; > > - mutex_unlock(&userspace_mutex); > > } > > > > static int cpufreq_userspace_policy_start(struct cpufreq_policy *policy) > > { > > - unsigned int *setspeed = policy->governor_data; > > + struct userspace_policy *userspace = policy->governor_data; > > > > BUG_ON(!policy->cur); > > pr_debug("started managing cpu %u\n", policy->cpu); > > > > - mutex_lock(&userspace_mutex); > > + mutex_lock(&userspace->mutex); > > per_cpu(cpu_is_managed, policy->cpu) = 1; > > - *setspeed = policy->cur; > > - mutex_unlock(&userspace_mutex); > > + userspace->setspeed = policy->cur; > > + mutex_unlock(&userspace->mutex); > > return 0; > > } > > > > static void cpufreq_userspace_policy_stop(struct cpufreq_policy *policy) > > { > > - unsigned int *setspeed = policy->governor_data; > > + struct userspace_policy *userspace = policy->governor_data; > > > > pr_debug("managing cpu %u stopped\n", policy->cpu); > > > > - mutex_lock(&userspace_mutex); > > + mutex_lock(&userspace->mutex); > > per_cpu(cpu_is_managed, policy->cpu) = 0; > > - *setspeed = 0; > > - mutex_unlock(&userspace_mutex); > > + userspace->setspeed = 0; > > + mutex_unlock(&userspace->mutex); > > } > > > > static void cpufreq_userspace_policy_limits(struct cpufreq_policy *policy) > > { > > - unsigned int *setspeed = policy->governor_data; > > + struct userspace_policy *userspace = policy->governor_data; > > > > - mutex_lock(&userspace_mutex); > > + mutex_lock(&userspace->mutex); > > > > pr_debug("limit event for cpu %u: %u - %u kHz, currently %u kHz, last set to %u kHz\n", > > - policy->cpu, policy->min, policy->max, policy->cur, *setspeed); > > - > > - if (policy->max < *setspeed) > > - __cpufreq_driver_target(policy, policy->max, CPUFREQ_RELATION_H); > > - else if (policy->min > *setspeed) > > - __cpufreq_driver_target(policy, policy->min, CPUFREQ_RELATION_L); > > + policy->cpu, policy->min, policy->max, policy->cur, userspace->setspeed); > > + > > + if (policy->max < userspace->setspeed) > > + __cpufreq_driver_target(policy, policy->max, > > + CPUFREQ_RELATION_H); > > + else if (policy->min > userspace->setspeed) > > + __cpufreq_driver_target(policy, policy->min, > > + CPUFREQ_RELATION_L); > > else > > - __cpufreq_driver_target(policy, *setspeed, CPUFREQ_RELATION_L); > > + __cpufreq_driver_target(policy, userspace->setspeed, > > + CPUFREQ_RELATION_L); > > > > - mutex_unlock(&userspace_mutex); > > + mutex_unlock(&userspace->mutex); > > } > > > > static struct cpufreq_governor cpufreq_gov_userspace = { > > Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
Applied as 6.7 material along with the [2/2], thanks!
| |