[libvirt] [PATCH 0/5] Add additional data to the NUMA topology information
See patch 2/5 for explaination. Peter Krempa (5): schema: Make the cpuset type reusable across schema files schemas: Add schemas for more CPU topology information in the caps XML conf: Split out NUMA topology formatting to simplify access to data capabilities: Switch CPU data in NUMA topology to a struct capabilities: Add additional data to the NUMA topology info docs/schemas/basictypes.rng | 6 ++++ docs/schemas/capability.rng | 11 ++++++ docs/schemas/domaincommon.rng | 5 --- src/conf/capabilities.c | 80 ++++++++++++++++++++++++++++--------------- src/conf/capabilities.h | 14 ++++++-- src/nodeinfo.c | 75 +++++++++++++++++++++++++++++++++------- src/qemu/qemu_process.c | 2 +- src/test/test_driver.c | 18 ++++++++-- src/xen/xend_internal.c | 27 +++++++++------ 9 files changed, 176 insertions(+), 62 deletions(-) -- 1.8.1.1
--- docs/schemas/basictypes.rng | 6 ++++++ docs/schemas/domaincommon.rng | 5 ----- 2 files changed, 6 insertions(+), 5 deletions(-) diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 38cab16..8e44e8d 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -219,4 +219,10 @@ </data> </define> + <define name="cpuset"> + <data type="string"> + <param name="pattern">([0-9]+(-[0-9]+)?|\^[0-9]+)(,([0-9]+(-[0-9]+)?|\^[0-9]+))*</param> + </data> + </define> + </grammar> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 67ae864..008a62f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3672,11 +3672,6 @@ <!-- Type library --> - <define name="cpuset"> - <data type="string"> - <param name="pattern">([0-9]+(-[0-9]+)?|\^[0-9]+)(,([0-9]+(-[0-9]+)?|\^[0-9]+))*</param> - </data> - </define> <define name="countCPU"> <data type="unsignedShort"> <param name="pattern">[0-9]+</param> -- 1.8.1.1
On 01/19/2013 12:06 AM, Peter Krempa wrote:
--- docs/schemas/basictypes.rng | 6 ++++++ docs/schemas/domaincommon.rng | 5 ----- 2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 38cab16..8e44e8d 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -219,4 +219,10 @@ </data> </define>
+ <define name="cpuset"> + <data type="string"> + <param name="pattern">([0-9]+(-[0-9]+)?|\^[0-9]+)(,([0-9]+(-[0-9]+)?|\^[0-9]+))*</param> + </data> + </define> + </grammar> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 67ae864..008a62f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3672,11 +3672,6 @@ <!-- Type library --> - <define name="cpuset"> - <data type="string"> - <param name="pattern">([0-9]+(-[0-9]+)?|\^[0-9]+)(,([0-9]+(-[0-9]+)?|\^[0-9]+))*</param> - </data> - </define> <define name="countCPU"> <data type="unsignedShort"> <param name="pattern">[0-9]+</param>
I'd suggest naming this type 'bitmap' to possibly accommodate other bit maps in the future, but since this was pre-existing it has nothing to do with this patch, just sayin'. ACK, Martin
On Sat, Jan 19, 2013 at 12:06:38AM +0100, Peter Krempa wrote:
--- docs/schemas/basictypes.rng | 6 ++++++ docs/schemas/domaincommon.rng | 5 ----- 2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/docs/schemas/basictypes.rng b/docs/schemas/basictypes.rng index 38cab16..8e44e8d 100644 --- a/docs/schemas/basictypes.rng +++ b/docs/schemas/basictypes.rng @@ -219,4 +219,10 @@ </data> </define>
+ <define name="cpuset"> + <data type="string"> + <param name="pattern">([0-9]+(-[0-9]+)?|\^[0-9]+)(,([0-9]+(-[0-9]+)?|\^[0-9]+))*</param> + </data> + </define> + </grammar> diff --git a/docs/schemas/domaincommon.rng b/docs/schemas/domaincommon.rng index 67ae864..008a62f 100644 --- a/docs/schemas/domaincommon.rng +++ b/docs/schemas/domaincommon.rng @@ -3672,11 +3672,6 @@ <!-- Type library --> - <define name="cpuset"> - <data type="string"> - <param name="pattern">([0-9]+(-[0-9]+)?|\^[0-9]+)(,([0-9]+(-[0-9]+)?|\^[0-9]+))*</param> - </data> - </define> <define name="countCPU"> <data type="unsignedShort"> <param name="pattern">[0-9]+</param>
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This patch adds RNG schemas for adding more information in the topology output of the NUMA section in the capabilities XML. The added elements are designed to provide more information about the placement and topology of the processors in the system to management applications. A demonstration of supported XML added by this patch: <capabilities> <host> <topology> <cells num='3'> <cell id='0'> <cpus num='4'> <!-- this is node with Hyperthreading --> <cpu id='0' socket_id='0' core_id='0' siblings='0-1'/> <cpu id='1' socket_id='0' core_id='0' siblings='0-1'/> <cpu id='2' socket_id='0' core_id='1' siblings='2-3'/> <cpu id='3' socket_id='0' core_id='1' siblings='2-3'/> </cpus> </cell> <cell id='1'> <cpus num='4'> <!-- this is node with modules (Bulldozer) --> <cpu id='4' socket_id='0' core_id='2' siblings='4-5'/> <cpu id='5' socket_id='0' core_id='3' siblings='4-5'/> <cpu id='6' socket_id='0' core_id='4' siblings='6-7'/> <cpu id='7' socket_id='0' core_id='5' siblings='6-7'/> </cpus> </cell> <cell id='2'> <cpus num='4'> <!-- this is a normal multi-core node --> <cpu id='8' socket_id='1' core_id='0' siblings='8'/> <cpu id='9' socket_id='1' core_id='1' siblings='9'/> <cpu id='10' socket_id='1' core_id='2' siblings='10'/> <cpu id='11' socket_id='1' core_id='3' siblings='11'/> </cpus> </cell> </cells> </topology> </host> </capabilities> The socket_id field represents identification of the physical socket the CPU is plugged in. This ID may not be identical to the physical socket ID reported by the kernel. The core_id identifies a core within a socket. Also this field may not accurately represent physical ID's. The core_id is guaranteed to be unique within a cell and a socket. There may be duplicates between sockets. Only cores sharing core_id within one cell and one socket can be considered as threads. Cores sharing core_id within sparate cells are distinct cores. The siblings field is a list of CPU id's the cpu id's the CPU is sibling with - thus a thread. The list is in the cpuset format. --- docs/schemas/capability.rng | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng index 8c928bc..734ed81 100644 --- a/docs/schemas/capability.rng +++ b/docs/schemas/capability.rng @@ -193,6 +193,17 @@ <attribute name='id'> <ref name='unsignedInt'/> </attribute> + <optional> + <attribute name='socket_id'> + <ref name='unsignedInt'/> + </attribute> + <attribute name='core_id'> + <ref name='unsignedInt'/> + </attribute> + <attribute name='siblings'> + <ref name='cpuset'/> + </attribute> + </optional> </element> </define> -- 1.8.1.1
On 01/19/2013 12:06 AM, Peter Krempa wrote:
This patch adds RNG schemas for adding more information in the topology output of the NUMA section in the capabilities XML.
The added elements are designed to provide more information about the placement and topology of the processors in the system to management applications.
A demonstration of supported XML added by this patch: <capabilities> <host> <topology> <cells num='3'> <cell id='0'> <cpus num='4'> <!-- this is node with Hyperthreading --> <cpu id='0' socket_id='0' core_id='0' siblings='0-1'/> <cpu id='1' socket_id='0' core_id='0' siblings='0-1'/> <cpu id='2' socket_id='0' core_id='1' siblings='2-3'/> <cpu id='3' socket_id='0' core_id='1' siblings='2-3'/> </cpus> </cell> <cell id='1'> <cpus num='4'> <!-- this is node with modules (Bulldozer) --> <cpu id='4' socket_id='0' core_id='2' siblings='4-5'/> <cpu id='5' socket_id='0' core_id='3' siblings='4-5'/> <cpu id='6' socket_id='0' core_id='4' siblings='6-7'/> <cpu id='7' socket_id='0' core_id='5' siblings='6-7'/> </cpus> </cell> <cell id='2'> <cpus num='4'> <!-- this is a normal multi-core node --> <cpu id='8' socket_id='1' core_id='0' siblings='8'/> <cpu id='9' socket_id='1' core_id='1' siblings='9'/> <cpu id='10' socket_id='1' core_id='2' siblings='10'/> <cpu id='11' socket_id='1' core_id='3' siblings='11'/> </cpus> </cell> </cells> </topology> </host> </capabilities>
The socket_id field represents identification of the physical socket the CPU is plugged in. This ID may not be identical to the physical socket ID reported by the kernel.
The core_id identifies a core within a socket. Also this field may not accurately represent physical ID's.
The core_id is guaranteed to be unique within a cell and a socket. There may be duplicates between sockets. Only cores sharing core_id within one cell and one socket can be considered as threads. Cores sharing core_id within sparate cells are distinct cores.
s/sparate/separate/
The siblings field is a list of CPU id's the cpu id's the CPU is sibling with - thus a thread. The list is in the cpuset format.
s/CPU id's the cpu id's/CPU IDs/
--- docs/schemas/capability.rng | 11 +++++++++++ 1 file changed, 11 insertions(+)
diff --git a/docs/schemas/capability.rng b/docs/schemas/capability.rng index 8c928bc..734ed81 100644 --- a/docs/schemas/capability.rng +++ b/docs/schemas/capability.rng @@ -193,6 +193,17 @@ <attribute name='id'> <ref name='unsignedInt'/> </attribute> + <optional> + <attribute name='socket_id'> + <ref name='unsignedInt'/> + </attribute> + <attribute name='core_id'> + <ref name='unsignedInt'/> + </attribute> + <attribute name='siblings'> + <ref name='cpuset'/> + </attribute> + </optional> </element> </define>
IMHO this looks very good and it is understandable for management applications. I like that with this approach libvirt isn't obfuscating modules as neither threads nor cores, but rather providing all the information available, so management application can decide what it wants (but doesn't have to). However, I wasn't part of your previous discussion about this and I don't want to skip other's decision on this by simply ACKing it without others expressing their opinion, so small ACK from me. I'd say it can go in if nobody is particularly against it until the freeze. Martin
On 01/21/13 09:53, Martin Kletzander wrote:
On 01/19/2013 12:06 AM, Peter Krempa wrote:
[...]
IMHO this looks very good and it is understandable for management applications. I like that with this approach libvirt isn't obfuscating modules as neither threads nor cores, but rather providing all the information available, so management application can decide what it wants (but doesn't have to).
However, I wasn't part of your previous discussion about this and I don't want to skip other's decision on this by simply ACKing it without others expressing their opinion, so small ACK from me. I'd say it can go in if nobody is particularly against it until the freeze.
Thanks for the review. Anybody else to review the design? (... also known as "Ping?") PEter
Martin
-- libvir-list mailing list libvir-list@redhat.com https://www.redhat.com/mailman/listinfo/libvir-list
On Sat, Jan 19, 2013 at 12:06:39AM +0100, Peter Krempa wrote:
This patch adds RNG schemas for adding more information in the topology output of the NUMA section in the capabilities XML.
The added elements are designed to provide more information about the placement and topology of the processors in the system to management applications.
A demonstration of supported XML added by this patch: <capabilities> <host> <topology> <cells num='3'> <cell id='0'> <cpus num='4'> <!-- this is node with Hyperthreading --> <cpu id='0' socket_id='0' core_id='0' siblings='0-1'/> <cpu id='1' socket_id='0' core_id='0' siblings='0-1'/> <cpu id='2' socket_id='0' core_id='1' siblings='2-3'/> <cpu id='3' socket_id='0' core_id='1' siblings='2-3'/> </cpus> </cell> <cell id='1'> <cpus num='4'> <!-- this is node with modules (Bulldozer) --> <cpu id='4' socket_id='0' core_id='2' siblings='4-5'/> <cpu id='5' socket_id='0' core_id='3' siblings='4-5'/> <cpu id='6' socket_id='0' core_id='4' siblings='6-7'/> <cpu id='7' socket_id='0' core_id='5' siblings='6-7'/> </cpus> </cell> <cell id='2'> <cpus num='4'> <!-- this is a normal multi-core node --> <cpu id='8' socket_id='1' core_id='0' siblings='8'/> <cpu id='9' socket_id='1' core_id='1' siblings='9'/> <cpu id='10' socket_id='1' core_id='2' siblings='10'/> <cpu id='11' socket_id='1' core_id='3' siblings='11'/> </cpus> </cell> </cells> </topology> </host> </capabilities>
The socket_id field represents identification of the physical socket the CPU is plugged in. This ID may not be identical to the physical socket ID reported by the kernel.
The core_id identifies a core within a socket. Also this field may not accurately represent physical ID's.
The core_id is guaranteed to be unique within a cell and a socket. There may be duplicates between sockets. Only cores sharing core_id within one cell and one socket can be considered as threads. Cores sharing core_id within sparate cells are distinct cores.
The siblings field is a list of CPU id's the cpu id's the CPU is sibling with - thus a thread. The list is in the cpuset format. --- docs/schemas/capability.rng | 11 +++++++++++ 1 file changed, 11 insertions(+)
ACK, this looks much better. I was about to complain about the lack of docs for the new XML, but I see we've not documented *any* of the capabilities XML, so I wouldn't block you on that. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
On 01/22/13 18:00, Daniel P. Berrange wrote:
On Sat, Jan 19, 2013 at 12:06:39AM +0100, Peter Krempa wrote:
docs/schemas/capability.rng | 11 +++++++++++ 1 file changed, 11 insertions(+)
ACK, this looks much better.
I was about to complain about the lack of docs for the new XML, but I see we've not documented *any* of the capabilities XML, so I wouldn't block you on that.
I'm planing on changing that, but I wanted to get the design right at first. Peter
Daniel
--- src/conf/capabilities.c | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 365c511..0d2512e 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -678,6 +678,28 @@ virCapabilitiesDefaultGuestEmulator(virCapsPtr caps, return NULL; } +static void +virCapabilitiesFormatNUMATopology(virBufferPtr xml, + size_t ncells, + virCapsHostNUMACellPtr *cells) +{ + int i; + int j; + + virBufferAddLit(xml, " <topology>\n"); + virBufferAsprintf(xml, " <cells num='%zu'>\n", ncells); + for (i = 0; i < ncells; i++) { + virBufferAsprintf(xml, " <cell id='%d'>\n", cells[i]->num); + virBufferAsprintf(xml, " <cpus num='%d'>\n", cells[i]->ncpus); + for (j = 0; j < cells[i]->ncpus; j++) + virBufferAsprintf(xml, " <cpu id='%d'/>\n", + cells[i]->cpus[j]); + virBufferAddLit(xml, " </cpus>\n"); + virBufferAddLit(xml, " </cell>\n"); + } + virBufferAddLit(xml, " </cells>\n"); + virBufferAddLit(xml, " </topology>\n"); +} /** * virCapabilitiesFormatXML: @@ -752,24 +774,9 @@ virCapabilitiesFormatXML(virCapsPtr caps) virBufferAddLit(&xml, " </migration_features>\n"); } - if (caps->host.nnumaCell) { - virBufferAddLit(&xml, " <topology>\n"); - virBufferAsprintf(&xml, " <cells num='%zu'>\n", - caps->host.nnumaCell); - for (i = 0 ; i < caps->host.nnumaCell ; i++) { - virBufferAsprintf(&xml, " <cell id='%d'>\n", - caps->host.numaCell[i]->num); - virBufferAsprintf(&xml, " <cpus num='%d'>\n", - caps->host.numaCell[i]->ncpus); - for (j = 0 ; j < caps->host.numaCell[i]->ncpus ; j++) - virBufferAsprintf(&xml, " <cpu id='%d'/>\n", - caps->host.numaCell[i]->cpus[j]); - virBufferAddLit(&xml, " </cpus>\n"); - virBufferAddLit(&xml, " </cell>\n"); - } - virBufferAddLit(&xml, " </cells>\n"); - virBufferAddLit(&xml, " </topology>\n"); - } + if (caps->host.nnumaCell) + virCapabilitiesFormatNUMATopology(&xml, caps->host.nnumaCell, + caps->host.numaCell); for (i = 0; i < caps->host.nsecModels; i++) { virBufferAddLit(&xml, " <secmodel>\n"); -- 1.8.1.1
On 01/19/2013 12:06 AM, Peter Krempa wrote:
--- src/conf/capabilities.c | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 365c511..0d2512e 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -678,6 +678,28 @@ virCapabilitiesDefaultGuestEmulator(virCapsPtr caps, return NULL; }
+static void +virCapabilitiesFormatNUMATopology(virBufferPtr xml, + size_t ncells, + virCapsHostNUMACellPtr *cells) +{ + int i; + int j; + + virBufferAddLit(xml, " <topology>\n"); + virBufferAsprintf(xml, " <cells num='%zu'>\n", ncells); + for (i = 0; i < ncells; i++) { + virBufferAsprintf(xml, " <cell id='%d'>\n", cells[i]->num); + virBufferAsprintf(xml, " <cpus num='%d'>\n", cells[i]->ncpus); + for (j = 0; j < cells[i]->ncpus; j++) + virBufferAsprintf(xml, " <cpu id='%d'/>\n", + cells[i]->cpus[j]); + virBufferAddLit(xml, " </cpus>\n"); + virBufferAddLit(xml, " </cell>\n"); + } + virBufferAddLit(xml, " </cells>\n"); + virBufferAddLit(xml, " </topology>\n"); +}
/** * virCapabilitiesFormatXML: @@ -752,24 +774,9 @@ virCapabilitiesFormatXML(virCapsPtr caps) virBufferAddLit(&xml, " </migration_features>\n"); }
- if (caps->host.nnumaCell) { - virBufferAddLit(&xml, " <topology>\n"); - virBufferAsprintf(&xml, " <cells num='%zu'>\n", - caps->host.nnumaCell); - for (i = 0 ; i < caps->host.nnumaCell ; i++) { - virBufferAsprintf(&xml, " <cell id='%d'>\n", - caps->host.numaCell[i]->num); - virBufferAsprintf(&xml, " <cpus num='%d'>\n", - caps->host.numaCell[i]->ncpus); - for (j = 0 ; j < caps->host.numaCell[i]->ncpus ; j++) - virBufferAsprintf(&xml, " <cpu id='%d'/>\n", - caps->host.numaCell[i]->cpus[j]); - virBufferAddLit(&xml, " </cpus>\n"); - virBufferAddLit(&xml, " </cell>\n"); - } - virBufferAddLit(&xml, " </cells>\n"); - virBufferAddLit(&xml, " </topology>\n"); - } + if (caps->host.nnumaCell) + virCapabilitiesFormatNUMATopology(&xml, caps->host.nnumaCell, + caps->host.numaCell);
for (i = 0; i < caps->host.nsecModels; i++) { virBufferAddLit(&xml, " <secmodel>\n");
ACK, Martin
On Sat, Jan 19, 2013 at 12:06:40AM +0100, Peter Krempa wrote:
--- src/conf/capabilities.c | 43 +++++++++++++++++++++++++------------------ 1 file changed, 25 insertions(+), 18 deletions(-)
ACK Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This will allow storing additional topology data in the NUMA topology definition. This patch changes the storage type and fixes fallback of the change across the drivers using it. This patch also changes semantics of adding new NUMA cell information. Until now the data were re-allocated and copied to the topology definition. This patch changes the addition function to steal the pointer to a pre-allocated structure to simplify the code. --- src/conf/capabilities.c | 29 ++++++++++++++++++----------- src/conf/capabilities.h | 14 ++++++++++++-- src/nodeinfo.c | 22 ++++++++++++---------- src/qemu/qemu_process.c | 2 +- src/test/test_driver.c | 15 ++++++++++++--- src/xen/xend_internal.c | 24 +++++++++++++----------- 6 files changed, 68 insertions(+), 38 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 0d2512e..814c4d6 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -65,12 +65,26 @@ virCapabilitiesNew(virArch hostarch, return caps; } +void +virCapabilitiesFreeHostNUMACellCPU(virCapsHostNUMACellCPUPtr cpu) +{ + if (!cpu) + return; + + VIR_FREE(cpu->siblings_list); +} + static void virCapabilitiesFreeHostNUMACell(virCapsHostNUMACellPtr cell) { + int i; + if (cell == NULL) return; + for (i = 0; i < cell->ncpus; i++) + virCapabilitiesFreeHostNUMACellCPU(cell->cpus + i); + VIR_FREE(cell->cpus); VIR_FREE(cell); } @@ -236,7 +250,7 @@ virCapabilitiesAddHostMigrateTransport(virCapsPtr caps, * @caps: capabilities to extend * @num: ID number of NUMA cell * @ncpus: number of CPUs in cell - * @cpus: array of CPU ID numbers for cell + * @cpus: array of CPU definition structures, the pointer is stolen * * Registers a new NUMA cell for a host, passing in a * array of CPU IDs belonging to the cell @@ -245,7 +259,7 @@ int virCapabilitiesAddHostNUMACell(virCapsPtr caps, int num, int ncpus, - const int *cpus) + virCapsHostNUMACellCPUPtr cpus) { virCapsHostNUMACellPtr cell; @@ -256,16 +270,9 @@ virCapabilitiesAddHostNUMACell(virCapsPtr caps, if (VIR_ALLOC(cell) < 0) return -1; - if (VIR_ALLOC_N(cell->cpus, ncpus) < 0) { - VIR_FREE(cell); - return -1; - } - memcpy(cell->cpus, - cpus, - ncpus * sizeof(*cpus)); - cell->ncpus = ncpus; cell->num = num; + cell->cpus = cpus; caps->host.numaCell[caps->host.nnumaCell++] = cell; @@ -693,7 +700,7 @@ virCapabilitiesFormatNUMATopology(virBufferPtr xml, virBufferAsprintf(xml, " <cpus num='%d'>\n", cells[i]->ncpus); for (j = 0; j < cells[i]->ncpus; j++) virBufferAsprintf(xml, " <cpu id='%d'/>\n", - cells[i]->cpus[j]); + cells[i]->cpus[j].id); virBufferAddLit(xml, " </cpus>\n"); virBufferAddLit(xml, " </cell>\n"); } diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 19b99c6..124d8c1 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -84,12 +84,21 @@ struct _virCapsGuest { virCapsGuestFeaturePtr *features; }; +typedef struct _virCapsHostNUMACellCPU virCapsHostNUMACellCPU; +typedef virCapsHostNUMACellCPU *virCapsHostNUMACellCPUPtr; +struct _virCapsHostNUMACellCPU { + int id; + int socket_id; + int core_id; + char *siblings_list; +}; + typedef struct _virCapsHostNUMACell virCapsHostNUMACell; typedef virCapsHostNUMACell *virCapsHostNUMACellPtr; struct _virCapsHostNUMACell { int num; int ncpus; - int *cpus; + virCapsHostNUMACellCPUPtr cpus; }; typedef struct _virCapsHostSecModel virCapsHostSecModel; @@ -201,7 +210,7 @@ extern int virCapabilitiesAddHostNUMACell(virCapsPtr caps, int num, int ncpus, - const int *cpus); + virCapsHostNUMACellCPUPtr cpus); extern int @@ -250,6 +259,7 @@ virCapabilitiesSupportsGuestOSTypeArch(virCapsPtr caps, const char *ostype, virArch arch); +void virCapabilitiesFreeHostNUMACellCPU(virCapsHostNUMACellCPUPtr cpu); extern virArch virCapabilitiesDefaultGuestArch(virCapsPtr caps, diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 477104f..dffe7d1 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -79,9 +79,11 @@ freebsdNodeGetCPUCount(void) #ifdef __linux__ # define CPUINFO_PATH "/proc/cpuinfo" # define SYSFS_SYSTEM_PATH "/sys/devices/system" +# define SYSFS_CPU_PATH SYSFS_SYSTEM_PATH"/cpu" # define PROCSTAT_PATH "/proc/stat" # define MEMINFO_PATH "/proc/meminfo" # define SYSFS_MEMORY_SHARED_PATH "/sys/kernel/mm/ksm" +# define SYSFS_THREAD_SIBLINGS_LIST_LENGHT_MAX 1024 # define LINUX_NB_CPU_STATS 4 # define LINUX_NB_MEMORY_STATS_ALL 4 @@ -1476,9 +1478,10 @@ nodeCapsInitNUMA(virCapsPtr caps) int n; unsigned long *mask = NULL; unsigned long *allonesmask = NULL; - int *cpus = NULL; + virCapsHostNUMACellCPUPtr cpus = NULL; int ret = -1; int max_n_cpus = NUMA_MAX_N_CPUS; + int ncpus = 0; if (numa_available() < 0) return 0; @@ -1492,7 +1495,6 @@ nodeCapsInitNUMA(virCapsPtr caps) for (n = 0 ; n <= numa_max_node() ; n++) { int i; - int ncpus; /* The first time this returns -1, ENOENT if node doesn't exist... */ if (numa_node_to_cpus(n, mask, mask_n_bytes) < 0) { VIR_WARN("NUMA topology for cell %d of %d not available, ignoring", @@ -1515,21 +1517,21 @@ nodeCapsInitNUMA(virCapsPtr caps) for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++) if (MASK_CPU_ISSET(mask, i)) - cpus[ncpus++] = i; + cpus[ncpus++].id = i; - if (virCapabilitiesAddHostNUMACell(caps, - n, - ncpus, - cpus) < 0) + if (virCapabilitiesAddHostNUMACell(caps, n, ncpus, cpus) < 0) goto cleanup; - - VIR_FREE(cpus); + cpus = NULL; } ret = 0; cleanup: - VIR_FREE(cpus); + if (cpus) { + while (ncpus--) + virCapabilitiesFreeHostNUMACellCPU(cpus + ncpus); + VIR_FREE(cpus); + } VIR_FREE(mask); VIR_FREE(allonesmask); return ret; diff --git a/src/qemu/qemu_process.c b/src/qemu/qemu_process.c index 2f08215..509cc39 100644 --- a/src/qemu/qemu_process.c +++ b/src/qemu/qemu_process.c @@ -2017,7 +2017,7 @@ qemuPrepareCpumap(virQEMUDriverPtr driver, if (result) { for (j = 0; j < cur_ncpus; j++) ignore_value(virBitmapSetBit(cpumap, - driver->caps->host.numaCell[i]->cpus[j])); + driver->caps->host.numaCell[i]->cpus[j].id)); } } } diff --git a/src/test/test_driver.c b/src/test/test_driver.c index a10a377..6909fa4 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -69,7 +69,7 @@ typedef struct _testDomainObjPrivate *testDomainObjPrivatePtr; struct _testCell { unsigned long mem; int numCpus; - int cpus[MAX_CPUS]; + virCapsHostNUMACellCPU cpus[MAX_CPUS]; }; typedef struct _testCell testCell; typedef struct _testCell *testCellPtr; @@ -174,8 +174,17 @@ testBuildCapabilities(virConnectPtr conn) { goto no_memory; for (i = 0; i < privconn->numCells; i++) { + virCapsHostNUMACellCPUPtr cpu_cells; + + if (VIR_ALLOC_N(cpu_cells, privconn->cells[i].numCpus) < 0) + goto no_memory; + + memcpy(cpu_cells, privconn->cells[i].cpus, + sizeof(*cpu_cells) * privconn->cells[i].numCpus); + + if (virCapabilitiesAddHostNUMACell(caps, i, privconn->cells[i].numCpus, - privconn->cells[i].cpus) < 0) + cpu_cells) < 0) goto no_memory; } @@ -549,7 +558,7 @@ static int testOpenDefault(virConnectPtr conn) { privconn->cells[u].mem = (u + 1) * 2048 * 1024; } for (u = 0 ; u < 16 ; u++) { - privconn->cells[u % 2].cpus[(u / 2)] = u; + privconn->cells[u % 2].cpus[(u / 2)].id = u; } if (!(privconn->caps = testBuildCapabilities(conn))) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index 038dd1e..d39d0b1 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1112,7 +1112,7 @@ sexpr_to_xend_topology(const struct sexpr *root, { const char *nodeToCpu; const char *cur; - int *cpuNums = NULL; + virCapsHostNUMACellCPUPtr cpuInfo = NULL; int cell, cpu, nb_cpus; int n = 0; int numCpus; @@ -1124,9 +1124,6 @@ sexpr_to_xend_topology(const struct sexpr *root, numCpus = sexpr_int(root, "node/nr_cpus"); - if (VIR_ALLOC_N(cpuNums, numCpus) < 0) - goto memory_error; - cur = nodeToCpu; while (*cur != 0) { virBitmapPtr cpuset = NULL; @@ -1155,31 +1152,36 @@ sexpr_to_xend_topology(const struct sexpr *root, goto error; } + if (VIR_ALLOC_N(cpuInfo, numCpus) < 0) + goto memory_error; + for (n = 0, cpu = 0; cpu < numCpus; cpu++) { bool used; ignore_value(virBitmapGetBit(cpuset, cpu, &used)); - if (used) - cpuNums[n++] = cpu; + if (used) { + cpuInfo[n].id = cpu; + n++; + } } virBitmapFree(cpuset); - if (virCapabilitiesAddHostNUMACell(caps, cell, nb_cpus, cpuNums) < 0) + if (virCapabilitiesAddHostNUMACell(caps, cell, nb_cpus, cpuInfo) < 0) goto memory_error; + cpuInfo = NULL; } - VIR_FREE(cpuNums); + return 0; parse_error: virReportError(VIR_ERR_XEN_CALL, "%s", _("topology syntax error")); error: - VIR_FREE(cpuNums); + VIR_FREE(cpuInfo); return -1; memory_error: - VIR_FREE(cpuNums); virReportOOMError(); - return -1; + goto error; } -- 1.8.1.1
On 01/19/2013 12:06 AM, Peter Krempa wrote:
This will allow storing additional topology data in the NUMA topology definition.
This patch changes the storage type and fixes fallback of the change across the drivers using it.
This patch also changes semantics of adding new NUMA cell information. Until now the data were re-allocated and copied to the topology definition. This patch changes the addition function to steal the pointer to a pre-allocated structure to simplify the code. --- src/conf/capabilities.c | 29 ++++++++++++++++++----------- src/conf/capabilities.h | 14 ++++++++++++-- src/nodeinfo.c | 22 ++++++++++++---------- src/qemu/qemu_process.c | 2 +- src/test/test_driver.c | 15 ++++++++++++--- src/xen/xend_internal.c | 24 +++++++++++++----------- 6 files changed, 68 insertions(+), 38 deletions(-)
[...]
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 19b99c6..124d8c1 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -84,12 +84,21 @@ struct _virCapsGuest { virCapsGuestFeaturePtr *features; };
+typedef struct _virCapsHostNUMACellCPU virCapsHostNUMACellCPU; +typedef virCapsHostNUMACellCPU *virCapsHostNUMACellCPUPtr; +struct _virCapsHostNUMACellCPU { + int id; + int socket_id; + int core_id; + char *siblings_list;
We don't actually need this for anything right now, but it would be nice to have it stored in virBitmap in case we'll want to work with it in the future. [...]
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 477104f..dffe7d1 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -79,9 +79,11 @@ freebsdNodeGetCPUCount(void) #ifdef __linux__ # define CPUINFO_PATH "/proc/cpuinfo" # define SYSFS_SYSTEM_PATH "/sys/devices/system" +# define SYSFS_CPU_PATH SYSFS_SYSTEM_PATH"/cpu"
s/PATH"/PATH "/
# define PROCSTAT_PATH "/proc/stat" # define MEMINFO_PATH "/proc/meminfo" # define SYSFS_MEMORY_SHARED_PATH "/sys/kernel/mm/ksm" +# define SYSFS_THREAD_SIBLINGS_LIST_LENGHT_MAX 1024
# define LINUX_NB_CPU_STATS 4 # define LINUX_NB_MEMORY_STATS_ALL 4 @@ -1476,9 +1478,10 @@ nodeCapsInitNUMA(virCapsPtr caps) int n; unsigned long *mask = NULL; unsigned long *allonesmask = NULL; - int *cpus = NULL; + virCapsHostNUMACellCPUPtr cpus = NULL; int ret = -1; int max_n_cpus = NUMA_MAX_N_CPUS; + int ncpus = 0;
if (numa_available() < 0) return 0; @@ -1492,7 +1495,6 @@ nodeCapsInitNUMA(virCapsPtr caps)
for (n = 0 ; n <= numa_max_node() ; n++) { int i; - int ncpus; /* The first time this returns -1, ENOENT if node doesn't exist... */ if (numa_node_to_cpus(n, mask, mask_n_bytes) < 0) { VIR_WARN("NUMA topology for cell %d of %d not available, ignoring", @@ -1515,21 +1517,21 @@ nodeCapsInitNUMA(virCapsPtr caps)
for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++) if (MASK_CPU_ISSET(mask, i)) - cpus[ncpus++] = i; + cpus[ncpus++].id = i;
- if (virCapabilitiesAddHostNUMACell(caps, - n, - ncpus, - cpus) < 0) + if (virCapabilitiesAddHostNUMACell(caps, n, ncpus, cpus) < 0) goto cleanup; - - VIR_FREE(cpus); + cpus = NULL;
I generally don't like stealing pointers because of similar contraptions. Even though it is clear from memory allocation POV and it saves a lot of allocations in this case, it's a bit harder to read IMHO. Being the devil's advocate, I must say I'd be nice to have the tests for this since you've started modifying the test driver. Also documentation for all the new information should be added into 'docs/formatcaps.html.in' with some explanation. Xen part of this patch looks OK, although I don't have any XEN machine to try it on. Because this and [PATCH 5/5] both require [PATCH 3/5] to be in, I'm cond-ACKing this patch with the same condition as [PATCH 3/5]. Martin
On 01/21/13 12:31, Martin Kletzander wrote:
On 01/19/2013 12:06 AM, Peter Krempa wrote:
This will allow storing additional topology data in the NUMA topology definition.
This patch changes the storage type and fixes fallback of the change across the drivers using it.
This patch also changes semantics of adding new NUMA cell information. Until now the data were re-allocated and copied to the topology definition. This patch changes the addition function to steal the pointer to a pre-allocated structure to simplify the code. --- src/conf/capabilities.c | 29 ++++++++++++++++++----------- src/conf/capabilities.h | 14 ++++++++++++-- src/nodeinfo.c | 22 ++++++++++++---------- src/qemu/qemu_process.c | 2 +- src/test/test_driver.c | 15 ++++++++++++--- src/xen/xend_internal.c | 24 +++++++++++++----------- 6 files changed, 68 insertions(+), 38 deletions(-)
[...]
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 19b99c6..124d8c1 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -84,12 +84,21 @@ struct _virCapsGuest { virCapsGuestFeaturePtr *features; };
+typedef struct _virCapsHostNUMACellCPU virCapsHostNUMACellCPU; +typedef virCapsHostNUMACellCPU *virCapsHostNUMACellCPUPtr; +struct _virCapsHostNUMACellCPU { + int id; + int socket_id; + int core_id; + char *siblings_list;
We don't actually need this for anything right now, but it would be nice to have it stored in virBitmap in case we'll want to work with it in the future.
[...]
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 477104f..dffe7d1 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -79,9 +79,11 @@ freebsdNodeGetCPUCount(void) #ifdef __linux__ # define CPUINFO_PATH "/proc/cpuinfo" # define SYSFS_SYSTEM_PATH "/sys/devices/system" +# define SYSFS_CPU_PATH SYSFS_SYSTEM_PATH"/cpu"
s/PATH"/PATH "/
# define PROCSTAT_PATH "/proc/stat" # define MEMINFO_PATH "/proc/meminfo" # define SYSFS_MEMORY_SHARED_PATH "/sys/kernel/mm/ksm" +# define SYSFS_THREAD_SIBLINGS_LIST_LENGHT_MAX 1024
# define LINUX_NB_CPU_STATS 4 # define LINUX_NB_MEMORY_STATS_ALL 4 @@ -1476,9 +1478,10 @@ nodeCapsInitNUMA(virCapsPtr caps) int n; unsigned long *mask = NULL; unsigned long *allonesmask = NULL; - int *cpus = NULL; + virCapsHostNUMACellCPUPtr cpus = NULL; int ret = -1; int max_n_cpus = NUMA_MAX_N_CPUS; + int ncpus = 0;
if (numa_available() < 0) return 0; @@ -1492,7 +1495,6 @@ nodeCapsInitNUMA(virCapsPtr caps)
for (n = 0 ; n <= numa_max_node() ; n++) { int i; - int ncpus; /* The first time this returns -1, ENOENT if node doesn't exist... */ if (numa_node_to_cpus(n, mask, mask_n_bytes) < 0) { VIR_WARN("NUMA topology for cell %d of %d not available, ignoring", @@ -1515,21 +1517,21 @@ nodeCapsInitNUMA(virCapsPtr caps)
for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++) if (MASK_CPU_ISSET(mask, i)) - cpus[ncpus++] = i; + cpus[ncpus++].id = i;
- if (virCapabilitiesAddHostNUMACell(caps, - n, - ncpus, - cpus) < 0) + if (virCapabilitiesAddHostNUMACell(caps, n, ncpus, cpus) < 0) goto cleanup; - - VIR_FREE(cpus); + cpus = NULL;
I generally don't like stealing pointers because of similar contraptions. Even though it is clear from memory allocation POV and it saves a lot of allocations in this case, it's a bit harder to read IMHO.
Well, yeah, it might be harder to read, but I wanted to avoid doing a deep copy on each iteration. And the callers of this function freed the original array anyways so I think it was really a waste of operations.
Being the devil's advocate, I must say I'd be nice to have the tests for this since you've started modifying the test driver. Also documentation for all the new information should be added into 'docs/formatcaps.html.in' with some explanation.
I'm planing on adding docs and tests, but I was waiting for a review to settle the format before doing so. (And I forgot to write about that)
Xen part of this patch looks OK, although I don't have any XEN machine to try it on.
Because this and [PATCH 5/5] both require [PATCH 3/5] to be in, I'm cond-ACKing this patch with the same condition as [PATCH 3/5].
I assume that you mean patch 2/5 :)
Martin
On 01/21/2013 02:10 PM, Peter Krempa wrote:
On 01/21/13 12:31, Martin Kletzander wrote:
[...]
Because this and [PATCH 5/5] both require [PATCH 3/5] to be in, I'm cond-ACKing this patch with the same condition as [PATCH 3/5].
I assume that you mean patch 2/5 :)
Sorry, of course I meant [2/5] instead of [3/5]
Martin
On Sat, Jan 19, 2013 at 12:06:41AM +0100, Peter Krempa wrote:
This will allow storing additional topology data in the NUMA topology definition.
This patch changes the storage type and fixes fallback of the change
s/fallback/fallout/
across the drivers using it.
This patch also changes semantics of adding new NUMA cell information. Until now the data were re-allocated and copied to the topology definition. This patch changes the addition function to steal the pointer to a pre-allocated structure to simplify the code. --- src/conf/capabilities.c | 29 ++++++++++++++++++----------- src/conf/capabilities.h | 14 ++++++++++++-- src/nodeinfo.c | 22 ++++++++++++---------- src/qemu/qemu_process.c | 2 +- src/test/test_driver.c | 15 ++++++++++++--- src/xen/xend_internal.c | 24 +++++++++++++----------- 6 files changed, 68 insertions(+), 38 deletions(-)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 0d2512e..814c4d6 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -65,12 +65,26 @@ virCapabilitiesNew(virArch hostarch, return caps; }
+void +virCapabilitiesFreeHostNUMACellCPU(virCapsHostNUMACellCPUPtr cpu) +{ + if (!cpu) + return; + + VIR_FREE(cpu->siblings_list); +} + static void virCapabilitiesFreeHostNUMACell(virCapsHostNUMACellPtr cell) { + int i;
s/int/size_t/
+ if (cell == NULL) return;
+ for (i = 0; i < cell->ncpus; i++) + virCapabilitiesFreeHostNUMACellCPU(cell->cpus + i); + VIR_FREE(cell->cpus); VIR_FREE(cell); }
diff --git a/src/conf/capabilities.h b/src/conf/capabilities.h index 19b99c6..124d8c1 100644 --- a/src/conf/capabilities.h +++ b/src/conf/capabilities.h @@ -84,12 +84,21 @@ struct _virCapsGuest { virCapsGuestFeaturePtr *features; };
+typedef struct _virCapsHostNUMACellCPU virCapsHostNUMACellCPU; +typedef virCapsHostNUMACellCPU *virCapsHostNUMACellCPUPtr; +struct _virCapsHostNUMACellCPU { + int id; + int socket_id; + int core_id;
s/int/unsigned/ since we don't need to store -ve numbers
+ char *siblings_list;
Agree that this would be nicer as a virBitmap
diff --git a/src/nodeinfo.c b/src/nodeinfo.c index 477104f..dffe7d1 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -79,9 +79,11 @@ freebsdNodeGetCPUCount(void) #ifdef __linux__ # define CPUINFO_PATH "/proc/cpuinfo" # define SYSFS_SYSTEM_PATH "/sys/devices/system" +# define SYSFS_CPU_PATH SYSFS_SYSTEM_PATH"/cpu" # define PROCSTAT_PATH "/proc/stat" # define MEMINFO_PATH "/proc/meminfo" # define SYSFS_MEMORY_SHARED_PATH "/sys/kernel/mm/ksm" +# define SYSFS_THREAD_SIBLINGS_LIST_LENGHT_MAX 1024
I think these two additions were meant for the next patch Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
This patch adds data gathering to the NUMA gathering files and adds support for outputting the data. The test driver and xend driver need to be adapted to fill sensible data to the structure. --- src/conf/capabilities.c | 14 +++++++++++-- src/nodeinfo.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++--- src/test/test_driver.c | 3 +++ src/xen/xend_internal.c | 3 +++ 4 files changed, 70 insertions(+), 5 deletions(-) diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 814c4d6..49eb0a7 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -698,9 +698,19 @@ virCapabilitiesFormatNUMATopology(virBufferPtr xml, for (i = 0; i < ncells; i++) { virBufferAsprintf(xml, " <cell id='%d'>\n", cells[i]->num); virBufferAsprintf(xml, " <cpus num='%d'>\n", cells[i]->ncpus); - for (j = 0; j < cells[i]->ncpus; j++) - virBufferAsprintf(xml, " <cpu id='%d'/>\n", + for (j = 0; j < cells[i]->ncpus; j++) { + virBufferAsprintf(xml, " <cpu id='%d'", cells[i]->cpus[j].id); + if (cells[i]->cpus[j].core_id >= 0 && + cells[i]->cpus[j].socket_id >= 0 && + cells[i]->cpus[j].siblings_list) + virBufferAsprintf(xml, " socket_id='%d' core_id='%d' siblings='%s'", + cells[i]->cpus[j].socket_id, + cells[i]->cpus[j].core_id, + cells[i]->cpus[j].siblings_list); + virBufferAddLit(xml, "/>\n"); + } + virBufferAddLit(xml, " </cpus>\n"); virBufferAddLit(xml, " </cell>\n"); } diff --git a/src/nodeinfo.c b/src/nodeinfo.c index dffe7d1..ec20609 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1472,6 +1472,52 @@ cleanup: # define MASK_CPU_ISSET(mask, cpu) \ (((mask)[((cpu) / n_bits(*(mask)))] >> ((cpu) % n_bits(*(mask)))) & 1) +static char * +virNodeGetSiblingsList(const char *dir, int cpu_id) +{ + char *path = NULL; + char *buf = NULL; + char *newline; + + if (virAsprintf(&path, "%s/cpu%u/topology/thread_siblings_list", + dir, cpu_id) < 0) { + virReportOOMError(); + goto error; + } + + if (virFileReadAll(path, SYSFS_THREAD_SIBLINGS_LIST_LENGHT_MAX, &buf) < 0) + goto error; + + if ((newline = strchr(buf, '\n'))) + *newline = '\0'; + + VIR_FREE(path); + return buf; + +error: + VIR_FREE(path); + VIR_FREE(buf); + return NULL; +} + +static int +virNodeCapsFillCPUInfo(int cpu_id, virCapsHostNUMACellCPUPtr cpu) +{ + cpu->id = cpu_id; + cpu->socket_id = virNodeGetCpuValue(SYSFS_CPU_PATH, cpu_id, + "topology/physical_package_id", -1); + cpu->core_id = virNodeGetCpuValue(SYSFS_CPU_PATH, cpu_id, + "topology/core_id", -1); + + if (cpu->socket_id == -1 || cpu->core_id == -1) + return 0; + + if (!(cpu->siblings_list = virNodeGetSiblingsList(SYSFS_CPU_PATH, cpu_id))) + return -1; + + return 0; +} + int nodeCapsInitNUMA(virCapsPtr caps) { @@ -1515,9 +1561,12 @@ nodeCapsInitNUMA(virCapsPtr caps) if (VIR_ALLOC_N(cpus, ncpus) < 0) goto cleanup; - for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++) - if (MASK_CPU_ISSET(mask, i)) - cpus[ncpus++].id = i; + for (ncpus = 0, i = 0 ; i < max_n_cpus ; i++) { + if (MASK_CPU_ISSET(mask, i)) { + if (virNodeCapsFillCPUInfo(i, cpus + ncpus++) < 0) + goto cleanup; + } + } if (virCapabilitiesAddHostNUMACell(caps, n, ncpus, cpus) < 0) goto cleanup; diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6909fa4..7693f75 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -559,6 +559,9 @@ static int testOpenDefault(virConnectPtr conn) { } for (u = 0 ; u < 16 ; u++) { privconn->cells[u % 2].cpus[(u / 2)].id = u; + privconn->cells[u % 2].cpus[(u / 2)].socket_id = -1; + privconn->cells[u % 2].cpus[(u / 2)].core_id = -1; + privconn->cells[u % 2].cpus[(u / 2)].siblings_list = NULL; } if (!(privconn->caps = testBuildCapabilities(conn))) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index d39d0b1..bfb02f7 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1161,6 +1161,9 @@ sexpr_to_xend_topology(const struct sexpr *root, ignore_value(virBitmapGetBit(cpuset, cpu, &used)); if (used) { cpuInfo[n].id = cpu; + cpuInfo[n].core_id = -1; + cpuInfo[n].socket_id = -1; + cpuInfo[n].siblings_list = NULL; n++; } } -- 1.8.1.1
On 01/19/2013 12:06 AM, Peter Krempa wrote:
This patch adds data gathering to the NUMA gathering files and adds support for outputting the data. The test driver and xend driver need to be adapted to fill sensible data to the structure. --- src/conf/capabilities.c | 14 +++++++++++-- src/nodeinfo.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++--- src/test/test_driver.c | 3 +++ src/xen/xend_internal.c | 3 +++ 4 files changed, 70 insertions(+), 5 deletions(-)
ACK with the same condition as [2/5] and [4/5] of course. Martin
On Sat, Jan 19, 2013 at 12:06:42AM +0100, Peter Krempa wrote:
This patch adds data gathering to the NUMA gathering files and adds support for outputting the data. The test driver and xend driver need to be adapted to fill sensible data to the structure. --- src/conf/capabilities.c | 14 +++++++++++-- src/nodeinfo.c | 55 ++++++++++++++++++++++++++++++++++++++++++++++--- src/test/test_driver.c | 3 +++ src/xen/xend_internal.c | 3 +++ 4 files changed, 70 insertions(+), 5 deletions(-)
diff --git a/src/conf/capabilities.c b/src/conf/capabilities.c index 814c4d6..49eb0a7 100644 --- a/src/conf/capabilities.c +++ b/src/conf/capabilities.c @@ -698,9 +698,19 @@ virCapabilitiesFormatNUMATopology(virBufferPtr xml, for (i = 0; i < ncells; i++) { virBufferAsprintf(xml, " <cell id='%d'>\n", cells[i]->num); virBufferAsprintf(xml, " <cpus num='%d'>\n", cells[i]->ncpus); - for (j = 0; j < cells[i]->ncpus; j++) - virBufferAsprintf(xml, " <cpu id='%d'/>\n", + for (j = 0; j < cells[i]->ncpus; j++) { + virBufferAsprintf(xml, " <cpu id='%d'", cells[i]->cpus[j].id); + if (cells[i]->cpus[j].core_id >= 0 && + cells[i]->cpus[j].socket_id >= 0 && + cells[i]->cpus[j].siblings_list) + virBufferAsprintf(xml, " socket_id='%d' core_id='%d' siblings='%s'", + cells[i]->cpus[j].socket_id, + cells[i]->cpus[j].core_id, + cells[i]->cpus[j].siblings_list); + virBufferAddLit(xml, "/>\n"); + } + virBufferAddLit(xml, " </cpus>\n"); virBufferAddLit(xml, " </cell>\n"); } diff --git a/src/nodeinfo.c b/src/nodeinfo.c index dffe7d1..ec20609 100644 --- a/src/nodeinfo.c +++ b/src/nodeinfo.c @@ -1472,6 +1472,52 @@ cleanup: # define MASK_CPU_ISSET(mask, cpu) \ (((mask)[((cpu) / n_bits(*(mask)))] >> ((cpu) % n_bits(*(mask)))) & 1)
+static char * +virNodeGetSiblingsList(const char *dir, int cpu_id)
Make this return a virBitmapPtr
diff --git a/src/test/test_driver.c b/src/test/test_driver.c index 6909fa4..7693f75 100644 --- a/src/test/test_driver.c +++ b/src/test/test_driver.c @@ -559,6 +559,9 @@ static int testOpenDefault(virConnectPtr conn) { } for (u = 0 ; u < 16 ; u++) { privconn->cells[u % 2].cpus[(u / 2)].id = u; + privconn->cells[u % 2].cpus[(u / 2)].socket_id = -1; + privconn->cells[u % 2].cpus[(u / 2)].core_id = -1; + privconn->cells[u % 2].cpus[(u / 2)].siblings_list = NULL; }
We should be able to fill in proper data here - after all in the test driver we can invent whatever data is required.
if (!(privconn->caps = testBuildCapabilities(conn))) diff --git a/src/xen/xend_internal.c b/src/xen/xend_internal.c index d39d0b1..bfb02f7 100644 --- a/src/xen/xend_internal.c +++ b/src/xen/xend_internal.c @@ -1161,6 +1161,9 @@ sexpr_to_xend_topology(const struct sexpr *root, ignore_value(virBitmapGetBit(cpuset, cpu, &used)); if (used) { cpuInfo[n].id = cpu; + cpuInfo[n].core_id = -1; + cpuInfo[n].socket_id = -1; + cpuInfo[n].siblings_list = NULL; n++;
Xen's nodeinfo provides data about the number of cores/sockets/threads which could be used to populate this data correctly. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (3)
-
Daniel P. Berrange -
Martin Kletzander -
Peter Krempa