Re: [Xen-devel] [PATCH v5 15/18] xen/mem_sharing: VM forking

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Thu, Jan 23, 2020 at 10:21 AM George Dunlap <george.dunlap@xxxxxxxxxx> wrote:
>
> On 1/21/20 5:49 PM, Tamas K Lengyel wrote:
> > VM forking is the process of creating a domain with an empty memory space 
> > and a
> > parent domain specified from which to populate the memory when necessary. 
> > For
> > the new domain to be functional the VM state is copied over as part of the 
> > fork
> > operation (HVM params, hap allocation, etc).
> >
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
>
> Overall this looks really good.  Just a few questions.
>
> > ---
> >  xen/arch/x86/domain.c             |   9 ++
> >  xen/arch/x86/hvm/hvm.c            |   2 +-
> >  xen/arch/x86/mm/mem_sharing.c     | 220 ++++++++++++++++++++++++++++++
> >  xen/arch/x86/mm/p2m.c             |  11 +-
> >  xen/include/asm-x86/mem_sharing.h |  20 ++-
> >  xen/include/public/memory.h       |   5 +
> >  xen/include/xen/sched.h           |   2 +
> >  7 files changed, 265 insertions(+), 4 deletions(-)
> >
> > diff --git a/xen/arch/x86/domain.c b/xen/arch/x86/domain.c
> > index 28fefa1f81..953abcc1fc 100644
> > --- a/xen/arch/x86/domain.c
> > +++ b/xen/arch/x86/domain.c
> > @@ -2198,6 +2198,15 @@ int domain_relinquish_resources(struct domain *d)
> >              ret = relinquish_shared_pages(d);
> >              if ( ret )
> >                  return ret;
> > +
> > +            /*
> > +             * If the domain is forked, decrement the parent's pause count.
> > +             */
> > +            if ( d->parent )
> > +            {
> > +                domain_unpause(d->parent);
> > +                d->parent = NULL;
>
> Did this want to be `if ( d->parent_paused )`?

If the domain has the parent pointer set, it's guaranteed that the
parent is paused. It's paused before the parent pointer is set during
the fork hypercall.

>
> > +static int bring_up_vcpus(struct domain *cd, struct cpupool *cpupool)
> > +{
> > +    int ret;
> > +    unsigned int i;
> > +
> > +    if ( (ret = cpupool_move_domain(cd, cpupool)) )
> > +        return ret;
> > +
> > +    for ( i = 0; i < cd->max_vcpus; i++ )
> > +    {
> > +        if ( cd->vcpu[i] )
> > +            continue;
> > +
> > +        if ( !vcpu_create(cd, i) )
> > +            return -EINVAL;
>
> You're not copying the contents of the vcpu registers or anything here
> -- is that something you're leaving to your dom0 agent?

The registers are being copied as part of the HVM contexts.

>
> > +static int mem_sharing_fork(struct domain *d, struct domain *cd)
> > +{
> > +    int rc = -EINVAL;
> > +
> > +    if ( !cd->controller_pause_count )
> > +        return rc;
> > +
> > +    /*
> > +     * We only want to pause the parent once, not each time this
> > +     * operation is restarted due to preemption.
> > +     */
> > +    if ( !cd->parent_paused )
> > +    {
> > +        domain_pause(d);
> > +        cd->parent_paused = true;
> > +    }
> > +
> > +    cd->max_pages = d->max_pages;
> > +    cd->max_vcpus = d->max_vcpus;
> > +
> > +    /* this is preemptible so it's the first to get done */
> > +    if ( (rc = fork_hap_allocation(d, cd)) )
> > +        goto done;
> > +
> > +    if ( (rc = bring_up_vcpus(cd, d->cpupool)) )
> > +        goto done;
> > +
> > +    if ( (rc = hvm_copy_context_and_params(d, cd)) )
> > +        goto done;
> > +
> > +    fork_tsc(d, cd);
> > +
> > +    cd->parent = d;
>
> What happens if the parent dies?
>
> It seems like we might want to do get_domain(parent) here, and
> put_domain(parent) in domain_destroy.

If forks are still active when someone destroys the parent than Xen
will crash I assume. Right now it's a requirement that the parent
remains in existence - and it's paused - while there are forks active.
We enforce the pause state but making the parent undestroyable is not
implemented right now. We just trust that the user of this
experimental system won't do that.

But yes, doing the get_domain()/put_domain() dance would be an easy
way to do that. Will add that and then we don't have to worry about
the parent getting pulled from under our feat.

>
> I'll probably need to come back to this, at which point I may have more
> questions.

Thanks!
Tamas

_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxxxxxxxxx
https://lists.xenproject.org/mailman/listinfo/xen-devel