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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Fri, Oct 9, 2015 at 1:51 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

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

Pointless but harmless and I like this style better.

Â

> +next:

Labels indented by at least one space please.

Ack.

Â

> +Â Â Â Â ++(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).

It's reused in the caller to indicate where the mso copyback happens and rc is of type int in the caller.

Â

> +Â Â Â Â }
> +Â Â }
> +
> +Â Â 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").

It does but it's return is assigned to rc which is used to decide where copyback happens.

Â

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

It's expected that the user has exclusive tool-side lock on the domains before issuing this hypercall and that the domains are paused already.

Â

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

I don't see where the confusion is - rc indicates there is work left to do and hypercall continuation needs to be setup. I could move this block into bulk_share itself.

Â

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

I don't really have a use-case for that at the moment and having it simply as "bulk" is not specific enough IMHO.

Â

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

In my book IN means it's used strictly only to pass input and it's value may or may not be the same afterwards.

Â

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

Sure, I'll add a comment for that effect - the toolstack already memsets the mso struct to zero but I agree it's better to explicitly state that.

Â

Tamas

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