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

[Tamas K Lengyel <tamas@xxxxxxxxxxxxx>]

On Mon, Oct 12, 2015 at 12:42 AM, Jan Beulich <JBeulich@xxxxxxxx> wrote:

>>> On 09.10.15 at 19:55, <tamas@xxxxxxxxxxxxx> wrote:
> 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.

Well, it's code you're the maintainer for, so you know, but generally
I think parentheses should be used to clarify things, not to make
code harder to read (the effect is minimal here, but extended to a
more complex _expression_ this may well matter).

It might just be me but that's exactly what the parentheses do here. I tend to read operation from left-to-right and the ++ on the left actually is the last operation which will be performed after the pointer dereference. The parentheses make that explicit.

Â

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

But you're actually abusing the int nature of rc in the caller to store
a boolean value. I'd really like to see this be done consistently -
either use another variable (or, as suggested, no intermediate
variable in the caller at all), or (also taking into consideration Andrew's
comments) propagate an actual -E value from here (along with not
losing errors).

So the way we return >0 values is copied from the hypercall continuation handler of mem_access - there it returns the GFN value which was the same approach I used in the first version of this patch. Now since we store the gfn value in the mso struct itself, this just returns an indicator that there is more work to be done. Otherwise the logic and setup is the same. I rather not mix these as long as mem_access does it similarly (returning a positive value indicating more work is to be done).

Â

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

As said -Â in that case, please enforce this (by bailing if not so).

Ack.

Â

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

I tend to disagree, together wit OP_BULK_SHARE the structure
name doesn't need to be mode specific - as said, it can easily serve
for all kinds of bulk operations.

Sure, I'll change it.

Â

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

I think our annotations are pretty consistent about marking
modified fields as IN/OUT. (Otoh we don't normally modify fields
when their value is of no relevance to the caller.)

Making it an IN/OUT would suggest it has a value that the user could need - which is not the case. I can describe this in the header that the field is used internally and the value may not be the same after the hypercall finished but the value it changed to has no particular importance for the caller.

Tamas

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