On Mon, Oct 12, 2015 at 12:42 AM, Jan Beulich <jbeul...@suse.com> wrote:
> >>> On 09.10.15 at 19:55, <ta...@tklengyel.com> wrote:
> > On Fri, Oct 9, 2015 at 1:51 AM, Jan Beulich <jbeul...@suse.com> wrote:
> >
> >> >>> On 08.10.15 at 22:57, <ta...@tklengyel.com> 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@lists.xen.org
http://lists.xen.org/xen-devel
