Re: [Xen-devel] [PATCH v9 4/5] x86/mem_sharing: reset a fork

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Feb 24, 2020 at 8:13 AM Roger Pau Monné <roger.pau@xxxxxxxxxx> wrote:
>
> On Fri, Feb 21, 2020 at 10:49:22AM -0800, Tamas K Lengyel wrote:
> > Implement hypercall that allows a fork to shed all memory that got allocated
> > for it during its execution and re-load its vCPU context from the parent VM.
> > This allows the forked VM to reset into the same state the parent VM is in a
> > faster way then creating a new fork would be. Measurements show about a 2x
> > speedup during normal fuzzing operations. Performance may vary depending how
>                                                                           ^ on
> > much memory got allocated for the forked VM. If it has been completely
> > deduplicated from the parent VM then creating a new fork would likely be 
> > more
> > performant.
> >
> > Signed-off-by: Tamas K Lengyel <tamas.lengyel@xxxxxxxxx>
> > ---
> >  xen/arch/x86/mm/mem_sharing.c | 76 +++++++++++++++++++++++++++++++++++
> >  xen/include/public/memory.h   |  1 +
> >  2 files changed, 77 insertions(+)
> >
> > diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> > index ad5db9d8d5..fb6892aaa6 100644
> > --- a/xen/arch/x86/mm/mem_sharing.c
> > +++ b/xen/arch/x86/mm/mem_sharing.c
> > @@ -1636,6 +1636,59 @@ static int mem_sharing_fork(struct domain *d, struct 
> > domain *cd)
> >      return rc;
> >  }
> >
> > +/*
> > + * The fork reset operation is intended to be used on short-lived forks 
> > only.
> > + * There is no hypercall continuation operation implemented for this 
> > reason.
> > + * For forks that obtain a larger memory footprint it is likely going to be
> > + * more performant to create a new fork instead of resetting an existing 
> > one.
> > + *
> > + * TODO: In case this hypercall would become useful on forks with larger 
> > memory
> > + * footprints the hypercall continuation should be implemented.
>
> I'm afraid this is not safe, as users don't have an easy way to know
> whether a fork will have a large memory footprint or not.

They do, getdomaininfo tells a user exactly how much memory has been
allocated for a domain.

>
> IMO you either need some kind of check that prevents this function
> from being executed when the domain as too much memory assigned, or
> you need to implement continuations.

I really don't think we need continuation here with the usecase we
have for this function but I'm also tired of arguing about it, so I'll
just add it even if its going to be dead code.

>
> Or else this is very likely to trip over the watchdog.

The watchdog?

>
> > + */
> > +static int mem_sharing_fork_reset(struct domain *d, struct domain *cd)
>
> No need for the mem_sharing prefix, as it's a static function.

Sure.

>
> > +{
> > +    int rc;
> > +    struct p2m_domain* p2m = p2m_get_hostp2m(cd);
> > +    struct page_info *page, *tmp;
> > +
> > +    domain_pause(cd);
> > +
> > +    page_list_for_each_safe(page, tmp, &cd->page_list)
>
> You can use something like:
>
> while ( (page = page_list_remove_head(&cd->page_list) != NULL )
> {
>     ...
> }
>
> And avoid the *tmp local variable.

Sure.

>
> > +    {
> > +        p2m_type_t p2mt;
> > +        p2m_access_t p2ma;
> > +        gfn_t gfn;
> > +        mfn_t mfn = page_to_mfn(page);
> > +
> > +        if ( !mfn_valid(mfn) )
> > +            continue;
> > +
> > +        gfn = mfn_to_gfn(cd, mfn);
> > +        mfn = __get_gfn_type_access(p2m, gfn_x(gfn), &p2mt, &p2ma,
> > +                                    0, NULL, false);
> > +
> > +        if ( !p2m_is_ram(p2mt) || p2m_is_shared(p2mt) )
> > +            continue;
> > +
> > +        /* take an extra reference */
> > +        if ( !get_page(page, cd) )
> > +            continue;
> > +
> > +        rc = p2m->set_entry(p2m, gfn, INVALID_MFN, PAGE_ORDER_4K,
> > +                            p2m_invalid, p2m_access_rwx, -1);
> > +        ASSERT(!rc);
>
> Can you handle this gracefully?

Nope. This should never happen, so if it does, something is very wrong
in some other part of Xen.

>
> > +
> > +        put_page_alloc_ref(page);
> > +        put_page(page);
> > +    }
> > +
> > +    if ( !(rc = hvm_copy_context_and_params(cd, d)) )
> > +        fork_tsc(cd, d);
> > +
> > +    domain_unpause(cd);
> > +    return rc;
> > +}
> > +
> >  int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
> >  {
> >      int rc;
> > @@ -1920,6 +1973,29 @@ int 
> > mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
> >          break;
> >      }
> >
> > +    case XENMEM_sharing_op_fork_reset:
> > +    {
> > +        struct domain *pd;
> > +
> > +        rc = -EINVAL;
> > +        if ( mso.u.fork._pad[0] || mso.u.fork._pad[1] ||
> > +             mso.u.fork._pad[2] )
> > +            goto out;
> > +
> > +        rc = -ENOSYS;
> > +        if ( !d->parent )
>
> Why not use mem_sharing_is_fork?

I could.

>
> Also I think EINVAL would be more suitable here, as the passed domid
> doesn't belong to a fork?

Sure.

Thanks,
Tamas

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