Re: [PATCH v7] x86/mem-sharing: mem-sharing a range of memory

[Andrew Cooper <andrew.cooper3@citrix.com>]

On 18/07/2016 22:14, Tamas K Lengyel wrote:
> Currently mem-sharing can be performed on a page-by-page basis from the control
> domain. However, this process is quite wasteful when a range of pages have to
> be deduplicated.
>
> This patch introduces a new mem_sharing memop for range sharing where
> the user doesn't have to separately nominate each page in both the source and
> destination domain, and the looping over all pages happen in the hypervisor.
> This significantly reduces the overhead of sharing a range of memory.
>
> Signed-off-by: Tamas K Lengyel <tamas.lengyel@zentific.com>
> Acked-by: Wei Liu <wei.liu2@citrix.com>

Some style nits, and one functional suggestion.

If you are happy with the suggestion, then Reviewed-by: Andrew Cooper
<andrew.cooper3@citrix.com>

> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index a522423..6d00228 100644
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1294,6 +1294,58 @@ int relinquish_shared_pages(struct domain *d)
>      return rc;
>  }
>  
> +static int range_share(struct domain *d, struct domain *cd,
> +                      struct mem_sharing_op_range *range)

Alignment.

> +{
> +    int rc = 0;
> +    shr_handle_t sh, ch;
> +    unsigned long start =
> +        range->_scratchspace ? range->_scratchspace : range->start;

This can be shortened to "unsigned long start = range->_scratchspace ?:
range->start;" and fit on a single line.

> +
> +    while( range->end >= start )
> +    {
> +        /*
> +         * We only break out if we run out of memory as individual pages may
> +         * legitimately be unsharable and we just want to skip over those.
> +         */
> +        rc = mem_sharing_nominate_page(d, start, 0, &sh);
> +        if ( rc == -ENOMEM )
> +            break;

Newline here please

> +        if ( !rc )
> +        {
> +            rc = mem_sharing_nominate_page(cd, start, 0, &ch);
> +            if ( rc == -ENOMEM )
> +                break;

And here.

> +            if ( !rc )
> +            {
> +                /* If we get here this should be guaranteed to succeed. */
> +                rc = mem_sharing_share_pages(d, start, sh,
> +                                             cd, start, ch);
> +                ASSERT(!rc);
> +            }
> +        }
> +
> +        /* Check for continuation if it's not the last iteration. */
> +        if ( range->end >= ++start && hypercall_preempt_check() )
> +        {
> +            rc = 1;
> +            break;
> +        }
> +    }
> +
> +    range->_scratchspace = start;
> +
> +    /*
> +     * We only propagate -ENOMEM as individual pages may fail with -EINVAL,
> +     * and for range sharing we only care if -ENOMEM was encountered so we reset
> +     * rc here.
> +     */
> +    if ( rc < 0 && rc != -ENOMEM )

Would you mind putting in an ASSERT(rc == -EINVAL) here, if we believe
that to be an ok case to ignore?  In the future if more errors get
raised, we don't want to silently lose a more serious error which should
be propagated up.

> +        rc = 0;
> +
> +    return rc;
> +}
> +
>  int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>  {
>      int rc;
> @@ -1468,6 +1520,94 @@ int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>          }
>          break;
>  
> +        case XENMEM_sharing_op_range_share:
> +        {
> +            unsigned long max_sgfn, max_cgfn;
> +            struct domain *cd;
> +
> +            rc = -EINVAL;
> +            if( mso.u.range._pad[0] || mso.u.range._pad[1] ||
> +                mso.u.range._pad[2] )
> +                goto out;
> +
> +            /*
> +             * We use _scratchscape for the hypercall continuation value.
> +             * Ideally the user sets this to 0 in the beginning but
> +             * there is no good way of enforcing that here, so we just check
> +             * that it's at least in range.
> +             */
> +            if ( mso.u.range._scratchspace &&
> +                (mso.u.range._scratchspace < mso.u.range.start ||
> +                 mso.u.range._scratchspace > mso.u.range.end) )

Alignment (extra space) for these two lines.

> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 29ec571..e0bc018 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -500,7 +501,14 @@ struct xen_mem_sharing_op {
>              uint64_aligned_t client_gfn;    /* IN: the client gfn */
>              uint64_aligned_t client_handle; /* IN: handle to the client page */
>              domid_t  client_domain; /* IN: the client domain id */
> -        } share; 
> +        } share;
> +        struct mem_sharing_op_range {         /* OP_RANGE_SHARE */

Alignment of the comment.

~Andrew

> +            uint64_aligned_t start;          /* IN: start gfn. */
> +            uint64_aligned_t end;            /* IN: end gfn (inclusive) */
> +            uint64_aligned_t _scratchspace;  /* Must be set to 0 */
> +            domid_t client_domain;           /* IN: the client domain id */
> +            uint16_t _pad[3];                /* Must be set to 0 */
> +        } range;
>          struct mem_sharing_op_debug {     /* OP_DEBUG_xxx */
>              union {
>                  uint64_aligned_t gfn;      /* IN: gfn to debug          */


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