Nir Soffer has posted comments on this change. Change subject: lib: daemon: cpu affinity support using taskset ......................................................................
Patch Set 3: (15 comments) https://gerrit.ovirt.org/#/c/45738/3/configure.ac File configure.ac: Line 312: AC_PATH_PROG([SU_PATH], [su], [/bin/su]) Line 313: AC_PATH_PROG([SYSCTL_PATH], [sysctl], [/sbin/sysctl]) Line 314: AC_PATH_PROG([SYSTEMD_RUN_PATH], [systemd-run], [/usr/bin/systemd-run]) Line 315: AC_PATH_PROG([TAR_PATH], [tar], [/bin/tar]) Line 316: AC_PATH_PROG([TASKSET_PATH], [taskset], [/usr/bin/taskset]) Why not use CommandPath? can avoid changes both here and in constants.py.in. Line 317: AC_PATH_PROG([TC_PATH], [tc], [/sbin/tc]) Line 318: AC_PATH_PROG([TEE_PATH], [tee], [/usr/bin/tee]) Line 319: AC_PATH_PROG([TOUCH_PATH], [touch], [/bin/touch]) Line 320: AC_PATH_PROG([TUNE2FS_PATH], [tune2fs], [/sbin/tune2fs]) https://gerrit.ovirt.org/#/c/45738/3/lib/vdsm/cmdutils.py File lib/vdsm/cmdutils.py: Line 73: command = [constants.EXT_TASKSET, cpumask] Line 74: else: Line 75: command = [constants.EXT_TASKSET, '-p', cpumask, '%s' % pid] Line 76: command.extend(cmd) Line 77: return command Too complicated, does too much stuff that we don't want to do here. We can do: def taskset(cmd, cpu_list): command = [TASKSET.cmd, "-c", ",".join(cpu_list)] command.extend(cmd) return command For setting and getting cpu affinity, lets add a tiny module: - taskset.set(pid, cpu_list) -> None - taskset.get(pid) -> [n...] Line 78: Line 79: Line 80: CPU_MIN = 0 Line 81: CPU_MAX = 63 # TODO: lift this constraint Line 92: if index < CPU_MIN or index > CPU_MAX: Line 93: raise ValueError('cpu index %i outside limits [%i, %i]', Line 94: index, CPU_MIN, CPU_MAX) Line 95: mask |= (1 << index) Line 96: return mask All this is not needed if we use --cpu-list option (avilable in el6) Line 97: Line 98: # This function returns truthy value if its argument contains unsafe characters Line 99: # for including in a command passed to the shell. The safe characters were Line 100: # stolen from pipes._safechars. https://gerrit.ovirt.org/#/c/45738/3/lib/vdsm/config.py.in File lib/vdsm/config.py.in: Line 207: Line 208: ('connection_stats_timeout', '3600', Line 209: 'Time in seconds defining how frequently we log transport stats'), Line 210: Line 211: ('cpu_allowed', '', cpu_affinity? Line 212: 'Comma separated whitelist of CPU cores on which VDSM is allowed ' Line 213: 'to run. Only the first 64 CPUs can be specified. ' Line 214: 'Default is empty list, meaning VDSM can be scheduled by the OS ' Line 215: 'to run on any core. ' https://gerrit.ovirt.org/#/c/45738/3/lib/vdsm/constants.py.in File lib/vdsm/constants.py.in: Line 137: EXT_SU = '@SU_PATH@' Line 138: EXT_SUDO = '@SUDO_PATH@' Line 139: Line 140: EXT_TAR = '@TAR_PATH@' Line 141: EXT_TASKSET = '@TASKSET_PATH@' Why not CommandPath? Line 142: EXT_TUNE2FS = '@TUNE2FS_PATH@' Line 143: Line 144: EXT_UMOUNT = '@UMOUNT_PATH@' Line 145: https://gerrit.ovirt.org/#/c/45738/3/lib/vdsm/utils.py File lib/vdsm/utils.py: Line 628: Line 629: def execCmd(command, sudo=False, cwd=None, data=None, raw=False, Line 630: printable=None, env=None, sync=True, nice=None, ioclass=None, Line 631: ioclassdata=None, setsid=False, execCmdLogger=logging.root, Line 632: deathSignal=0, childUmask=None, resetAffinity=True): resetCpuAffinity Line 633: """ Line 634: Executes an external command, optionally via sudo. Line 635: Line 636: IMPORTANT NOTE: the new process would receive `deathSignal` when the Line 640: """ Line 641: Line 642: if resetAffinity: Line 643: # only the main VDSM process whould be bound Line 644: command = cmdutils.taskset(command, cmdutils.CPU_ANY) This is needed only if config.cpu_affinity is not empty. Otherwise we just adding useless fork/exec to any command. We can do: if config.get('vars', 'cpu_affinity') and resetCpuAffinity: ... Line 645: Line 646: if ioclass is not None: Line 647: command = cmdutils.ionice(command, ioclass=ioclass, Line 648: ioclassdata=ioclassdata) https://gerrit.ovirt.org/#/c/45738/3/tests/cmdutilsTests.py File tests/cmdutilsTests.py: Line 86: self.assertEquals(cmdutils._list2cmdline([]), "") Line 87: Line 88: Line 89: @expandPermutations Line 90: class CpuMaskTests(VdsmTestCase): Nice tests, but not needed since we can use --cpu-list, no need to create a mask. Line 91: Line 92: @permutations([ Line 93: ['a'], Line 94: ['1,a'], https://gerrit.ovirt.org/#/c/45738/3/tests/utilsTests.py File tests/utilsTests.py: Line 963: Line 964: cpu_mask = 1 Line 965: rc1, _, stderr = utils.execCmd(cmdutils.taskset([], cpu_mask, Line 966: pid), Line 967: resetAffinity=False) Do not change this process, create a child process (with fork) and change the child mask. If we have a taskset module, this would become: taskset.set(os.getpid(), [1]) Line 968: self.assertEqual(rc1, 0) Line 969: Line 970: cmd = [constants.EXT_TASKSET, '-p', '%i' % pid] Line 971: rc2, stdout, _ = utils.execCmd(cmd, resetAffinity=False) Line 964: cpu_mask = 1 Line 965: rc1, _, stderr = utils.execCmd(cmdutils.taskset([], cpu_mask, Line 966: pid), Line 967: resetAffinity=False) Line 968: self.assertEqual(rc1, 0) If taskset.set() raises on failures, no assert needed... Line 969: Line 970: cmd = [constants.EXT_TASKSET, '-p', '%i' % pid] Line 971: rc2, stdout, _ = utils.execCmd(cmd, resetAffinity=False) Line 972: self.assertEqual(rc2, 0) Line 967: resetAffinity=False) Line 968: self.assertEqual(rc1, 0) Line 969: Line 970: cmd = [constants.EXT_TASKSET, '-p', '%i' % pid] Line 971: rc2, stdout, _ = utils.execCmd(cmd, resetAffinity=False) Whould be nicer as self.assertEqual(taskset.get(pid), [1]) Line 972: self.assertEqual(rc2, 0) Line 973: Line 974: self.assertEqual(stdout[0], Line 975: "pid %i's current affinity mask: %s" % ( https://gerrit.ovirt.org/#/c/45738/3/vdsm/vdsm File vdsm/vdsm: Line 249: "(stderr: %s). Verify sudoer rules configuration" % Line 250: (stderr)) Line 251: Line 252: Line 253: def _setAffinity(): _setCPUAffinity? Line 254: cpu_allowed = config.get('vars', 'cpu_allowed') Line 255: if not cpu_allowed: Line 256: return Line 257: Line 251: Line 252: Line 253: def _setAffinity(): Line 254: cpu_allowed = config.get('vars', 'cpu_allowed') Line 255: if not cpu_allowed: cpu_affinity is a better name, this is the term used everywhere (e.g. systemd, syscalls). Why invent new name? And the code would read better: if not cpu_affinity: return Line 256: return Line 257: Line 258: cpu_mask = cmdutils.cpu_mask_to_bits(cpu_allowed) Line 259: rc, _, stderr = utils.execCmd(cmdutils.taskset([], cpu_mask, Line 254: cpu_allowed = config.get('vars', 'cpu_allowed') Line 255: if not cpu_allowed: Line 256: return Line 257: Line 258: cpu_mask = cmdutils.cpu_mask_to_bits(cpu_allowed) Not needed (--cpu-list) Line 259: rc, _, stderr = utils.execCmd(cmdutils.taskset([], cpu_mask, Line 260: os.getpid()), Line 261: resetAffinity=False) Line 262: Line 255: if not cpu_allowed: Line 256: return Line 257: Line 258: cpu_mask = cmdutils.cpu_mask_to_bits(cpu_allowed) Line 259: rc, _, stderr = utils.execCmd(cmdutils.taskset([], cpu_mask, This works, but is too hacky :-) cmdutils.saskset is a modifier for running another command with taskset. It should not be used in the way you use it, and it should not have the pid argument. I think we need a module for running taskset on existing process, can be used also for the tests to get the cpu affinity. So this code will be something like: taskset.set(os.getpid(), cpu_affinity.split(',')) Which will raise taskset.Error, no need to check the return value. Line 260: os.getpid()), Line 261: resetAffinity=False) Line 262: Line 263: if rc != 0: -- 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: 3 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Adam Litke <ali...@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: Martin Sivák <msi...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <michal.skriva...@redhat.com> Gerrit-Reviewer: Michal Skrivanek <mskri...@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: automat...@ovirt.org Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
