Message ID | 51F638D9.7050904@linux.vnet.ibm.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers | show |
On Mon, Jul 29, 2013 at 3:11 PM, Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> wrote: > I'm assuming that the module_get() is used in the cpufreq core to ensure that > until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend > driver module (eg: acpi-cpufreq) can't be removed. I missed this simple stuff in my email.. :( > But the cpufreq_add_dev() function does a module *put* at the end of > initializing a fresh CPU. > > 1057 kobject_uevent(&policy->kobj, KOBJ_ADD); > 1058 module_put(cpufreq_driver->owner); > 1059 pr_debug("initialization complete\n"); > 1060 > 1061 return 0; That actually looks wrong. And shoud be fixed. > So, I wonder if it would be a good idea to instead allow that CPU to take a > module refcount as well. That way, the problem that Toralf reported would go > away, and at the same time, we can ensure that as long as the cpufreq core is > managing CPUs, the cpufreq-backend-driver module refcount never drops to zero. > > Something like this: > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index a4ad733..ecfbc52 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -878,9 +878,14 @@ static int cpufreq_add_dev_interface(unsigned int cpu, > } > write_unlock_irqrestore(&cpufreq_driver_lock, flags); > > + /* Bump up the refcount for this CPU */ > + policy = cpufreq_cpu_get(cpu); > + > ret = cpufreq_add_dev_symlink(cpu, policy); > - if (ret) > + if (ret) { > + cpufreq_cpu_put(policy); > goto err_out_kobj_put; > + } That will do an extra kobject_get() which we don't require.. Only removing what I mentioned earlier should be good enough, I believe. Over that, I think below code in __cpufreq_governor() is also wrong. /* we keep one module reference alive for each CPU governed by this CPU */ if ((event != CPUFREQ_GOV_START) || ret) module_put(policy->governor->owner); if ((event == CPUFREQ_GOV_STOP) && !ret) module_put(policy->governor->owner); The second if is sensible as it puts count when governor is stopped. But the first one decrements the count when we failed for anything other than START.. But this routine is called for other stuff too: - INIT/Exit - LIMITS, And so, count must be incremented for a passed INIT call and decremented for a passed EXIT call, otherwise shouldn't be touched. -- 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 07/29/2013 05:19 PM, Rafael J. Wysocki wrote: > On Monday, July 29, 2013 03:11:45 PM Srivatsa S. Bhat wrote: >> On 07/29/2013 04:50 AM, Rafael J. Wysocki wrote: >>> On Monday, July 29, 2013 12:43:59 AM Rafael J. Wysocki wrote: >>>> On Monday, July 29, 2013 12:11:18 AM Rafael J. Wysocki wrote: >>>>> On Sunday, July 28, 2013 12:21:22 PM Toralf Förster wrote: >>>>>> On 07/28/2013 01:39 AM, Rafael J. Wysocki wrote: >>>>>>> On Saturday, July 27, 2013 07:40:34 PM Toralf Förster wrote: >>>>>>>> it gives at a ThinkPad T420: >>>>>>>> >>>>>>>> tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq >>>>>>>> acpi_cpufreq 12902 2147483647 >>>>>>> >>>>>>> That is -1, which indicates some module refcount woes. >>>>>> >>>>>> yes, ofc. >>>>>> >>>>>> The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1. >>>>>> >>>>>>> I definitely can't see that with the mainline on my machines. >>>>>> >>>>>> It is in mainline too. >>>>> >>>>> Does the appended patch help? >>>> >>>> Actually, something as simple as this also should help: >>>> >>>> --- >>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> >>>> Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume >>>> >>>> Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the >>>> driver module refcount, __cpufreq_remove_dev() causes that refcount >>>> to become negative after a suspend/resume cycle, for example. >>>> >>>> To prevent this from happening make __cpufreq_remove_dev() put >>>> the policy kobject only instead of calling cpufreq_cpu_put(). >>> >>> Having a deeper look at it, though, I see that in fact the whole >>> cpufreq_cpu_put() is needed if the CPU is not the last one for the given >>> policy and is not needed at all otherwise (as described in the changelog >>> of the patch below). >>> >>> Srivatsa, does this make sense to you? >>> >> >> Code-wise, your patch looks good to me. But one thing in the existing code >> struck me as a little strange. >> >> I'm assuming that the module_get() is used in the cpufreq core to ensure that >> until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend >> driver module (eg: acpi-cpufreq) can't be removed. > > Quite frankly, I'm not sure about that. If that were the case, > cpufreq_add_dev() would not call module_put() which it does. > > That may be a bug, I agree, but that's not for the present release cycle. For > now, we need to ensure that the reference counts are *balanced* . > Sure, in that case, I agree that your patch is the right fix at this point, since it resolves the immediate problem that we have with the refcounts. Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> Regards, Srivatsa S. Bhat -- 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 Monday, July 29, 2013 03:11:45 PM Srivatsa S. Bhat wrote: > On 07/29/2013 04:50 AM, Rafael J. Wysocki wrote: > > On Monday, July 29, 2013 12:43:59 AM Rafael J. Wysocki wrote: > >> On Monday, July 29, 2013 12:11:18 AM Rafael J. Wysocki wrote: > >>> On Sunday, July 28, 2013 12:21:22 PM Toralf Förster wrote: > >>>> On 07/28/2013 01:39 AM, Rafael J. Wysocki wrote: > >>>>> On Saturday, July 27, 2013 07:40:34 PM Toralf Förster wrote: > >>>>>> it gives at a ThinkPad T420: > >>>>>> > >>>>>> tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq > >>>>>> acpi_cpufreq 12902 2147483647 > >>>>> > >>>>> That is -1, which indicates some module refcount woes. > >>>> > >>>> yes, ofc. > >>>> > >>>> The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1. > >>>> > >>>>> I definitely can't see that with the mainline on my machines. > >>>> > >>>> It is in mainline too. > >>> > >>> Does the appended patch help? > >> > >> Actually, something as simple as this also should help: > >> > >> --- > >> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >> Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume > >> > >> Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the > >> driver module refcount, __cpufreq_remove_dev() causes that refcount > >> to become negative after a suspend/resume cycle, for example. > >> > >> To prevent this from happening make __cpufreq_remove_dev() put > >> the policy kobject only instead of calling cpufreq_cpu_put(). > > > > Having a deeper look at it, though, I see that in fact the whole > > cpufreq_cpu_put() is needed if the CPU is not the last one for the given > > policy and is not needed at all otherwise (as described in the changelog > > of the patch below). > > > > Srivatsa, does this make sense to you? > > > > Code-wise, your patch looks good to me. But one thing in the existing code > struck me as a little strange. > > I'm assuming that the module_get() is used in the cpufreq core to ensure that > until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend > driver module (eg: acpi-cpufreq) can't be removed. Quite frankly, I'm not sure about that. If that were the case, cpufreq_add_dev() would not call module_put() which it does. That may be a bug, I agree, but that's not for the present release cycle. For now, we need to ensure that the reference counts are *balanced* . Thanks, Rafael
On Monday, July 29, 2013 05:14:25 PM Srivatsa S. Bhat wrote: > On 07/29/2013 05:19 PM, Rafael J. Wysocki wrote: > > On Monday, July 29, 2013 03:11:45 PM Srivatsa S. Bhat wrote: > >> On 07/29/2013 04:50 AM, Rafael J. Wysocki wrote: > >>> On Monday, July 29, 2013 12:43:59 AM Rafael J. Wysocki wrote: > >>>> On Monday, July 29, 2013 12:11:18 AM Rafael J. Wysocki wrote: > >>>>> On Sunday, July 28, 2013 12:21:22 PM Toralf Förster wrote: > >>>>>> On 07/28/2013 01:39 AM, Rafael J. Wysocki wrote: > >>>>>>> On Saturday, July 27, 2013 07:40:34 PM Toralf Förster wrote: > >>>>>>>> it gives at a ThinkPad T420: > >>>>>>>> > >>>>>>>> tfoerste@n22 ~/tmp $ lsmod | grep ^acpi_cpufreq > >>>>>>>> acpi_cpufreq 12902 2147483647 > >>>>>>> > >>>>>>> That is -1, which indicates some module refcount woes. > >>>>>> > >>>>>> yes, ofc. > >>>>>> > >>>>>> The issue apears after 1 s2ram/resume cycle, before s2ram the refcount is 1. > >>>>>> > >>>>>>> I definitely can't see that with the mainline on my machines. > >>>>>> > >>>>>> It is in mainline too. > >>>>> > >>>>> Does the appended patch help? > >>>> > >>>> Actually, something as simple as this also should help: > >>>> > >>>> --- > >>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > >>>> Subject: cpufreq: Fix cpufreq driver module refcount balance after suspend/resume > >>>> > >>>> Since cpufreq_cpu_put() called by __cpufreq_remove_dev() drops the > >>>> driver module refcount, __cpufreq_remove_dev() causes that refcount > >>>> to become negative after a suspend/resume cycle, for example. > >>>> > >>>> To prevent this from happening make __cpufreq_remove_dev() put > >>>> the policy kobject only instead of calling cpufreq_cpu_put(). > >>> > >>> Having a deeper look at it, though, I see that in fact the whole > >>> cpufreq_cpu_put() is needed if the CPU is not the last one for the given > >>> policy and is not needed at all otherwise (as described in the changelog > >>> of the patch below). > >>> > >>> Srivatsa, does this make sense to you? > >>> > >> > >> Code-wise, your patch looks good to me. But one thing in the existing code > >> struck me as a little strange. > >> > >> I'm assuming that the module_get() is used in the cpufreq core to ensure that > >> until the cpufreq core is managing (atleast one) CPU(s), the cpufreq backend > >> driver module (eg: acpi-cpufreq) can't be removed. > > > > Quite frankly, I'm not sure about that. If that were the case, > > cpufreq_add_dev() would not call module_put() which it does. > > > > That may be a bug, I agree, but that's not for the present release cycle. For > > now, we need to ensure that the reference counts are *balanced* . > > > > Sure, in that case, I agree that your patch is the right fix at this point, > since it resolves the immediate problem that we have with the refcounts. > > Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com> Great, thanks! Does your patchset avoiding the creation/removal of sysfs directories over suspend/resume need to be modified to take this patch into account? Rafael
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index a4ad733..ecfbc52 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -878,9 +878,14 @@ static int cpufreq_add_dev_interface(unsigned int cpu, } write_unlock_irqrestore(&cpufreq_driver_lock, flags); + /* Bump up the refcount for this CPU */ + policy = cpufreq_cpu_get(cpu); + ret = cpufreq_add_dev_symlink(cpu, policy); - if (ret) + if (ret) { + cpufreq_cpu_put(policy); goto err_out_kobj_put; + } memcpy(&new_policy, policy, sizeof(struct cpufreq_policy)); /* assure that the starting sequence is run in __cpufreq_set_policy */