Change in vdsm[master]: scale: limit cpu usage using cpu-affinity

fromani at redhat.com fromani at redhat.com
Mon Sep 7 12:49:59 UTC 2015


Francesco Romani has posted comments on this change.

Change subject: scale: limit cpu usage using cpu-affinity
......................................................................


Patch Set 5:

(15 comments)

https://gerrit.ovirt.org/#/c/45738/5/lib/vdsm/cmdutils.py
File lib/vdsm/cmdutils.py:

Line 69: 
Line 70: def taskset(cmd, cpu_list, all_tasks=True):
Line 71:     command = [constants.EXT_TASKSET,
Line 72:                '-a' if all_tasks else '',
Line 73:                '-c', ','.join(cpu_list)]
> I think we should use the long options for better readability in the code a
OK for long options. Unfortunately, taskset is quite picky about the order of the options:

  $ taskset --cpu-list 1,2 --all-tasks /bin/sleep 1m
taskset: failed to execute --all-tasks: No such file or directory
Line 74:     command.extend(cmd)
Line 75:     return command
Line 76: 
Line 77: 


https://gerrit.ovirt.org/#/c/45738/5/lib/vdsm/config.py.in
File lib/vdsm/config.py.in:

Line 209:             'Time in seconds defining how frequently we log transport stats'),
Line 210: 
Line 211:         ('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. '
> Are you sure about 64 limit? taskset(1) does not mention this. Maybe this i
right, let me drop this reference.
Line 214:             'Default is empty list, meaning VDSM can be scheduled by the OS '
Line 215:             'to run on any core. '
Line 216:             'Valid examples: "0,1", "0,2,3"')
Line 217: 


Line 210: 
Line 211:         ('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 '
> "empty list" is confusing - use ""?
Done
Line 215:             'to run on any core. '
Line 216:             'Valid examples: "0,1", "0,2,3"')
Line 217: 
Line 218:     ]),


https://gerrit.ovirt.org/#/c/45738/5/lib/vdsm/taskset.py
File lib/vdsm/taskset.py:

Line 26: 
Line 27: class Error(Exception):
Line 28:     def __init__(self, ecode, stderr):
Line 29:         self.ecode = ecode
Line 30:         self.stderr = stderr
> Lets use same attributes as in udevadm.Error, and include also the output o
Done. I can count at least three nearly-identical clones (udevadm, taskset, virtsparsify). We'll remove this duplication in a later patch.
Line 31: 
Line 32:     def __str__(self):
Line 33:         return '[Error %d] %s' % (self.ecode, self.stderr)
Line 34: 


Line 35: 
Line 36: def get(pid, all_tasks=True):
Line 37:     # TODO: check pid
Line 38:     cmd = [constants.EXT_TASKSET,
Line 39:            '-a' if all_tasks else '',
> Too dirty, lets simplify
I agree it is dirty, simplification is not trivial due to the taskset pickiness.
Line 40:            '-c',
Line 41:            '-p', str(pid)]
Line 42: 
Line 43:     rc, out, err = utils.execCmd(cmd)


Line 37:     # TODO: check pid
Line 38:     cmd = [constants.EXT_TASKSET,
Line 39:            '-a' if all_tasks else '',
Line 40:            '-c',
Line 41:            '-p', str(pid)]
> Use long arguments for more clear code and logs?
Done
Line 42: 
Line 43:     rc, out, err = utils.execCmd(cmd)
Line 44: 
Line 45:     if rc != 0:


Line 39:            '-a' if all_tasks else '',
Line 40:            '-c',
Line 41:            '-p', str(pid)]
Line 42: 
Line 43:     rc, out, err = utils.execCmd(cmd)
> This will run taskset within taskset, I think we can use resetCpuAffinity=F
OOps. stupid error. Thanks, fixed.
Line 44: 
Line 45:     if rc != 0:
Line 46:         raise Error(rc, err)
Line 47: 


Line 48:     return _expand_list(_extract_list(out))
Line 49: 
Line 50: 
Line 51: def set(pid, cpu_list, all_tasks=True):
Line 52:     # TODO: check pid
> Why kind of check? If you provide junk, the call will fail, so I'm not sure
Agreed. We should follow the GIGO (Garbage In, Garbage Out) rule. Let's remove it
Line 53:     # warning: the order of options is significant.
Line 54:     cmd = [constants.EXT_TASKSET,
Line 55:            '-a' if all_tasks else '',
Line 56:            '-p',


Line 51: def set(pid, cpu_list, all_tasks=True):
Line 52:     # TODO: check pid
Line 53:     # warning: the order of options is significant.
Line 54:     cmd = [constants.EXT_TASKSET,
Line 55:            '-a' if all_tasks else '',
> Dirty, simplify
I'd love to. See above/elsewhere.
Line 56:            '-p',
Line 57:            '-c', ','.join(cpu_list),
Line 58:            str(pid)]
Line 59: 


Line 52:     # TODO: check pid
Line 53:     # warning: the order of options is significant.
Line 54:     cmd = [constants.EXT_TASKSET,
Line 55:            '-a' if all_tasks else '',
Line 56:            '-p',
> -p should be before -c?
Yep. More taskset oddities. Example:

  68 14:30:55 fromani at musashi ~/Projects/upstream/vdsm $ taskset -a -p -c 1,2 4671
  pid 4671's current affinity list: 0-3
  pid 4671's new affinity list: 1,2

  69 14:31:24 fromani at musashi ~/Projects/upstream/vdsm $ taskset -a -c 1,2 -p 4671
  taskset: failed to execute -p: No such file or directory


(Here "4671" is my own shell)
Line 57:            '-c', ','.join(cpu_list),
Line 58:            str(pid)]
Line 59: 
Line 60:     rc, _, err = utils.execCmd(cmd)


Line 56:            '-p',
Line 57:            '-c', ','.join(cpu_list),
Line 58:            str(pid)]
Line 59: 
Line 60:     rc, _, err = utils.execCmd(cmd)
> Avoid running taskset within taskset
Done
Line 61: 
Line 62:     if rc != 0:
Line 63:         raise Error(rc, err)
Line 64: 


Line 68:     extractst the cpu list from taskset output, in the form
Line 69:       pid XXXX's current affinity list: CPU_LIST
Line 70:     """
Line 71:     text, cpu_list = taskset_output[0].split(': ', 1)
Line 72:     return cpu_list
> I think we can inline this into get()
Done
Line 73: 
Line 74: 
Line 75: def _expand_list(cpu_list):
Line 76:     """


Line 83:             for index in range(int(begin), int(end)+1):
Line 84:                 cpus.append(str(index))
Line 85:         else:
Line 86:             cpus.append(item)
Line 87:     return cpus
> Both can be simplified, see my comment in https://gerrit.ovirt.org/#/c/4573
Done


https://gerrit.ovirt.org/#/c/45738/5/lib/vdsm/utils.py
File lib/vdsm/utils.py:

Line 626:         self._poller.close()
Line 627: 
Line 628: 
Line 629: _MAX_CPU = 1023  # FIXME: lift this limit
Line 630: _ALL_CPU = range(_MAX_CPU)
> I share your dislike. How about:
My only concern here is how sysconf deals with hotplug, but (glancing at the manpage) should be OK.
Line 631: 
Line 632: 
Line 633: def execCmd(command, sudo=False, cwd=None, data=None, raw=False,
Line 634:             printable=None, env=None, sync=True, nice=None, ioclass=None,


Line 644:     """
Line 645: 
Line 646:     if resetCpuAffinity and config.get('vars', 'cpu_affinity'):
Line 647:         # only the main VDSM process whould be bound
Line 648:         command = cmdutils.taskset(command, _ALL_CPU)
> Why utils.pidStat can be broken?
there is one test that does logic on the 'comm' attribute, which holds argv[0]. (see utilsTests.PidStatTests).

I don't know yet if is just bad test or other code can be broken.
Line 649: 
Line 650:     if ioclass is not None:
Line 651:         command = cmdutils.ionice(command, ioclass=ioclass,
Line 652:                                   ioclassdata=ioclassdata)


-- 
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: 5
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Adam Litke <alitke at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Jenkins CI
Gerrit-Reviewer: Martin Polednik <mpolednik at redhat.com>
Gerrit-Reviewer: Martin Sivák <msivak at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <mskrivan at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Piotr Kliczewski <piotr.kliczewski at gmail.com>
Gerrit-Reviewer: Vinzenz Feenstra <vfeenstr at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-HasComments: Yes


More information about the vdsm-patches mailing list