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

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

[-- Attachment #1.1: Type: text/plain, Size: 5597 bytes --]

On 09/10/15 19:13, Tamas K Lengyel wrote:
>
>
> On Fri, Oct 9, 2015 at 7:26 AM, Andrew Cooper
> <andrew.cooper3@citrix.com <mailto:andrew.cooper3@citrix.com>> 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

[-- Attachment #1.2: Type: text/html, Size: 10559 bytes --]

[-- Attachment #2: Type: text/plain, Size: 126 bytes --]

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