Re: [Xen-devel] [PATCH v8 3/5] xen/mem_sharing: VM forking

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Feb 21, 2020 at 10:47 AM Andrew Cooper
<andrew.cooper3@xxxxxxxxxx> wrote:
>
> On 21/02/2020 17:07, Tamas K Lengyel wrote:
> > On Fri, Feb 21, 2020 at 7:42 AM Andrew Cooper <andrew.cooper3@xxxxxxxxxx> 
> > wrote:
> >> On 10/02/2020 19:21, Tamas K Lengyel wrote:
> >>> +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 get and pause the parent once, not each time this
> >>> +     * operation is restarted due to preemption.
> >>> +     */
> >>> +    if ( !cd->parent_paused )
> >>> +    {
> >>> +        ASSERT(get_domain(d));
> >>> +        domain_pause(d);
> >>> +
> >>> +        cd->parent_paused = true;
> >>> +        cd->max_pages = d->max_pages;
> >>> +        cd->max_vcpus = d->max_vcpus;
> >> Sorry, I spoke too soon.  You can't modify max_vcpus here, because it
> >> violates the invariant that domain_vcpu() depends upon for safety.
> >>
> >> If the toolstack gets things wrong, Xen will either leak struct vcpu's
> >> on cd's teardown, or corrupt memory beyond the end of the cd->vcpu[] array.
> >>
> >> Looking at the hypercall semantics, userspace creates a new domain
> >> (which specifies max_cpus), then calls mem_sharing_fork(parent_dom,
> >> new_dom);  Forking should be rejected if toolstack hasn't chosen the
> >> same number of vcpus for the new domain.
> > That's unfortunate since this would require an extra hypercall just to
> > get information Xen already has. I think instead of what you recommend
> > what I'll do is extend XEN_DOMCTL_createdomain to include the parent
> > domain's ID already so Xen can gather these information automatically
> > without the toolstack having to do it this roundabout way.
>
> Conceptually, I think that is cleaner.  You really do want to duplicate
> the parent domain, whatever its settings are.
>
> However, I'd suggest not extending createdomain.  What should the
> semantics be for such a call which passes conflicting settings?

I'm not sure what conflicting settings you worry about? I simply add a
field to xen_domctl_createdomain called parent_domain, then in the
domctl handler if its set we copy the max_vcpus value directly from
there:

        parent_dom = op->u.createdomain.parent_domid;
        if ( parent_dom )
        {
            struct domain *pd = rcu_lock_domain_by_id(parent_dom);

            ret = -EINVAL;
            if ( !pd )
                break;

            op->u.createdomain.max_vcpus = pd->max_vcpus;
            rcu_unlock_domain(pd);
        }

>
> How about a new top level domctl for clone_domain, taking a parent
> domain identifier, and optionally providing a specific new domid?  This
> way, it is impossible to provide conflicting settings, and the semantics
> should be quite clear.  Internally, Xen can do whatever it needs to copy
> appropriate settings, and get things sorted before the domain becomes
> globally visible.

I already have a hypercall added for fork_domain and I even tried
doing everything in a single hypercall. The problem I ran into with
that was it required a lot of refactoring within Xen since
domain_create is not preemptible right now, while some other parts of
forking need to be preemptible. So it was just easier to do it with 2
hypercalls. One just creates the domain shell via
XEN_DOMCTL_createdomain and the second actually sets it up as a fork.

>
> One question needing considering is whether such a hypercall could in
> theory be useful without the mem_sharing infrastructure, or not.  (i.e.
> how tightly integrated we should aim for.)
>
> >> This raises the question of whether the same should be true for
> >> max_pages as well.
> > Could you expand on this?
>
> Well - these two are a very odd subset of things to blindly copy.  The
> max_cpus side is wrong, which makes max_pages likely to be wrong as well.

The max_cpus side is clearer why it's wrong since there is an
allocation during domain_create based on that number. I haven't ran
into that issue it seems because all the domains I tested with had
only a single vCPU. But max_pages should be safe to copy, since any
page that would get accessed up to max_pages would get forked from the
parent. I haven't run into any issues with that. So I don't really see
what's the problem there.

Tamas

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