From nobody Tue Feb 6 22:14:24 2024 X-Original-To: dev-commits-src-all@mlmmj.nyi.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2610:1c1:1:606c::19:1]) by mlmmj.nyi.freebsd.org (Postfix) with ESMTP id 4TTyGQ6bFbz5B3vK for ; Tue, 6 Feb 2024 22:14:38 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Received: from mail-wr1-x430.google.com (mail-wr1-x430.google.com [IPv6:2a00:1450:4864:20::430]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits) key-exchange X25519 server-signature RSA-PSS (4096 bits) server-digest SHA256 client-signature RSA-PSS (2048 bits) client-digest SHA256) (Client CN "smtp.gmail.com", Issuer "GTS CA 1D4" (verified OK)) by mx1.freebsd.org (Postfix) with ESMTPS id 4TTyGQ3kMQz4ktn for ; Tue, 6 Feb 2024 22:14:38 +0000 (UTC) (envelope-from wlosh@bsdimp.com) Authentication-Results: mx1.freebsd.org; none Received: by mail-wr1-x430.google.com with SMTP id ffacd0b85a97d-3392b12dd21so19762f8f.0 for ; Tue, 06 Feb 2024 14:14:38 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=bsdimp-com.20230601.gappssmtp.com; s=20230601; t=1707257677; x=1707862477; darn=freebsd.org; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date:message-id:reply-to; bh=nNysDbpyDM+nc96i5XcJ52VgsUY2jNl5VYl5anvGsu4=; b=2Z4ZA4nb0Uhbe9PIKmIB6+RB767jq8yXcb7RXSB/ZGbepe5ryhpF5NkWCUPB7e9Knb T9/1uizvVc1xMmRZDevLeIMn8WHNGhovKUvVKkVkivMJHJV5eIIPNVnUUzc17z1C3sri FT8kzk15qZs4c3j1M2MSUs64h+Z7WHH+nUdheh+G5JYFfm2W2A0kS3d+xXs6mJuC3kj4 IiGuP4NhP9ohUyjQAFJWQAPcbJ3pBn6S0llApfJMk5YVA7d07KkEJDZYg6w5xl/2Bsa1 8hduitpSjOwTNC+5TMvg0ZACmffL2QSJJOuXaFnXROfdSACqvMnfzKIiO+wIb586497P FLSw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1707257677; x=1707862477; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=nNysDbpyDM+nc96i5XcJ52VgsUY2jNl5VYl5anvGsu4=; b=fIwJIaKiv8x9lsS+yeVLgLFaE8Y90+DjvoqrEdUkKEG5uhVDAOSW9iCidvmsS6chL8 07dN5q0NnERuQZlohTcQ+UWMvnxIQbYEcveRfAnRMqSoRzaErrgY3OdZbaMbXewuRGf2 GAiQ6K0Imjh5cTsZ4IgnrT1hCAiVrZ1SRfnbQtMwD0k6OP3j0PEvffYf4OIhJV1CyuAx Yd9MysD8dlKN8EYnKs2ms4GCDUGAQFt0fxP9rdtTZhCkg7+yYdL9KxIUqBdX9g42ko1K 4xcIyzWaCofgvxcqGEPdpSEbS9z8TnKM6NJ1vwHFXmkbntEs4XqvkstDue7k/02oBRzx vurA== X-Gm-Message-State: AOJu0YwhVuueeh/ZcJTf3h4hBH3bc5M+CuQDVpL5+OHdJghgIn4+fkbJ IFgwnDaokXTShHDGW6h+DEPzLvM9V5ItTCLhyAo2nam0Dtu9Akh2vGLMHZLdQvNXYC1jkkel+v4 m93IImzP4bcSbzAk/5YqLHVEXdPOc5j0skxYcVQVA0tUGNn52/NI= X-Google-Smtp-Source: AGHT+IHFDkL36qwDJliTtwaUXXj0kQEiX6nDhmMkPMHWRS43kxAtNqkAL9ZDUI7lhQVNuTUMRartNyfHxRMPdB8ldZ8= X-Received: by 2002:a5d:440b:0:b0:33a:eb5b:f8cd with SMTP id z11-20020a5d440b000000b0033aeb5bf8cdmr1785517wrq.7.1707257676689; Tue, 06 Feb 2024 14:14:36 -0800 (PST) List-Id: Commit messages for all branches of the src repository List-Archive: https://lists.freebsd.org/archives/dev-commits-src-all List-Help: List-Post: List-Subscribe: List-Unsubscribe: Sender: owner-dev-commits-src-all@freebsd.org X-BeenThere: dev-commits-src-all@freebsd.org MIME-Version: 1.0 References: <72def5a9-ffcc-4dcc-9b85-875ba7f46539@FreeBSD.org> In-Reply-To: <72def5a9-ffcc-4dcc-9b85-875ba7f46539@FreeBSD.org> From: Warner Losh Date: Tue, 6 Feb 2024 15:14:24 -0700 Message-ID: Subject: Re: git: e4ab361e5394 - main - fix poweroff regression from 9cdf326b4f by delaying shutdown_halt To: Andriy Gapon Cc: src-committers@freebsd.org, dev-commits-src-all@freebsd.org, dev-commits-src-main@freebsd.org Content-Type: multipart/alternative; boundary="0000000000005c13d20610bde77f" X-Rspamd-Queue-Id: 4TTyGQ3kMQz4ktn X-Spamd-Bar: ---- X-Rspamd-Pre-Result: action=no action; module=replies; Message is reply to one we originated X-Spamd-Result: default: False [-4.00 / 15.00]; REPLY(-4.00)[]; ASN(0.00)[asn:15169, ipnet:2a00:1450::/32, country:US] --0000000000005c13d20610bde77f Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable On Tue, Feb 6, 2024 at 3:13=E2=80=AFAM Andriy Gapon wrote= : > On 06/02/2024 11:41, Andriy Gapon wrote: > > The branch main has been updated by avg: > > > > URL: > https://cgit.FreeBSD.org/src/commit/?id=3De4ab361e53945a6c3e9d68c5e5ffc11= de40a35f2 > > > > commit e4ab361e53945a6c3e9d68c5e5ffc11de40a35f2 > > Author: Andriy Gapon > > AuthorDate: 2024-02-06 08:55:13 +0000 > > Commit: Andriy Gapon > > CommitDate: 2024-02-06 08:55:13 +0000 > > > > fix poweroff regression from 9cdf326b4f by delaying shutdown_halt > > > > The regression affected ACPI-based systems without EFI poweroff > support > > (including VMs). > > > > The key reason for the regression is that I overlooked that > poweroff is > > requested by RB_POWEROFF | RB_HALT combination of flags. In my > opinion, > > that command is a bit bipolar, but since we've been doing that > forever, > > then so be it. Because of that flag combination, the order of > > shutdown_final handlers that check for either flag does matter. > > > > Some additional complexity comes from platform-specific > shutdown_final > > handlers that aim to handle multiple reboot options at once. E.g.= , > > acpi_shutdown_final handles both poweroff and reboot / reset. As > > explained in 9cdf326b4f, such a handler must run after > shutdown_panic to > > give it a chance. But as the change revealed, the handler must > also run > > before shutdown_halt, so that the system can actually power off > before > > entering the halt limbo. > > > > Previously, shutdown_panic and shutdown_halt had the same priority > which > > appears to be incompatible with handlers that can do both poweroff > and > > reset. > > I want to add that having many handlers with priorities expressed like > SHUTDOWN_PRI_LAST =C2=B1 N while some of those handlers have implicit > inter-dependencies (interactions, interference) also does not help to see > a > clear picture. > > Perhaps it would be better to handle all (reasonable) RB flag combination= s > centrally in kern_reboot and then dispatch events like shutdown_reset, > shutdown_poweroff, etc. Handlers for those events would have a single an= d > simple job of performing that one action (perhaps failing and letting > another > handler try). > > Also, I would split reboot howto into command and flag portions, so that > only > one command can be specified at a time. E.g., I would consider > RB_AUTOBOOT > ("RB_REBOOT"), RB_POWEROFF, RB_HALT to be distinct commands. Then, flags > like > RB_NOSYNC or RB_DUMP could be optional flags. > Part of the problem is that RB_AUTOBOOT's value is 0. And we're using bits to describe what to do (was the fashion in the late 80s/90s, bio used to have its commands as bits, not a bit field). You also didn't include RB_POWERCYCLE which is a new bit in this list. It's a mess. As an aside, some flags documented for reboot(2) do not seem to have much > to do > with reboot. E.g., RB_DFLTROOT affects how a system boots up, but not ho= w > the > system goes for a reboot. Not surprisingly, that option is not handled b= y > anything kicked off with reboot(2). > Maybe, it would make more sense if we had fast reboot support and the > running > kernel could instruct the next kernel directly. But, it's still a bit > weird > that flags like RB_POWEROFF and RB_DFLTROOT belong in the same domain and > can be > set together. > More like 'support again' since this interface is from 4BSD and hasn't been updated in a very long time. It made sense when you could tell the VAX's firmware details about the next reboot, but we don't really have that short of implementing kexec... Though to fix it we should maybe just have a number of handlers that are called at each stage, and we deal with only one bit at a time (POWERCYCLE > POWEROFF > HALT) and your drivers register a separate one for each... It would be a bit more rework in the tree, and there'd be a few more functions called, but it would be a minimal change. But it kinda feels like we should just bite the bullet and have 3 handlers for these cases. One to power cycle, one to power off and one to halt. Then the drivers wouldn't care which ones have priority, they'd just check a bit and do what they are told (or maybe we say that they only run when the bit is set to make that code simpler). And if one bit of hardware can do all three, they'd have to implement 3 handlers... Tha ambiguity would be gone and the ordering wouldn't matter. Warner --0000000000005c13d20610bde77f Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable


=
On Tue, Feb 6, 2024 at 3:13=E2=80=AFA= M Andriy Gapon <avg@freebsd.org&g= t; wrote:
On 06/= 02/2024 11:41, Andriy Gapon wrote:
> The branch main has been updated by avg:
>
> URL: https://= cgit.FreeBSD.org/src/commit/?id=3De4ab361e53945a6c3e9d68c5e5ffc11de40a35f2<= /a>
>
> commit e4ab361e53945a6c3e9d68c5e5ffc11de40a35f2
> Author:=C2=A0 =C2=A0 =C2=A0Andriy Gapon <avg@FreeBSD.org>
> AuthorDate: 2024-02-06 08:55:13 +0000
> Commit:=C2=A0 =C2=A0 =C2=A0Andriy Gapon <avg@FreeBSD.org>
> CommitDate: 2024-02-06 08:55:13 +0000
>
>=C2=A0 =C2=A0 =C2=A0 fix poweroff regression from 9cdf326b4f by delayin= g shutdown_halt
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 The regression affected ACPI-based systems without= EFI poweroff support
>=C2=A0 =C2=A0 =C2=A0 (including VMs).
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 The key reason for the regression is that I overlo= oked that poweroff is
>=C2=A0 =C2=A0 =C2=A0 requested by RB_POWEROFF | RB_HALT combination of = flags.=C2=A0 In my opinion,
>=C2=A0 =C2=A0 =C2=A0 that command is a bit bipolar, but since we've= been doing that forever,
>=C2=A0 =C2=A0 =C2=A0 then so be it.=C2=A0 Because of that flag combinat= ion, the order of
>=C2=A0 =C2=A0 =C2=A0 shutdown_final handlers that check for either flag= does matter.
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 Some additional complexity comes from platform-spe= cific shutdown_final
>=C2=A0 =C2=A0 =C2=A0 handlers that aim to handle multiple reboot option= s at once.=C2=A0 E.g.,
>=C2=A0 =C2=A0 =C2=A0 acpi_shutdown_final handles both poweroff and rebo= ot / reset.=C2=A0 As
>=C2=A0 =C2=A0 =C2=A0 explained in 9cdf326b4f, such a handler must run a= fter shutdown_panic to
>=C2=A0 =C2=A0 =C2=A0 give it a chance.=C2=A0 But as the change revealed= , the handler must also run
>=C2=A0 =C2=A0 =C2=A0 before shutdown_halt, so that the system can actua= lly power off before
>=C2=A0 =C2=A0 =C2=A0 entering the halt limbo.
>=C2=A0 =C2=A0 =C2=A0
>=C2=A0 =C2=A0 =C2=A0 Previously, shutdown_panic and shutdown_halt had t= he same priority which
>=C2=A0 =C2=A0 =C2=A0 appears to be incompatible with handlers that can = do both poweroff and
>=C2=A0 =C2=A0 =C2=A0 reset.

I want to add that having many handlers with priorities expressed like
SHUTDOWN_PRI_LAST =C2=B1 N while some of those handlers have implicit
inter-dependencies (interactions, interference) also does not help to see a=
clear picture.

Perhaps it would be better to handle all (reasonable) RB flag combinations =
centrally in kern_reboot and then dispatch events like shutdown_reset,
shutdown_poweroff, etc.=C2=A0 Handlers for those events would have a single= and
simple job of performing that one action (perhaps failing and letting anoth= er
handler try).

Also, I would split reboot howto into command and flag portions, so that on= ly
one command can be specified at a time.=C2=A0 E.g., I would consider RB_AUT= OBOOT
("RB_REBOOT"), RB_POWEROFF, RB_HALT to be distinct commands.=C2= =A0 Then, flags like
RB_NOSYNC or RB_DUMP could be optional flags.
describe what to do (was the f= ashion in the late 80s/90s, bio used to have its
commands as bits, not a bit field). You also didn't include RB_PO= WERCYCLE which
is a new bit in this list.

It'= s a mess.

As an aside, some flags documented for reboot(2) do not seem to have much t= o do
with reboot.=C2=A0 E.g., RB_DFLTROOT affects how a system boots up, but not= how the
system goes for a reboot.=C2=A0 Not surprisingly, that option is not handle= d by
anything kicked off with reboot(2).
Maybe, it would make more sense if we had fast reboot support and the runni= ng
kernel could instruct the next kernel directly.=C2=A0 But, it's still a= bit weird
that flags like RB_POWEROFF and RB_DFLTROOT belong in the same domain and c= an be
set together.

More like 'support ag= ain' since this interface is from 4BSD and hasn't been updated
in a very long time. It made sense when you could tell the VAX's = firmware details about
the next reboot, but we don't really h= ave that short of implementing kexec...

Though= to fix it we should maybe just have a number of handlers that are called
at each stage, and we deal with only one bit at a time (POWERCYCLE= > POWEROFF > HALT)
and your drivers register a separate on= e for each...=C2=A0 It would be a bit more rework
in the tree, an= d there'd be a few more functions called, but it would be a minimal cha= nge.

But it kinda feels like we should just bite t= he bullet and have 3 handlers for these cases.
One to power cycle= , one to power off and one to halt. Then the drivers wouldn't care whic= h ones
have priority, they'd just check a bit and do what the= y are told=C2=A0 (or maybe we say that they
only run when the bit= is set to make that code simpler). And if one bit of hardware can do
=
all three, they'd have to implement 3 handlers... Tha ambiguity wo= uld be gone and the ordering
wouldn't matter.

<= /div>
Warner
--0000000000005c13d20610bde77f--