From patchwork Wed Jun 22 13:29:18 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Wei Liu X-Patchwork-Id: 9192915 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 5906B601C0 for ; Wed, 22 Jun 2016 13:31:23 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 4911528400 for ; Wed, 22 Jun 2016 13:31:23 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 3DBD028408; Wed, 22 Jun 2016 13:31:23 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=2.0 tests=BAYES_00, RCVD_IN_DNSWL_MED autolearn=ham version=3.3.1 Received: from lists.xenproject.org (lists.xenproject.org [192.237.175.120]) (using TLSv1.2 with cipher AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 3A18A28400 for ; Wed, 22 Jun 2016 13:31:22 +0000 (UTC) Received: from localhost ([127.0.0.1] helo=lists.xenproject.org) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bFiDc-0002wW-EC; Wed, 22 Jun 2016 13:29:24 +0000 Received: from mail6.bemta5.messagelabs.com ([195.245.231.135]) by lists.xenproject.org with esmtp (Exim 4.84_2) (envelope-from ) id 1bFiDa-0002vo-Oz for xen-devel@lists.xenproject.org; Wed, 22 Jun 2016 13:29:22 +0000 Received: from [85.158.139.211] by server-11.bemta-5.messagelabs.com id B7/5D-04210-2B29A675; Wed, 22 Jun 2016 13:29:22 +0000 X-Brightmail-Tracker: H4sIAAAAAAAAA+NgFtrMIsWRWlGSWpSXmKPExsXitHSDve7GSVn hBjvuyVp83zKZyYHR4/CHKywBjFGsmXlJ+RUJrBlH5goXbDWrWLPtMXMD4yStLkZODgkBf4k7 K9ayg9gsAqoSl7efZgKx2QSUJX529rKB2CICWhLN878zdjFycTALPGOU6P92lBkkISwQInH+x DQWEJtXQE/i1a65bCBFQgJvGSXmtE6GSghKnJz5BMxmFtCRWLD7E1ARB5AtLbH8HwdImBNoQd eRz2AzRQVUJK5MeAt2kJCAgkTH9GNMIOUSAtwSf7vtJzDyz0IydBaSobMQhi5gZF7FqFGcWlS WWqRraK6XVJSZnlGSm5iZo2toYKqXm1pcnJiempOYVKyXnJ+7iREYgAxAsIPx4mnPQ4ySHExK orzyylnhQnxJ+SmVGYnFGfFFpTmpxYcYNTg4BPrWrL7AKMWSl5+XqiTBu2EiUJ1gUWp6akVaZ g4wRmBKJTh4lER4d4KkeYsLEnOLM9MhUqcYFaXEeeNBEgIgiYzSPLg2WFxeYpSVEuZlBDpKiK cgtSg3swRV/hWjOAejkjDvbpApPJl5JXDTXwEtZgJavKw/HWRxSSJCSqqBcWpm8heFyJd85n8 kjXLUXr9uKuapDrNcNvlNt9h169aPk94z9QmKdb+MrBAwuf3Nt/sHzzn2jgT/sodJ896+qa5+ xq6wK+Gmez1v/Msea/Wah9IqyhNWn471X/x6bbu6pmGy9DPPUsHAyf7bjVNvaJo69POdTkkq+ /n9E09pwzZ5ncp7T+WUWIozEg21mIuKEwH6BSbMxgIAAA== X-Env-Sender: prvs=9744cf058=wei.liu2@citrix.com X-Msg-Ref: server-7.tower-206.messagelabs.com!1466602159!45989291!1 X-Originating-IP: [66.165.176.63] X-SpamReason: No, hits=0.0 required=7.0 tests=sa_preprocessor: VHJ1c3RlZCBJUDogNjYuMTY1LjE3Ni42MyA9PiAzMDYwNDg=\n, received_headers: No Received headers X-StarScan-Received: X-StarScan-Version: 8.46; banners=-,-,- X-VirusChecked: Checked Received: (qmail 50233 invoked from network); 22 Jun 2016 13:29:21 -0000 Received: from smtp02.citrix.com (HELO SMTP02.CITRIX.COM) (66.165.176.63) by server-7.tower-206.messagelabs.com with RC4-SHA encrypted SMTP; 22 Jun 2016 13:29:21 -0000 X-IronPort-AV: E=Sophos;i="5.26,509,1459814400"; d="scan'208";a="368950427" Date: Wed, 22 Jun 2016 14:29:18 +0100 From: Wei Liu To: David Vrabel Message-ID: <20160622132918.GD1790@citrix.com> References: <1466584733-19459-1-git-send-email-paulinaszubarczyk@gmail.com> <1466584733-19459-2-git-send-email-paulinaszubarczyk@gmail.com> <576A5C54.7000601@citrix.com> <20160622112118.GB1790@citrix.com> <576A869E.6090400@citrix.com> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <576A869E.6090400@citrix.com> User-Agent: Mutt/1.5.23 (2014-03-12) X-DLP: MIA1 Cc: sstabellini@kernel.org, Wei Liu , ian.jackson@eu.citrix.com, Paulina Szubarczyk , anthony.perard@citrix.com, xen-devel@lists.xenproject.org, roger.pau@citrix.com Subject: Re: [Xen-devel] [PATCH v3 1/2] Interface for grant copy operation in libs. X-BeenThere: xen-devel@lists.xen.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Xen developer discussion List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Errors-To: xen-devel-bounces@lists.xen.org Sender: "Xen-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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, ©); > >>> + 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 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 --- 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);