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

[Roger Pau Monné <roger.pau@xxxxxxxxxx>]

On Mon, Feb 24, 2020 at 08:45:05AM -0700, Tamas K Lengyel wrote:
> On Mon, Feb 24, 2020 at 5:39 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
> >
> > On Fri, Feb 21, 2020 at 10:49:21AM -0800, Tamas K Lengyel wrote:
> > > +    }
> > > +
> > > +    /*
> > > +     * If it's a write access (ie. unsharing) or if adding a shared 
> > > entry to
> > > +     * the physmap failed we'll fork the page directly.
> > > +     */
> > > +    p2m = p2m_get_hostp2m(d);
> > > +    parent = d->parent;
> > > +
> > > +    while ( parent )
> > > +    {
> > > +        mfn = get_gfn_query(parent, gfn_l, &p2mt);
> > > +
> > > +        if ( mfn_valid(mfn) && p2m_is_any_ram(p2mt) )
> >
> > This would also cover grants, but I'm not sure how those are handled
> > by forking, as access to those is granted on a per-domain basis. Ie:
> > the parent will have access to the grant, but not the child.
> 
> Good question. Grants are not sharable because their reference count
> will prevent sharing, so here the page content would just get copied
> as a regular page into the fork. I can check that if in the usecase we
> have anything breaks if we just skip grants completely, I don't think
> a regular domain has any grants by default.

Hm, I don't have a good suggestion for this. Since the domain is not
aware of the fork there's no way for it to notice grant maps have
become stale.

Can you add a note in this regard? And maybe crashing the fork when a
grant is found would be fine, until we figure out how to handle them
properly.

> >
> > > +            break;
> > > +
> > > +        put_gfn(parent, gfn_l);
> > > +        parent = parent->parent;
> > > +    }
> > > +
> > > +    if ( !parent )
> > > +        return -ENOENT;
> > > +
> > > +    if ( !(page = alloc_domheap_page(d, 0)) )
> > > +    {
> > > +        put_gfn(parent, gfn_l);
> > > +        return -ENOMEM;
> > > +    }
> > > +
> > > +    new_mfn = page_to_mfn(page);
> > > +    copy_domain_page(new_mfn, mfn);
> > > +    set_gpfn_from_mfn(mfn_x(new_mfn), gfn_l);
> > > +
> > > +    put_gfn(parent, gfn_l);
> > > +
> > > +    return p2m->set_entry(p2m, gfn, new_mfn, PAGE_ORDER_4K, p2m_ram_rw,
> >
> > So the child p2m is going to be populated using 4K pages exclusively?
> > Maybe it would make sense to try to use 2M if the parent domain page
> > is also a 2M page or larger?
> 
> No, memory sharing only works on a 4k granularity so that's what we
> are going with. No reason to copy 2M pages when most of it can just be
> shared when broken up.

Oh, OK. For your use-case it likely doesn't matter that much, but
long-running forks would get better performance if using large pages.

> > > @@ -509,6 +509,14 @@ mfn_t __get_gfn_type_access(struct p2m_domain *p2m, 
> > > unsigned long gfn_l,
> > >
> > >      mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order, NULL);
> > >
> > > +    /* Check if we need to fork the page */
> > > +    if ( (q & P2M_ALLOC) && p2m_is_hole(*t) &&
> > > +         !mem_sharing_fork_page(p2m->domain, gfn, !!(q & P2M_UNSHARE)) )
> > > +    {
> > > +        mfn = p2m->get_entry(p2m, gfn, t, a, q, page_order, NULL);
> > > +    }
> >
> > No need for the braces.
> 
> I would keep them, it helps with readability in this case.

CODING_STYLE mentions that braces should be omitted for blocks with a
single statement, but I'm not sure how strictly we enforce this.

Thanks, Roger.

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