[Libvir] [RFC][PATCH 1/2] NUMA memory and topology patches

[Daniel Veillard]

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 at redhat.com  | libxml GNOME XML XSLT toolkit  http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine  http://rpmfind.net/