Messages in this thread |  | | Date | Thu, 06 Feb 2014 21:39:58 +0530 | From | "Srivatsa S. Bhat" <> | Subject | Re: [PATCH 00/51] CPU hotplug: Fix issues with callback registration |
| |
On 02/06/2014 05:44 PM, Gautham R Shenoy wrote: > On Thu, Feb 06, 2014 at 04:34:33PM +0530, Srivatsa S. Bhat wrote: >>> >>> CPU_POST_DEAD notification, is invoked with the cpu_hotplug.lock >>> dropped. This was necessary for subsystems which would be waiting for >>> some other thread to finish some work, and that other thread could >>> invoke get_online_cpus(). If CPU_POST_DEAD notification were issued >>> without dropping the cpu_hotplug.lock, this would lead to a deadlock >>> as the notifier would be left stuck waiting for the thread which is >>> blocked in get_online_cpus(). >>> >>> It was introduced to ensure that multithreaded workqueues can safely >>> use get_online_cpus() [https://lkml.org/lkml/2008/6/29/121]. >>> >>> As of now, only two subsystems use this notification and workqueues is >>> _not_ one of them! >>> * arch/x86/kernel/cpu/mcheck/mce.c:mce_cpu_callback() >>> * drivers/cpufreq/cpufreq.c:cpufreq_cpu_callback() >>> I haven't yet audited these two cases to see if they really need this >>> to be handled in CPU_POST_DEAD or if they can be handled in CPU_DEAD. >>> >> >> Well, cpufreq had a legitimate need to use POST_DEAD to avoid the >> deadlock described in commit 1aee40ac9c. However, there had been some >> discussion some time ago about reorganizing the cpufreq's hotplug callback >> so as to move most (but not all) of its work outside of POST_DEAD [1]. >> But as it stands, I don't think it would be easy to totally get rid of >> cpufreq's dependence on the POST_DEAD notifier. >> > > Right, I see the reason why cpufreq needs POST_DEAD. > >> Besides, I think its good to retain the POST_DEAD notifier option in >> the CPU hotplug core code. It has come handy several times to fix hard >> deadlock issues. >> > > I know. I am not denying the usefulness of POST_DEAD. But the fact > that some of the CPU_* notifiers are invoked with the cpu_hotplug.lock > held while CPU_POST_DEAD is invoked with the lock dropped looks a bit > asymmetrical. At the moment I cannot think of a simpler alternative. >
Hmmm...
> >>> Also can we have an alternate API, something like >>> cpu_hotplug_register_begin/end() instead of reusing >>> cpu_maps_update_begin/end() for this usage, since in most of the >>> patches that follow, we're not touching the any of the cpu_*_maps! >>> >> >> Yes, the function names cpu_maps_update_begin/end() don't really suit >> the kind of usage I'm proposing in this patchset, and hence is kind of >> a misnomer. For better readability, I'm thinking of defining a macro >> such as say, cpu_hotplug_notifier_lock()/unlock() that redirects to >> cpu_maps_update_begin/end() respectively. That way, we can export just >> those former symbols for use by modules, and thereby the code would look >> more intuitive, like this: >> >> cpu_hotplug_notifier_lock(); >> >> for_each_online_cpu(cpu) >> init_cpu(cpu); >> >> /* This doesn't take the cpu_add_remove_lock */ >> __register_cpu_notifier(&foobar_cpu_notifier); >> >> cpu_hotplug_notifier_unlock(); >> >> What do you think? > > Sounds good.
Cool! If there are no objections, I'll use this naming for the APIs and spin a v2 of the patchset soon.
Thank you!
Regards, Srivatsa S. Bhat
|  |