| Message ID | 20230912061057.2516963-1-liaochang1@huawei.com |
|---|---|
| State | Accepted |
| Commit | 285189c57391360701af348cd57ca0ba8cbf7ff6 |
| Headers | show |
| Series | [1/2] cpufreq: userspace: Use fine-grained mutex in userspace governor | expand |
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>
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!
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 = {
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(-)