Date: Tue, 1 Nov 2011 00:43:03 +0100 From: Attilio Rao <attilio@freebsd.org> To: mdf@freebsd.org Cc: FreeBSD Current <freebsd-current@freebsd.org>, Ryan Stone <rysto32@gmail.com>, mlaier@freebsd.org Subject: Re: smp_rendezvous runs with interrupts and preemption enabled on unicore systems Message-ID: <CAJ-FndCoYfEGDj2xH=TiEatQ149-AdRON6Aoi_zLiVKbsqb_xA@mail.gmail.com> In-Reply-To: <CAMBSHm9fxL23Bydnw6_Fned1sNPCq1aYr7As4=a91vVKcW3u1w@mail.gmail.com> References: <CAFMmRNwWBE6idGJCsNPago4-N1ohAaCD02_D-LCMzDg8pbjE1A@mail.gmail.com> <CAMBSHm9fxL23Bydnw6_Fned1sNPCq1aYr7As4=a91vVKcW3u1w@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
2011/10/28 <mdf@freebsd.org>:
> On Fri, Oct 28, 2011 at 8:37 AM, Ryan Stone <rysto32@gmail.com> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndCoYfEGDj2xH=TiEatQ149-AdRON6Aoi_zLiVKbsqb_xA>