Re: [Xen-devel] [PATCH v2 1/2] x86/mem-sharing: Bulk mem-sharing entire domains

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 08.10.15 at 22:57, <tamas@xxxxxxxxxxxxx> wrote:
> --- a/xen/arch/x86/mm/mem_sharing.c
> +++ b/xen/arch/x86/mm/mem_sharing.c
> @@ -1293,6 +1293,37 @@ int relinquish_shared_pages(struct domain *d)
>      return rc;
>  }
>  
> +static int bulk_share(struct domain *d, struct domain *cd, unsigned long max,
> +                      struct mem_sharing_op_bulk_share *bulk)
> +{
> +    int rc = 0;
> +    shr_handle_t sh, ch;
> +
> +    while( bulk->start <= max )
> +    {
> +        if ( mem_sharing_nominate_page(d, bulk->start, 0, &sh) != 0 )
> +            goto next;
> +
> +        if ( mem_sharing_nominate_page(cd, bulk->start, 0, &ch) != 0 )
> +            goto next;
> +
> +        if ( !mem_sharing_share_pages(d, bulk->start, sh, cd, bulk->start, 
> ch) )
> +            ++(bulk->shared);

Pointless parentheses.

> +next:

Labels indented by at least one space please.

> +        ++(bulk->start);
> +
> +        /* Check for continuation if it's not the last iteration. */
> +        if ( bulk->start < max && hypercall_preempt_check() )
> +        {
> +            rc = 1;
> +            break;

This could simple be a return statement, avoiding the need for a
local variable (the type of which would need to be changed, see
below).

> +        }
> +    }
> +
> +    return rc;
> +}

So effectively the function right now returns a boolean. If that's
intended, its return type should reflect that (but I then wonder
whether the sense of the values shouldn't be inverted, to have
"true" mean "done").

> @@ -1467,6 +1498,59 @@ int 
> mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>          }
>          break;
>  
> +        case XENMEM_sharing_op_bulk_share:
> +        {
> +            unsigned long max_sgfn, max_cgfn;
> +            struct domain *cd;
> +
> +            rc = -EINVAL;
> +            if ( !mem_sharing_enabled(d) )
> +                goto out;
> +
> +            rc = rcu_lock_live_remote_domain_by_id(mso.u.bulk.client_domain,
> +                                                   &cd);
> +            if ( rc )
> +                goto out;
> +
> +            rc = xsm_mem_sharing_op(XSM_DM_PRIV, d, cd, mso.op);
> +            if ( rc )
> +            {
> +                rcu_unlock_domain(cd);
> +                goto out;
> +            }
> +
> +            if ( !mem_sharing_enabled(cd) )
> +            {
> +                rcu_unlock_domain(cd);
> +                rc = -EINVAL;
> +                goto out;
> +            }
> +
> +            max_sgfn = domain_get_maximum_gpfn(d);
> +            max_cgfn = domain_get_maximum_gpfn(cd);
> +
> +            if ( max_sgfn != max_cgfn || max_sgfn < mso.u.bulk.start )

Are both domains required to be paused for this operation? If so,
shouldn't you enforce this? If not, by the time you reach the if()
the values are stale.

> +            {
> +                rcu_unlock_domain(cd);
> +                rc = -EINVAL;
> +                goto out;
> +            }
> +
> +            rc = bulk_share(d, cd, max_sgfn, &mso.u.bulk);
> +            if ( rc )

With the boolean nature the use of "rc" here is rather confusing;
I'd suggest avoiding the use of in intermediate variable in this case.

> +            {
> +                if ( __copy_to_guest(arg, &mso, 1) )
> +                    rc = -EFAULT;
> +                else
> +                    rc = 
> hypercall_create_continuation(__HYPERVISOR_memory_op,
> +                                                       "lh", 
> XENMEM_sharing_op,
> +                                                       arg);
> +            }
> +
> +            rcu_unlock_domain(cd);
> +        }
> +        break;
> +
>          case XENMEM_sharing_op_debug_gfn:
>          {
>              unsigned long gfn = mso.u.debug.u.gfn;
> diff --git a/xen/include/public/memory.h b/xen/include/public/memory.h
> index 320de91..0299746 100644
> --- a/xen/include/public/memory.h
> +++ b/xen/include/public/memory.h
> @@ -447,6 +447,7 @@ DEFINE_XEN_GUEST_HANDLE(xen_mem_access_op_t);
>  #define XENMEM_sharing_op_debug_gref        5
>  #define XENMEM_sharing_op_add_physmap       6
>  #define XENMEM_sharing_op_audit             7
> +#define XENMEM_sharing_op_bulk_share        8
>  
>  #define XENMEM_SHARING_OP_S_HANDLE_INVALID  (-10)
>  #define XENMEM_SHARING_OP_C_HANDLE_INVALID  (-9)
> @@ -482,7 +483,13 @@ 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_bulk_share {   /* OP_BULK_SHARE */

Is the _share suffix really useful here? Even more, if you dropped
it and renamed "shared" below to "done", the same structure could
be used for a hypothetical bulk-unshare operation.

> +            domid_t client_domain;           /* IN: the client domain id */
> +            uint64_aligned_t start;          /* IN: start gfn (normally 0) */

Marking this as just IN implies that the value doesn't change across
the operation.

> +            uint64_aligned_t shared;         /* OUT: the number of
> +                                                successfully shared pages */

For this to be correct, the value is required to be zero initialized by
the caller at present. Either this needs to be documented, or you
should zero the field on any non-continuation invocation.

Jan

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