[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [Xen-devel] [PATCHv1] grant-table: defer releasing pages acquired in a grant copy



>>> On 13.01.15 at 11:46, <david.vrabel@xxxxxxxxxx> wrote:
> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2077,152 +2077,293 @@ __acquire_grant_for_copy(
>      return rc;
>  }
>  
> -static void
> -__gnttab_copy(
> -    struct gnttab_copy *op)
> +struct gnttab_copy_buf {
> +    /* guest provided. */
> +    domid_t domid;
> +    bool_t is_ref;
> +    union {
> +        grant_ref_t ref;
> +        xen_pfn_t gfn;
> +    } u;
> +    unsigned int offset;

So why are the respective parts above not simply an instance
of struct gnttab_copy_ptr?

> +static int gnttab_copy_lock_domain(struct gnttab_copy *op,
> +                                   domid_t domid, unsigned int gref_flag,
> +                                   struct gnttab_copy_buf *buf)
>  {
> -    struct domain *sd = NULL, *dd = NULL;
> -    unsigned long s_frame, d_frame;
> -    struct page_info *s_pg = NULL, *d_pg = NULL;
> -    char *sp, *dp;
> -    s16 rc = GNTST_okay;
> -    int have_d_grant = 0, have_s_grant = 0;
> -    int src_is_gref, dest_is_gref;
> +    s16 rc;
>  
> -    if ( ((op->source.offset + op->len) > PAGE_SIZE) ||
> -         ((op->dest.offset + op->len) > PAGE_SIZE) )
> -        PIN_FAIL(error_out, GNTST_bad_copy_arg, "copy beyond page area.\n");
> +    if ( domid != DOMID_SELF && !(op->flags & gref_flag) )
> +        PIN_FAIL(out, GNTST_permission_denied,
> +                 "only allow copy-by-mfn for DOMID_SELF.\n");
>  
> -    src_is_gref = op->flags & GNTCOPY_source_gref;
> -    dest_is_gref = op->flags & GNTCOPY_dest_gref;
> +    if ( domid == DOMID_SELF )
> +        buf->domain = rcu_lock_current_domain();
> +    else 
> +    {
> +        buf->domain = rcu_lock_domain_by_id(domid);
> +        if ( buf->domain == NULL )
> +            PIN_FAIL(out, GNTST_bad_domain,
> +                     "couldn't find %d\n", op->source.domid);
> +    }
>  
> -    if ( (op->source.domid != DOMID_SELF && !src_is_gref ) ||
> -         (op->dest.domid   != DOMID_SELF && !dest_is_gref)   )
> -        PIN_FAIL(error_out, GNTST_permission_denied,
> -                 "only allow copy-by-mfn for DOMID_SELF.\n");
> +    buf->domid = domid;
> +    rc = GNTST_okay;
> +out:

Labels should be indented by at least one space.

> +    return rc;
> +}
>  
> -    if ( op->source.domid == DOMID_SELF )
> -        sd = rcu_lock_current_domain();
> -    else if ( (sd = rcu_lock_domain_by_id(op->source.domid)) == NULL )
> -        PIN_FAIL(error_out, GNTST_bad_domain,
> -                 "couldn't find %d\n", op->source.domid);
> +static void gnttab_copy_unlock_domains(struct gnttab_copy_buf *src,
> +                                       struct gnttab_copy_buf *dest)
> +{
> +    if ( src->domain )
> +    {
> +        rcu_unlock_domain(src->domain);
> +        src->domain = NULL;
> +    }
> +    if ( dest->domain ) {

Figure brace goes on a separate line.

> +        rcu_unlock_domain(dest->domain);
> +        dest->domain = NULL;
> +    }
> +}
>  
> -    if ( op->dest.domid == DOMID_SELF )
> -        dd = rcu_lock_current_domain();
> -    else if ( (dd = rcu_lock_domain_by_id(op->dest.domid)) == NULL )
> -        PIN_FAIL(error_out, GNTST_bad_domain,
> -                 "couldn't find %d\n", op->dest.domid);
> +static s16 gnttab_copy_lock_domains(struct gnttab_copy *op,
> +                                    struct gnttab_copy_buf *src,
> +                                    struct gnttab_copy_buf *dest)
> +{
> +    s16 rc;
>  
> -    rc = xsm_grant_copy(XSM_HOOK, sd, dd);
> -    if ( rc )
> +    rc = gnttab_copy_lock_domain(op, op->source.domid, GNTCOPY_source_gref, 
> src);
> +    if ( rc < 0 )
> +        return rc;
> +    rc = gnttab_copy_lock_domain(op, op->dest.domid, GNTCOPY_dest_gref, 
> dest);
> +    if ( rc < 0 )
> +        return rc;
> +    
> +    rc = xsm_grant_copy(XSM_HOOK, src->domain, dest->domain);
> +    if ( rc < 0 )
>      {
> -        rc = GNTST_permission_denied;
> -        goto error_out;
> +        gnttab_copy_unlock_domains(src, dest);
> +        return GNTST_permission_denied;
>      }
>  
> -    if ( src_is_gref )
> +    return 0;
> +}

Wouldn't most of the changes up to here make a nice preparatory
cleanup patch, easing review?

> +static s16 gnttab_copy_claim_buf(
> +    struct gnttab_copy *op,
> +    struct gnttab_copy_ptr *ptr,
> +    struct gnttab_copy_buf *buf,
> +    bool_t read_only)
> +{
> +    unsigned int is_gref;
> +    s16 rc;
>  
> -    if ( dest_is_gref )
> +    is_gref = op->flags & (read_only ? GNTCOPY_source_gref : 
> GNTCOPY_dest_gref);

Elsewhere you have this mask passed as gref_flag - perhaps for
consistency this should be done here too replacing the read_only
parameter)?

> +static bool_t gnttab_copy_buf_valid(struct gnttab_copy *op,
> +                                    struct gnttab_copy_ptr *p,
> +                                    struct gnttab_copy_buf *b,

Constify as much of these as possible?

> +                                    unsigned int gref_flag)
> +{
> +    if ( !b->virt )
> +        return 0;
> +    if ( op->flags & gref_flag )

If this is not an "else if" ...

> +        return b->have_grant && p->u.ref == b->u.ref;
> +    else

... it's inconsistent to use "else" here.

> +static int gnttab_copy_buf(struct gnttab_copy *op,
> +                           struct gnttab_copy_buf *dest,
> +                           struct gnttab_copy_buf *src)
> +{
> +    s16 rc;
>  
> -    put_page_type(d_pg);
> - error_out:
> -    if ( d_pg )
> -        put_page(d_pg);
> -    if ( s_pg )
> -        put_page(s_pg);
> -    if ( have_s_grant )
> -        __release_grant_for_copy(sd, op->source.u.ref, 1);
> -    if ( have_d_grant )
> -        __release_grant_for_copy(dd, op->dest.u.ref, 0);
> -    if ( sd )
> -        rcu_unlock_domain(sd);
> -    if ( dd )
> -        rcu_unlock_domain(dd);
> -    op->status = rc;
> +    if ( ((op->source.offset + op->len) > PAGE_SIZE) ||
> +         ((op->dest.offset + op->len) > PAGE_SIZE) )
> +        PIN_FAIL(out, GNTST_bad_copy_arg, "copy beyond page area.\n");
> +
> +    if ( op->source.offset < src->offset ||
> +         op->source.offset + op->len > src->offset + src->len )
> +        PIN_FAIL(out, GNTST_general_error,
> +                 "copy source out of bounds: %d < %d || %d > %d\n",
> +                 op->source.offset, src->offset,
> +                 op->len, src->len);
> +
> +    if ( op->dest.offset < dest->offset ||
> +         op->dest.offset + op->len > dest->offset + dest->len )
> +        PIN_FAIL(out, GNTST_general_error,
> +                 "copy dest out of bounds: %d < %d || %d > %d\n",
> +                 op->dest.offset, dest->offset,
> +                     op->len, dest->len);

Indentation appears screwed up here.

> +static s16 gnttab_copy_one(struct gnttab_copy *op,
> +                           struct gnttab_copy_buf *dest,
> +                           struct gnttab_copy_buf *src)
> +{
> +    s16 rc;
> +
> +    if ( !src->domain || op->source.domid != src->domid
> +         || !dest->domain || op->dest.domid != dest->domid )

While personally I prefer it this way, consistency calls for the || to
be at the end of the first line rather than at the beginning of the
second one.

> +    {
> +        gnttab_copy_release_buf(src, 1);
> +        gnttab_copy_release_buf(dest, 0);
> +        gnttab_copy_unlock_domains(src, dest);
> +
> +        rc = gnttab_copy_lock_domains(op, src, dest);
> +        if ( rc < 0 )
> +            goto out;
> +    }
> +
> +    /* Different source? */
> +    if ( !gnttab_copy_buf_valid(op, &op->source, src, GNTCOPY_source_gref) )
> +    {
> +        gnttab_copy_release_buf(src, 1);
> +        rc = gnttab_copy_claim_buf(op, &op->source, src, 1);
> +        if ( rc < 0 )
> +            goto out;
> +    }
> +
> +    /* Different dest? */
> +    if ( !gnttab_copy_buf_valid(op, &op->dest, dest, GNTCOPY_dest_gref) )
> +    {
> +        gnttab_copy_release_buf(dest, 0);
> +        rc = gnttab_copy_claim_buf(op, &op->dest, dest, 0);
> +        if ( rc < 0 )
> +            goto out;
> +    }

Aren't these latter two if()-s effectively needed in the else case of
the first if()?

> +static long gnttab_copy(
>      XEN_GUEST_HANDLE_PARAM(gnttab_copy_t) uop, unsigned int count)
>  {
> -    int i;
> +    unsigned int i;
>      struct gnttab_copy op;
> +    struct gnttab_copy_buf src = { 0, };
> +    struct gnttab_copy_buf dest = { 0, };

Just {} will do.

Leaving aside those mostly mechanical comments, content wise the
patch looks good to me, but I'd hope for at least one other review.

Jan

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


 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.