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

[Tamas K Lengyel <tamas.lengyel@zentific.com>]

On Mon, Jul 18, 2016 at 3:47 PM, Andrew Cooper
<andrew.cooper3@citrix.com> wrote:
> 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>

Thanks!

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

I'm not that familiar with this style of the syntax, does that have
the effect of setting start = _scratchspace when _scratchspace is not
0?

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

Well, in that case I can just change the if statement to rc == -EINVAL.

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

You mean add an extra space or that there is an extra space?

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