On Tue, Sep 25, 2007 at 09:20:37AM -0400, beth kon wrote:

Richard W.M. Jones wrote:

...

beth kon wrote:

...

[PATCH 1/2] - add capability to access free memory information on each NUMA cell.

+ +/* number of cells in the node will be saved for later use */ +int nbNodeCells;

This should be static I think.

As the code stands now, I don't think so. It is defined in xend_internal.c but used in xen_internal.c

...

A bigger problem here is that I think the call to xenDaemonNodeGetInfo should happen inside xen_internal.c:xenHypervisorNodeGetCellsFreeMemory, something like this:

xenHypervisorNodeGetCellsFreeMemory (virConnectPtr conn, ....) { static int nb_nodes = -1;

if (nb_nodes == -1) { virNodeInfo info; if (xenDaemonNodeGetInfo (conn, &info) == -1) { ... (error) ... } nb_nodes = info->nodes; }

... }

Note also the explicit test for == -1

I like this solution. I thought that Daniel wanted it out of the xenHypervisorNodeGetCellsFreeMemory call, and in some init code. Daniel? Do you have an opinion on this?

Not really. The number of cells on the host is currently only useful to only one routine, so either solution is fine by me. In general I try to avoid exporting global variables, in that case a simple routine set in xen_unified.c and exported from xen_unified.h could do that lookup and keep the value as a static variable. That would allow to share as in your code, while avoiding a global variable like in Rich suggestion. But at this point this is nitpicking a bit, we will have to refactor this slightly anyway I think :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/

Daniel Veillard wrote:

On Mon, Sep 24, 2007 at 11:29:07PM -0400, beth kon wrote:

...

[PATCH 1/2] - add capability to access free memory information on each NUMA cell.

[...]

...

diff -urpN libvirt.danielpatch/src/xen_internal.c libvirt.cellsMemory/src/xen_internal.c --- libvirt.danielpatch/src/xen_internal.c 2007-09-11 15:29:44.000000000 -0400 +++ libvirt.cellsMemory/src/xen_internal.c 2007-09-24 22:04:05.000000000 -0400

[...]

...

* Returns the number of entries filled in freeMems, or -1 in case of error. */ int -xenHypervisorNodeGetCellsFreeMemory(virConnectPtr conn, unsigned long *freeMems, - int nbCells) +xenHypervisorNodeGetCellsFreeMemory(virConnectPtr conn, long long *freeMems, + int startCell, int maxCells) { - if ((conn == NULL) || (freeMems == NULL) || (nbCells < 0)) - return -1; + xen_op_v2_sys op_sys; + int i, j, ret; + xenUnifiedPrivatePtr priv; +

+ if ((conn == NULL) || (maxCells < 1) || (startCell > nbNodeCells)) { + virXenErrorFunc (VIR_ERR_INVALID_ARG, __FUNCTION__, + "invalid argument", 0); + return -1; + } /* - * TODO: - * - get the number of cell in the node - * - if not NUMA returns the available memeoy directly in freeMems[0] - * return 1 - * - if NUMA iterates over the cells using a specific hypercall - * filling up entries until full or at the end of the NUMA cells + * Support only sys_interface_version >=4 */ - return(-1); + if (sys_interface_version < 4) { + virXenErrorFunc (VIR_ERR_NO_XEN, __FUNCTION__, + "unsupported in sys interface < 4", 0); + return -1; + }

I found out that using VIR_ERR_NO_XEN here isn't a good idea, first it's not that Xen hypervisoris not found but that this specific call is not available, and second virsh filter out automatically VIR_ERR_NO_XEN. It is better to use VIR_ERR_XEN_CALL , you then get:

[root@test2 src]# ./virsh freecell 1 libvir: Xen error : failed Xen syscall xenHypervisorNodeGetCellsFreeMemory: unsupported in sys interface < 4 0

[root@test2 src]# echo $? 1 [root@test2 src]#

Which is a correct error report IMHO.

Daniel

Ok, that sounds good. -- Elizabeth Kon (Beth) IBM Linux Technology Center Open Hypervisor Team email: eak@us.ibm.com