Nir Soffer has posted comments on this change. Change subject: scale: limit cpu usage using cpu-affinity ......................................................................
Patch Set 22: Code-Review-1 (9 comments) https://gerrit.ovirt.org/#/c/45738/22/lib/vdsm/taskset.py File lib/vdsm/taskset.py: Line 42: We assume all threads of the process have the same affinity, because Line 43: this is the only usecase VDSM cares about - and requires. Line 44: Return a frozenset of ints, each one being a cpu indices on which the Line 45: process can run. Line 46: Example: frozenset(0, 1, 2, 3) Should be: frozenset([1, 2, 3]) Line 47: Raise taskset.Error on failure. Line 48: """ Line 49: command = [constants.EXT_TASKSET, '--pid', str(pid)] Line 50: https://gerrit.ovirt.org/#/c/45738/22/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 625: def __del__(self): Line 626: self._poller.close() Line 627: Line 628: Line 629: _ANY_CPU = ["0-%d" % (os.sysconf('SC_NPROCESSORS_ONLN') - 1)] See note about cpu indexes in testlib. Line 630: Line 631: Line 632: _USING_CPU_AFFINITY = config.get('vars', 'cpu_affinity') != "" Line 633: Line 627: Line 628: Line 629: _ANY_CPU = ["0-%d" % (os.sysconf('SC_NPROCESSORS_ONLN') - 1)] Line 630: Line 631: The blank lines between the constants is unneeded. Line 632: _USING_CPU_AFFINITY = config.get('vars', 'cpu_affinity') != "" Line 633: Line 634: Line 635: def execCmd(command, sudo=False, cwd=None, data=None, raw=False, https://gerrit.ovirt.org/#/c/45738/22/tests/tasksetTests.py File tests/tasksetTests.py: Line 54: def test_get(self): Line 55: Line 56: def _run_helper(): Line 57: self.running.set() Line 58: self.stop.wait() Having 2 lines helper, I don't think this worth a helper function, the inline code is very clear. Line 59: Line 60: self.proc = multiprocessing.Process(target=_run_helper) Line 61: self.proc.start() Line 62: if not self.running.wait(0.5): Line 113: Line 114: # TODO: find a clean way to make this a decorator Line 115: def validate_running_with_enough_cpus(cpu_set): Line 116: max_available_cpu = sorted(online_cpus())[-1] Line 117: max_required_cpu = sorted(cpu_set)[-1] See note about cpu indexes in testlib.py Line 118: Line 119: if max_available_cpu < max_required_cpu: Line 120: raise SkipTest( Line 121: "This test requires at least %i available CPUs" https://gerrit.ovirt.org/#/c/45738/22/tests/testlib.py File tests/testlib.py: Line 466: return wrapper Line 467: Line 468: Line 469: def online_cpus(): Line 470: return frozenset(range(os.sysconf('SC_NPROCESSORS_ONLN'))) I'm not sure it is correct to use the number of online cpus to construct a range of cpu numbers or check the highest cpu index. What if I have 4 cpus: [0, 1, 2, 3] And I disable cpu 1: sysconf('SC_NPROCESSORS_ONLN') will return now 3 - but what are the availale cpu numbers? [0, 1, 2] Or: [0, 2, 3] Please check. If the later is the case, we should use SC_NPROCESSORS_CONF instead for creating ranges or checking the highest cpu number. https://gerrit.ovirt.org/#/c/45738/22/tests/utilsTests.py File tests/utilsTests.py: Line 514: Line 515: Line 516: class ExecCmdAffinityTests(TestCaseBase): Line 517: Line 518: CPU_SET = frozenset((0,)) Minor: frozenset([0]) is nicer. Line 519: Line 520: @forked Line 521: @MonkeyPatch(utils, '_USING_CPU_AFFINITY', False) Line 522: def testResetAffinityByDefault(self): https://gerrit.ovirt.org/#/c/45738/22/vdsm/vdsm File vdsm/vdsm: Line 232: Line 233: def __set_cpu_affinity(): Line 234: cpu_affinity = config.get('vars', 'cpu_affinity') Line 235: if cpu_affinity: Line 236: cpu_list = [cpu.strip() for cpu in cpu_affinity.split(',')] Should be: cpu_set = frozenset([int(cpu.strip()) for cpu in cpu_affinity.split(",")) And we should fail if cpu_set is empty, or move the check after parsing the config: cpu_set = ... if not cpu_set: return In this case, we should parse also cpu affinify in utils for setting _USING_CPU_AFFINITY. If current code works - you cannot make it fail with empty input, I would leave it as is for now. Line 237: Line 238: log = logging.getLogger('vds') Line 239: log.info('VDSM will run with cpu affinity: %s', cpu_list) Line 240: Line 237: Line 238: log = logging.getLogger('vds') Line 239: log.info('VDSM will run with cpu affinity: %s', cpu_list) Line 240: Line 241: taskset.set(os.getpid(), cpu_list, all_tasks=True) This requires now iteratble of ints. Line 242: Line 243: Line 244: def main(): Line 245: __assertVdsmUser() -- To view, visit https://gerrit.ovirt.org/45738 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I3f7f68d65eddb5a21afbc3809ea79cd1dee67984 Gerrit-PatchSet: 22 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Adam Litke <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Ido Barkan <[email protected]> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <[email protected]> Gerrit-Reviewer: Martin Sivák <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Michal Skrivanek <[email protected]> Gerrit-Reviewer: Nir Soffer <[email protected]> Gerrit-Reviewer: Piotr Kliczewski <[email protected]> Gerrit-Reviewer: Vinzenz Feenstra <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
