From owner-freebsd-current@FreeBSD.ORG Wed Nov 2 18:05:02 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 86457106566B; Wed, 2 Nov 2011 18:05:02 +0000 (UTC) (envelope-from jhb@freebsd.org) Received: from cyrus.watson.org (cyrus.watson.org [65.122.17.42]) by mx1.freebsd.org (Postfix) with ESMTP id 4418C8FC43; Wed, 2 Nov 2011 18:05:02 +0000 (UTC) Received: from bigwig.baldwin.cx (66.111.2.69.static.nyinternet.net [66.111.2.69]) by cyrus.watson.org (Postfix) with ESMTPSA id E7F6346B0A; Wed, 2 Nov 2011 14:05:01 -0400 (EDT) Received: from jhbbsd.localnet (unknown [209.249.190.124]) by bigwig.baldwin.cx (Postfix) with ESMTPSA id 5086A8A02F; Wed, 2 Nov 2011 14:05:01 -0400 (EDT) From: John Baldwin To: freebsd-current@freebsd.org Date: Wed, 2 Nov 2011 11:45:02 -0400 User-Agent: KMail/1.13.5 (FreeBSD/8.2-CBSD-20110714-p8; KDE/4.5.5; amd64; ; ) References: In-Reply-To: MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201111021145.02622.jhb@freebsd.org> X-Greylist: Sender succeeded SMTP AUTH, not delayed by milter-greylist-4.2.6 (bigwig.baldwin.cx); Wed, 02 Nov 2011 14:05:01 -0400 (EDT) Cc: Attilio Rao , mdf@freebsd.org, mlaier@freebsd.org, Ryan Stone Subject: Re: smp_rendezvous runs with interrupts and preemption enabled on unicore systems 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: Wed, 02 Nov 2011 18:05:02 -0000 On Monday, October 31, 2011 7:43:03 pm Attilio Rao wrote: > 2011/10/28 : > > On Fri, Oct 28, 2011 at 8:37 AM, Ryan Stone wrote: > >> I'm seeing issues on a unicore systems running a derivative of FreeBSD > >> 8.2-RELEASE if something calls mem_range_attr_set. It turns out that > >> the root cause is a bug in smp_rendezvous_cpus. The first part of > >> smp_rendezvous_cpus attempts to short-circuit the non-SMP case(note > >> that smp_started is never set to 1 on a unicore system): > >> > >> if (!smp_started) { > >> if (setup_func != NULL) > >> setup_func(arg); > >> if (action_func != NULL) > >> action_func(arg); > >> if (teardown_func != NULL) > >> teardown_func(arg); > >> return; > >> } > >> > >> The problem is that this runs with interrupts enabled, outside of a > >> critical section. My system runs with device_polling enabled with hz > >> set to 2500, so its quite easy to wedge the system by having a thread > >> run mem_range_attr_set. That has to do a smp_rendezvous, and if a > >> timer interrupt happens to go off half-way through the action_func and > >> preempt this thread, the system ends up deadlocked(although once it's > >> wedged, typing at the serial console stands a good chance of unwedging > >> the system. Go figure). > > I'm not entirely sure why this exactly breaks though (do you see that > happening with a random rendezvous callback or it is always the > same?), because that just becames a simple function calling on cpu0, > even if I think that there is still a bug as smp_rendezvous callbacks > may expect to have interrupts and preemption disabled and the > short-circuit breaks that assumption. > > >> I know that smp_rendezvous was reworked substantially on HEAD, but by > >> inspection it looks like the bug is still present, as the > >> short-circuit behaviour is still there. > >> > >> I am not entirely sure of the best way to fix this. Is it as simple > >> as doing a spinlock_enter before setup_func and a spinlock_exit after > >> teardown_func? It seems to boot fine, but I'm not at all confident > >> that I understand the nuances of smp_rendezvous to be sure that there > >> aren't side effects that I don't know about. > > > > Looks like Max didn't have time to upstream this fix: > > > > struct mtx smp_ipi_mtx; > > +MTX_SYSINIT(smp_ipi_mtx, &smp_ipi_mtx, "smp rendezvous", MTX_SPIN); > > > > ... > > > > static void > > mp_start(void *dummy) > > { > > > > - mtx_init(&smp_ipi_mtx, "smp rendezvous", NULL, MTX_SPIN); > > > > ... > > > > if (!smp_started) { > > + mtx_lock_spin(&smp_ipi_mtx); > > if (setup_func != NULL) > > setup_func(arg); > > if (action_func != NULL) > > action_func(arg); > > if (teardown_func != NULL) > > teardown_func(arg); > > + mtx_unlock_spin(&smp_ipi_mtx); > > return; > > } > > I don't think that we strictly need the lock here, my preferred > solution would be (only test-compiled): > Index: sys/kern/subr_smp.c > =================================================================== > --- sys/kern/subr_smp.c (revision 226972) > +++ sys/kern/subr_smp.c (working copy) > @@ -415,13 +415,16 @@ smp_rendezvous_cpus(cpuset_t map, > { > int curcpumap, i, ncpus = 0; > > + /* Look comments in the !SMP case. */ > if (!smp_started) { > + spinlock_enter(); > if (setup_func != NULL) > setup_func(arg); > if (action_func != NULL) > action_func(arg); > if (teardown_func != NULL) > teardown_func(arg); > + spinlock_exit(); > return; > } > > @@ -666,12 +669,18 @@ smp_rendezvous_cpus(cpuset_t map, > void (*teardown_func)(void *), > void *arg) > { > + /* > + * In the !SMP case we just need to ensure the same initial conditions > + * as the SMP case. > + */ > + spinlock_enter(); > if (setup_func != NULL) > setup_func(arg); > if (action_func != NULL) > action_func(arg); > if (teardown_func != NULL) > teardown_func(arg); > + spinlock_exit(); > } > > void > @@ -681,12 +690,15 @@ smp_rendezvous(void (*setup_func)(void *), > void *arg) > { > > + /* Look comments in the smp_rendezvous_cpus() case. */ > + spinlock_enter(); > if (setup_func != NULL) > setup_func(arg); > if (action_func != NULL) > action_func(arg); > if (teardown_func != NULL) > teardown_func(arg); > + spinlock_exit(); > } I think this is fine, and probably correct. -- John Baldwin