Date: Thu, 22 Mar 2018 17:39:14 +1100 (EST) From: Bruce Evans <brde@optusnet.com.au> To: Warner Losh <imp@bsdimp.com> Cc: Warner Losh <imp@freebsd.org>, src-committers <src-committers@freebsd.org>, svn-src-all@freebsd.org, svn-src-head@freebsd.org Subject: Re: svn commit: r331298 - head/sys/dev/syscons Message-ID: <20180322170345.J1053@besplex.bde.org> In-Reply-To: <CANCZdfoUCimTKFQJgBzZAEva4=vfF69s=nE36OvDEQwDxMmE7A@mail.gmail.com> References: <201803211447.w2LElDcK091988@repo.freebsd.org> <20180322024846.S4293@besplex.bde.org> <CANCZdfoUCimTKFQJgBzZAEva4=vfF69s=nE36OvDEQwDxMmE7A@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Wed, 21 Mar 2018, Warner Losh wrote:
> On Wed, Mar 21, 2018 at 11:53 AM, Bruce Evans <brde@optusnet.com.au> wrote:
>
>> On Wed, 21 Mar 2018, Warner Losh wrote:
>>
>> Log:
>>> Unlock giant when calling shutdown_nice()
>>
>> This breaks the driver. Giant is syscons' driver lock, and also the
>> interrupt handler lock for at least the atkbd keyboard driver, so vt
>> sometimes holds the lock for.
>
> OK. I got carried away. You're right. The proper fix is to unlock Giant at
> the top of kern_reboot() instead. This handles the case where we call
> shutdown_nice() with no init running and have to call kern_reboot directly.
> Otherwise it's perfectly fine to just call shutdown_nice() with Giant held
> since we just signal init from there. So I'll revert this change and make
> that other change instead.
> ...
> Good point. I think the following change is good for everything except
> calling shutdown_nice() from a fast interrupt handler with noinit running:
>
> diff --git a/sys/kern/kern_shutdown.c b/sys/kern/kern_shutdown.c
> index e5ea9644ad3f..564aecd811be 100644
> --- a/sys/kern/kern_shutdown.c
> +++ b/sys/kern/kern_shutdown.c
> @@ -366,6 +366,12 @@ kern_reboot(int howto)
> {
> static int once = 0;
>
> + /*
> + * Drop Giant once and for all.
> + */
> + while (mtx_owned(&Giant))
> + mtx_unlock(&Giant);
> +
> #if defined(SMP)
> /*
> * Bind us to the first CPU so that all shutdown code runs there.
> Some
>
> Comments?
Try putting this in vfs_mountroot_parse() and/or the second clause of
shutdown_nice() only. The only other calls are from is from sys_reboot()
and vpanic(). sys_reboot() is either MPSAFE or not, and it should know
when it is safe to drop its Giant lock if at all. vpanic() sets
SCHEDULER_STOPPED() to avoid seeing problems with Giant or any other
mutex.
Bruce
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20180322170345.J1053>