[PATCH 1/2] - add capability to access free memory information on each NUMA cell. Signed-off-by: Beth Kon (eak(a)us.ibm.com) -- Elizabeth Kon (Beth) IBM Linux Technology Center Open Hypervisor Team email: eak(a)us.ibm.com

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

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?

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