| Message ID | a71abc2219d80e9b8e77599d2e60612a96bb36a9.1417167599.git.viresh.kumar@linaro.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Eduardo Valentin |
| Headers | show |
Hi Viresh, On Fri, Nov 28, 2014 at 09:44:10AM +0000, Viresh Kumar wrote: > Locking around idr_alloc/idr_remove looks rather pointless as there is no race > it is trying to fix. Remove it. You are assuming that all cpufreq cooling devices are registered sequentially, one after the other. That doesn't need be the case. I don't think the performance that you get from this patch justifies the possible races that could be introduced by not having the locking. Why should we remove this? > get_idr() and release_idr() aren't much useful now, so get rid of them as well. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > @Eduardo: Same is true for thermal-core as well ? I think that my previous concern applies to thermal_core as well, thermal zones may not be initialised sequentially. Cheers, Javi -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, Nov 28, 2014 at 03:14:10PM +0530, Viresh Kumar wrote: > Locking around idr_alloc/idr_remove looks rather pointless as there is no race > it is trying to fix. Remove it. > Can you please elaborate on how the idr's are going to be serialize without the lock? > get_idr() and release_idr() aren't much useful now, so get rid of them as well. > > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> > --- > @Eduardo: Same is true for thermal-core as well ? Probably not ? > --- > drivers/thermal/cpu_cooling.c | 45 ++++--------------------------------------- > 1 file changed, 4 insertions(+), 41 deletions(-) > > diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c > index 032cba3..ca8f1bb 100644 > --- a/drivers/thermal/cpu_cooling.c > +++ b/drivers/thermal/cpu_cooling.c > @@ -73,42 +73,6 @@ static unsigned int cpufreq_dev_count; > #define NOTIFY_INVALID NULL > static struct cpufreq_cooling_device *notify_device; > > -/** > - * get_idr - function to get a unique id. > - * @idr: struct idr * handle used to create a id. > - * @id: int * value generated by this function. > - * > - * This function will populate @id with an unique > - * id, using the idr API. > - * > - * Return: 0 on success, an error code on failure. > - */ > -static int get_idr(struct idr *idr, int *id) > -{ > - int ret; > - > - mutex_lock(&cooling_cpufreq_lock); > - ret = idr_alloc(idr, NULL, 0, 0, GFP_KERNEL); > - mutex_unlock(&cooling_cpufreq_lock); > - if (unlikely(ret < 0)) > - return ret; > - *id = ret; > - > - return 0; > -} > - > -/** > - * release_idr - function to free the unique id. > - * @idr: struct idr * handle used for creating the id. > - * @id: int value representing the unique id. > - */ > -static void release_idr(struct idr *idr, int id) > -{ > - mutex_lock(&cooling_cpufreq_lock); > - idr_remove(idr, id); > - mutex_unlock(&cooling_cpufreq_lock); > -} > - > /* Below code defines functions to be used for cpufreq as cooling device */ > > enum cpufreq_cooling_property { > @@ -430,7 +394,6 @@ __cpufreq_cooling_register(struct device_node *np, > struct thermal_cooling_device *cool_dev; > struct cpufreq_cooling_device *cpufreq_dev; > char dev_name[THERMAL_NAME_LENGTH]; > - int ret = 0; > > cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL); > if (!cpufreq_dev) > @@ -438,8 +401,8 @@ __cpufreq_cooling_register(struct device_node *np, > > cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus); > > - ret = get_idr(&cpufreq_idr, &cpufreq_dev->id); > - if (ret) { > + cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL); > + if (unlikely(cpufreq_dev->id < 0)) { > cool_dev = ERR_PTR(cpufreq_dev->id); > goto free_cdev; > } > @@ -467,7 +430,7 @@ __cpufreq_cooling_register(struct device_node *np, > return cool_dev; > > remove_idr: > - release_idr(&cpufreq_idr, cpufreq_dev->id); > + idr_remove(&cpufreq_idr, cpufreq_dev->id); > free_cdev: > kfree(cpufreq_dev); > > @@ -540,7 +503,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) > mutex_unlock(&cooling_cpufreq_lock); > > thermal_cooling_device_unregister(cpufreq_dev->cool_dev); > - release_idr(&cpufreq_idr, cpufreq_dev->id); > + idr_remove(&cpufreq_idr, cpufreq_dev->id); > kfree(cpufreq_dev); > } > EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister); > -- > 2.0.3.693.g996b0fd >
On 3 December 2014 at 04:35, Eduardo Valentin <edubezval@gmail.com> wrote: > On Fri, Nov 28, 2014 at 03:14:10PM +0530, Viresh Kumar wrote: >> Locking around idr_alloc/idr_remove looks rather pointless as there is no race >> it is trying to fix. Remove it. >> > > Can you please elaborate on how the idr's are going to be serialize > without the lock? Dropped this patch, I was wrong .. -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/thermal/cpu_cooling.c b/drivers/thermal/cpu_cooling.c index 032cba3..ca8f1bb 100644 --- a/drivers/thermal/cpu_cooling.c +++ b/drivers/thermal/cpu_cooling.c @@ -73,42 +73,6 @@ static unsigned int cpufreq_dev_count; #define NOTIFY_INVALID NULL static struct cpufreq_cooling_device *notify_device; -/** - * get_idr - function to get a unique id. - * @idr: struct idr * handle used to create a id. - * @id: int * value generated by this function. - * - * This function will populate @id with an unique - * id, using the idr API. - * - * Return: 0 on success, an error code on failure. - */ -static int get_idr(struct idr *idr, int *id) -{ - int ret; - - mutex_lock(&cooling_cpufreq_lock); - ret = idr_alloc(idr, NULL, 0, 0, GFP_KERNEL); - mutex_unlock(&cooling_cpufreq_lock); - if (unlikely(ret < 0)) - return ret; - *id = ret; - - return 0; -} - -/** - * release_idr - function to free the unique id. - * @idr: struct idr * handle used for creating the id. - * @id: int value representing the unique id. - */ -static void release_idr(struct idr *idr, int id) -{ - mutex_lock(&cooling_cpufreq_lock); - idr_remove(idr, id); - mutex_unlock(&cooling_cpufreq_lock); -} - /* Below code defines functions to be used for cpufreq as cooling device */ enum cpufreq_cooling_property { @@ -430,7 +394,6 @@ __cpufreq_cooling_register(struct device_node *np, struct thermal_cooling_device *cool_dev; struct cpufreq_cooling_device *cpufreq_dev; char dev_name[THERMAL_NAME_LENGTH]; - int ret = 0; cpufreq_dev = kzalloc(sizeof(*cool_dev), GFP_KERNEL); if (!cpufreq_dev) @@ -438,8 +401,8 @@ __cpufreq_cooling_register(struct device_node *np, cpumask_copy(&cpufreq_dev->allowed_cpus, clip_cpus); - ret = get_idr(&cpufreq_idr, &cpufreq_dev->id); - if (ret) { + cpufreq_dev->id = idr_alloc(&cpufreq_idr, NULL, 0, 0, GFP_KERNEL); + if (unlikely(cpufreq_dev->id < 0)) { cool_dev = ERR_PTR(cpufreq_dev->id); goto free_cdev; } @@ -467,7 +430,7 @@ __cpufreq_cooling_register(struct device_node *np, return cool_dev; remove_idr: - release_idr(&cpufreq_idr, cpufreq_dev->id); + idr_remove(&cpufreq_idr, cpufreq_dev->id); free_cdev: kfree(cpufreq_dev); @@ -540,7 +503,7 @@ void cpufreq_cooling_unregister(struct thermal_cooling_device *cdev) mutex_unlock(&cooling_cpufreq_lock); thermal_cooling_device_unregister(cpufreq_dev->cool_dev); - release_idr(&cpufreq_idr, cpufreq_dev->id); + idr_remove(&cpufreq_idr, cpufreq_dev->id); kfree(cpufreq_dev); } EXPORT_SYMBOL_GPL(cpufreq_cooling_unregister);
Locking around idr_alloc/idr_remove looks rather pointless as there is no race it is trying to fix. Remove it. get_idr() and release_idr() aren't much useful now, so get rid of them as well. Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org> --- @Eduardo: Same is true for thermal-core as well ? --- drivers/thermal/cpu_cooling.c | 45 ++++--------------------------------------- 1 file changed, 4 insertions(+), 41 deletions(-)