From owner-freebsd-current@FreeBSD.ORG Mon Oct 31 23:43:05 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 7BDEF106564A for ; Mon, 31 Oct 2011 23:43:05 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-ww0-f50.google.com (mail-ww0-f50.google.com [74.125.82.50]) by mx1.freebsd.org (Postfix) with ESMTP id AE3798FC0C for ; Mon, 31 Oct 2011 23:43:04 +0000 (UTC) Received: by wwp14 with SMTP id 14so1445878wwp.31 for ; Mon, 31 Oct 2011 16:43:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=gamma; h=mime-version:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type :content-transfer-encoding; bh=w5SRXRbXpZY4RgvyhKLVwsUhgLA2JqvbDqciSfmM2hE=; b=paUy7jE9g7cR7XGyGrbTwRwy8iQqixEKZkZfu/CCIigTfRsAfdQ6Wkjauc78jRgfEf QDdCv2VBlf4hLlXkm1uRfFXH4O+o0TauSzlETkAzRxj8dgVNQXWhuxRRfmvIkinS+wWc 9AEBDyhPwY8SSDm+fJTbbqWEPrxXBJtD+wLz8= MIME-Version: 1.0 Received: by 10.216.134.93 with SMTP id r71mr5168855wei.59.1320104583486; Mon, 31 Oct 2011 16:43:03 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.216.182.3 with HTTP; Mon, 31 Oct 2011 16:43:03 -0700 (PDT) In-Reply-To: References: Date: Tue, 1 Nov 2011 00:43:03 +0100 X-Google-Sender-Auth: Pqa4TeibKRX5JJSKYPkBRfS-Hxo Message-ID: From: Attilio Rao To: mdf@freebsd.org Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable Cc: FreeBSD Current , Ryan Stone , mlaier@freebsd.org 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: Mon, 31 Oct 2011 23:43:05 -0000 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. =C2=A0It turns out th= at >> the root cause is a bug in smp_rendezvous_cpus. =C2=A0The 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): >> >> =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!smp_started) { >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (setup_func != =3D NULL) >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0setup_func(arg); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (action_func != =3D NULL) >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0action_func(arg); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (teardown_func= !=3D NULL) >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0teardown_func(arg); >> =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return; >> =C2=A0 =C2=A0 =C2=A0 =C2=A0} >> >> The problem is that this runs with interrupts enabled, outside of a >> critical section. =C2=A0My 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. =C2=A0That 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. =C2=A0Go 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. =C2=A0Is it as simpl= e >> as doing a spinlock_enter before setup_func and a spinlock_exit after >> teardown_func? =C2=A0It 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: > > =C2=A0struct mtx smp_ipi_mtx; > +MTX_SYSINIT(smp_ipi_mtx, &smp_ipi_mtx, "smp rendezvous", MTX_SPIN); > > ... > > =C2=A0static void > =C2=A0mp_start(void *dummy) > =C2=A0{ > > - =C2=A0 =C2=A0 =C2=A0 mtx_init(&smp_ipi_mtx, "smp rendezvous", NULL, MTX= _SPIN); > > ... > > =C2=A0 =C2=A0 =C2=A0 =C2=A0if (!smp_started) { > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mtx_lock_spin(&smp_ipi= _mtx); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (setup_func != =3D NULL) > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0setup_func(arg); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (action_func != =3D NULL) > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0action_func(arg); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0if (teardown_func = !=3D NULL) > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0teardown_func(arg); > + =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 mtx_unlock_spin(&smp_i= pi_mtx); > =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0return; > =C2=A0 =C2=A0 =C2=A0 =C2=A0} 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 =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D --- 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 =3D 0; + /* Look comments in the !SMP case. */ if (!smp_started) { + spinlock_enter(); if (setup_func !=3D NULL) setup_func(arg); if (action_func !=3D NULL) action_func(arg); if (teardown_func !=3D 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 conditi= ons + * as the SMP case. + */ + spinlock_enter(); if (setup_func !=3D NULL) setup_func(arg); if (action_func !=3D NULL) action_func(arg); if (teardown_func !=3D 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 !=3D NULL) setup_func(arg); if (action_func !=3D NULL) action_func(arg); if (teardown_func !=3D NULL) teardown_func(arg); + spinlock_exit(); } /* Thanks, Attilio --=20 Peace can only be achieved by understanding - A. Einstein