Re: [PATCH v3 1/2] Interface for grant copy operation in libs.

[Wei Liu <wei.liu2@citrix.com>]

On Wed, Jun 22, 2016 at 01:37:50PM +0100, David Vrabel wrote:
> On 22/06/16 12:21, Wei Liu wrote:
> > On Wed, Jun 22, 2016 at 10:37:24AM +0100, David Vrabel wrote:
> >> On 22/06/16 09:38, Paulina Szubarczyk wrote:
> >>> In a linux part an ioctl(gntdev, IOCTL_GNTDEV_GRANT_COPY, ..)
> >>> system call is invoked. In mini-os the operation is yet not
> >>> implemented. For other OSs there is a dummy implementation.
> >> [...]
> >>> --- a/tools/libs/gnttab/linux.c
> >>> +++ b/tools/libs/gnttab/linux.c
> >>> @@ -235,6 +235,51 @@ int osdep_gnttab_unmap(xengnttab_handle *xgt,
> >>>      return 0;
> >>>  }
> >>>  
> >>> +int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
> >>> +                            uint32_t count,
> >>> +                            xengnttab_grant_copy_segment_t *segs)
> >>> +{
> >>> +    int i, rc;
> >>> +    int fd = xgt->fd;
> >>> +    struct ioctl_gntdev_grant_copy copy;
> >>> +
> >>> +    copy.segments = calloc(count, sizeof(struct ioctl_gntdev_grant_copy_segment));
> >>> +    copy.count = count;
> >>> +    for (i = 0; i < count; i++)
> >>> +    {
> >>> +        copy.segments[i].flags = segs[i].flags;
> >>> +        copy.segments[i].len = segs[i].len;
> >>> +        if (segs[i].flags == GNTCOPY_dest_gref) 
> >>> +        {
> >>> +            copy.segments[i].dest.foreign.ref = segs[i].dest.foreign.ref;
> >>> +            copy.segments[i].dest.foreign.domid = segs[i].dest.foreign.domid;
> >>> +            copy.segments[i].dest.foreign.offset = segs[i].dest.foreign.offset;
> >>> +            copy.segments[i].source.virt = segs[i].source.virt;
> >>> +        } 
> >>> +        else 
> >>> +        {
> >>> +            copy.segments[i].source.foreign.ref = segs[i].source.foreign.ref;
> >>> +            copy.segments[i].source.foreign.domid = segs[i].source.foreign.domid;
> >>> +            copy.segments[i].source.foreign.offset = segs[i].source.foreign.offset;
> >>> +            copy.segments[i].dest.virt = segs[i].dest.virt;
> >>> +        }
> >>> +    }
> >>> +
> >>> +    rc = ioctl(fd, IOCTL_GNTDEV_GRANT_COPY, &copy);
> >>> +    if (rc) 
> >>> +    {
> >>> +        GTERROR(xgt->logger, "ioctl GRANT COPY failed %d ", errno);
> >>> +    }
> >>> +    else 
> >>> +    {
> >>> +        for (i = 0; i < count; i++)
> >>> +            segs[i].status = copy.segments[i].status;
> >>> +    }
> >>> +
> >>> +    free(copy.segments);
> >>> +    return rc;
> >>> +}
> >>
> >> I know Wei asked for this but you've replaced what should be a single
> >> pointer assignment with a memory allocation and two loops over all the
> >> segments.
> >>
> >> This is a hot path and the two structures (the libxengnttab one and the
> >> Linux kernel one) are both part of their respective ABIs and won't
> >> change so Wei's concern that they might change in the future is unfounded.
> >>
> > 
> > The fundamental question is: will the ABI between the library and the
> > kernel ever go mismatch?
> > 
> > My answer is "maybe".  My rationale is that everything goes across
> > boundary of components need to be considered with caution. And I tend to
> > assume the worst things will happen.
> > 
> > To guarantee that they will never go mismatch is to have
> > 
> >    typedef ioctl_gntdev_grant_copy_segment xengnttab_grant_copy_segment_t;
> > 
> > But that's not how the code is written.
> > 
> > I would like to hear a third opinion. Is my concern unfounded? Am I too
> > cautious? Is there any compelling argument that I missed?
> > 
> > Somewhat related, can we have some numbers please? It could well be the
> > cost of the two loops is much cheaper than whatever is going on inside
> > the kernel / hypervisor. And it could turn out that the numbers render
> > this issue moot.
> 
> I did some (very) adhoc measurements and with the worst case of single
> short segments for each ioctl, the optimized version of
> osdep_gnttab_grant_copy() looks to be ~5% faster.
> 
> This is enough of a difference that we should use the optimized version.
> 
> The unoptimized version also adds an additional failure path (the
> calloc) which would be best avoided.
> 

Your test case includes a lot of  noise in libc allocator, so...

Can you give try the following patch (apply on top of Paulina's patch)?
The basic idea is to provide scratch space for the structures. Note, the
patch is compile test only.

---8<---
From e72c1abb9852f40db5eeee48ef208492c3283884 Mon Sep 17 00:00:00 2001
From: Wei Liu <wei.liu2@citrix.com>
Date: Wed, 22 Jun 2016 14:22:48 +0100
Subject: [PATCH] xengnttab: provide osdep cache and use it in Linux grant copy

Signed-off-by: Wei Liu <wei.liu2@citrix.com>
---
 tools/libs/gnttab/linux.c   | 35 +++++++++++++++++++++++++++++------
 tools/libs/gnttab/private.h |  2 ++
 2 files changed, 31 insertions(+), 6 deletions(-)

diff --git a/tools/libs/gnttab/linux.c b/tools/libs/gnttab/linux.c
index 62ad7bd..17d4d29 100644
--- a/tools/libs/gnttab/linux.c
+++ b/tools/libs/gnttab/linux.c
@@ -47,13 +47,28 @@
 #define O_CLOEXEC 0
 #endif
 
+#define COPY_SEGMENT_CACHE_SIZE 1024
+
 int osdep_gnttab_open(xengnttab_handle *xgt)
 {
-    int fd = open(DEVXEN "gntdev", O_RDWR|O_CLOEXEC);
-    if ( fd == -1 )
-        return -1;
-    xgt->fd = fd;
+    size_t s = COPY_SEGMENT_CACHE_SIZE *
+        sizeof(struct ioctl_gntdev_grant_copy_segment);
+
+    xgt->fd = open(DEVXEN "gntdev", O_RDWR|O_CLOEXEC);
+    if (xgt->fd == -1) goto err;
+
+    xgt->osdep_data = malloc(s);
+    if (!xgt->osdep_data) goto err;
+    xgt->osdep_data_size = s;
+
     return 0;
+err:
+    if (xgt->fd != -1) {
+        close(xgt->fd);
+        xgt->fd = -1;
+    }
+
+    return -1;
 }
 
 int osdep_gnttab_close(xengnttab_handle *xgt)
@@ -61,6 +76,10 @@ int osdep_gnttab_close(xengnttab_handle *xgt)
     if ( xgt->fd == -1 )
         return 0;
 
+    free(xgt->osdep_data);
+    xgt->osdep_data = NULL;
+    xgt->osdep_data_size = 0;
+
     return close(xgt->fd);
 }
 
@@ -243,7 +262,12 @@ int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
     int fd = xgt->fd;
     struct ioctl_gntdev_grant_copy copy;
 
-    copy.segments = calloc(count, sizeof(struct ioctl_gntdev_grant_copy_segment));
+    if (count > COPY_SEGMENT_CACHE_SIZE) {
+        errno = E2BIG;
+        return -1;
+    }
+
+    copy.segments = xgt->osdep_data;
     copy.count = count;
     for (i = 0; i < count; i++)
     {
@@ -276,7 +300,6 @@ int osdep_gnttab_grant_copy(xengnttab_handle *xgt,
             segs[i].status = copy.segments[i].status;
     }
 
-    free(copy.segments);
     return rc;
 }
 
diff --git a/tools/libs/gnttab/private.h b/tools/libs/gnttab/private.h
index d6c5594..e99a80d 100644
--- a/tools/libs/gnttab/private.h
+++ b/tools/libs/gnttab/private.h
@@ -7,6 +7,8 @@
 struct xengntdev_handle {
     xentoollog_logger *logger, *logger_tofree;
     int fd;
+    void *osdep_data;              /* osdep private data */
+    size_t osdep_data_size;        /* osdep private data size */
 };
 
 int osdep_gnttab_open(xengnttab_handle *xgt);
-- 
2.1.4


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