Re: [PATCH 2/2] cpufreq: Fix a circular lock dependency problem
On Mon, Jul 23, 2018 at 9:27 PM, Waiman Long wrote: > On 07/23/2018 03:16 PM, Peter Zijlstra wrote: >> On Mon, Jul 23, 2018 at 01:49:39PM -0400, Waiman Long wrote: >>> drivers/cpufreq/cpufreq.c | 16 +++- >>> 1 file changed, 15 insertions(+), 1 deletion(-) >>> >>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >>> index b0dfd32..9cf02d7 100644 >>> --- a/drivers/cpufreq/cpufreq.c >>> +++ b/drivers/cpufreq/cpufreq.c >>> @@ -922,8 +922,22 @@ static ssize_t store(struct kobject *kobj, struct >>> attribute *attr, >>> struct cpufreq_policy *policy = to_policy(kobj); >>> struct freq_attr *fattr = to_attr(attr); >>> ssize_t ret = -EINVAL; >>> +int retries = 3; >>> >>> -cpus_read_lock(); >>> +/* >>> + * cpus_read_trylock() is used here to work around a circular lock >>> + * dependency problem with respect to the cpufreq_register_driver(). >>> + * With a simple retry loop, the chance of not able to get the >>> + * read lock is extremely small. >>> + */ >>> +while (!cpus_read_trylock()) { >>> +if (retries-- <= 0) >>> +return -EBUSY; >>> +/* >>> + * Sleep for about 50ms and retry again. >>> + */ >>> +msleep(50); >>> +} >> That's atrocious. >> >> > I had thought about just returning an error if the trylock fails as CPU > hotplug rarely happened. I can revert to that simple case if others have > no objection. Yes, you can return -EBUSY or -EAGAIN right away from here if the cpus_read_trylock() is not successful. There is not much reason for the sysfs operation to continue in that case.
Re: [PATCH 2/2] cpufreq: Fix a circular lock dependency problem
On Mon, Jul 23, 2018 at 9:16 PM, Peter Zijlstra wrote: > On Mon, Jul 23, 2018 at 01:49:39PM -0400, Waiman Long wrote: >> drivers/cpufreq/cpufreq.c | 16 +++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index b0dfd32..9cf02d7 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -922,8 +922,22 @@ static ssize_t store(struct kobject *kobj, struct >> attribute *attr, >> struct cpufreq_policy *policy = to_policy(kobj); >> struct freq_attr *fattr = to_attr(attr); >> ssize_t ret = -EINVAL; >> + int retries = 3; >> >> - cpus_read_lock(); >> + /* >> + * cpus_read_trylock() is used here to work around a circular lock >> + * dependency problem with respect to the cpufreq_register_driver(). >> + * With a simple retry loop, the chance of not able to get the >> + * read lock is extremely small. >> + */ >> + while (!cpus_read_trylock()) { >> + if (retries-- <= 0) >> + return -EBUSY; >> + /* >> + * Sleep for about 50ms and retry again. >> + */ >> + msleep(50); >> + } > > That's atrocious. Agreed.
Re: [PATCH 2/2] cpufreq: Fix a circular lock dependency problem
On 07/23/2018 03:16 PM, Peter Zijlstra wrote: > On Mon, Jul 23, 2018 at 01:49:39PM -0400, Waiman Long wrote: >> drivers/cpufreq/cpufreq.c | 16 +++- >> 1 file changed, 15 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c >> index b0dfd32..9cf02d7 100644 >> --- a/drivers/cpufreq/cpufreq.c >> +++ b/drivers/cpufreq/cpufreq.c >> @@ -922,8 +922,22 @@ static ssize_t store(struct kobject *kobj, struct >> attribute *attr, >> struct cpufreq_policy *policy = to_policy(kobj); >> struct freq_attr *fattr = to_attr(attr); >> ssize_t ret = -EINVAL; >> +int retries = 3; >> >> -cpus_read_lock(); >> +/* >> + * cpus_read_trylock() is used here to work around a circular lock >> + * dependency problem with respect to the cpufreq_register_driver(). >> + * With a simple retry loop, the chance of not able to get the >> + * read lock is extremely small. >> + */ >> +while (!cpus_read_trylock()) { >> +if (retries-- <= 0) >> +return -EBUSY; >> +/* >> + * Sleep for about 50ms and retry again. >> + */ >> +msleep(50); >> +} > That's atrocious. > > I had thought about just returning an error if the trylock fails as CPU hotplug rarely happened. I can revert to that simple case if others have no objection. Cheers, Longman
Re: [PATCH 2/2] cpufreq: Fix a circular lock dependency problem
On Mon, Jul 23, 2018 at 01:49:39PM -0400, Waiman Long wrote: > drivers/cpufreq/cpufreq.c | 16 +++- > 1 file changed, 15 insertions(+), 1 deletion(-) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index b0dfd32..9cf02d7 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -922,8 +922,22 @@ static ssize_t store(struct kobject *kobj, struct > attribute *attr, > struct cpufreq_policy *policy = to_policy(kobj); > struct freq_attr *fattr = to_attr(attr); > ssize_t ret = -EINVAL; > + int retries = 3; > > - cpus_read_lock(); > + /* > + * cpus_read_trylock() is used here to work around a circular lock > + * dependency problem with respect to the cpufreq_register_driver(). > + * With a simple retry loop, the chance of not able to get the > + * read lock is extremely small. > + */ > + while (!cpus_read_trylock()) { > + if (retries-- <= 0) > + return -EBUSY; > + /* > + * Sleep for about 50ms and retry again. > + */ > + msleep(50); > + } That's atrocious.