[libvirt] freecell gives out bytes instead of kilobytes
The virsh-command freecell hands out bytes but affixes those with kB. The error ist found in virsh.c on line 1663 and following. I have corrected this and altered the output to the method i chose for virt-manager - in the days of 96 GB per socket (Nehalem-EP) no one cares even about a fraction of a Megabyte. --- a/src/virsh.c 2009-04-02 17:45:50.000000000 +0200 +++ b/src/virsh.c 2009-04-02 23:35:54.000000000 +0200 @@ -1661,10 +1661,23 @@ } if (cell == -1) - vshPrint(ctl, "%s: %llu kB\n", _("Total"), memory); + if (memory < 10*1024*1204*1024) + { + vshPrint(ctl, "%s: %.0f MB\n", _("Total"), (float)memory/1024/1024); + } + else + { + vshPrint(ctl, "%s: %.2f GB\n", _("Total"), (float)memory/1024/1024/1024); + } else - vshPrint(ctl, "%d: %llu kB\n", cell, memory); - + if (memory < 10*1024*1204*1024) + { + vshPrint(ctl, "%d: %.0f MB\n", cell, (float)memory/1024/1024); + } + else + { + vshPrint(ctl, "%d: %.2f GB\n", cell, (float)memory/1024/1024/1024); + } return TRUE; } eg: rr016# virsh freecell; free -m 0: 24 MB total used free shared buffers cached Mem: 3884 3859 24 0 60 1306 -/+ buffers/cache: 2492 1392 Swap: 2047 248 1798 rr017# virsh freecell; free -m 0: 1490 MB total used free shared buffers cached Mem: 2991 1501 1489 0 100 1283 -/+ buffers/cache: 117 2873 Swap: 2047 0 2047 rr019# virsh freecell; free -m 0: 1616 MB total used free shared buffers cached Mem: 3952 2337 1615 0 5 1209 -/+ buffers/cache: 1121 2831 Swap: 2047 0 2047 Is this okay with you? Or just the fix for the output removing the kB-suffix.
Gerrit Slomma a écrit :
--- a/src/virsh.c 2009-04-02 17:45:50.000000000 +0200 +++ b/src/virsh.c 2009-04-02 23:35:54.000000000 +0200 @@ -1661,10 +1661,23 @@ }
if (cell == -1) - vshPrint(ctl, "%s: %llu kB\n", _("Total"), memory); + if (memory < 10*1024*1204*1024)
If I may, it should probably read "1024", not "1204" here.
+ { + vshPrint(ctl, "%s: %.0f MB\n", _("Total"), (float)memory/1024/1024); + } + else + { + vshPrint(ctl, "%s: %.2f GB\n", _("Total"), (float)memory/1024/1024/1024); + } else - vshPrint(ctl, "%d: %llu kB\n", cell, memory); - + if (memory < 10*1024*1204*1024)
and here.
+ { + vshPrint(ctl, "%d: %.0f MB\n", cell, (float)memory/1024/1024); + } + else + { + vshPrint(ctl, "%d: %.2f GB\n", cell, (float)memory/1024/1024/1024); + } return TRUE; }
Florian
Florian Vichot schrieb:
Gerrit Slomma a écrit :
if (cell == -1) - vshPrint(ctl, "%s: %llu kB\n", _("Total"), memory); + if (memory < 10*1024*1204*1024)
If I may, it should probably read "1024", not "1204" here. and here.
+ { + vshPrint(ctl, "%d: %.0f MB\n", cell, (float)memory/1024/1024);
Florian
Yes you are right.
On Fri, Apr 03, 2009 at 12:04:05AM +0200, Gerrit Slomma wrote:
The virsh-command freecell hands out bytes but affixes those with kB. The error ist found in virsh.c on line 1663 and following. I have corrected this and altered the output to the method i chose for virt-manager - in the days of 96 GB per socket (Nehalem-EP) no one cares even about a fraction of a Megabyte.
For command line tools like virsh I prefer to have it consistently report in the same units, so if someone wants to script it from the shell they don't have to concern themselves with changing units. All the other virsh commands report in KB, so the simple fix is to just divide by 1024. In checking this I discovered a whole bunch of other fun bugs in the NUMA support :-) The QEMU impl was not returning the correct return code - it used -1 instead of 0. The QEMU impl was also not setting an error if the requested cell was out of range. The libvirtd remote driver was not correctly seeing return value of -1 due to casting it to an unsigned int. virsh was not checking return values correctly either. So I propose the following patch.... Index: qemud/remote.c =================================================================== RCS file: /data/cvs/libvirt/qemud/remote.c,v retrieving revision 1.65 diff -u -p -r1.65 remote.c --- qemud/remote.c 16 Mar 2009 10:33:01 -0000 1.65 +++ qemud/remote.c 3 Apr 2009 10:18:38 -0000 @@ -662,6 +662,7 @@ remoteDispatchNodeGetCellsFreeMemory (st remote_node_get_cells_free_memory_args *args, remote_node_get_cells_free_memory_ret *ret) { + int err; if (args->maxCells > REMOTE_NODE_MAX_CELLS) { remoteDispatchFormatError (rerr, @@ -675,15 +676,16 @@ remoteDispatchNodeGetCellsFreeMemory (st return -1; } - ret->freeMems.freeMems_len = virNodeGetCellsFreeMemory(conn, - (unsigned long long *)ret->freeMems.freeMems_val, - args->startCell, - args->maxCells); - if (ret->freeMems.freeMems_len == 0) { + err = virNodeGetCellsFreeMemory(conn, + (unsigned long long *)ret->freeMems.freeMems_val, + args->startCell, + args->maxCells); + if (err <= 0) { VIR_FREE(ret->freeMems.freeMems_val); remoteDispatchConnError(rerr, conn); return -1; } + ret->freeMems.freeMems_len = err; return 0; } Index: src/qemu_driver.c =================================================================== RCS file: /data/cvs/libvirt/src/qemu_driver.c,v retrieving revision 1.223 diff -u -p -r1.223 qemu_driver.c --- src/qemu_driver.c 1 Apr 2009 09:54:20 -0000 1.223 +++ src/qemu_driver.c 3 Apr 2009 10:18:38 -0000 @@ -1878,15 +1878,23 @@ qemudNodeGetCellsFreeMemory(virConnectPt { int n, lastCell, numCells; int ret = -1; + int maxCell; if (numa_available() < 0) { qemudReportError(conn, NULL, NULL, VIR_ERR_NO_SUPPORT, "%s", _("NUMA not supported on this host")); goto cleanup; } + maxCell = numa_max_node(); + if (startCell > maxCell) { + qemudReportError(conn, NULL, NULL, VIR_ERR_INTERNAL_ERROR, + _("start cell %d out of range (0-%d)"), + startCell, maxCell); + goto cleanup; + } lastCell = startCell + maxCells - 1; - if (lastCell > numa_max_node()) - lastCell = numa_max_node(); + if (lastCell > maxCell) + lastCell = maxCell; for (numCells = 0, n = startCell ; n <= lastCell ; n++) { long long mem; @@ -1906,7 +1914,7 @@ cleanup: static unsigned long long qemudNodeGetFreeMemory (virConnectPtr conn) { - unsigned long long freeMem = -1; + unsigned long long freeMem = 0; int n; if (numa_available() < 0) { Index: src/virsh.c =================================================================== RCS file: /data/cvs/libvirt/src/virsh.c,v retrieving revision 1.198 diff -u -p -r1.198 virsh.c --- src/virsh.c 1 Apr 2009 09:52:59 -0000 1.198 +++ src/virsh.c 3 Apr 2009 10:18:38 -0000 @@ -1654,6 +1654,8 @@ cmdFreecell(vshControl *ctl, const vshCm cell = vshCommandOptInt(cmd, "cellno", &cell_given); if (!cell_given) { memory = virNodeGetFreeMemory(ctl->conn); + if (memory == 0) + return FALSE; } else { ret = virNodeGetCellsFreeMemory(ctl->conn, &memory, cell, 1); if (ret != 1) @@ -1661,9 +1663,9 @@ cmdFreecell(vshControl *ctl, const vshCm } if (cell == -1) - vshPrint(ctl, "%s: %llu kB\n", _("Total"), memory); + vshPrint(ctl, "%s: %llu kB\n", _("Total"), (memory/1024)); else - vshPrint(ctl, "%d: %llu kB\n", cell, memory); + vshPrint(ctl, "%d: %llu kB\n", cell, (memory/1024)); return TRUE; } Daniel -- |: Red Hat, Engineering, London -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Fri, Apr 03, 2009 at 11:23:59AM +0100, Daniel P. Berrange wrote:
On Fri, Apr 03, 2009 at 12:04:05AM +0200, Gerrit Slomma wrote:
The virsh-command freecell hands out bytes but affixes those with kB. The error ist found in virsh.c on line 1663 and following. I have corrected this and altered the output to the method i chose for virt-manager - in the days of 96 GB per socket (Nehalem-EP) no one cares even about a fraction of a Megabyte.
For command line tools like virsh I prefer to have it consistently report in the same units, so if someone wants to script it from the shell they don't have to concern themselves with changing units.
All the other virsh commands report in KB, so the simple fix is to just divide by 1024.
Agreed, let's not make it too hard for tools who would parse virsh output, even though we should not encourage that practice :-)
In checking this I discovered a whole bunch of other fun bugs in the NUMA support :-) The QEMU impl was not returning the correct return code - it used -1 instead of 0. The QEMU impl was also not setting an error if the requested cell was out of range. The libvirtd remote driver was not correctly seeing return value of -1 due to casting it to an unsigned int. virsh was not checking return values correctly either.
Heh !
So I propose the following patch....
ACK, fine by me ! Daniel -- Daniel Veillard | libxml Gnome XML XSLT toolkit http://xmlsoft.org/ daniel@veillard.com | Rpmfind RPM search engine http://rpmfind.net/ http://veillard.com/ | virtualization library http://libvirt.org/
Daniel P. Berrange schrieb:
For command line tools like virsh I prefer to have it consistently report in the same units, so if someone wants to script it from the shell they don't have to concern themselves with changing units.
All the other virsh commands report in KB, so the simple fix is to just divide by 1024.
A-OK.
participants (4)
-
Daniel P. Berrange -
Daniel Veillard -
Florian Vichot -
Gerrit Slomma