Nir Soffer has posted comments on this change. Change subject: scale: limit cpu usage using cpu-affinity ......................................................................
Patch Set 21: (4 comments) https://gerrit.ovirt.org/#/c/45738/21/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 list of strings, each one being a cpu indices on which the Line 45: process can run. Line 46: Example: ['0', '1', '2', '3'] > It would be more noble (and in my opinion, safer) for this function to retu frozenset of ints would be nice, but now this does not match set() which would like get list of strings, where a string is either a cpu number "1" or a range "0-7". Using ranges is the best way when resetting the cpu affinity, as there is no way to reset the affinity except specifying all cpus. But everywhere else, working with frozenset of ints look nicer (e.g. testlib.online_cpus) Line 47: Raise taskset.Error on failure. Line 48: """ Line 49: command = [constants.EXT_TASKSET, '--pid', str(pid)] Line 50: Line 87: and return a list of strings, each one being is a cpu index. Line 88: """ Line 89: hexmask = line.rsplit(":", 1)[1].strip() Line 90: mask = int(hexmask, 16) Line 91: return sorted(str(i) for i in range(mask.bit_length()) if mask & (1 << i)) > why do we care about the order? strictly speaking, `taskset --pid` returns This list will actually be sorted by itself, because we iterate on the bits in order, so sorted is actually wrong. Lets return frozenset of ints https://gerrit.ovirt.org/#/c/45738/21/tests/tasksetTests.py File tests/tasksetTests.py: Line 57: self.proc = multiprocessing.Process(target=_run_helper) Line 58: self.proc.start() Line 59: self.assertTrue(self.running.wait(0.5)) Line 60: Line 61: self.assertEqual(taskset.get(self.proc.pid), online_cpus()) > this test would fail if the test runner is executed under a limiting taskse Right, lets compare with the taskset of the parent process. Line 62: Line 63: @permutations(_CPU_COMBINATIONS) Line 64: def test_set_from_parent(self, cpu_list): Line 65: https://gerrit.ovirt.org/#/c/45738/21/tests/testlib.py File tests/testlib.py: Line 467: Line 468: Line 469: def online_cpus(): Line 470: return sorted(str(index) for index in Line 471: range(os.sysconf('SC_NPROCESSORS_ONLN'))) This sort will not work we convert the index to string, and it seems that using ints is nicer any way, both here and as the return value of taskset.get(). Maybe we don't need this function at all, as the tests using it are incorrect - we should use the taskset of the parent process, which may be itself pinned to some cpus. -- 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: 21 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: Yaniv Kaul <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
