[PATCHv2 0/3] grant-table: optimize grant copies when crossing page boundaries

[PATCHv2 0/3] grant-table: optimize grant copies when crossing page boundaries

[David Vrabel]

Netback tries (where possible) to minimize the number of copy ops in
the to-guest direction (since acquiring a page is expensive) this
reduces the number acquire operations at the expensive of more Rx
slots and more frags in the guests.

By deferring the release of the pages we can optimize the case where
the grant copier splits a copy into two operations to cross a page
boundary (either to split a source GFN into two granted pages, or a
two source into the same granted page).

Changes in v2:
- Split refactor into separate patch.
- Use in struct grant_copy_ptr in struct grant_copy_buf.
- Various style fixes.

David

[PATCH 1/3] grant-table: use uint16_t consistently for grant copy offset and length

[David Vrabel]

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/common/grant_table.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index fe52b63..fb9d8f7 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1882,7 +1882,7 @@ static int
 __acquire_grant_for_copy(
     struct domain *rd, unsigned long gref, domid_t ldom, int readonly,
     unsigned long *frame, struct page_info **page, 
-    unsigned *page_off, unsigned *length, unsigned allow_transitive)
+    uint16_t *page_off, uint16_t *length, unsigned allow_transitive)
 {
     struct grant_table *rgt = rd->grant_table;
     grant_entry_v1_t *sha1;
@@ -1895,8 +1895,8 @@ __acquire_grant_for_copy(
     grant_ref_t trans_gref;
     struct domain *td;
     unsigned long grant_frame;
-    unsigned trans_page_off;
-    unsigned trans_length;
+    uint16_t trans_page_off;
+    uint16_t trans_length;
     int is_sub_page;
     s16 rc = GNTST_okay;
 
@@ -2122,7 +2122,7 @@ __gnttab_copy(
 
     if ( src_is_gref )
     {
-        unsigned source_off, source_len;
+        uint16_t source_off, source_len;
         rc = __acquire_grant_for_copy(sd, op->source.u.ref,
                                       current->domain->domain_id, 1,
                                       &s_frame, &s_pg,
@@ -2147,7 +2147,7 @@ __gnttab_copy(
 
     if ( dest_is_gref )
     {
-        unsigned dest_off, dest_len;
+        uint16_t 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);
-- 
1.7.10.4

[PATCH 2/3] grant-table: refactor grant copy to reduce duplicate code

[David Vrabel]

Much of the grant copy operation is identical for the source and
destination buffers.  Refactor the code into per-buffer functions.

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/common/grant_table.c         |  286 +++++++++++++++++++++++++-------------
 xen/include/public/grant_table.h |    2 +-
 2 files changed, 188 insertions(+), 100 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index fb9d8f7..770403b 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2077,139 +2077,225 @@ __acquire_grant_for_copy(
     return rc;
 }
 
-static void
-__gnttab_copy(
-    struct gnttab_copy *op)
-{
-    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;
+struct gnttab_copy_buf {
+    /* Guest provided. */
+    struct gnttab_copy_ptr ptr;
+    uint16_t len;
 
-    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");
+    /* Mapped etc. */
+    struct domain *domain;
+    unsigned long frame;
+    struct page_info *page;
+    void *virt;
+    bool_t is_ref;
+    bool_t read_only;
+    bool_t have_grant;
+    bool_t have_type;
+};
 
-    src_is_gref = op->flags & GNTCOPY_source_gref;
-    dest_is_gref = op->flags & GNTCOPY_dest_gref;
+static int gnttab_copy_lock_domain(const struct gnttab_copy *op,
+                                   domid_t domid, unsigned int gref_flag,
+                                   struct gnttab_copy_buf *buf)
+{
+    s16 rc;
 
-    if ( (op->source.domid != DOMID_SELF && !src_is_gref ) ||
-         (op->dest.domid   != DOMID_SELF && !dest_is_gref)   )
-        PIN_FAIL(error_out, GNTST_permission_denied,
+    if ( domid != DOMID_SELF && !(op->flags & gref_flag) )
+        PIN_FAIL(out, GNTST_permission_denied,
                  "only allow copy-by-mfn for DOMID_SELF.\n");
 
-    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);
+    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->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);
+    buf->ptr.domid = domid;
+    rc = GNTST_okay;
+ out:
+    return rc;
+}
 
-    rc = xsm_grant_copy(XSM_HOOK, sd, dd);
-    if ( rc )
+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 )
     {
-        rc = GNTST_permission_denied;
-        goto error_out;
+        rcu_unlock_domain(dest->domain);
+        dest->domain = NULL;
     }
+}
+
+static s16 gnttab_copy_lock_domains(const struct gnttab_copy *op,
+                                    struct gnttab_copy_buf *src,
+                                    struct gnttab_copy_buf *dest)
+{
+    s16 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;
 
-    if ( src_is_gref )
+    rc = xsm_grant_copy(XSM_HOOK, src->domain, dest->domain);
+    if ( rc < 0 )
     {
-        uint16_t 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);
+        gnttab_copy_unlock_domains(src, dest);
+        return GNTST_permission_denied;
     }
-    else
+
+    return 0;
+}
+
+static void gnttab_copy_release_buf(struct gnttab_copy_buf *buf)
+{
+    if ( buf->virt )
     {
-        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);
+        unmap_domain_page(buf->virt);
+        buf->virt = NULL;
     }
+    if ( buf->have_type )
+    {
+        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->ptr.u.ref, buf->read_only);
+        buf->have_grant = 0;
+    }
+}
+
+static s16 gnttab_copy_claim_buf(
+    const struct gnttab_copy *op,
+    const struct gnttab_copy_ptr *ptr,
+    struct gnttab_copy_buf *buf,
+    unsigned int gref_flag)
+{
+    s16 rc;
+
+    buf->read_only = gref_flag == GNTCOPY_source_gref;
 
-    if ( dest_is_gref )
+    if ( op->flags & gref_flag )
     {
-        uint16_t 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, buf->read_only,
+                                      &buf->frame, &buf->page,
+                                      &buf->ptr.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->ptr.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->ptr.u.gmfn = ptr->u.gmfn;
+        buf->ptr.offset = 0;
+        buf->len = PAGE_SIZE;
     }
 
-    if ( !get_page_type(d_pg, PGT_writable_page) )
+    if ( !buf->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 int gnttab_copy_buf(const struct gnttab_copy *op,
+                           struct gnttab_copy_buf *dest,
+                           const struct gnttab_copy_buf *src)
+{
+    s16 rc;
 
-    gnttab_mark_dirty(dd, d_frame);
+    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->ptr.offset ||
+         op->source.offset + op->len > src->ptr.offset + src->len )
+        PIN_FAIL(out, GNTST_general_error,
+                 "copy source out of bounds: %d < %d || %d > %d\n",
+                 op->source.offset, src->ptr.offset,
+                 op->len, src->len);
+
+    if ( op->dest.offset < dest->ptr.offset ||
+         op->dest.offset + op->len > dest->ptr.offset + dest->len )
+        PIN_FAIL(out, GNTST_general_error,
+                 "copy dest out of bounds: %d < %d || %d > %d\n",
+                 op->dest.offset, dest->ptr.offset,
+                 op->len, dest->len);
+
+    memcpy(dest->virt + op->dest.offset, src->virt + op->source.offset, op->len);
+    rc = GNTST_okay;
+ out:
+    return 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;
+static s16 gnttab_copy_one(const struct gnttab_copy *op,
+                           struct gnttab_copy_buf *dest,
+                           struct gnttab_copy_buf *src)
+{
+    s16 rc;
+
+    rc = gnttab_copy_lock_domains(op, src, dest);
+    if ( rc < 0 )
+        goto out;
+
+    rc = gnttab_copy_claim_buf(op, &op->source, src, GNTCOPY_source_gref);
+    if ( rc < 0 )
+        goto out;
+
+    rc = gnttab_copy_claim_buf(op, &op->dest, dest, GNTCOPY_dest_gref);
+    if ( rc < 0 )
+        goto out;
+
+    rc = gnttab_copy_buf(op, dest, src);
+ out:
+    gnttab_copy_release_buf(src);
+    gnttab_copy_release_buf(dest);
+    gnttab_copy_unlock_domains(src, dest);
+    return rc;
 }
 
-static long
-gnttab_copy(
+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 = {};
+    struct gnttab_copy_buf dest = {};
 
     for ( i = 0; i < count; i++ )
     {
@@ -2217,7 +2303,9 @@ gnttab_copy(
             return i;
         if ( unlikely(__copy_from_guest(&op, uop, 1)) )
             return -EFAULT;
-        __gnttab_copy(&op);
+
+        op.status = gnttab_copy_one(&op, &dest, &src);
+
         if ( unlikely(__copy_field_to_guest(uop, &op, status)) )
             return -EFAULT;
         guest_handle_add_offset(uop, 1);
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

[PATCH 3/3] grant-table: defer releasing pages acquired in a grant copy

[David Vrabel]

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.

XenServer's performance benchmarks show modest improvements in network
receive throughput (netback uses grant copy in the guest Rx path) and
no regressions in disk performamnce (using tapdisk3 which grant copies
as the backend).

                         Baseline   Deferred Release
Interhost receive to VM   7.2 Gb/s  ~9 Gbit/s
Interhost aggregate      24 Gb/s    28 Gb/s
Intrahost single stream  14 Gb/s    14 Gb/s
Intrahost aggregate      34 Gb/s    36 Gb/s
Aggregate disk write    900 MB/s   900 MB/s
Aggregate disk read     890 MB/s   890 MB/s

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
---
 xen/common/grant_table.c |   80 ++++++++++++++++++++++++++++++++++++----------
 1 file changed, 64 insertions(+), 16 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index 770403b..e9a7320 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -2233,6 +2233,18 @@ static s16 gnttab_copy_claim_buf(
     return rc;
 }
 
+static bool_t gnttab_copy_buf_valid(const struct gnttab_copy *op,
+                                    const struct gnttab_copy_ptr *p,
+                                    const 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->ptr.u.ref;
+    return p->u.gmfn == b->ptr.u.gmfn;
+}
+
 static int gnttab_copy_buf(const struct gnttab_copy *op,
                            struct gnttab_copy_buf *dest,
                            const struct gnttab_copy_buf *src)
@@ -2269,23 +2281,38 @@ static s16 gnttab_copy_one(const struct gnttab_copy *op,
 {
     s16 rc;
 
-    rc = gnttab_copy_lock_domains(op, src, dest);
-    if ( rc < 0 )
-        goto out;
+    if ( !src->domain || op->source.domid != src->ptr.domid ||
+         !dest->domain || op->dest.domid != dest->ptr.domid )
+    {
+        gnttab_copy_release_buf(src);
+        gnttab_copy_release_buf(dest);
+        gnttab_copy_unlock_domains(src, dest);
 
-    rc = gnttab_copy_claim_buf(op, &op->source, src, GNTCOPY_source_gref);
-    if ( rc < 0 )
-        goto out;
+        rc = gnttab_copy_lock_domains(op, src, dest);
+        if ( rc < 0 )
+            goto out;
+    }
 
-    rc = gnttab_copy_claim_buf(op, &op->dest, dest, GNTCOPY_dest_gref);
-    if ( rc < 0 )
-        goto out;
+    /* Different source? */
+    if ( !gnttab_copy_buf_valid(op, &op->source, src, GNTCOPY_source_gref) )
+    {
+        gnttab_copy_release_buf(src);
+        rc = gnttab_copy_claim_buf(op, &op->source, src, GNTCOPY_source_gref);
+        if ( rc < 0 )
+            goto out;
+    }
+
+    /* Different dest? */
+    if ( !gnttab_copy_buf_valid(op, &op->dest, dest, GNTCOPY_dest_gref) )
+    {
+        gnttab_copy_release_buf(dest);
+        rc = gnttab_copy_claim_buf(op, &op->dest, dest, GNTCOPY_dest_gref);
+        if ( rc < 0 )
+            goto out;
+    }
 
     rc = gnttab_copy_buf(op, dest, src);
  out:
-    gnttab_copy_release_buf(src);
-    gnttab_copy_release_buf(dest);
-    gnttab_copy_unlock_domains(src, dest);
     return rc;
 }
 
@@ -2296,21 +2323,42 @@ static long gnttab_copy(
     struct gnttab_copy op;
     struct gnttab_copy_buf src = {};
     struct gnttab_copy_buf dest = {};
+    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;
+        {
+            rc = -EFAULT;
+            break;
+        }
 
         op.status = gnttab_copy_one(&op, &dest, &src);
+        if ( op.status != GNTST_okay )
+        {
+            gnttab_copy_release_buf(&src);
+            gnttab_copy_release_buf(&dest);
+        }
 
         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);
+    gnttab_copy_release_buf(&dest);
+    gnttab_copy_unlock_domains(&src, &dest);
+
+    return rc;
 }
 
 static long
-- 
1.7.10.4

Re: [PATCH 2/3] grant-table: refactor grant copy to reduce duplicate code

[Jan Beulich]

>>> On 20.01.15 at 19:19, <david.vrabel@citrix.com> wrote:
> +static int gnttab_copy_buf(const struct gnttab_copy *op,
> +                           struct gnttab_copy_buf *dest,
> +                           const struct gnttab_copy_buf *src)
> +{
> +    s16 rc;

An s16 local variable used as return value in a function returning
int is kind of odd. Elsewhere there are also cases where the
function return types are also s16. I don't think that's particularly
efficient, and hence I think it should be changed in the places
where you add new helpers even if the original code used s16
for that purpose.

> -    gnttab_mark_dirty(dd, d_frame);

This one doesn't reappear anywhere.

Jan

Re: [PATCH 1/3] grant-table: use uint16_t consistently for grant copy offset and length

[Jan Beulich]

>>> On 20.01.15 at 19:19, <david.vrabel@citrix.com> wrote:
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Acked-by: Jan Beulich <jbeulich@suse.com>

Re: [PATCH 3/3] grant-table: defer releasing pages acquired in a grant copy

[Jan Beulich]

>>> On 20.01.15 at 19:19, <david.vrabel@citrix.com> wrote:
> 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.
> 
> XenServer's performance benchmarks show modest improvements in network
> receive throughput (netback uses grant copy in the guest Rx path) and
> no regressions in disk performamnce (using tapdisk3 which grant copies
> as the backend).
> 
>                          Baseline   Deferred Release
> Interhost receive to VM   7.2 Gb/s  ~9 Gbit/s
> Interhost aggregate      24 Gb/s    28 Gb/s
> Intrahost single stream  14 Gb/s    14 Gb/s
> Intrahost aggregate      34 Gb/s    36 Gb/s
> Aggregate disk write    900 MB/s   900 MB/s
> Aggregate disk read     890 MB/s   890 MB/s
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Reviewed-by: Jan Beulich <jbeulich@suse.com>
albeit I wonder ...

> --- a/xen/common/grant_table.c
> +++ b/xen/common/grant_table.c
> @@ -2233,6 +2233,18 @@ static s16 gnttab_copy_claim_buf(
>      return rc;
>  }
>  
> +static bool_t gnttab_copy_buf_valid(const struct gnttab_copy *op,
> +                                    const struct gnttab_copy_ptr *p,
> +                                    const 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->ptr.u.ref;
> +    return p->u.gmfn == b->ptr.u.gmfn;
> +}

... whether you wouldn't better fold the op and gref_flag parameters
into one, as they're used only once and only together.

Jan

Re: [PATCH 3/3] grant-table: defer releasing pages acquired in a grant copy

[David Vrabel]

On 22/01/15 14:34, Jan Beulich wrote:
>>>> On 20.01.15 at 19:19, <david.vrabel@citrix.com> wrote:
>> 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.
>>
>> XenServer's performance benchmarks show modest improvements in network
>> receive throughput (netback uses grant copy in the guest Rx path) and
>> no regressions in disk performamnce (using tapdisk3 which grant copies
>> as the backend).
[...]
>> +static bool_t gnttab_copy_buf_valid(const struct gnttab_copy *op,
>> +                                    const struct gnttab_copy_ptr *p,
>> +                                    const 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->ptr.u.ref;
>> +    return p->u.gmfn == b->ptr.u.gmfn;
>> +}
> 
> ... whether you wouldn't better fold the op and gref_flag parameters
> into one, as they're used only once and only together.

Do you mean something like?

static bool_t gnttab_copy_buf_valid(const struct gnttab_copy_ptr *p,
                                   const struct gnttab_copy_buf *b,
                                   bool_t is_gref)

David

Re: [PATCH 2/3] grant-table: refactor grant copy to reduce duplicate code

[David Vrabel]

On 22/01/15 14:24, Jan Beulich wrote:
>>>> On 20.01.15 at 19:19, <david.vrabel@citrix.com> wrote:
>> +static int gnttab_copy_buf(const struct gnttab_copy *op,
>> +                           struct gnttab_copy_buf *dest,
>> +                           const struct gnttab_copy_buf *src)
>> +{
>> +    s16 rc;
> 
> An s16 local variable used as return value in a function returning
> int is kind of odd. Elsewhere there are also cases where the
> function return types are also s16. I don't think that's particularly
> efficient, and hence I think it should be changed in the places
> where you add new helpers even if the original code used s16
> for that purpose.

I was (trying to) use s16 where we return a GNTST_* value instead of the
usual -ERRNO.  I can change them all to int if this is preferred
(although I not sure I get your efficiency argument).

>> -    gnttab_mark_dirty(dd, d_frame);
> 
> This one doesn't reappear anywhere.

Oops!

David

Re: [PATCH 3/3] grant-table: defer releasing pages acquired in a grant copy

[Jan Beulich]

>>> On 22.01.15 at 15:39, <david.vrabel@citrix.com> wrote:
> On 22/01/15 14:34, Jan Beulich wrote:
>>>>> On 20.01.15 at 19:19, <david.vrabel@citrix.com> wrote:
>>> 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.
>>>
>>> XenServer's performance benchmarks show modest improvements in network
>>> receive throughput (netback uses grant copy in the guest Rx path) and
>>> no regressions in disk performamnce (using tapdisk3 which grant copies
>>> as the backend).
> [...]
>>> +static bool_t gnttab_copy_buf_valid(const struct gnttab_copy *op,
>>> +                                    const struct gnttab_copy_ptr *p,
>>> +                                    const 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->ptr.u.ref;
>>> +    return p->u.gmfn == b->ptr.u.gmfn;
>>> +}
>> 
>> ... whether you wouldn't better fold the op and gref_flag parameters
>> into one, as they're used only once and only together.
> 
> Do you mean something like?
> 
> static bool_t gnttab_copy_buf_valid(const struct gnttab_copy_ptr *p,
>                                    const struct gnttab_copy_buf *b,
>                                    bool_t is_gref)

Yes.

Jan

Re: [PATCH 2/3] grant-table: refactor grant copy to reduce duplicate code

[Jan Beulich]

>>> On 22.01.15 at 15:42, <david.vrabel@citrix.com> wrote:
> On 22/01/15 14:24, Jan Beulich wrote:
>>>>> On 20.01.15 at 19:19, <david.vrabel@citrix.com> wrote:
>>> +static int gnttab_copy_buf(const struct gnttab_copy *op,
>>> +                           struct gnttab_copy_buf *dest,
>>> +                           const struct gnttab_copy_buf *src)
>>> +{
>>> +    s16 rc;
>> 
>> An s16 local variable used as return value in a function returning
>> int is kind of odd. Elsewhere there are also cases where the
>> function return types are also s16. I don't think that's particularly
>> efficient, and hence I think it should be changed in the places
>> where you add new helpers even if the original code used s16
>> for that purpose.
> 
> I was (trying to) use s16 where we return a GNTST_* value instead of the
> usual -ERRNO.  I can change them all to int if this is preferred
> (although I not sure I get your efficiency argument).

Operations on 16-bit registers are less efficient on x86 (due to
the extra operand size prefix byte). On ARM I don't even know
how they get carried out by the compiler, but there surely is no
way to access the low 16 bits of a register by other than
memory loads or stores.

Jan

Re: [PATCH 2/3] grant-table: refactor grant copy to reduce duplicate code

[Tim Deegan]

Hi,

At 18:19 +0000 on 20 Jan (1421774388), David Vrabel wrote:
> +static s16 gnttab_copy_lock_domains(const struct gnttab_copy *op,
> +                                    struct gnttab_copy_buf *src,
> +                                    struct gnttab_copy_buf *dest)
> +{
> +    s16 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;
>  
> -    if ( src_is_gref )
> +    rc = xsm_grant_copy(XSM_HOOK, src->domain, dest->domain);
> +    if ( rc < 0 )
>      {
[...]
> +        gnttab_copy_unlock_domains(src, dest);
> +        return GNTST_permission_denied;

This error path unwinds the locks we've already taken, where the one
above (failing to lock dest) doesn't.  AFAICS this is OK because the
only caller always unconditionally calls unlock, but they ought to be
consistent.

> +static s16 gnttab_copy_claim_buf(
> +    const struct gnttab_copy *op,
> +    const struct gnttab_copy_ptr *ptr,
> +    struct gnttab_copy_buf *buf,
> +    unsigned int gref_flag)
> +{
> +    s16 rc;
> +
> +    buf->read_only = gref_flag == GNTCOPY_source_gref;
>  
> -    if ( dest_is_gref )
> +    if ( op->flags & gref_flag )
>      {
[...]
> +        rc = __acquire_grant_for_copy(buf->domain, ptr->u.ref,
> +                                      current->domain->domain_id, buf->read_only,
> +                                      &buf->frame, &buf->page,
> +                                      &buf->ptr.offset, &buf->len, 1);
>          if ( rc != GNTST_okay )
[...]
> +            goto out;
> +        buf->ptr.u.ref = ptr->u.ref;
> +        buf->have_grant = 1;
>      }
>      else
>      {
[...]
> +        rc = __get_paged_frame(ptr->u.gmfn, &buf->frame, &buf->page, 1, buf->domain);

AFAICS this should pass buf->readonly as arg 4, so as not to break
copy-to-MFN.

Apart from that, and the mark-dirty that Jan spotted, look good to me.

Cheers,

Tim.

Re: [PATCH 3/3] grant-table: defer releasing pages acquired in a grant copy

[Tim Deegan]

At 18:19 +0000 on 20 Jan (1421774389), David Vrabel wrote:
> 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.
> 
> XenServer's performance benchmarks show modest improvements in network
> receive throughput (netback uses grant copy in the guest Rx path) and
> no regressions in disk performamnce (using tapdisk3 which grant copies
> as the backend).
> 
>                          Baseline   Deferred Release
> Interhost receive to VM   7.2 Gb/s  ~9 Gbit/s
> Interhost aggregate      24 Gb/s    28 Gb/s
> Intrahost single stream  14 Gb/s    14 Gb/s
> Intrahost aggregate      34 Gb/s    36 Gb/s
> Aggregate disk write    900 MB/s   900 MB/s
> Aggregate disk read     890 MB/s   890 MB/s
> 
> Signed-off-by: David Vrabel <david.vrabel@citrix.com>

Reviewed-by: Tim Deegan <tim@xen.org>

Though if you're doing a v3...

>      for ( i = 0; i < count; i++ )
>      {
>          if (i && hypercall_preempt_check())

while you're here, you could add the missing spaces to this if ( ).

Cheers,

Tim.

[PATCH 1/3] grant-table: use uint16_t consistently for grant copy offset and length

[David Vrabel]

Signed-off-by: David Vrabel <david.vrabel@citrix.com>
Acked-by: Jan Beulich <jbeulich@suse.com>
---
 xen/common/grant_table.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/xen/common/grant_table.c b/xen/common/grant_table.c
index fe52b63..fb9d8f7 100644
--- a/xen/common/grant_table.c
+++ b/xen/common/grant_table.c
@@ -1882,7 +1882,7 @@ static int
 __acquire_grant_for_copy(
     struct domain *rd, unsigned long gref, domid_t ldom, int readonly,
     unsigned long *frame, struct page_info **page, 
-    unsigned *page_off, unsigned *length, unsigned allow_transitive)
+    uint16_t *page_off, uint16_t *length, unsigned allow_transitive)
 {
     struct grant_table *rgt = rd->grant_table;
     grant_entry_v1_t *sha1;
@@ -1895,8 +1895,8 @@ __acquire_grant_for_copy(
     grant_ref_t trans_gref;
     struct domain *td;
     unsigned long grant_frame;
-    unsigned trans_page_off;
-    unsigned trans_length;
+    uint16_t trans_page_off;
+    uint16_t trans_length;
     int is_sub_page;
     s16 rc = GNTST_okay;
 
@@ -2122,7 +2122,7 @@ __gnttab_copy(
 
     if ( src_is_gref )
     {
-        unsigned source_off, source_len;
+        uint16_t source_off, source_len;
         rc = __acquire_grant_for_copy(sd, op->source.u.ref,
                                       current->domain->domain_id, 1,
                                       &s_frame, &s_pg,
@@ -2147,7 +2147,7 @@ __gnttab_copy(
 
     if ( dest_is_gref )
     {
-        unsigned dest_off, dest_len;
+        uint16_t 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);
-- 
1.7.10.4