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

[Andrew Cooper <andrew.cooper3@xxxxxxxxxx>]

On 09/10/15 19:13, Tamas K Lengyel wrote:

On Fri, Oct 9, 2015 at 7:26 AM, Andrew Cooper <andrew.cooper3@xxxxxxxxxx> wrote:

On 08/10/15 21:57, Tamas K Lengyel wrote:
> diff --git a/xen/arch/x86/mm/mem_sharing.c b/xen/arch/x86/mm/mem_sharing.c
> index a95e105..4cdddb1 100644
> --- 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 )

This swallows the error from mem_sharing_nominate_page().  Some errors
might be safe to ignore in this context, but ones like ENOMEM most
certainly are not.

You should record the error into rc and switch ( rc ) to ignore/process
the error, passing hard errors straight up.

In my experience all errors here are safe to ignore. Yes, if an ENOMEM condition presents itself the sharing will be incomplete (or even 0). There isn't much the user can do about that other than killing another domain to free up memory..

There is plenty which can be done, including ballooning down other domains to make more memory (Which is an option XenServer will take if the admin chooses).  The point is that it puts the decision in the hands of the toolstack, but that can only happen when errors are correctly propagated back to userspace.

which will happen anyway automatically when a clone domain first hits an unshare operation. These conditions are better handled with the memsharing event listener.

 

> +            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);
> +
> +next:
> +        ++(bulk->start);
> +
> +        /* Check for continuation if it's not the last iteration. */
> +        if ( bulk->start < max && hypercall_preempt_check() )
> +        {
> +            rc = 1;

Using -ERESTART here allows...

> +            break;
> +        }
> +    }
> +
> +    return rc;
> +}
> +
>  int mem_sharing_memop(XEN_GUEST_HANDLE_PARAM(xen_mem_sharing_op_t) arg)
>  {
>      int rc;
> @@ -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 )
> +            {
> +                rcu_unlock_domain(cd);
> +                rc = -EINVAL;
> +                goto out;
> +            }
> +
> +            rc = bulk_share(d, cd, max_sgfn, &mso.u.bulk);
> +            if ( rc )

... this check to be selective.

Sure, but I don't have a usecase for returning the error code to the user from the nominate/sharing op. The reason for that is that just return the error condition by itself is not very uself. Furthermore, the gfn at which the operation failed would have to be passed to allow for debugging. But for debugging the user could also just do this loop on his side where he would readily have this information. So I would rather just keep this op as simple as possible.

 

> +            {
> +                if ( __copy_to_guest(arg, &mso, 1) )

This __copy_to_guest() needs to happen unconditionally before creating
the continuation, as it contains the continuation information.

It does happen before setting up the continuation and the continuation only gets setup if this succeeds.

 

It also needs to happen on the success path, so .shared is correct.

It does happen on the success path below for !rc for all sharing ops.

Ah - so it does.  Sorry for the noise.

~Andrew

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