Re: [PATCH 2/3] x86/mem_sharing: use dom_cow as placeholder parent until fork is complete

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Mar 28, 2022 at 9:32 AM Jan Beulich <jbeulich@xxxxxxxx> wrote:
>
> 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.

Hm, yea, domain_relinquish_resources really ought to check if there is
any parent at all, while fork_page needs to check whether there is a
parent and it's not dom_cow. I think I just need two separate
mem_sharing_is_fork functions, one would be the current
mem_sharing_is_fork() and a new one mem_sharing_is_fork_complete(),
that would make it a bit clearer on what they do.

>
> > --- 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.

Ack.

Thanks,
Tamas