From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH v5 7/8] libxl/libxc: Move libxl_get_numainfo()'s hypercall buffer management to libxc Date: Fri, 20 Mar 2015 10:10:33 -0400 Message-ID: <550C2A59.4080904@oracle.com> References: <1426802044-19444-1-git-send-email-boris.ostrovsky@oracle.com> <1426802044-19444-8-git-send-email-boris.ostrovsky@oracle.com> <20150320134901.GE29188@l.oracle.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <20150320134901.GE29188@l.oracle.com> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Konrad Rzeszutek Wilk Cc: elena.ufimtseva@oracle.com, keir@xen.org, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, andrew.cooper3@citrix.com, dario.faggioli@citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, jbeulich@suse.com, wei.liu2@citrix.com List-Id: xen-devel@lists.xenproject.org On 03/20/2015 09:49 AM, Konrad Rzeszutek Wilk wrote: > On Thu, Mar 19, 2015 at 05:54:03PM -0400, Boris Ostrovsky wrote: >> xc_numainfo() is not expected to be used on a hot path and therefore >> hypercall buffer management can be pushed into libxc. This will simplify >> life for callers. >> >> Also update error logging macros. >> >> Signed-off-by: Boris Ostrovsky >> --- >> Changes in v5: >> * Adjust to new interface (as described in changelog for patch 4/8) >> >> tools/libxc/include/xenctrl.h | 4 ++- >> tools/libxc/xc_misc.c | 41 ++++++++++++++++++++++++----- >> tools/libxl/libxl.c | 52 ++++++++++++------------------------- >> tools/python/xen/lowlevel/xc/xc.c | 38 ++++++++++----------------- >> 4 files changed, 68 insertions(+), 67 deletions(-) >> >> diff --git a/tools/libxc/include/xenctrl.h b/tools/libxc/include/xenctrl.h >> index 14d22ce..54540e7 100644 >> --- a/tools/libxc/include/xenctrl.h >> +++ b/tools/libxc/include/xenctrl.h >> @@ -1228,6 +1228,7 @@ int xc_send_debug_keys(xc_interface *xch, char *keys); >> typedef xen_sysctl_physinfo_t xc_physinfo_t; >> typedef xen_sysctl_cputopo_t xc_cputopo_t; >> typedef xen_sysctl_numainfo_t xc_numainfo_t; >> +typedef xen_sysctl_meminfo_t xc_meminfo_t; >> >> typedef uint32_t xc_cpu_to_node_t; >> typedef uint32_t xc_cpu_to_socket_t; >> @@ -1239,7 +1240,8 @@ typedef uint32_t xc_node_to_node_dist_t; >> int xc_physinfo(xc_interface *xch, xc_physinfo_t *info); >> int xc_cputopoinfo(xc_interface *xch, unsigned *max_cpus, >> xc_cputopo_t *cputopo); >> -int xc_numainfo(xc_interface *xch, xc_numainfo_t *info); >> +int xc_numainfo(xc_interface *xch, unsigned *max_nodes, >> + xc_meminfo_t *meminfo, uint32_t *distance); >> >> int xc_sched_id(xc_interface *xch, >> int *sched_id); >> diff --git a/tools/libxc/xc_misc.c b/tools/libxc/xc_misc.c >> index 411128e..607ae61 100644 >> --- a/tools/libxc/xc_misc.c >> +++ b/tools/libxc/xc_misc.c >> @@ -209,22 +209,49 @@ out: >> return ret; >> } >> >> -int xc_numainfo(xc_interface *xch, >> - xc_numainfo_t *put_info) >> +int xc_numainfo(xc_interface *xch, unsigned *max_nodes, >> + xc_meminfo_t *meminfo, uint32_t *distance) >> { >> int ret; >> DECLARE_SYSCTL; >> + DECLARE_HYPERCALL_BOUNCE(meminfo, *max_nodes * sizeof(*meminfo), >> + XC_HYPERCALL_BUFFER_BOUNCE_OUT); >> + DECLARE_HYPERCALL_BOUNCE(distance, >> + *max_nodes * *max_nodes * sizeof(*distance), >> + XC_HYPERCALL_BUFFER_BOUNCE_OUT); >> >> - sysctl.cmd = XEN_SYSCTL_numainfo; >> + if (meminfo && distance) { > The syntax in xc_misc.c looks mostly to be of the > 'if ( meminfo && distance ) > { > ' type. > >> + if ((ret = xc_hypercall_bounce_pre(xch, meminfo))) >> + goto out; >> + if ((ret = xc_hypercall_bounce_pre(xch, distance))) >> + goto out; >> >> - memcpy(&sysctl.u.numainfo, put_info, sizeof(*put_info)); >> + sysctl.u.numainfo.num_nodes = *max_nodes; >> + set_xen_guest_handle(sysctl.u.numainfo.meminfo, meminfo); >> + set_xen_guest_handle(sysctl.u.numainfo.distance, distance); >> + } else if (meminfo || distance) { >> + errno = EINVAL; >> + return -1; >> + } else { >> + set_xen_guest_handle(sysctl.u.numainfo.meminfo, >> + HYPERCALL_BUFFER_NULL); >> + set_xen_guest_handle(sysctl.u.numainfo.distance, >> + HYPERCALL_BUFFER_NULL); >> + } >> >> + sysctl.cmd = XEN_SYSCTL_numainfo; >> if ((ret = do_sysctl(xch, &sysctl)) != 0) >> - return ret; >> + goto out; >> >> - memcpy(put_info, &sysctl.u.numainfo, sizeof(*put_info)); >> + *max_nodes = sysctl.u.numainfo.num_nodes; >> >> - return 0; >> +out: >> + if (meminfo && distance) { >> + xc_hypercall_bounce_post(xch, meminfo); >> + xc_hypercall_bounce_post(xch, distance); >> + } >> + >> + return ret; >> } >> >> >> diff --git a/tools/libxl/libxl.c b/tools/libxl/libxl.c >> index 2b7d19c..ee97a54 100644 >> --- a/tools/libxl/libxl.c >> +++ b/tools/libxl/libxl.c >> @@ -5094,62 +5094,44 @@ libxl_cputopology *libxl_get_cpu_topology(libxl_ctx *ctx, int *nb_cpu_out) >> libxl_numainfo *libxl_get_numainfo(libxl_ctx *ctx, int *nr) >> { >> GC_INIT(ctx); >> - xc_numainfo_t ninfo; >> - DECLARE_HYPERCALL_BUFFER(xen_sysctl_meminfo_t, meminfo); >> - DECLARE_HYPERCALL_BUFFER(uint32_t, distance); >> + xc_meminfo_t *meminfo; >> + uint32_t *distance; >> libxl_numainfo *ret = NULL; >> int i, j; >> + unsigned num_nodes; >> >> - set_xen_guest_handle(ninfo.meminfo, HYPERCALL_BUFFER_NULL); >> - set_xen_guest_handle(ninfo.distance, HYPERCALL_BUFFER_NULL); >> - if ( xc_numainfo(ctx->xch, &ninfo) != 0) >> - { >> - LIBXL__LOG(ctx, XTL_ERROR, "Unable to determine number of NODES"); >> - ret = NULL; >> + if (xc_numainfo(ctx->xch, &num_nodes, NULL, NULL)) { > I think for this one you need 'if ( xc_numa.. ) > {' I actually think it's the other way around: when I made the first change in 4/8 I added spaces, which this file's style generally doesn't use. Which, conveniently, is different from the style used by xc_misc.c And I am quite confused about curly bracket positions. They are all over the place (pun intended). -boris