* [PATCHv1] grant-table: defer releasing pages acquired in a grant copy
@ 2015-01-13 10:46 David Vrabel
2015-01-15 16:18 ` Jan Beulich
0 siblings, 1 reply; 3+ messages in thread
From: David Vrabel @ 2015-01-13 10:46 UTC (permalink / raw)
To: xen-devel
Cc: Keir Fraser, Tim Deegan, David Vrabel, Jan Beulich, Ian Campbell
Acquiring a page for the source or destination of a grant copy is an
expensive operation. A common use case is for two adjacent grant copy
ops to operate on either the same source or the same destination page.
Instead of always acquiring and releasing destination and source pages
for each operation, release the page once it is no longer valid for
the next op.
If either the source or destination domains changes both pages are
released as it is unlikely that either will still be valid.
Netback uses grant copies in its guest Rx path and initial performance
measurements show significant gains in guest Rx from off-host (e.g.,
from 7.2 Gbit/s to 9.6 Gbit/s). This improves to 11 Gbit/s if netback
fully coaleceses to guest packets into the least number of ring slots.
Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
xen/common/grant_table.c | 347 +++++++++++++++++++++++++++-----------
xen/include/public/grant_table.h | 2 +-
2 files changed, 245 insertions(+), 104 deletions(-)
diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index fe52b63..6db46b4 100644
--- 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;
+ unsigned int len;
+
+ /* mapped etc. */
+ struct domain *domain;
+ unsigned long frame;
+ struct page_info *page;
+ void *virt;
+ bool_t have_grant;
+ bool_t have_type;
+};
+
+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:
+ 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 ) {
+ 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;
+}
+
+static void gnttab_copy_release_buf(struct gnttab_copy_buf *buf, bool_t read_only)
+{
+ if ( buf->virt )
{
- unsigned source_off, source_len;
- rc = __acquire_grant_for_copy(sd, op->source.u.ref,
- current->domain->domain_id, 1,
- &s_frame, &s_pg,
- &source_off, &source_len, 1);
- if ( rc != GNTST_okay )
- goto error_out;
- have_s_grant = 1;
- if ( op->source.offset < source_off ||
- op->len > source_len )
- PIN_FAIL(error_out, GNTST_general_error,
- "copy source out of bounds: %d < %d || %d > %d\n",
- op->source.offset, source_off,
- op->len, source_len);
+ unmap_domain_page(buf->virt);
+ buf->virt = NULL;
}
- else
+ if ( buf->have_type )
{
- rc = __get_paged_frame(op->source.u.gmfn, &s_frame, &s_pg, 1, sd);
- if ( rc != GNTST_okay )
- PIN_FAIL(error_out, rc,
- "source frame %lx invalid.\n", s_frame);
+ put_page_type(buf->page);
+ buf->have_type = 0;
}
+ if ( buf->page )
+ {
+ put_page(buf->page);
+ buf->page = NULL;
+ }
+ if ( buf->have_grant )
+ {
+ __release_grant_for_copy(buf->domain, buf->u.ref, read_only);
+ buf->have_grant = 0;
+ }
+}
+
+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);
+
+ if ( is_gref )
{
- unsigned dest_off, dest_len;
- rc = __acquire_grant_for_copy(dd, op->dest.u.ref,
- current->domain->domain_id, 0,
- &d_frame, &d_pg, &dest_off, &dest_len, 1);
+ rc = __acquire_grant_for_copy(buf->domain, ptr->u.ref,
+ current->domain->domain_id, read_only,
+ &buf->frame, &buf->page,
+ &buf->offset, &buf->len, 1);
if ( rc != GNTST_okay )
- goto error_out;
- have_d_grant = 1;
- if ( op->dest.offset < dest_off ||
- op->len > dest_len )
- PIN_FAIL(error_out, GNTST_general_error,
- "copy dest out of bounds: %d < %d || %d > %d\n",
- op->dest.offset, dest_off,
- op->len, dest_len);
+ goto out;
+ buf->u.ref = ptr->u.ref;
+ buf->have_grant = 1;
}
else
{
- rc = __get_paged_frame(op->dest.u.gmfn, &d_frame, &d_pg, 0, dd);
+ rc = __get_paged_frame(ptr->u.gmfn, &buf->frame, &buf->page, 1, buf->domain);
if ( rc != GNTST_okay )
- PIN_FAIL(error_out, rc,
- "destination frame %lx invalid.\n", d_frame);
+ PIN_FAIL(out, rc,
+ "source frame %lx invalid.\n", ptr->u.gmfn);
+
+ buf->u.gfn = ptr->u.gmfn;
+ buf->offset = 0;
+ buf->len = PAGE_SIZE;
}
- if ( !get_page_type(d_pg, PGT_writable_page) )
+ if ( !read_only )
{
- if ( !dd->is_dying )
- gdprintk(XENLOG_WARNING, "Could not get dst frame %lx\n", d_frame);
- rc = GNTST_general_error;
- goto error_out;
+ if ( !get_page_type(buf->page, PGT_writable_page) )
+ {
+ if ( !buf->domain->is_dying )
+ gdprintk(XENLOG_WARNING, "Could not get writable frame %lx\n", buf->frame);
+ rc = GNTST_general_error;
+ goto out;
+ }
+ buf->have_type = 1;
}
- sp = map_domain_page(s_frame);
- dp = map_domain_page(d_frame);
+ buf->virt = map_domain_page(buf->frame);
+ rc = GNTST_okay;
- memcpy(dp + op->dest.offset, sp + op->source.offset, op->len);
+out:
+ return rc;
+}
- unmap_domain_page(dp);
- unmap_domain_page(sp);
+static bool_t gnttab_copy_buf_valid(struct gnttab_copy *op,
+ struct gnttab_copy_ptr *p,
+ struct gnttab_copy_buf *b,
+ unsigned int gref_flag)
+{
+ if ( !b->virt )
+ return 0;
+ if ( op->flags & gref_flag )
+ return b->have_grant && p->u.ref == b->u.ref;
+ else
+ return p->u.gmfn == b->u.gfn;
+}
- gnttab_mark_dirty(dd, d_frame);
+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);
+
+ memcpy(dest->virt + op->dest.offset, src->virt + op->source.offset, op->len);
+ rc = GNTST_okay;
+out:
+ return rc;
}
-static long
-gnttab_copy(
+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 )
+ {
+ 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;
+ }
+
+ rc = gnttab_copy_buf(op, dest, src);
+out:
+ return rc;
+}
+
+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, };
+ long rc = 0;
for ( i = 0; i < count; i++ )
{
if (i && hypercall_preempt_check())
- return i;
+ {
+ rc = i;
+ break;
+ }
+
if ( unlikely(__copy_from_guest(&op, uop, 1)) )
- return -EFAULT;
- __gnttab_copy(&op);
+ {
+ rc = -EFAULT;
+ break;
+ }
+
+ op.status = gnttab_copy_one(&op, &dest, &src);
+ if ( op.status != GNTST_okay )
+ {
+ gnttab_copy_release_buf(&src, 1);
+ gnttab_copy_release_buf(&dest, 0);
+ }
+
if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
- return -EFAULT;
+ {
+ rc = -EFAULT;
+ break;
+ }
guest_handle_add_offset(uop, 1);
}
- return 0;
+
+ gnttab_copy_release_buf(&src, 1);
+ gnttab_copy_release_buf(&dest, 0);
+ gnttab_copy_unlock_domains(&src, &dest);
+
+ return rc;
}
static long
diff --git a/xen/include/public/grant_table.h b/xen/include/public/grant_table.h
index 20d4e77..c8619ed 100644
--- a/xen/include/public/grant_table.h
+++ b/xen/include/public/grant_table.h
@@ -453,7 +453,7 @@ DEFINE_XEN_GUEST_HANDLE(gnttab_transfer_t);
struct gnttab_copy {
/* IN parameters. */
- struct {
+ struct gnttab_copy_ptr {
union {
grant_ref_t ref;
xen_pfn_t gmfn;
--
1.7.10.4
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCHv1] grant-table: defer releasing pages acquired in a grant copy
2015-01-13 10:46 [PATCHv1] grant-table: defer releasing pages acquired in a grant copy David Vrabel
@ 2015-01-15 16:18 ` Jan Beulich
2015-01-16 10:42 ` David Vrabel
0 siblings, 1 reply; 3+ messages in thread
From: Jan Beulich @ 2015-01-15 16:18 UTC (permalink / raw)
To: David Vrabel; +Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
>>> On 13.01.15 at 11:46, <david.vrabel@citrix.com> 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
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCHv1] grant-table: defer releasing pages acquired in a grant copy
2015-01-15 16:18 ` Jan Beulich
@ 2015-01-16 10:42 ` David Vrabel
0 siblings, 0 replies; 3+ messages in thread
From: David Vrabel @ 2015-01-16 10:42 UTC (permalink / raw)
To: Jan Beulich, David Vrabel
Cc: xen-devel, Keir Fraser, Ian Campbell, Tim Deegan
On 15/01/15 16:18, Jan Beulich wrote:
>
> Wouldn't most of the changes up to here make a nice preparatory
> cleanup patch, easing review?
Yes, so...
> Leaving aside those mostly mechanical comments, content wise the
> patch looks good to me, but I'd hope for at least one other review.
,.. it might be best to wait for v2 before reviewing.
David
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-01-16 10:47 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-01-13 10:46 [PATCHv1] grant-table: defer releasing pages acquired in a grant copy David Vrabel
2015-01-15 16:18 ` Jan Beulich
2015-01-16 10:42 ` David Vrabel
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.