On 01/01/15 17:51, Mark Goodwin wrote:
Fix for the kernel.pernode.cpu problem reported by Shirshendu Chakrabarti
where kernel.pernode.cpu.user was incorrectly zero on single CPU systems.
Plus a new QA test to verify the fix.
...
Mark,
Thanks for this. I've cherry picked these commits into my tree (and
will push them upstream after review). On a single CPU machine the QA
tests fails before and pass after the pmda change, so that's good.
For the code change, I have a couple of observations. For the most part
these are not in your changes but in the context around them ...
1. The last sprintf() in map_cpu_nodes() should be a snprintf() ... I
think this is part of our de facto virtual style guide, so I've made
that change.
2. This part at the head of map_cpu_nodes() confused me ...
pmdaIndom *idp = PMDAINDOM(NODE_INDOM);
for (i = 0; i < proc_cpuinfo->cpuindom->it_numinst; i++)
because proc_cpuinfo->cpuindom must be == idp for this to all make
sense, so I'd expect the references to the indom control struct in this
routine to all be consistent. I think using idp->it_numinst in the loop
control will work, and removes the inconsistency.
3. But my real question is about the sizing of the two arrays (which
need to end up the same), proc_cpuinfo->cpuinfo[] and idp->it_set[].
The latter is resized and initialized at the end of map_cpu_nodes() but
on entry to map_cpu_nodes() there is an assumption that
proc_cpuinfo->cpuinfo[] is already the big enough to accommodate all the
cpus found in the directory traversal. I think there needs to be a
comment describing why this assumption is expected to be true and/or a
check in the loop before
proc_cpuinfo->cpuinfo[cpu].node = node;
to ensure cpu is not too large
Cheers and Happy New Year, Ken.
|