From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752774AbbJORGP (ORCPT ); Thu, 15 Oct 2015 13:06:15 -0400 Received: from mx1.redhat.com ([209.132.183.28]:54239 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751517AbbJORGO (ORCPT ); Thu, 15 Oct 2015 13:06:14 -0400 Date: Thu, 15 Oct 2015 19:02:47 +0200 From: Oleg Nesterov To: Peter Zijlstra Cc: heiko.carstens@de.ibm.com, Tejun Heo , Ingo Molnar , Rik van Riel , Thomas Gleixner , linux-kernel@vger.kernel.org Subject: Re: [PATCH 1/3] stop_machine: ensure that a queued callback will be called before cpu_stop_park() Message-ID: <20151015170247.GA26019@redhat.com> References: <20151008145059.GA17916@redhat.com> <20151008145131.GA18139@redhat.com> <20151014153432.GU3604@twins.programming.kicks-ass.net> <20151014190356.GA8905@redhat.com> <20151014203245.GW3604@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20151014203245.GW3604@twins.programming.kicks-ass.net> User-Agent: Mutt/1.5.18 (2008-05-17) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 10/14, Peter Zijlstra wrote: > > 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: > > > > > > > > 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(); Yes, so I added the comment to explain that this is fine. > 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. Yes. IOW, this relies on ->selfparking == T which implies "flush before park". But even if we change cpu_down() to avoid stop_machine() I think we need to keep this "selfparking" logic. In a sense that, for example, this code void func(int cpu, cpu_stop_fn_t fn) { get_online_cpus(); if (cpu_online(cpu) { int ret = stop_one_cpu(cpu, fn, NULL); BUG_ON(ret == -ENOENT); } put_online_cpus(); } should be correct. Actually this example is not very good, it would be better to use stop_one_cpu_nowait() but currently it returns "void" and hmm, it looks buggy ;) I'll send the fix on top of this series if you accept it. Oleg.