Fix race condition in CpuInfoProvider::QueryCpuTimePerProcessor

chrome/browser/extensions/api/system_cpu/cpu_info_provider_linux.cc

Index: chrome/browser/extensions/api/system_cpu/cpu_info_provider_linux.cc
diff --git a/chrome/browser/extensions/api/system_cpu/cpu_info_provider_linux.cc b/chrome/browser/extensions/api/system_cpu/cpu_info_provider_linux.cc
index 242226289a7fe0b34dae61e3abd50612dd978f26..1d19549735b4a7de47e1260416cd5d07a5c91415 100644
--- a/chrome/browser/extensions/api/system_cpu/cpu_info_provider_linux.cc
+++ b/chrome/browser/extensions/api/system_cpu/cpu_info_provider_linux.cc
@@ -37,19 +37,38 @@ bool CpuInfoProvider::QueryCpuTimePerProcessor(
     if (line.compare(0, 3, "cpu") != 0)
       continue;
 
+    // Due to a race condition in the kernel, we cannot trust the kernel to

[Lei Zhang, 2014/08/07 06:19:37]

Can you mention the number of CPUs might have changed between the call to
base::SysInfo::NumberOfProcessors() and here? It's not really a race condition
in the kernel.

+    // always return the same number of entries in /proc/stat as the number of
+    // online processors.
+    //
+    // If the number of /proc/stat is different than the number of online
+    // processors, it is treated as an intermittent problem in the kernel, and

[Pawel Osciak, 2014/08/04 09:22:54]

This is not really an intermittent problem on some platforms. It may be a normal
situation to have less online than present, which means this may never pass, it
will always return false. Is that still ok?

[Justin Chuang, 2014/08/04 11:55:02]

Interesting. Luckily it's not the case. On Nyan, this method returns *true* most
of the time. Only at the moment when that the number of online cpu changes, this
method may return false due to delay in the kernel of updating /proc/stat.

/proc/stat is provided by fs/proc/stat.c and kernel/sched/cpuacct.c. The code is
mostly platform-independent. So I expect the behavior of /proc/stat is platform
independent, too. It should not introduce the scenario you're worrying ("always
return false") on other Linux platforms.

If we want this method to always return true in normal case, I described another
method in Review #7, which will require more efforts and change the API
behavior.

(As a side note) actually the kernel keeps account of both online and offline
cpu. Pity that current design of /proc/stat uses 'for_each_online_cpu()' to
iterate online cpu cores only. We need to modify kernel code to include
online+offline cpus.

+    // this function will return false.
+    //
+    // In addition to the kernel issue, there is another less-likely race
+    // condition that the number of online processors is changed after the value
+    // has been decided in CpuInfoProvider::QueryInfo().
+    if (i == infos->size()) {

[Pawel Osciak, 2014/08/04 09:22:54]

s/==/>=/

[Justin Chuang, 2014/08/04 09:37:23]

'i' won't be greater infos->size() unless std::vector.size() returns negative
value. Shall I still change this?

[Pawel Osciak, 2014/08/05 07:48:16]

Of course, sorry, I misread the code. No need to change.

+      LOG(ERROR) << "Got more entries in /proc/stat than online CPUs";
+      return false;
+    }
+
     uint64 user = 0, nice = 0, sys = 0, idle = 0;
     int vals = sscanf(line.c_str(), "%*s %"PRIu64" %"PRIu64" %"PRIu64" %"PRIu64,
            &user, &nice, &sys, &idle);
     DCHECK_EQ(4, vals);
 
-    DCHECK(i < infos->size());
     infos->at(i)->usage.kernel = static_cast<double>(sys);
     infos->at(i)->usage.user = static_cast<double>(user + nice);
     infos->at(i)->usage.idle = static_cast<double>(idle);
     infos->at(i)->usage.total = static_cast<double>(sys + user + nice + idle);
     ++i;
   }
-  DCHECK_EQ(infos->size(), i);
+  if (i < infos->size()) {

[Pawel Osciak, 2014/08/04 09:22:54]

Is this also an actual issue? Having less CPUs present than online sounds much
worse than having less online than present...

[Justin Chuang, 2014/08/04 09:37:23]

Yes, I have seen both cases on actual nyan device. Not easy to reproduce though.

I haven't checked kernel code, but I think it's caused by delay in statistics
routine.

+    LOG(ERROR) << "Got fewer entries in /proc/stat than online CPUs";
+    return false;
+  }
+
   return true;
 }