Re: [Xen-devel] [PATCH v10 2/3] x86/mem_sharing: reset a fork

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

On Tue, Feb 25, 2020 at 11:17:56AM -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
> 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>
> ---
> v10: implemented hypercall continuation similar to the existing range_share op
> ---
>  xen/arch/x86/mm/mem_sharing.c | 126 +++++++++++++++++++++++++++++++++-
>  xen/include/public/memory.h   |   4 ++
>  2 files changed, 129 insertions(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index 8ee37e6943..aa4358aae4 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1673,7 +1673,6 @@ static int fork(struct domain *d, struct domain *cd)
>          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 */
> @@ -1704,6 +1703,91 @@ static int fork(struct domain *d, struct domain *cd)
>      return rc;
>  }
>  
> +/*
> + * The fork reset operation is intended to be used on short-lived forks only.
> + */
> +static int fork_reset(struct domain *d, struct domain *cd,
> +                      struct mem_sharing_op_fork_reset *fr)
> +{
> +    int rc = 0;
> +    struct p2m_domain* p2m = p2m_get_hostp2m(cd);
> +    struct page_info *page, *tmp;
> +    unsigned long list_position = 0, preempt_count = 0, start = fr->opaque;
> +
> +    domain_pause(cd);
> +
> +    page_list_for_each_safe(page, tmp, &cd->page_list)
> +    {
> +        p2m_type_t p2mt;
> +        p2m_access_t p2ma;
> +        gfn_t gfn;
> +        mfn_t mfn;
> +        bool shared = false;
> +
> +        list_position++;
> +
> +        /* Resume were we left of before preemption */
> +        if ( start && list_position < start )
> +            continue;
> +
> +        mfn = page_to_mfn(page);
> +        if ( mfn_valid(mfn) )
> +        {
> +
> +            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) )
> +            {
> +                /* take an extra reference, must work for a shared page */
> +                if( !get_page(page, cd) )
> +                {
> +                    ASSERT_UNREACHABLE();
> +                    return -EINVAL;
> +                }
> +
> +                shared = true;
> +                preempt_count += 0x10;
> +
> +                /*
> +                 * Must succeed, it's a shared page that exists and
> +                 * thus its size is guaranteed to be 4k so we are not 
> splitting
> +                 * large pages.
> +                 */
> +                rc = p2m->set_entry(p2m, gfn, INVALID_MFN, PAGE_ORDER_4K,
> +                                    p2m_invalid, p2m_access_rwx, -1);
> +                ASSERT(!rc);
> +
> +                put_page_alloc_ref(page);
> +                put_page(page);
> +            }
> +        }
> +
> +        if ( !shared )
> +            preempt_count++;
> +
> +        /* Preempt every 2MiB (shared) or 32MiB (unshared) - arbitrary. */
> +        if ( preempt_count >= 0x2000 )
> +        {
> +            if ( hypercall_preempt_check() )
> +            {
> +                rc = -ERESTART;
> +                break;
> +            }
> +            preempt_count = 0;
> +        }
> +    }
> +
> +    if ( rc )
> +        fr->opaque = list_position;
> +    else if ( !(rc = hvm_copy_context_and_params(cd, d)) )
> +        fork_tsc(cd, d);

You also need to reset the contents of the special pages, the
vcpu_info pages and the shared_info page in order to match the state
the VM was in when forking.

PV timers should also be reset to parent's state AFAICT, or else you
will get spurious timer interrupts.

In fact you should check against the state of the parent, because the
fork might have changed the position of the shared info or any other
of those magic areas, and that should be reverted to the state they
are in the parent.

Thanks, Roger.

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