From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752580AbbFZUrk (ORCPT ); Fri, 26 Jun 2015 16:47:40 -0400 Received: from mx1.redhat.com ([209.132.183.28]:36297 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752036AbbFZUrc (ORCPT ); Fri, 26 Jun 2015 16:47:32 -0400 Date: Fri, 26 Jun 2015 22:46:12 +0200 From: Oleg Nesterov To: Peter Zijlstra Cc: paulmck@linux.vnet.ibm.com, tj@kernel.org, mingo@redhat.com, der.herr@hofr.at, dave@stgolabs.net, riel@redhat.com, viro@ZenIV.linux.org.uk, torvalds@linux-foundation.org, linux-kernel@vger.kernel.org Subject: Re: [RFC PATCH 0/6] stop_machine: kill stop_cpus_mutex and stop_cpus_lock Message-ID: <20150626204612.GA14573@redhat.com> References: <20150626021455.GA5675@redhat.com> <20150626122330.GY19282@twins.programming.kicks-ass.net> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20150626122330.GY19282@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 06/26, Peter Zijlstra wrote: > > On Fri, Jun 26, 2015 at 04:14:55AM +0200, Oleg Nesterov wrote: > > Not sure. > > > > And note that this series kills stop_cpus_mutex, so that multiple > > stop_cpus()'s / stop_machine()'s can run in parallel if cpumask's > > do not overlap. > > > > Note also the changelog in 6/6, we can simplify + optimize this code > > a bit more. > > > > What do you think? > > The problem I have with this is that it makes the better operation > (stop_two_cpus) slower while improving the worse operation (stop_cpus). > > I would much prefer to keep stop_two_cpus() as proposed with taking two > cpu_stopper::lock instances and replacing the stop_cpu_mutex with a > percpu-rwsem. OK, lets avoid cpumask in stop_two_cpus, int stop_two_cpus(unsigned int cpu1, unsigned int cpu2, cpu_stop_fn_t fn, void *arg) { struct multi_stop_data msdata; struct cpu_stop_done done; struct cpu_stop_work *work1, *work2; msdata = (struct multi_stop_data){ .fn = fn, .data = arg, .num_threads = 2, .active_cpus = cpumask_of(cpu1), }; cpu_stop_init_done(&done, 2); set_state(&msdata, MULTI_STOP_PREPARE); if (cpu1 > cpu2) swap(cpu1, cpu2); work1 = stop_work_alloc_one(cpu1, true); work2 = stop_work_alloc_one(cpu1, true); *work1 = *work2 = (struct cpu_stop_work) { .fn = multi_cpu_stop, .arg = &msdata, .done = &done }; preempt_disable(); cpu_stop_queue_work(cpu1, work1); cpu_stop_queue_work(cpu2, work2); preempt_enable(); wait_for_completion(&done.completion); stop_work_free_one(cpu1); stop_work_free_one(cpu2); wake_up(&stop_work_wq); return done.executed ? done.ret : -ENOENT; } 2 cmpxchg()'s vs 2 spin_lock()'s. Plus wake_up(), but we can check waitqueue_active(). Do you think thi will be noticeably slower? Of course, if it races with another stop_two_cpus/stop_cpus it will sleep, but in this case we need to wait anyway. And I don't think that percpu-rwsem instead of stop_cpu_mutex makes sense. at least I don't understand how can it help. OK, stop_two_cpus() can use percpu_down_read() to avoid the deadlock with stop_cpus(), but you still need double-lock... So I don't think this will make it faster, this will just penalize stop_cpus(). Or I misunderstood. So I am still not convinced... But probably I am too biased ;) Btw. I can't understand the cpu_active() checks in stop_two_cpus(). Do we really need them? Oleg.