From owner-svn-src-all@freebsd.org Thu Mar 22 14:57:49 2018 Return-Path: Delivered-To: svn-src-all@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 570E9F4B393 for ; Thu, 22 Mar 2018 14:57:49 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-io0-x22f.google.com (mail-io0-x22f.google.com [IPv6:2607:f8b0:4001:c06::22f]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (Client CN "smtp.gmail.com", Issuer "Google Internet Authority G2" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id D736C78FCA for ; Thu, 22 Mar 2018 14:57:48 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: by mail-io0-x22f.google.com with SMTP id q80so414823ioi.13 for ; Thu, 22 Mar 2018 07:57:48 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20150623.gappssmtp.com; s=20150623; h=mime-version:sender:in-reply-to:references:from:date:message-id :subject:to:cc; bh=IKfpoCv2+ZMQrSqOcjqNT/2jCK965pV2H0xxJD0A1P0=; b=iS6brQNsL9aNdBH7JwAZPR4uMHZHCR3HL3yeMVLTTe7x+1GS6wyQoJbjv2ZN8z4Oc5 3L2IODiWg/0GQUH6wX5D0E4VLgeyX7inn8yV7OoMan6e4L199vjhW9XScFXZ12tVcdF5 DnWrsFonARfUjalYUNeEwjyAohdvOy6D59BcOA6KV98XwiIklWZjb/zvDwKTUbR4w4v6 f56kW08DzjV/EPVsbzAxyviQkvwk+FzFVNI6FylhQECmSDbmOG++YVLUcFAIlfKCkJIm SkR1IKC0vBgAVnrZUEALnl6pz/sACdHyCN8QN170hGxaOJZtBVaNU3G0F1A3lq1q/M7B azXA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:sender:in-reply-to:references:from :date:message-id:subject:to:cc; bh=IKfpoCv2+ZMQrSqOcjqNT/2jCK965pV2H0xxJD0A1P0=; b=Mza+pqchkVlqNlFLrHeOPg1zCK4psx3/hiZjGM7BtwdPiMG7oky2Nx/0bsyktPQTLk r8f/rrMvWnUtBX92XHabCQKY49Ee8dS4D8bEgnLvKKIFWTWZGQ5evXnpv8nCRwYa6tOm jvSwynNWOF5ZE9DNZxonGPSOXUKMddFyl4eXR4fQoBqJgRoCFWgILnZTqL+o/GKultYf MFHr6HRJBDguZVDcKvC34fXPrszMy35RgRJ/BHuf7dtO7fqOfGPjQ42yj/fQjGjzoIuX 7XRldNw05LtQD42dKOUya+1YBfKrlyLeXwNWBpE0SoB/ij8UJTSmq1OlwSO5fPJh8ZRT 0qpA== X-Gm-Message-State: AElRT7GYjPQWi0+7KSFBNjaN71s2Mkq9Gzwdh0UpPxsCOD9tIGzZnChV 2qGBdhKy2PpvuuI4nvAfc/8uIzUdnpaVIhjmkMRpCA== X-Google-Smtp-Source: AG47ELt61niFwEgc4gmfgi3ChVdX/PICs4qhLedsvgxG9W4r+CZUVFMyzkesp29RFVLz+EQTY46wFciHnxD+Cui2UqA= X-Received: by 10.107.12.230 with SMTP id 99mr25324573iom.117.1521730667830; Thu, 22 Mar 2018 07:57:47 -0700 (PDT) MIME-Version: 1.0 Sender: wlosh@bsdimp.com Received: by 10.79.203.196 with HTTP; Thu, 22 Mar 2018 07:57:47 -0700 (PDT) X-Originating-IP: [2603:300b:6:5100:18a2:a4f7:170:8dd9] In-Reply-To: <20180322114213.GR76926@kib.kiev.ua> References: <201803211447.w2LElDcK091988@repo.freebsd.org> <20180322024846.S4293@besplex.bde.org> <20180321202752.GO76926@kib.kiev.ua> <20180322174025.Q1053@besplex.bde.org> <20180322114213.GR76926@kib.kiev.ua> From: Warner Losh Date: Thu, 22 Mar 2018 08:57:47 -0600 X-Google-Sender-Auth: vOax_in7ZnGHFQzXf8DMpL9pVoM Message-ID: Subject: Re: svn commit: r331298 - head/sys/dev/syscons To: Konstantin Belousov Cc: Bruce Evans , Warner Losh , src-committers , svn-src-all@freebsd.org, svn-src-head@freebsd.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.25 X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.25 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 22 Mar 2018 14:57:49 -0000 On Thu, Mar 22, 2018 at 5:42 AM, Konstantin Belousov wrote: > On Thu, Mar 22, 2018 at 05:50:57PM +1100, Bruce Evans wrote: > > On Wed, 21 Mar 2018, Warner Losh wrote: > > > > > On Wed, Mar 21, 2018 at 2:27 PM, Konstantin Belousov < > kostikbel@gmail.com> > > > wrote: > > >> ... > > >> Are you saying that fast interrupt handlers call shutdown_nice() ? > This > > >> is the quite serious bug on its own. To fix it, shutdown_nice() > should > > >> use a fast taskqueue to schedule the task which would lock the process > > >> and send the signal. > > > > > > Is there some way we know we're in a fast interrupt handler? If so, it > > > should be simple to fix. If not, then there's an API change ahead of > us... > > > > There is a td_intr_nesting_level flag that might work. (I invented this, > > but don't like its current use.) > But why do we need to know this ? We can always schedule a task in the > fast taskqueue if init is present and can be scheduled. Ah, good point. Seems like a poster child example of doing a fast task queue deferment. That's always safe, except maybe in BDE's fast interrupt world. > > But bde is right: the system has to be in good enough shape to cope. I > > > wonder if we should put that coping into kdb_reboot() instead. It's > only an > > > issue for TILDE ^R, which is a fairly edge case. > > > > shutdown_nice() is also called directly from syscons and vt for the > reboot, > > halt and poweroff keys. This happens in normal interrupt handler > context, > > so there is only a problem when init is not running. > > diff --git a/sys/kern/kern_shutdown.c b/sys/kern/kern_shutdown.c > index e5ea9644ad3..e7c6d4c92b2 100644 > --- a/sys/kern/kern_shutdown.c > +++ b/sys/kern/kern_shutdown.c > @@ -72,6 +72,7 @@ __FBSDID("$FreeBSD$"); > #include > #include > #include > +#include > #include > #include > > @@ -276,6 +277,28 @@ sys_reboot(struct thread *td, struct reboot_args *uap) > return (error); > } > > +static void > +shutdown_nice_task_fn(void *arg, int pending __unused) > +{ > + int howto; > + > + howto = (uintptr_t)arg; > + /* Send a signal to init(8) and have it shutdown the world. */ > + PROC_LOCK(initproc); > + if (howto & RB_POWEROFF) > + kern_psignal(initproc, SIGUSR2); > + else if (howto & RB_POWERCYCLE) > + kern_psignal(initproc, SIGWINCH); > + else if (howto & RB_HALT) > + kern_psignal(initproc, SIGUSR1); > + else > + kern_psignal(initproc, SIGINT); > + PROC_UNLOCK(initproc); > +} > + > +static struct task shutdown_nice_task = TASK_INITIALIZER(0, > + &shutdown_nice_task_fn, NULL); > + > /* > * Called by events that want to shut down.. e.g on a PC > */ > @@ -283,20 +306,14 @@ void > shutdown_nice(int howto) > { > > - if (initproc != NULL) { > - /* Send a signal to init(8) and have it shutdown the > world. */ > - PROC_LOCK(initproc); > - if (howto & RB_POWEROFF) > - kern_psignal(initproc, SIGUSR2); > - else if (howto & RB_POWERCYCLE) > - kern_psignal(initproc, SIGWINCH); > - else if (howto & RB_HALT) > - kern_psignal(initproc, SIGUSR1); > - else > - kern_psignal(initproc, SIGINT); > - PROC_UNLOCK(initproc); > + if (initproc != NULL && !SCHEDULER_STOPPED()) { > + shutdown_nice_task.ta_context = (void *)(uintptr_t)howto; > + taskqueue_enqueue(taskqueue_fast, &shutdown_nice_task); > } else { > - /* No init(8) running, so simply reboot. */ > + /* > + * No init(8) running, or scheduler would not allow it > + * to run, so simply reboot. > + */ > kern_reboot(howto | RB_NOSYNC); > } > } > I like this elegance. I think we should commit this. While it would be nice to support the super-fast interrupts that bde has done, we already don't support it in a number of places. Conceptually, I like the notion of the tilde escape machine in the tty layer, but I don't like the trade offs. Here we only defer to reboot, which requires a fair amount of machinery working to work right. it's not the dependencies I'd prefer, but usually it doesn't matter. However, to do that, it would also mean that breaking to the debugger would need to be done in the tty layer, which requires more of the context switching mechanisms to be working, which would be a bigger loss. So this is a good trade-off. If kern_reboot() can't be called from a fast interrupt handler in bdeBSD, it would be a simple matter to put a wrapper around it here so his kernel can context switch. Though, if the scheduler isn't running, I'm not sure what you can do in this situation. The system is already hung or in a really bad way, so it would have to jump to somewhere late in the kern_reboot() code, though I'm not sure. It's kinda hard to know what's safe just from descriptions from bde. We're already not synching the data to the disks, so I'm not sure what more can be done. I think we should commit this change. It makes things better, but Warner