Dan Kenigsberg has posted comments on this change. Change subject: BZ#833425 Change source of CPU sockets/cores to libvirt ......................................................................
Patch Set 16: I would prefer that you didn't submit this (7 inline comments) .................................................... Commit Message Line 3: AuthorDate: 2012-10-01 16:28:55 -0300 Line 4: Commit: Amador Pahim <[email protected]> Line 5: CommitDate: 2012-10-02 11:49:05 -0300 Line 6: Line 7: BZ#833425 Change source of CPU sockets/cores to libvirt I hate to be the one enforcing this, but please use the new tag Bug-Url: https://rbz.com/# Line 8: Line 9: /proc/cpuinfo is not enough to provide full CPU Line 10: sockets/cores/threads information in some systems, Line 11: e.g. multi NUMA AMD Magny-Cours processors. To bypass .................................................... File tests/capsTests.py Line 31: testPath = os.path.realpath(__file__) Line 32: dirName = os.path.split(testPath)[0] Line 33: path = os.path.join(dirName, "cpu_info.out") Line 34: c = caps.CpuInfo(path) Line 35: # Due to change sockets/cores to libvirt, the related "assert" that comment is not completely accurate. I believe that you are dumping the asserts since since libvirt reports a different value from the one expected by cpu_info.out. a better solution would be to supply a caps_libvirt.out with capabilities that match the cpu_info.out. If we use data from two different sources, they should at least match. Line 36: # was removed. Libvirt has enough tests. Line 37: self.assertEqual(set(c.flags()), set("""fpu vme de pse tsc msr pae Line 38: mce cx8 apic mtrr pge mca Line 39: cmov pat pse36 clflush dts .................................................... File vdsm/caps.py Line 88: self._topology = _getCpuTopology() Line 89: Line 90: def threads(self): Line 91: return (self._topology['numa'] * Line 92: self._topology['socket'] * you are still assuming that all numa cells are born equal. I believe that this is wrong. Some may have more sockets than others, etc. It is not a simple multiplications. Line 93: self._topology['core'] * Line 94: self._topology['thread']) Line 95: Line 96: def cores(self): Line 104: Line 105: Line 106: @utils.memoized Line 107: def _getCpuTopology(): Line 108: try: why's the double indent? Line 109: c = libvirtconnection.get() Line 110: caps = minidom.parseString(c.getCapabilities()) Line 111: host = caps.getElementsByTagName('host')[0] Line 112: cpu = host.getElementsByTagName('cpu')[0] Line 109: c = libvirtconnection.get() Line 110: caps = minidom.parseString(c.getCapabilities()) Line 111: host = caps.getElementsByTagName('host')[0] Line 112: cpu = host.getElementsByTagName('cpu')[0] Line 113: topology = {'numa': int(host.getElementsByTagName('cells')[0]. libvirt calls this "cells". I find the name clearer than "numa". Line 114: getAttribute('num')), Line 115: 'socket': int(cpu.getElementsByTagName('topology')[0]. Line 116: getAttribute('sockets')), Line 117: 'core': int(cpu.getElementsByTagName('topology')[0]. Line 118: getAttribute('cores')), Line 119: 'thread': int(cpu.getElementsByTagName('topology')[0]. Line 120: getAttribute('threads'))} Line 121: except: Line 122: logging.error('error calculating the topology data', exc_info=True) if possible, let the test framework supply a fake <capabilities> xml to this function. that would be much better than a catch-all except. Line 123: # During tests/make rpm, libvirt may not be available/configured. Line 124: # Hack arround it. Line 125: topology = {'numa': 1, Line 126: 'socket': 1, .................................................... File vdsm/vm.py Line 259: Runs Qemu in a subprocess and communicates with it, and monitors Line 260: its behaviour. Line 261: """ Line 262: log = logging.getLogger("vm.Vm") Line 263: _ongoingCreations = threading.BoundedSemaphore(caps.CpuTopology().cores()) oops, this requires a rebase due to http://gerrit.ovirt.org/8295 Line 264: MigrationSourceThreadClass = MigrationSourceThread Line 265: Line 266: def __init__(self, cif, params): Line 267: """ -- To view, visit http://gerrit.ovirt.org/5481 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I1619e3d9e042bc801c988f099d3b84922f4e03d3 Gerrit-PatchSet: 16 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Amador Pahim <[email protected]> Gerrit-Reviewer: Amador Pahim <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Doron Fediuck <[email protected]> Gerrit-Reviewer: Douglas Schilling Landgraf <[email protected]> Gerrit-Reviewer: Gal Hammer <[email protected]> Gerrit-Reviewer: Laszlo Hornyak <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Shu Ming <[email protected]> Gerrit-Reviewer: oVirt Jenkins CI Server _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
