On Wed, Oct 14, 2015 at 09:03:56PM +0200, Oleg Nesterov wrote: > On 10/14, Peter Zijlstra wrote: > > > > On Thu, Oct 08, 2015 at 04:51:31PM +0200, Oleg Nesterov wrote: > > > cpu_stop_queue_work() checks stopper->enabled before it queues the > > > work, but ->enabled == T can only guarantee cpu_stop_signal_done() > > > if we race with cpu_down(). > > > > > > This is not enough for stop_two_cpus() or stop_machine(), they will > > > deadlock if multi_cpu_stop() won't be called by one of the target > > > CPU's. stop_machine/stop_cpus are fine, they rely on stop_cpus_mutex. > > > But stop_two_cpus() has to check cpu_active() to avoid the same race > > > with hotplug, and this check is very unobvious and probably not even > > > correct if we race with cpu_up(). > > > > > > Change cpu_down() pass to clear ->enabled before cpu_stopper_thread() > > > flushes the pending ->works and returns with KTHREAD_SHOULD_PARK set. > > > > > > Note also that smpboot_thread_call() calls cpu_stop_unpark() which > > > sets enabled == T at CPU_ONLINE stage, so this CPU can't go away until > > > cpu_stopper_thread() is called at least once. This all means that if > > > cpu_stop_queue_work() succeeds, we know that work->fn() will be called. > > > > This hard relies on the fact that cpu_down uses stop machine, right? > > Not really. > > > IIRC part of the hotplug rework Thomas is doing is geared towards > > breaking away from stop machine. There is nothing fundamental about > > hot-unplug that requires stop machine. > > cpu_down() should park/kill/whatever the percpu stopper thread anyway. > And this path should clear ->enabled, it can also flush the pending > works.
So the proposed patch does: ->enabled=false; park();, which can race with if (->enabled) wake(); smpboot_thread_fn() will not call ->thread_fn() when should_park(), and thus any pending work will not get flushed. It only works now because the stopper task calls park(), which means cpu_stopper_thread() will flush, but that very much relies on the stopper thread calling park in itself. Or I'm just terminally confused.. which is entirely possible. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
