Change in vdsm[master]: Change source of CPU sockets/cores to "/sys" and add cpuThre...

danken at redhat.com danken at redhat.com
Tue Jul 10 21:26:40 UTC 2012


Dan Kenigsberg has posted comments on this change.

Change subject: Change source of CPU sockets/cores to "/sys" and add cpuThreads.
......................................................................


Patch Set 4: I would prefer that you didn't submit this

(7 inline comments)

only a partial review.

I'll need more introduction about what information you are trying to extract from /topology.

....................................................
Commit Message
Line 15: systems. Moreover adds the cpuThreads to getVdsCapabilities.
so why keep part of the code parsing /proc/cpuinfo?

....................................................
File vdsm/caps.py
Line 64:     def __init__(self, cpuinfo='/proc/cpuinfo', \
no need for trailing backslash here (and elsewhere inside parenthesis)

Line 82:             r = re.compile('^cpu[0-9]+')
as Gal mentioned, it would save some time to compile on import time.

Line 95:             # Cleaning the topology due to error.
do you ever expect /sys to be missing? we use it in so many places in the code. If it gives more information, and exists on all of our supported platforms, let's collect the information only from /sys/dev/system/cpu.

Line 101:     def siblings(self, cputopology, cpudir, siblingsfile):
I suspect it's only a helper function (so name it _sibling). Do you really need the siblings' identity? or wouldn't the count be enough?

Line 124:                                                                  self.sockets()
this multiplication assumes that all sockets are alike. do we want that?

Line 143:             return len(set([p.get('physical id', '0') \
why was it important to drop the temporary variable phys_ids? I found that it has improved readability.

--
To view, visit http://gerrit.ovirt.org/5481
To unsubscribe, visit http://gerrit.ovirt.org/settings

Gerrit-MessageType: comment
Gerrit-Change-Id: I1619e3d9e042bc801c988f099d3b84922f4e03d3
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Amador Pahim <apahim at redhat.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Gal Hammer <ghammer at redhat.com>
Gerrit-Reviewer: Michal Skrivanek <michal.skrivanek at redhat.com>
Gerrit-Reviewer: Shu Ming <shuming at linux.vnet.ibm.com>


More information about the vdsm-patches mailing list