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

[Mykola Kvach <xakep.amatop@xxxxxxxxx>]

Thanks, Jan. I agree with your comments and will rework this for v3.

The main change will be that domain_shutting_down() preserves the old
is_shutting_down semantics. It will be true once shutdown has started
and remain true after shutdown completion until domain_resume() resets
the state.

On Wed, Apr 8, 2026 at 4:06 PM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> On 01.04.2026 08:41, Mykola Kvach wrote:
> > From: Mykola Kvach <mykola_kvach@xxxxxxxx>
> >
> > The shutdown flow currently overloads is_shutting_down and
> > is_shut_down to represent multiple phases of the shutdown lifecycle.
> > Some users treat is_shutting_down narrowly as "shutdown still needs to
> > be driven to completion", while others rely on it more broadly as
> > "the domain is no longer in its normal running state".
> >
> > Make the lifecycle explicit by introducing enum
> > domain_shutdown_state and helper predicates whose names match their
> > semantics: domain_shutting_down() for the transient phase,
> > domain_shutdown_completed() for the final state, and
> > domain_in_shutdown_state() for checks that need the union of both.
> >
> > The conversion is intentionally not mechanical. The old flags were not
> > mutually exclusive: once a domain became fully shut down,
> > is_shutting_down remained set. As a result, sites that previously
> > used the absence of is_shutting_down to exclude both the transient and
> > completed states now use domain_in_shutdown_state(), sites that care
> > specifically about the final state use domain_shutdown_completed(),
> > and only paths that still have work to do before shutdown finalization
> > use domain_shutting_down().
>
> In how far is the distinction between domain_shutting_down() and
> domain_in_shutdown_state() really necessary?

Agreed. I will drop domain_in_shutdown_state() in v3.

domain_shutting_down() will preserve the old is_shutting_down
semantics, rather than mean only the transient state. That is, it will
return true from the point where shutdown has started until
domain_resume() resets the state, including after shutdown has
completed.

> still leaves me pretty clueless as to almost(?) all of the uses of the
> latter (where I think the former could be used just as well). Many
> paths simply can't be taken anymore for a fully shut down domain, so
> the distinction between the two is moot there.
>
> I'm also concerned of the name domain_in_shutdown_state() itself. To

Yes, that name is misleading. Since the helper is not needed anymore,
I will remove it in v3.

> me this is far closer to domain_shutdown_completed() than to the union
> of both states. See how e.g. x86 CPUs can be "in shutdown state", i.e.
> not responding to anything but a reset or init signal.
>
> IOW an entirely mechanical replacement might in fact be easier to look
> at. And in the one rare case where the distinction might indeed be
> relevant, "domain_shutting_down() || domain_shutdown_completed()" could
> be used.

Agreed. I will follow this approach in v3.

The old is_shutting_down users will use domain_shutting_down(), with
the same effective semantics as before. The only users converted to
domain_shutdown_completed() will be the ones that previously checked
is_shut_down.

>
> > @@ -1423,9 +1423,12 @@ int domain_shutdown(struct domain *d, u8 reason)
> >      return 0;
> >  }
> >
> > -void domain_resume(struct domain *d)
> > +int domain_resume(struct domain *d)
> >  {
> >      struct vcpu *v;
> > +    enum domain_shutdown_state shutdown_state;
> > +    unsigned int shutdown_code;
> > +    int rc = 0;
> >
> >      /*
> >       * Some code paths assume that shutdown status does not get reset under
> > @@ -1435,10 +1438,18 @@ void domain_resume(struct domain *d)
> >
> >      spin_lock(&d->shutdown_lock);
> >
> > -    d->is_shutting_down = d->is_shut_down = 0;
> > +    shutdown_state = d->shutdown_state;
> > +    shutdown_code = d->shutdown_code;
> > +
> > +    if ( !domain_shutdown_completed(d) )
> > +    {
> > +        rc = -EINVAL;
> > +        goto out_unlock;
> > +    }
>
> I can't help the impression that you're mixing two things here - adjustment
> to how shutdown state is tracked, and enforcement of completed shutdown
> right here. This separate change likely would better be split out, and then
> come with a description saying not only what is being changed, but also why,
> and why the change is correct for both call sites.

Agreed. I will drop this part from v3.

domain_resume() will remain void and the callers will be left
unchanged. This patch should only make the existing shutdown state
tracking explicit. If resume-state validation is needed, it can be done
as a separate change with its own justification for the affected call
sites.

>
> > @@ -1448,9 +1459,17 @@ void domain_resume(struct domain *d)
> >          v->paused_for_shutdown = 0;
> >      }
> >
> > + out_unlock:
> >      spin_unlock(&d->shutdown_lock);
> >
> >      domain_unpause(d);
> > +
> > +    if ( rc )
> > +        dprintk(XENLOG_WARNING,
> > +                "%pd: Invalid domain state for resume: shutdown_state=%u, 
> > shutdown_code=%u\n",
> > +                d, shutdown_state, shutdown_code);
>
> The shutdown code doesn't affect whether the function would fail. The extra
> variable just to log it anyway is therefore a little irritating. Is there
> any reason to this that I'm overlooking?

The reason was only to avoid printing while holding shutdown_lock.

I wanted to sample the state under the lock and emit the diagnostic
after dropping it. shutdown_code was included only as extra context for
that warning, not because it affected the decision.

That said, I agree this is not needed here. Since I will drop the
resume-state validation from v3, the warning and the extra cached
shutdown_code will go away as well.

Best regards,
Mykola