|
[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
|
 |
Lists.xenproject.org is hosted with RackSpace, monitoring our |