From mboxrd@z Thu Jan 1 00:00:00 1970 From: Boris Ostrovsky Subject: Re: [PATCH 2/4] sysctl/libxl: Add interface for returning IO topology data Date: Thu, 04 Dec 2014 11:26:08 -0500 Message-ID: <54808B20.9080503@oracle.com> References: <1417556050-23364-1-git-send-email-boris.ostrovsky@oracle.com> <1417556050-23364-3-git-send-email-boris.ostrovsky@oracle.com> <1417694153.31647.32.camel@Abyss.station> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii"; Format="flowed" Content-Transfer-Encoding: 7bit Return-path: In-Reply-To: <1417694153.31647.32.camel@Abyss.station> List-Unsubscribe: , List-Post: List-Help: List-Subscribe: , Sender: xen-devel-bounces@lists.xen.org Errors-To: xen-devel-bounces@lists.xen.org To: Dario Faggioli Cc: wei.liu2@citrix.com, ian.campbell@citrix.com, stefano.stabellini@eu.citrix.com, ian.jackson@eu.citrix.com, xen-devel@lists.xen.org, jbeulich@suse.com, keir@xen.org, ufimtseva@gmail.com List-Id: xen-devel@lists.xenproject.org On 12/04/2014 06:55 AM, Dario Faggioli wrote: > On Tue, 2014-12-02 at 16:34 -0500, Boris Ostrovsky wrote: >> Make XEN_SYSCTL_topologyinfo more generic so that it can return both >> CPU and IO topology (support for returning the latter is added in the >> subsequent patch) >> > Is it always the case that we need the full information (i.e., both CPU > and IO topology), at all levels above Xen? > > I appreciate the performance implications (one hcall instead of two) in > cases where we do, but I still think, as Andrew suggested, that having a > new XEN_SYSCTL_iotopology (or something like that) and leaving > *_topologyinfo alone would make a better low-level interface. > > All IMHO, of course. I don't feel strongly either way so I can go that route (and it will make patch 4 completely unnecessary). (I am not sure though I understood Andrew's reasoning for splitting into two sysctls. >> To do so move (and rename) previously used cpu_to_core/cpu_to_socket/ >> cpu_to_node into struct xen_sysctl_cputopo and move it, together with >> newly added struct xen_sysctl_iotopo, to xen_sysctl_topologyinfo. >> >> Add libxl_get_topology() to handle new interface and modify >> libxl_get_cpu_topology() to be a wrapper around it. >> > This is, I think, useful, and I'd go for it, even if we adding a new > hypercall at the Xen and xc level. > > Of course, it would work the other way round: the new libxl function > would wrap the existing libxl_get_cpu_topology() and a new libxl call > (something like libxl_get_io_topology() ?). > >> Adjust xenpm and python's low-level C libraries for new interface. >> > All in one patch? Wouldn't splitting it at least in two (hypervisor and > tools parts) be better? If it were me, I'd probably split even more... I could not split it because this patch changes sysctl interface (more specifically, xen_sysctl_topologyinfo_t/xc_topologyinfo_t) so anyone who uses this struct needed to be updated at the same time. Of course, if I were to leave current interface intact and add another sysctl for IO topology then this will not be necessary > >> This change requires bumping XEN_SYSCTL_INTERFACE_VERSION >> > Which would not be the case if we take the approach of adding a new, > iotopology specific, hcall, would it? I would think that any changes to a public interface, including adding a new function, require new version. (And if we get a new version, even if we split topology into CPU and IO sysctls, I'd still like to put cpu_to_[core|socket||node] into a single structure). Thanks. -boris