Re: git: e4ab361e5394 - main - fix poweroff regression from 9cdf326b4f by delaying shutdown_halt

Hey John,

On Thu, Feb 8, 2024 at 10:52 AM John Baldwin <jhb@freebsd.org> wrote:

> On 2/6/24 2:13 AM, 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=e4ab361e53945a6c3e9d68c5e5ffc11de40a35f2
> >>
> >> commit e4ab361e53945a6c3e9d68c5e5ffc11de40a35f2
> >> Author:     Andriy Gapon <avg@FreeBSD.org>
> >> AuthorDate: 2024-02-06 08:55:13 +0000
> >> Commit:     Andriy Gapon <avg@FreeBSD.org>
> >> 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 ± 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.  Handlers for those events would have a single
> and
> > simple job of performing that one action (perhaps failing and letting
> another
> > handler try).
>
> I think having separate eventhandlers for shutdown, reset, and poweroff
> seems
> sensible.  It also permits a given driver to use different priorities
> (maybe it
> wants to be first for poweroff but last for reset, etc.)
>

I'd come to this conclusion as well. The handlers shouldn't even look at
the flags
IMHO. We can create a hierarchy of power cycle > reset > power off > halt
with
power unchanged easily enough, and call the handlers in that order, letting
individual
drivers duke it out.


> > 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.
> >
> > 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
> how the
> > system goes for a reboot.  Not surprisingly, that option is not handled
> by
> > 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.
>
> I would suggest deprecating flags that are no-ops.  In modern systems if
> you
> want to control the next boot you do it via other means (nextboot,
> efibootmgr,
> etc.) and reboot(2) is not a good API for that.
>

Part of the problem is that they aren't NO-OPs. We use the same howto flags
in the early boot that we use for reboot. There the flags mean something.
This
is passed in by the boot loader, and in this case, still does something.
This dates
as near as I can tell, to the VAX and other early Unix machines being able
to pass
a word (and maybe a little more) from one kernel to the next, a feature
that's
fallen out of fashion.


> It might be hard to fully cleanup some of the hackiness here, but if you
> can
> at least isolate the flag weirdness handling in kern_reboot by having the
> more
> specific eventhandlers then that might fix most of the ugliness.
>

Yea, I think we should isolate the drivers from looking at 'howto' and have
separate handlers for the following cases: power cycle, power off, reset
and halt.
I agree that some of the features that were hung on this word should be torn
down and only done via boot next or possibly from the boot loader -> kernel
handoff only.

Now, what we do with the 'reboot' system call? It seems like we should maybe
rework it in some way?

Warner