Parsing /proc/cpuinfo for model on Android and Aura

Parsing /proc/cpuinfo for cpu branding information on Android and Aura.

BUG=313454
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=266575

[rptr, 2014-03-06 06:34:48]

Hello ,

This fix parses /proc/cpuinfo for cpu branding information on Android/Aura.
As per discussion posted on crbug.com/313454 we need to parse cpuinfo before
sandbox is created.
Is there any reason to parse /proc/cpuinfo before sandbox creation?.

We checked that with this patch we can parse /proc/cpuinfo even after sandbox is
created for eg. after Render process is created, in RenderWidget::OnWasShown().

With this patch we were able to get cpu_brand info in RenderWidget::OnWasShown()
using follwing snippet even after render process is created.

void RenderWidget::OnWasShown(bool needs_repainting) {
		base::CPU cpu_info;
		VLOG(0)<<"cpu_brand_ = "<<cpu_info.cpu_brand(); // cpu_brand_ = ARMv7
Processor rev 2 (v7l)
		

Please take a look at this patch.

Regards,
Puttaraju

[Mark Mentovai, 2014-03-06 14:41:55]

The message you sent with your code review request says

> This fix parses /proc/cpuinfo for cpu branding information on Android/Aura.
> As per discussion posted on crbug.com/313454 we need to parse cpuinfo
> before sandbox is created.
> Is there any reason to parse /proc/cpuinfo before sandbox creation?.
>
> We checked that with this patch we can parse /proc/cpuinfo even after
> sandbox is created for eg. after Render process is created, in
> RenderWidget::OnWasShown().

Some of this seems to contradict itself. It makes an assertion that cpuinfo
needs to be parsed before the sandbox is created, then it asks whether cpuinfo
needs to be parsed before the sandbox is created, and finally it asserts that
cpuinfo can be parsed after the sandbox is created.

https://codereview.chromium.org/188443003/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/188443003/diff/1/AUTHORS#newcode253
AUTHORS:253: Puttaraju R <puttaraju.r@samsung.com>
You don’t show up in the list of contributors Samsung has approved with its
corporate contributor license agreement. Probably Laszlo can correct this.

https://codereview.chromium.org/188443003/diff/1/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/188443003/diff/1/base/cpu.cc#newcode165
base/cpu.cc:165: ReadFileToString(FilePath("/proc/cpuinfo"), &contents);
How is this file formatted on ARM?

It’s probably too long to stick in a comment, but it’s helpful during review to
know what the entire file, and specifically the Processor line, actually looks
like.

[piman, 2014-03-06 20:58:17]

+jln: is it expected that we can open and read /proc/cpuinfo from the sandbox?

[jln (very slow on Chromium), 2014-03-06 23:08:05]

On 2014/03/06 20:58:17, piman wrote:
> +jln: is it expected that we can open and read /proc/cpuinfo from the sandbox?

On Android, yes (for now), on Linux and Chrome OS, no.

In the past, we've added "caching" helpers in base/ that would get "pre-warmed"
before the sandbox is engaged.

For instance, see https://codereview.chromium.org/55913004/.

[vivekg, 2014-03-07 11:59:52]

https://codereview.chromium.org/188443003/diff/1/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/188443003/diff/1/base/cpu.cc#newcode173
base/cpu.cc:173: cpu_brand_.assign(line.substr(pos + 2));
You probably need to break here to avoid processing further.

[Mark Mentovai, 2014-03-07 15:19:18]

Good catch. This too:

https://codereview.chromium.org/188443003/diff/1/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/188443003/diff/1/base/cpu.cc#newcode172
base/cpu.cc:172: size_t pos = line.find(": ");
What if ": " was not found?

[rptr, 2014-03-08 06:13:44]

On 2014/03/06 23:08:05, jln wrote:
> On 2014/03/06 20:58:17, piman wrote:
> > +jln: is it expected that we can open and read /proc/cpuinfo from the
sandbox?
> 
> On Android, yes (for now), on Linux and Chrome OS, no.
> 
> In the past, we've added "caching" helpers in base/ that would get
"pre-warmed"
> before the sandbox is engaged.
> 
> For instance, see https://codereview.chromium.org/55913004/.

We can trigger caching of CPU Model for linux from PreSandboxInit() similar to
above fix.
For Android we can perform this caching by overriding PreSandboxStartup() in
ChromeMainDelegateAndroid instead of doing inside sandbox.
This will access /proc/cpuinfo from outside sandbox for android.
//chrome_main_delegate_android.cc
void ChromeMainDelegateAndroid::PreSandboxStartup(){
  ChromeMainDelegate::PreSandboxStartup();
  base::CPU::SetCPUBrand();
}

[rptr, 2014-03-08 06:26:59]

As commented by jln, piman we can access /proc/cpuinfo from inside sandbox in
Android but not possible in Linux/chrome - Aura.
This is the reason i was able to access cpuinfo from RenderWidget.

I will upload one more fix which will access /proc/cpuinfo from outside sandbox
both for linux and Android.
We can cache CPU model caching in PreSandboxInit() similar to
https://codereview.chromium.org/55913004/

For Android we can perform caching CPU model by implementing
ChromeMainDelegateAndroid::PreSandboxStartup() which will happen before sandbox
init.

https://codereview.chromium.org/188443003/diff/1/AUTHORS
File AUTHORS (right):

https://codereview.chromium.org/188443003/diff/1/AUTHORS#newcode253
AUTHORS:253: Puttaraju R <puttaraju.r@samsung.com>
On 2014/03/06 14:41:55, Mark Mentovai wrote:
> You don’t show up in the list of contributors Samsung has approved with its
> corporate contributor license agreement. Probably Laszlo can correct this.

We have requested Laszlo to add name to Samsung contributors list, i will update
on the same.

https://codereview.chromium.org/188443003/diff/1/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/188443003/diff/1/base/cpu.cc#newcode165
base/cpu.cc:165: ReadFileToString(FilePath("/proc/cpuinfo"), &contents);
This is how /proc/cpuinfo looks in Samsung Galaxy S2.

Processor       : ARMv7 Processor rev 1 (v7l)
processor       : 0
BogoMIPS        : 1592.52

processor       : 1
BogoMIPS        : 2388.78

Features        : swp half thumb fastmult vfp edsp neon vfpv3 tls
CPU implementer : 0x41
CPU architecture: 7
CPU variant     : 0x2
CPU part        : 0xc09
CPU revision    : 1

Hardware        : SMDK4210
Revision        : 000e
Serial          : *****

Same format is seen in all other android devices which i checked(Nexus S, Note
etc).

[rptr, 2014-03-08 13:03:06]

Please take a look at this patch.
Fix takes care of review comments and caching cpu_brand information.

https://codereview.chromium.org/188443003/diff/20001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/188443003/diff/20001/base/cpu.cc#newcode105
base/cpu.cc:105: if (pos != std::string::npos) {
Takes care of review comment if ": " is not found in Processor string.

https://codereview.chromium.org/188443003/diff/20001/base/cpu.cc#newcode107
base/cpu.cc:107: break;
Takes care of review comment: Do file lookup untill Processor line is found or
EOF.

https://codereview.chromium.org/188443003/diff/20001/base/cpu.cc#newcode115
base/cpu.cc:115: template <typename T, T (*F)(void)>
Perform cpu brand caching using lazy instance as suggested by review comments.

https://codereview.chromium.org/188443003/diff/20001/base/cpu.h
File base/cpu.h (right):

https://codereview.chromium.org/188443003/diff/20001/base/cpu.h#newcode61
base/cpu.h:61: static std::string CpuBrandInfo();
Used for caching CPU brand string before sandbox is initialized.

https://codereview.chromium.org/188443003/diff/20001/content/shell/app/shell_...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/188443003/diff/20001/content/shell/app/shell_...
content/shell/app/shell_main_delegate.cc:189: #if defined(ARCH_CPU_ARM_FAMILY)
&& defined(OS_ANDROID)
This initializes cpu brand info only for content shell. 
Need to move this to a common PreSandboxStarup() if there is one for other apps
as well. 
And this should happen only once for browser instance before sandbox init.
Please do suggest for the same.

[Mark Mentovai, 2014-03-10 14:16:23]

https://codereview.chromium.org/188443003/diff/20001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/188443003/diff/20001/base/cpu.cc#newcode115
base/cpu.cc:115: template <typename T, T (*F)(void)>
Why is this templated? You only use it once.

https://codereview.chromium.org/188443003/diff/20001/base/cpu.cc#newcode131
base/cpu.cc:131: std::string CPU::CpuBrandInfo() { return
g_lazy_cpu_brand.Get().value(); }
Why not a const string&?

https://codereview.chromium.org/188443003/diff/20001/base/cpu.cc#newcode208
base/cpu.cc:208: #if defined(OS_ANDROID) || defined(USE_AURA)
What’s this got to do with Aura?

https://codereview.chromium.org/188443003/diff/20001/base/cpu.h
File base/cpu.h (right):

https://codereview.chromium.org/188443003/diff/20001/base/cpu.h#newcode61
base/cpu.h:61: static std::string CpuBrandInfo();
ptr wrote:
> Used for caching CPU brand string before sandbox is initialized.

A comment like that would belong in the code, not in a code review comment. But…

It seems that this is an implementation detail, and it doesn’t need to be
exposed from this class.

The “sandbox warmup” can simply be creating an instance of a CPU class, since
the constructor does what’s necessary to populate cpu_brand_. You don’t need
this to be exposed separately.

[rptr, 2014-03-11 15:47:13]

Please take a look at this patch.

https://codereview.chromium.org/188443003/diff/20001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/188443003/diff/20001/base/cpu.cc#newcode115
base/cpu.cc:115: template <typename T, T (*F)(void)>
On 2014/03/10 14:16:24, Mark Mentovai wrote:
> Why is this templated? You only use it once.

Done.

https://codereview.chromium.org/188443003/diff/20001/base/cpu.cc#newcode131
base/cpu.cc:131: std::string CPU::CpuBrandInfo() { return
g_lazy_cpu_brand.Get().value(); }
On 2014/03/10 14:16:24, Mark Mentovai wrote:
> Why not a const string&?

Done.

https://codereview.chromium.org/188443003/diff/20001/base/cpu.cc#newcode208
base/cpu.cc:208: #if defined(OS_ANDROID) || defined(USE_AURA)
My understanding is that (from comments of piman and siva in crbug:313454)
ARCH_CPU_ARM is currently run on android and linux with aura. Please correct me
if its wrong.

https://codereview.chromium.org/188443003/diff/20001/base/cpu.h
File base/cpu.h (right):

https://codereview.chromium.org/188443003/diff/20001/base/cpu.h#newcode61
base/cpu.h:61: static std::string CpuBrandInfo();
Done.

[Mark Mentovai, 2014-03-11 16:17:48]

https://codereview.chromium.org/188443003/diff/40001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/188443003/diff/40001/base/cpu.cc#newcode92
base/cpu.cc:92: #if defined(OS_ANDROID) || defined(USE_AURA)
This code is valid on any ARM Linux, right? That’s why I’m asking if Aura is
really the right thing to key off of. Isn’t OS_ANDROID || OS_LINUX really what
you mean?

https://codereview.chromium.org/188443003/diff/40001/base/cpu.cc#newcode94
base/cpu.cc:94: std::string ParseCpuInfo() {
All of this should be in the unnamed namespace.

https://codereview.chromium.org/188443003/diff/40001/content/shell/app/shell_...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/188443003/diff/40001/content/shell/app/shell_...
content/shell/app/shell_main_delegate.cc:190: #if defined(OS_ANDROID) ||
defined(USE_AURA)
Pull this whole thing into a single #ifdef:

#if defined(ARCH_CPU_ARM_FAMILY) && (defined(OS_ANDROID) || defined(OS_LINUX))

https://codereview.chromium.org/188443003/diff/40001/content/shell/app/shell_...
content/shell/app/shell_main_delegate.cc:191: // Create an instance CPU class to
parse /proc/cpuinfo and cache cpu_brand
“instance of the CPU class”

https://codereview.chromium.org/188443003/diff/40001/content/zygote/zygote_ma...
File content/zygote/zygote_main_linux.cc (right):

https://codereview.chromium.org/188443003/diff/40001/content/zygote/zygote_ma...
content/zygote/zygote_main_linux.cc:302: #if defined(ARCH_CPU_ARM_FAMILY) &&
defined(USE_AURA)
Probably just ARCH_CPU_ARM_FAMILY.

https://codereview.chromium.org/188443003/diff/40001/content/zygote/zygote_ma...
content/zygote/zygote_main_linux.cc:303: // Create an instance of CPU class to
parse /proc/cpuinfo and cache cpu_brand
As before.

[rptr, 2014-03-12 15:26:24]

PTAL.

https://codereview.chromium.org/188443003/diff/40001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/188443003/diff/40001/base/cpu.cc#newcode92
base/cpu.cc:92: #if defined(OS_ANDROID) || defined(USE_AURA)
On 2014/03/11 16:17:48, Mark Mentovai wrote:
> This code is valid on any ARM Linux, right? That’s why I’m asking if Aura is
> really the right thing to key off of. Isn’t OS_ANDROID || OS_LINUX really what
> you mean?

Done.

https://codereview.chromium.org/188443003/diff/40001/base/cpu.cc#newcode94
base/cpu.cc:94: std::string ParseCpuInfo() {
On 2014/03/11 16:17:48, Mark Mentovai wrote:
> All of this should be in the unnamed namespace.

Done.

https://codereview.chromium.org/188443003/diff/40001/content/shell/app/shell_...
File content/shell/app/shell_main_delegate.cc (right):

https://codereview.chromium.org/188443003/diff/40001/content/shell/app/shell_...
content/shell/app/shell_main_delegate.cc:190: #if defined(OS_ANDROID) ||
defined(USE_AURA)
On 2014/03/11 16:17:48, Mark Mentovai wrote:
> Pull this whole thing into a single #ifdef:
> 
> #if defined(ARCH_CPU_ARM_FAMILY) && (defined(OS_ANDROID) || defined(OS_LINUX))

Done.

https://codereview.chromium.org/188443003/diff/40001/content/shell/app/shell_...
content/shell/app/shell_main_delegate.cc:191: // Create an instance CPU class to
parse /proc/cpuinfo and cache cpu_brand
On 2014/03/11 16:17:48, Mark Mentovai wrote:
> “instance of the CPU class”

Done.

https://codereview.chromium.org/188443003/diff/40001/content/zygote/zygote_ma...
File content/zygote/zygote_main_linux.cc (right):

https://codereview.chromium.org/188443003/diff/40001/content/zygote/zygote_ma...
content/zygote/zygote_main_linux.cc:302: #if defined(ARCH_CPU_ARM_FAMILY) &&
defined(USE_AURA)
On 2014/03/11 16:17:48, Mark Mentovai wrote:
> Probably just ARCH_CPU_ARM_FAMILY.

Done.

https://codereview.chromium.org/188443003/diff/40001/content/zygote/zygote_ma...
content/zygote/zygote_main_linux.cc:303: // Create an instance of CPU class to
parse /proc/cpuinfo and cache cpu_brand
On 2014/03/11 16:17:48, Mark Mentovai wrote:
> As before.

Done.

[Mark Mentovai, 2014-03-12 15:31:44]

Great, this looks good now. We’re still waiting on you being added to the list
of approved Samsung contributors. This can land once that’s done.

[piman, 2014-03-12 18:43:47]

Will this work in the GPU process? It doesn't use the zygote.

[rptr, 2014-03-13 17:17:47]

On 2014/03/12 18:43:47, piman wrote:
> Will this work in the GPU process? It doesn't use the zygote.

PTAL.  Now this patch works for GPU process.

[Mark Mentovai, 2014-03-13 17:50:53]

Also good. Please poke this again when the CLA stuff is done.

[piman, 2014-03-13 18:39:58]

LGTM for my parts, maybe jln@ can vet that the PreSandboxStartup is the right
place.

[jln (very slow on Chromium), 2014-03-20 02:36:26]

lgtm

[rptr, 2014-04-08 11:41:42]

Hi Mark,
Laszlo informed us that name is now updated in CLA list. 
Could you please check the same.

[Mark Mentovai, 2014-04-08 16:12:29]

I don’t see it in the list yet.

[Mark Mentovai, 2014-04-15 13:42:10]

LGTM

[Mark Mentovai, 2014-04-15 13:42:38]

The CQ bit was checked by mark@chromium.org

[commit-bot: I haz the power, 2014-04-15 13:43:28]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/puttaraju.r@samsung.com/188443003/100001

[commit-bot: I haz the power, 2014-04-15 14:34:40]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-15 14:34:40]

Retried try job too often on chromium_presubmit for step(s) presubmit
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=chromium_p...

[rptr, 2014-04-15 15:03:44]

Adding cpu_@ for chrome_main_delegate.cc file

[rptr, 2014-04-15 15:09:39]

Added pfeldman@ for content/shell/app/shell_main_delegate.cc

[pfeldman, 2014-04-16 05:46:31]

shell rslgtm

[cpu_(ooo_6.6-7.5), 2014-04-24 03:36:49]

lgtm with nit.

https://codereview.chromium.org/188443003/diff/120001/base/cpu.cc
File base/cpu.cc (right):

https://codereview.chromium.org/188443003/diff/120001/base/cpu.cc#newcode12
base/cpu.cc:12: #include "base/file_util.h"
can we put the file_util.h on an #ifdef, in most platforms this whole class is
just a wrapper to the cpuid instruction. So it is pretty surprising to find file
IO here.

[rptr, 2014-04-24 06:25:33]

The CQ bit was checked by puttaraju.r@samsung.com

[commit-bot: I haz the power, 2014-04-24 06:25:54]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/puttaraju.r@samsung.com/188443003/140001

[commit-bot: I haz the power, 2014-04-24 08:27:27]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-24 08:27:28]

Try jobs failed on following builders:
  tryserver.chromium on chromium_presubmit

[rptr, 2014-04-24 08:35:44]

The CQ bit was checked by puttaraju.r@samsung.com

[commit-bot: I haz the power, 2014-04-24 08:35:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/puttaraju.r@samsung.com/188443003/140001

[rptr, 2014-04-24 18:37:31]

The CQ bit was unchecked by puttaraju.r@samsung.com

[commit-bot: I haz the power, 2014-04-24 21:06:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-24 21:07:00]

Retried try job too often on android_dbg for step(s) slave_steps
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[rptr, 2014-04-25 05:20:56]

The CQ bit was checked by puttaraju.r@samsung.com

[commit-bot: I haz the power, 2014-04-25 07:37:00]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/puttaraju.r@samsung.com/188443003/160001

[commit-bot: I haz the power, 2014-04-25 08:35:09]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-25 08:35:11]

Try jobs failed on following builders:
  tryserver.chromium on linux_chromium_chromeos_rel

[rptr, 2014-04-25 09:29:08]

The CQ bit was checked by puttaraju.r@samsung.com

[commit-bot: I haz the power, 2014-04-25 12:52:27]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/puttaraju.r@samsung.com/188443003/160001

[commit-bot: I haz the power, 2014-04-26 06:32:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-04-26 06:32:28]

The commit queue went berserk retrying too often for a
seemingly flaky test on builder android_dbg:
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android_db...

[rptr, 2014-04-28 10:22:11]

+torne for android webview.

[Torne, 2014-04-28 11:09:28]

LGTM with one nit:

https://chromiumcodereview.appspot.com/188443003/diff/180001/android_webview/...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://chromiumcodereview.appspot.com/188443003/diff/180001/android_webview/...
android_webview/lib/main/aw_main_delegate.cc:87: #if
defined(ARCH_CPU_ARM_FAMILY) && (defined(OS_ANDROID) || defined(OS_LINUX))
There's no need to test OS_ANDROID or OS_LINUX here; android_webview is always
Android.

[rptr, 2014-04-28 11:28:03]

https://chromiumcodereview.appspot.com/188443003/diff/180001/android_webview/...
File android_webview/lib/main/aw_main_delegate.cc (right):

https://chromiumcodereview.appspot.com/188443003/diff/180001/android_webview/...
android_webview/lib/main/aw_main_delegate.cc:87: #if
defined(ARCH_CPU_ARM_FAMILY) && (defined(OS_ANDROID) || defined(OS_LINUX))
On 2014/04/28 11:09:29, Torne wrote:
> There's no need to test OS_ANDROID or OS_LINUX here; android_webview is always
> Android.

Done.

[rptr, 2014-04-28 11:28:37]

The CQ bit was checked by puttaraju.r@samsung.com

[commit-bot: I haz the power, 2014-04-28 11:28:52]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/puttaraju.r@samsung.com/188443003/200001

[commit-bot: I haz the power, 2014-04-28 15:33:29]

Change committed as 266575