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

["Roger Pau Monné" <roger.pau@citrix.com>]

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@intel.com>
> ---
>  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.

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.

Or else this is very likely to trip over 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.

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

> +    {
> +        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?

> +
> +        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?

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

Thanks, Roger.

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