Nir Soffer has posted comments on this change. Change subject: lib: daemon: autodetect online cpus for affinity ......................................................................
Patch Set 8: Code-Review-1 (21 comments) https://gerrit.ovirt.org/#/c/49402/8/lib/vdsm/config.py.in File lib/vdsm/config.py.in: Line 206: ('connection_stats_timeout', '3600', Line 207: 'Time in seconds defining how frequently we log transport stats'), Line 208: Line 209: ('cpu_affinity', 'auto', Line 210: 'Comma separated whitelist of CPU cores, expressed as integers ' Describe the comma separated list format after auto, since auto is preferred. Line 211: 'starting from zero, or the special string value "auto".' Line 212: 'This options selects the CPU cores on which VDSM is allowed ' Line 213: 'to run. Use the empty value to disable the affinity, allowing ' Line 214: 'Vdsm to run on all the online cores. ' Line 209: ('cpu_affinity', 'auto', Line 210: 'Comma separated whitelist of CPU cores, expressed as integers ' Line 211: 'starting from zero, or the special string value "auto".' Line 212: 'This options selects the CPU cores on which VDSM is allowed ' Line 213: 'to run. Use the empty value to disable the affinity, allowing ' Describe the empty value last, as this is the worst option for users. Line 214: 'Vdsm to run on all the online cores. ' Line 215: 'The default is "auto", meaning Vdsm will pick the first online ' Line 216: 'core, starting with the second one. If only the first core is ' Line 217: 'online, it will pin on the first core.' Line 213: 'to run. Use the empty value to disable the affinity, allowing ' Line 214: 'Vdsm to run on all the online cores. ' Line 215: 'The default is "auto", meaning Vdsm will pick the first online ' Line 216: 'core, starting with the second one. If only the first core is ' Line 217: 'online, it will pin on the first core.' Describe the default first, as this is the option that most people will use. Line 218: 'Valid examples: "1", "0,1", "0,2,3", "auto"'), Line 219: Line 220: ('ssl_implementation', '@SSL_IMPLEMENTATION@', Line 221: 'Specifies which ssl implementation should be used. ' Line 214: 'Vdsm to run on all the online cores. ' Line 215: 'The default is "auto", meaning Vdsm will pick the first online ' Line 216: 'core, starting with the second one. If only the first core is ' Line 217: 'online, it will pin on the first core.' Line 218: 'Valid examples: "1", "0,1", "0,2,3", "auto"'), Show valid examples in order of preferences: "auto", "1", "1,2", "" Don't show 0,1 or 0,2,3, these are bad settings. Line 219: Line 220: ('ssl_implementation', '@SSL_IMPLEMENTATION@', Line 221: 'Specifies which ssl implementation should be used. ' Line 222: 'There are 2 options: ' https://gerrit.ovirt.org/#/c/49402/8/lib/vdsm/taskset.py File lib/vdsm/taskset.py: Line 24: from . import constants Line 25: from . import utils Line 26: Line 27: Line 28: AUTOMATIC = "auto" AUTO? make it look like the value. Line 29: Line 30: Line 31: _SYS_ONLINE_CPUS = "/sys/devices/system/cpu/online" Line 32: Line 26: Line 27: Line 28: AUTOMATIC = "auto" Line 29: Line 30: Les keep one or zero empty line between these constants. Line 31: _SYS_ONLINE_CPUS = "/sys/devices/system/cpu/online" Line 32: Line 33: Line 34: def get(pid): Line 81: """ Line 82: Return a frozenset which contains identifiers of online CPUs, Line 83: as non-negative integers. Line 84: """ Line 85: return _expand_range(_read_contents(_SYS_ONLINE_CPUS)) _read_contents does not add any value, why not inline it here? Line 86: Line 87: Line 88: def pick_cpu(cpu_set): Line 89: """ Line 84: """ Line 85: return _expand_range(_read_contents(_SYS_ONLINE_CPUS)) Line 86: Line 87: Line 88: def pick_cpu(cpu_set): Why pick a cpu from the given set, instead of reading the online cpus and returning the best cpu? Line 89: """ Line 90: Autoselect the best CPU VDSM should pin to. Line 91: `cpu_set' is any iterable which produces the sequence of all Line 92: available CPUs, among which VDSM should pick the best one. Line 86: Line 87: Line 88: def pick_cpu(cpu_set): Line 89: """ Line 90: Autoselect the best CPU VDSM should pin to. Autoselect -> Select Line 91: `cpu_set' is any iterable which produces the sequence of all Line 92: available CPUs, among which VDSM should pick the best one. Line 93: """ Line 94: cpu_list = sorted(cpu_set) Line 102: # Let's play nice, and try to find a quieter spot. Line 103: return cpu_list[1] Line 104: Line 105: # no special care, just pick the first one Line 106: return cpu_list[0] Do we really want to return the first online cpu? I think we can simplify, returning the second online cpu, or the single online cpu if only one cpu is online: return cpu_list[:2][-1] >>> [0,1,2][:2][-1] 1 >>> [1,2,3][:2][-1] 2 >>> [0][:2][-1] 0 Line 107: Line 108: Line 109: def _cpu_set_from_output(line): Line 110: """ Line 116: mask = int(hexmask, 16) Line 117: return frozenset(i for i in range(mask.bit_length()) if mask & (1 << i)) Line 118: Line 119: Line 120: def _expand_range(cpu_range): Lets name it after the kernel function doing the same thing, cpulist_parse(). See https://www.kernel.org/doc/Documentation/cputopology.txt Line 121: """ Line 122: Expand the range syntax (e.g. 0-2,5) into a plain Line 123: frozenset of integers (e.g. frozenset([0,1,2,5])) Line 124: The input format is like the content of the special file Line 131: begin, end = item.split('-', 1) Line 132: cpus.extend(range(int(begin), int(end)+1)) Line 133: else: Line 134: cpus.append(int(item)) Line 135: return frozenset(cpus) This is very nice and clear, but maybe we can make a little bit nicer like this: def parse(item): if '-' in item: begin, end = items.split(',', 1) return range(int(begin), int(end) + 1) else: return int(item) return frozenset(parse(item) for item in data.split(',')) Line 136: Line 137: Line 138: def _read_contents(path): Line 139: with open(path, 'rt') as src: Line 135: return frozenset(cpus) Line 136: Line 137: Line 138: def _read_contents(path): Line 139: with open(path, 'rt') as src: Why t? Line 136: Line 137: Line 138: def _read_contents(path): Line 139: with open(path, 'rt') as src: Line 140: return src.read() Should be readline() https://gerrit.ovirt.org/#/c/49402/8/tests/tasksetTests.py File tests/tasksetTests.py: Line 105: Line 106: @expandPermutations Line 107: class OnlineCpusFunctionsTests(VdsmTestCase): Line 108: Line 109: @permutations([ Permutations are more clear when we add a comment like this: # raw_value, cpu_set Line 110: ['0', set((0,))], Line 111: ['0,1,2,3', set(range(4))], Line 112: ['0-3', set(range(4))], Line 113: ['0-1,3', set((0, 1, 3))], Line 117: Line 118: def _fake_sys(unused): Line 119: return raw_value Line 120: Line 121: with MonkeyPatchScope([(taskset, "_read_contents", _fake_sys)]): Lets monkeypatch the path (/sys/devices...), so online cpu will read from our temporary file instead. This way we check also the code reading from the file, and we don't need to add unneeded function line _read_contents. Line 122: self.assertEqual(taskset.online_cpus(), cpu_set) Line 123: Line 124: def test_online_cpus_sparse_ppc64(self): Line 125: # as seen on ppc64 20151130 Line 120: Line 121: with MonkeyPatchScope([(taskset, "_read_contents", _fake_sys)]): Line 122: self.assertEqual(taskset.online_cpus(), cpu_set) Line 123: Line 124: def test_online_cpus_sparse_ppc64(self): Why do we need separate test? this can be just another permutation. Line 125: # as seen on ppc64 20151130 Line 126: cpus = set( Line 127: (8, 16, 24, 32, 40, 48, 56, 64, 72, 80, Line 128: 88, 96, 104, 112, 120, 128, 136, 144, 152) Line 137: @permutations([ Line 138: [frozenset((0,)), 0], Line 139: [frozenset((1,)), 1], Line 140: [frozenset(range(4)), 1], Line 141: [frozenset(range(1, 4)), 1], We should pick 4, 1 is probably more busy Line 142: [frozenset(range(3, 9)), 3], Line 143: ]) Line 144: def test_pick_cpu(self, cpu_set, expected): Line 145: self.assertEqual(taskset.pick_cpu(cpu_set), expected) Line 138: [frozenset((0,)), 0], Line 139: [frozenset((1,)), 1], Line 140: [frozenset(range(4)), 1], Line 141: [frozenset(range(1, 4)), 1], Line 142: [frozenset(range(3, 9)), 3], Same Line 143: ]) Line 144: def test_pick_cpu(self, cpu_set, expected): Line 145: self.assertEqual(taskset.pick_cpu(cpu_set), expected) Line 146: https://gerrit.ovirt.org/#/c/49402/8/vdsm/vdsm File vdsm/vdsm: Line 215 Line 216 Line 217 Line 218 Line 219 Lets also fix this condition to early return style in another patch: if cpu_affinity == "": return Line 216: Line 217: def __set_cpu_affinity(): Line 218: cpu_affinity = config.get('vars', 'cpu_affinity') Line 219: if cpu_affinity != "": Line 220: online_cpus = taskset.online_cpus() Lets bail out here (with debug log) - if we have one cpu, user setting does not matter. if len(oneline_cpus) == 1: log.debug... return Line 221: Line 222: if cpu_affinity.lower() == taskset.AUTOMATIC: Line 223: cpu_set = frozenset((taskset.pick_cpu(online_cpus),)) Line 224: else: -- To view, visit https://gerrit.ovirt.org/49402 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: Iba834f67a43ce766308036cbd079107340ed69d8 Gerrit-PatchSet: 8 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Dan Kenigsberg <dan...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik <mpoled...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@redhat.com> Gerrit-Reviewer: Milan Zamazal <mzama...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczew...@gmail.com> Gerrit-Reviewer: Vinzenz Feenstra <vfeen...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: gerrit-hooks <automat...@ovirt.org> Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
