Linux MinidumpWriter should properly set number_of_processors on ARM

Currently MinidumpWriter::WriteCPUInformation early-returns because it tries to
find the model/stepping/cpu family info from /proc/cpuinfo, which isn't present
on ARM. While it does this, it fails to set the number_of_processors field in
the SystemInfo. This patch changes it to only attempt to find these x86-specific
fields on x86/x86-64.

[digit]

lgtm.

For the record, on ARM, the 'processor' field only appears in /proc/cpuinfo on
multi-core devices. For single-core ones, it simply won't appear and the current
code will report 0 CPUs :-)

An alternative is to count the number of 'BogoMips' entries, or add a simple
check (e.g. is num_cpus < 0 then num_cpus = 1).

Also, /proc/cpuinfo only counts the number of active cores, and recent hardware
do dynamically shut down some of them to save power. If you're interested in the
total number of cores, you have to parse different files under /sys/

fwiw, I'm working on a more extensive patch to improve the quality ARM CPU
reporting, but this one is a good intermediate step.

[Ted Mielczarek]

On Fri, Aug 3, 2012 at 7:52 AM,  <digit@chromium.org> wrote:
> lgtm.
>
> For the record, on ARM, the 'processor' field only appears in
> /proc/cpuinfo on multi-core devices. For single-core ones, it simply
> won't appear and the current code will report 0 CPUs :-)

I was just reading the kernel source while writing this patch and I
totally missed that. I guess it depends on whether the kernel is built
without SMP support for those devices. Testing on a cheap ZTE armv6
phone I have confirms this.

> An alternative is to count the number of 'BogoMips' entries, or add a
> simple check (e.g. is num_cpus < 0 then num_cpus = 1).

This turns out to be trickier than I thought. :-/ If there aren't any
"processor" entries then this code will do the early-return as before.
Counting BogoMIPS entries would require a bit of rework on this code
because it expects to read the value out of the right hand side. (It
special-cases the "processor" field by incrementing it.)

Do you think it makes sense to land this patch as-is and fix the
non-SMP ARM case in a followup? (Or perhaps the patches you're working
on can fix this the right way?)

> Also, /proc/cpuinfo only counts the number of active cores, and recent
> hardware do dynamically shut down some of them to save power. If you're
> interested in the total number of cores, you have to parse different
> files under /sys/

This would probably be a good followup.

> fwiw, I'm working on a more extensive patch to improve the quality ARM
> CPU reporting, but this one is a good intermediate step.

Looking forward to it! I was investigating how hard it'd be to add the
ARM architecture into our reporting, since we're shipping on ARMv6
devices now, which is when I noticed this code. That was more work
than I wanted to invest in at the moment.

-Ted

[digit]

On Fri, Aug 3, 2012 at 4:56 PM, Ted Mielczarek <ted.mielczarek@gmail.com>wrote:

>
> > An alternative is to count the number of 'BogoMips' entries, or add a
> > simple check (e.g. is num_cpus < 0 then num_cpus = 1).
>
> This turns out to be trickier than I thought. :-/ If there aren't any
> "processor" entries then this code will do the early-return as before.
> Counting BogoMIPS entries would require a bit of rework on this code
> because it expects to read the value out of the right hand side. (It
> special-cases the "processor" field by incrementing it.)
>
> Ah, I havent' seen this.


> Do you think it makes sense to land this patch as-is and fix the
> non-SMP ARM case in a followup? (Or perhaps the patches you're working
> on can fix this the right way?)
>
> Fwiw, I've upload my patch at https://breakpad.appspot.com/427002/
Note that this doesn't parse the /sys/ files to get the correct amount of
CPU cores for node,
I think I'll add that later.


>
> -Ted
>

[Ted Mielczarek]

On Fri, Aug 3, 2012 at 11:33 AM, David Turner <digit@chromium.org> wrote:
> Fwiw, I've upload my patch at https://breakpad.appspot.com/427002/
> Note that this doesn't parse the /sys/ files to get the correct amount of
> CPU cores for node,
> I think I'll add that later.

Okay, I'll just drop this patch and review yours instead.

-Ted

[Ted Mielczarek]

I realized your patch stalled out, so I pushed this patch as a stopgap.