From owner-freebsd-current@FreeBSD.ORG Tue May 17 08:03:41 2011 Return-Path: Delivered-To: freebsd-current@FreeBSD.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id F273A1065674; Tue, 17 May 2011 08:03:40 +0000 (UTC) (envelope-from avg@FreeBSD.org) Received: from citadel.icyb.net.ua (citadel.icyb.net.ua [212.40.38.140]) by mx1.freebsd.org (Postfix) with ESMTP id 7B7018FC13; Tue, 17 May 2011 08:03:39 +0000 (UTC) Received: from porto.topspin.kiev.ua (porto-e.starpoint.kiev.ua [212.40.38.100]) by citadel.icyb.net.ua (8.8.8p3/ICyb-2.3exp) with ESMTP id LAA18348; Tue, 17 May 2011 11:03:38 +0300 (EEST) (envelope-from avg@FreeBSD.org) Received: from localhost.topspin.kiev.ua ([127.0.0.1]) by porto.topspin.kiev.ua with esmtp (Exim 4.34 (FreeBSD)) id 1QMFFh-00070m-PJ; Tue, 17 May 2011 11:03:37 +0300 Message-ID: <4DD22BD9.6070504@FreeBSD.org> Date: Tue, 17 May 2011 11:03:37 +0300 From: Andriy Gapon User-Agent: Mozilla/5.0 (X11; U; FreeBSD amd64; en-US; rv:1.9.2.17) Gecko/20110503 Lightning/1.0b2 Thunderbird/3.1.10 MIME-Version: 1.0 To: John Baldwin References: <4DCD357D.6000109@FreeBSD.org> <201105161421.27665.jhb@freebsd.org> <4DD17AB3.1070606@FreeBSD.org> <201105161609.21898.jhb@freebsd.org> In-Reply-To: <201105161609.21898.jhb@freebsd.org> X-Enigmail-Version: 1.1.2 Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: 7bit Cc: Max Laier , FreeBSD current , neel@FreeBSD.org, Peter Grehan Subject: Re: proposed smp_rendezvous change X-BeenThere: freebsd-current@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: Discussions about the use of FreeBSD-current List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 17 May 2011 08:03:41 -0000 on 16/05/2011 23:09 John Baldwin said the following: > On Monday, May 16, 2011 3:27:47 pm Andriy Gapon wrote: >> on 16/05/2011 21:21 John Baldwin said the following: >>> How about this: >> ... >>> /* >>> * Shared mutex to restrict busywaits between smp_rendezvous() and >>> @@ -311,39 +312,62 @@ restart_cpus(cpumask_t map) >>> void >>> smp_rendezvous_action(void) >>> { >>> - void* local_func_arg = smp_rv_func_arg; >>> - void (*local_setup_func)(void*) = smp_rv_setup_func; >>> - void (*local_action_func)(void*) = smp_rv_action_func; >>> - void (*local_teardown_func)(void*) = smp_rv_teardown_func; >>> + void *local_func_arg; >>> + void (*local_setup_func)(void*); >>> + void (*local_action_func)(void*); >>> + void (*local_teardown_func)(void*); >>> + int generation; >>> >>> /* Ensure we have up-to-date values. */ >>> atomic_add_acq_int(&smp_rv_waiters[0], 1); >>> while (smp_rv_waiters[0] < smp_rv_ncpus) >>> cpu_spinwait(); >>> >>> - /* setup function */ >>> + /* Fetch rendezvous parameters after acquire barrier. */ >>> + local_func_arg = smp_rv_func_arg; >>> + local_setup_func = smp_rv_setup_func; >>> + local_action_func = smp_rv_action_func; >>> + local_teardown_func = smp_rv_teardown_func; >> >> I want to ask once again - please pretty please convince me that the above >> cpu_spinwait() loop is really needed and, by extension, that the assignments >> should be moved behind it. >> Please :) > > Well, moving the assignments down is a style fix for one, and we can always > remove the first rendezvous as a follow up if desired. OK, makes sense. > However, suppose you have an arch where sending an IPI is not a barrier > (this seems unlikely). In that case, the atomic_add_acq_int() will not > succeed (and return) until it has seen the earlier write by the CPU > initiating the rendezvous to clear smp_rv_waiters[0] to zero. The actual > spin on the smp_rv_waiters[] value is not strictly necessary however and On this below. > is probably just cut and pasted to match the other uses of values in > the smp_rv_waiters[] array. > > (atomic_add_acq_int() could spin on architectures where it is implemented > using compare-and-swap (e.g. sparc64) or locked-load conditional-store (e.g. > Alpha).) When you say "not strictly necessary", do you mean "not necessary"? If you do not mean that, then when could it be (non-strictly) necessary? :) Couldn't [Shouldn't] the whole: >>> /* Ensure we have up-to-date values. */ >>> atomic_add_acq_int(&smp_rv_waiters[0], 1); >>> while (smp_rv_waiters[0] < smp_rv_ncpus) >>> cpu_spinwait(); be just replaced with: rmb(); Or a proper MI function that does just a read memory barrier, if rmb() is not that. -- Andriy Gapon