Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Content-Type: text/plain; charset="utf-8"
MIME-Version: 1.0
Content-Transfer-Encoding: quoted-printable
In-Reply-To: <20220810125625.45295-2-imbrenda@linux.ibm.com>
References: <20220810125625.45295-1-imbrenda@linux.ibm.com> <20220810125625.45295-2-imbrenda@linux.ibm.com>
To:     Claudio Imbrenda <imbrenda@linux.ibm.com>, kvm@vger.kernel.org
From:   Nico Boehr <nrb@linux.ibm.com>
Subject: Re: [PATCH v13 1/6] KVM: s390: pv: asynchronous destroy for reboot
Cc:     borntraeger@de.ibm.com, frankja@linux.ibm.com, thuth@redhat.com,
        pasic@linux.ibm.com, david@redhat.com, linux-s390@vger.kernel.org,
        linux-kernel@vger.kernel.org, scgl@linux.ibm.com,
        seiden@linux.ibm.com
Message-ID: <166030300049.24812.17231340197534047162@localhost.localdomain>
User-Agent: alot/0.8.1
Date:   Fri, 12 Aug 2022 13:16:40 +0200
Precedence: bulk

Quoting Claudio Imbrenda (2022-08-10 14:56:20)
> Until now, destroying a protected guest was an entirely synchronous
> operation that could potentially take a very long time, depending on
> the size of the guest, due to the time needed to clean up the address
> space from protected pages.
>=20
> This patch implements an asynchronous destroy mechanism, that allows a
> protected guest to reboot significantly faster than previously.
>=20
> This is achieved by clearing the pages of the old guest in background.
> In case of reboot, the new guest will be able to run in the same
> address space almost immediately.
>=20
> The old protected guest is then only destroyed when all of its memory has
> been destroyed or otherwise made non protected.
>=20
> Two new PV commands are added for the KVM_S390_PV_COMMAND ioctl:
>=20
> KVM_PV_ASYNC_CLEANUP_PREPARE: prepares the current protected VM for
> asynchronous teardown.=20

I would add:

A protected VM which has been prepared for asynchronous teardown is called =
a *set aside VM*. A set a side VM never has any CPUs associated with it, bu=
t is still registered with the Ultravisor and may have memory.

> The current VM will then continue immediately
> as non-protected. If a protected VM had already been set aside without
> starting the teardown process, this call will fail. There can be at
> most one prepared VM at any time.
>=20
> KVM_PV_ASYNC_CLEANUP_PERFORM: tears down the protected VM previously
> set aside for asynchronous teardown. This PV command should ideally be
> issued by userspace from a separate thread. If a fatal signal is
> received (or the process terminates naturally), the command will
> terminate immediately without completing.=20

I would add:

A set aside VM where cleanup started but was interrupted is called a *lefto=
ver VM*. There can be multiple leftovers per VM, which are tracked in the n=
eed_cleanup list.

I would like a more consistent language. We have three different names for =
the leftover in the patch below:
- leftover,
- the struct describing a leftover is called pv_vm_to_be_destroyed,
- the list of leftover vms is called need_cleanup.

It would be nice if this were a little more consistent, e.g. by renaming pv=
_vm_to_be_destroyed to pvm_vm_leftover and maybe need_cleanup to leftovers.

Otherwise, this looks good. I would be glad if you pick up my naming sugges=
tions, but feel free to add:

Reviewed-by: Nico Boehr <nrb@linux.ibm.com>