|
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v3 1/2] x86/mem-sharing: Bulk mem-sharing entire domains
>>> On 13.05.16 at 16:50, <tamas@xxxxxxxxxxxxx> wrote:
> On Fri, May 13, 2016 at 6:00 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:
>>>>> On 12.05.16 at 17:25, <tamas@xxxxxxxxxxxxx> wrote:
>>> + if ( !rc )
>>> + mem_sharing_share_pages(d, bulk->start, sh, cd,
>>> bulk->start, ch);
>>
>> You shouldn't be ignoring errors here.
>
> The only error this function returns is if the sh/ch handles are
> invalid. However we obtained those just now from successful
> nominations, so we are guaranteed to have valid handles. This error
> checking is only important when nominations/sharing happen
> independently where the handle may go stale in-between. Here that is
> not possible.
You describe the state of things right now. What if someone
adds an error path to that function without inspecting all callers,
as it is clear that the function could have returned errors before.
Ignoring errors is plain bad. If you're sure there can't be errors,
at least add a respective ASSERT().
>>> + }
>>> +
>>> + ++(bulk->start);
>>
>> Pointless parentheses.
>
> Pointless but I prefer this style.
But it's in contrast to what we do almost everywhere else ...
>>> + /* Check for continuation if it's not the last iteration. */
>>> + if ( bulk->start < max && hypercall_preempt_check() )
>>
>> The loop head has <=; why < here?
>
> Because we only do preempt check if there are more then one pages left
> (as the comment states).
No, the comment says "last iteration", whereas the check is for
"next to last" (as the increment has already happened). And I
also don't see when between any two iterations preemption is
possible, just not between the second from last and the last one.
>>> + break;
>>> + }
>>> + }
>>> +
>>> + /* We only propagate -ENOMEM so reset rc here */
>>> + if ( rc < 0 && rc != -ENOMEM )
>>> + rc = 0;
>>
>> What's the rationale for discarding all other errors? At least the
>> patch description, but perhaps even the comment (which btw
>> is lacking a full stop) should be explaining this.
>
> The reason we swallow errors here other then ENOMEM is that it's quite
> possible that max_gpfn page is unsharable for example, thus rc would
> have an EINVAL final error value. However, we don't care about the
> success/fail of individual pages, we only care about the overall
> state. For that only ENOMEM is critical.
And you think no possible caller would care about the hypercall
reporting success yet not everything having got done that was
requested? Sounds strange to me, but as said - at least a bold
comment please.
>>> @@ -1468,6 +1505,69 @@ 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);
>>
>> Either you pass XENMEM_sharing_op_share here, or you need to
>> update xen/xsm/flask/policy/access_vectors (even if it's only a
>> comment which needs updating).
>
> Right, it should actually be sharing_op_share here.
>
>>
>> That said - are this and the similar pre-existing XSM checks actually
>> correct? I.e. is one of the two domains here really controlling the
>> other? I would have expected that a tool stack domain initiates the
>> sharing between two domains it controls...
>
> Not sure what was the original rationale behind it either.
Daniel - any opinion on this one?
>>> + if ( rc )
>>> + {
>>> + rcu_unlock_domain(cd);
>>> + goto out;
>>> + }
>>> +
>>> + if ( !mem_sharing_enabled(cd) )
>>> + {
>>> + rcu_unlock_domain(cd);
>>> + rc = -EINVAL;
>>> + goto out;
>>> + }
>>> +
>>> + if ( !atomic_read(&d->pause_count) ||
>>> + !atomic_read(&cd->pause_count) )
>>> + {
>>> + 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 )
>>
>> Why would the two domains need to agree in their maximum
>> GPFN? There's nothing similar in this file so far. Nor does the
>> right side of the || match anything pre-existing...
>
> The use-case for this function is to deduplicate identical VMs, not to
> blindly share pages across arbitrary domains. So this is a safety
> check to avoid accidentally running this function on domains that
> obviously are not identical. The right hand size is a safety check
> against not properly initialized input structs where the start point
> is obviously outside the memory of the domain.
Is that use case the only possible one, or merely the one you care
about? In the former case, I'd be okay as long as a respctive
comment got added.
>>> @@ -488,7 +489,18 @@ 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 { /* OP_BULK_SHARE */
>>> + uint64_aligned_t start; /* IN: start gfn. Set to 0 for
>>> + full deduplication. Field
>>> is
>>> + used internally and may
>>> change
>>> + when the hypercall
>>> returns. */
>>> + uint64_aligned_t shared; /* OUT: the number of gfns
>>> + that are shared after this
>>> + operation including pages
>>> + already shared before */
>>> + domid_t client_domain; /* IN: the client domain id */
>>> + } bulk;
>>
>> Let's not repeat pre-existing mistakes: There is explicit padding
>> missing here, which then also ought to be checked to be zero on
>> input.
>
> This struct is part of a union and is smaller then largest struct in
> the union, even with padding. So how would padding have any effect on
> anything?
Being able to use the space for future extensions without having
to bump the interface version. In domctl-s it's not as important
due to them being separately versioned, but anyway.
Jan
_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |