On 22.03.2022 18:41, Tamas K Lengyel wrote:
> For the duration of the fork memop set dom_cow as a placeholder parent. This
> gets updated to the real parent when the fork operation completes, or to NULL
> in case the fork failed.
I am concerned of this, in particular because the state may last across
perhaps a long series of preemptions. Furthermore ...
> --- a/xen/arch/x86/include/asm/mem_sharing.h
> +++ b/xen/arch/x86/include/asm/mem_sharing.h
> @@ -79,7 +79,7 @@ static inline int mem_sharing_unshare_page(struct domain *d,
>
> static inline bool mem_sharing_is_fork(const struct domain *d)
> {
> - return d->parent;
> + return d->parent && d->parent != dom_cow;
> }
... this now makes the function "lie" (the domain is a fork already
while being constructed). Hence at the very least a comment would want
to appear here explaining why this is wanted despite not really being
correct. This "lying" for example means a partly set up fork would be
skipped by domain_relinquish_resources(), in case the tool stack
decided to kill the domain instead of waiting for its creation to
finish.
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1850,7 +1850,9 @@ static int fork(struct domain *cd, struct domain *d,
> uint16_t flags)
> *cd->arch.cpuid = *d->arch.cpuid;
> *cd->arch.msr = *d->arch.msr;
> cd->vmtrace_size = d->vmtrace_size;
> - cd->parent = d;
> +
> + /* use dom_cow as a placeholder until we are all done */
Nit: As per ./CODING_STYLE you want to at least start the comment with
a capital letter.
Jan
