Re: [PATCH] xen/domain: make shutdown state explicit

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Hi Jan,

Thank you for the review.

On Thu, Mar 19, 2026 at 12:32 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 19.03.2026 00:25, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > The domain shutdown flow currently overloads is_shutting_down and
> > is_shut_down to represent multiple phases of the shutdown lifecycle,
> > while some users treat is_shutting_down as a broader "domain is no
> > longer normal" condition.
> >
> > Make the shutdown lifecycle explicit by introducing
> > enum domain_shutdown_state and converting the existing users to helper
> > predicates describing whether shutdown is in progress, complete, or
> > active.
>
> Mind me asking what the distinction is between "active" and "in progress"?
> They feel like synonyms to me. To me "is shutting down" vs "was shut down"
> would be the clearer distinction (i.e. domain_shutting_down() and
> domain_shut_down() or some such, albeit for the latter I could also live
> with domain_shutdown_complete() as you have it, or maybe slightly less
> ambiguously domain_shutdown_completed()). Yet then I'm not a native
> speaker.

Yes, the distinction I was trying to make is:

  - in progress: shutdown has been initiated, but not all vCPUs may
    have reached the paused-for-shutdown state yet

  - complete: shutdown has been finalized, i.e. all vCPUs are paused
    and the domain is fully shut down

  - active: a shorthand for "shutdown_state != none", i.e. covering
    both of the above

So "active" was meant as a broader "the domain is already in shutdown
state", rather than as a synonym for "shutdown is currently progressing".

>
> Further, I can't quite derive upon what criteria you chose whether
> ->is_shutting_down checks are to be converted to domain_shutdown_active()
> vs domain_shutdown_in_progress(). This could do with writing down. (It
> also might be easier with the suggested alternative naming.)

Likewise, the conversions from is_shutting_down/is_shut_down were not
meant to be mechanical. The criterion I used was:

  - domain_shutdown_in_progress() where the code cares specifically
    about the transient phase before shutdown is fully finalized

  - domain_shutdown_complete() where the code is specifically about
    the fully shut down state being reported or tested

  - domain_shutdown_active() where the old logic was effectively using
    the combined condition "the domain is already somewhere in the
    shutdown lifecycle"

One detail which likely wasn't obvious from the patch is that the old
flags were not mutually exclusive. Once is_shut_down became true,
is_shutting_down still remains set as well.

Because of that, the conversion was not just about replacing old
is_shutting_down checks. I also had to account for sites using
is_shut_down explicitly, so that the resulting logic would preserve the
old distinctions between the in-progress, complete, and combined cases.

In particular, old !is_shutting_down checks implicitly excluded both
the in-progress and fully shut down states. Those are the places where
I used domain_shutdown_active(). Sites specifically testing the fully
shut down state were converted to domain_shutdown_complete(), while the
ones caring about the transient shutdown phase were converted to
domain_shutdown_in_progress().

vcpu_check_shutdown() is an example of the latter. That path exists to
drive a shutdown already in flight to completion: it may still need to
pause this vCPU, clear defer_shutdown, and then re-check whether
shutdown can now be finalized. Once shutdown is already complete,
there is nothing left for that path to do, so using the broader
combined state there would not be appropriate.

I agree this selection criterion is not obvious enough from the patch
as posted. I'll make that rationale explicit in the commit message.


If you think naming is part of the confusion, I could switch to
something along these lines instead:

  - domain_shutting_down()      for the in-progress state
  - domain_shutdown_completed() for the finalized state
  - domain_in_shutdown_state()  for the union of both

Would that look better to you?

>
> > @@ -1444,9 +1458,17 @@ void domain_resume(struct domain *d)
> >          v->paused_for_shutdown = 0;
> >      }
> >
> > +out_unlock:
>
> Nit (style): Labels indented by at least one blank please.

Ack.

Best regards,
Mykola

>
> Jan