684038 - Report cpuid in telemetry

Status: RESOLVED FIXED

(Toolkit :: Telemetry, defect)

Comment 1

[Ryan Jones-Ward [:sciguyryan]]

I'm not sure if the privacy review is needed on this though I am curious what useful information could be extracted from this data.

I've used the CPUID command extensively in various instances in the past and there really isn't anything there that could compromise privacy or be used in tracking.

The only thing of interest might be the processor serial number (see http://en.wikipedia.org/wiki/CPUID#EAX.3D3:_Processor_Serial_Number) but no modern CPUs implement this feature (I believe they implemented it in Pentium III but then stopped due to privacy concerns of their own).

This article on MSDN is worth noting here for reference of the other information that can be obtained: http://msdn.microsoft.com/en-us/library/hskdteyh.aspx

Comment 2

[Justin Lebar (not reading bugmail)]

We're definitely not going to read the PSN.  :)

> I am curious what useful information could be extracted from this data.

I think primarily, "what fraction of our users have NEON / SSE2?"

Comment 3

[Ryan Jones-Ward [:sciguyryan]]

(In reply to Justin Lebar [:jlebar] from comment #2)
> We're definitely not going to read the PSN.  :)
> 
> > I am curious what useful information could be extracted from this data.
> 
> I think primarily, "what fraction of our users have NEON / SSE2?"

Then I think I can quite confidently say that there isn't going to be a privacy concern here.

Are you guys planning on sending the whole CPUID code or are you just going to extract the bits you need and submit those?

If the former then you need only blank out the serial number data before the information is sent and that will be that. If the latter then there really shouldn't be a cast for privacy invasion here.

Either way I believe this is quite a safe thing to handle and hopefully the references above will be useful in any event!

Comment 4

[Justin Lebar (not reading bugmail)]

Not having spoken to Taras about what he's planning to do, I imagine we'd send the EAX=1 (and maybe also EAX=2?) data.

Comment 5

[(dormant account)]

(In reply to Justin Lebar [:jlebar] from comment #4)
> Not having spoken to Taras about what he's planning to do, I imagine we'd
> send the EAX=1 (and maybe also EAX=2?) data.

I don't know much about cpuid, filed this so people in the know can implement it

Comment 6

[Ryan Jones-Ward [:sciguyryan]]

(In reply to Justin Lebar [:jlebar] from comment #4)
> Not having spoken to Taras about what he's planning to do, I imagine we'd
> send the EAX=1 (and maybe also EAX=2?) data.

Then I can say pretty confidently that you guys are not going to hit any privacy snags. The data in those are pretty standard. EAX=1 being feature bits and EAX=2 being cache information.

The only other one you may want is the EAX=80000001h as there is some more information in there that may or may not be useful. Either way it is standard information again and no good for tracing.

Hopefully that helps with the privacy concerns guys.

Comment 7

[Justin Lebar (not reading bugmail)]

Taras, do we have a way to report in Telemetry things that aren't histograms?  Or do we have to shim this data into a histogram?

It would also be interesting to report the amount of RAM in the machine.

Comment 8

[(dormant account)]

(In reply to Justin Lebar [:jlebar] from comment #7)
> Taras, do we have a way to report in Telemetry things that aren't
> histograms?  Or do we have to shim this data into a histogram?
> 
> It would also be interesting to report the amount of RAM in the machine.

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/telemetry/TelemetryPing.js#160 see memsize

Comment 9

[Justin Lebar (not reading bugmail)]

"Has/doesn't have SSE" sounds safe, but are we concerned about the possibility of identifying a user who has a rare CPU by including the full cpuid output?  A relatively rare CPU plus a rare memsize could narrow things down to just a few users...

Comment 10

[Ryan Jones-Ward [:sciguyryan]]

(In reply to Justin Lebar [:jlebar] from comment #9)
> "Has/doesn't have SSE" sounds safe, but are we concerned about the
> possibility of identifying a user who has a rare CPU by including the full
> cpuid output?  A relatively rare CPU plus a rare memsize could narrow things
> down to just a few users...

Again the feature set is pretty much standard among most CPUs these days. There are few (if any) exotic features that will be easy to trace down to a person.

There is a large database of CPUID listings here: http://www.cpu-world.com/cgi-bin/CPUID.pl - it could be worth asking them about it since they should be able to run all sorts of queries on the data they have.

I still think it is very unlikely that this poses any sort of privacy risk to anyone and stating that it does is likely to induce extreme paranoia in some users due to the fact that other programs have been using this for many years.

Comment 11

[Nathan Froyd [:froydnj]]

Attachment: first cut at adding cpuid telemetry

This patch doesn't include all the CPUID information; it just returns what we get from the mozilla::supports_FOO functions.  I think this makes more sense than returning raw CPUID info, since CPUID doesn't apply for, say, ARM.

Do I need review for the telemetry bits too, or are those obvious once the xpcom changes are in?

Comment 12

[Justin Lebar (not reading bugmail)]

Can you include supports_mmx and supports_sse in there too, please?

Comment 13

[Nathan Froyd [:froydnj]]

Attachment: adding cpuid telemetry, v2

v2, now with mmx and sse bits

Comment 14

[Marco Bonardo [:mak]]

Comment on attachment 559515 [details] [diff] [review]
adding cpuid telemetry, v2

Review of attachment 559515 [details] [diff] [review]:
-----------------------------------------------------------------

it looks fine to me, apart a couple questions I have:

::: toolkit/components/telemetry/TelemetryPing.js
@@ +244,5 @@
>      // sysinfo fields are not always available, get what we can.
>      let sysInfo = Cc["@mozilla.org/system-info;1"].getService(Ci.nsIPropertyBag2);
> +    let fields = ["cpucount", "memsize", "arch", "version", "device", "manufacturer", "hardware",
> +		  "hasSSE2", "hasSSE3", "hasSSSE3", "hasSSE4A", "hasSSE4_1", "hasSSE4_2",
> +		  "hasEDSP", "hasARMv6", "hasNEON"];

it's unclear if this shoud also report hasMMX and hasSSE, those have been added to nsSystemInfo but not here. Was it requested for telemetry? (the question is probably for jlebar)

::: xpcom/base/nsSystemInfo.cpp
@@ +113,5 @@
> +        { "hasARMv6", mozilla::supports_armv6 },
> +        { "hasNEON", mozilla::supports_neon }
> +    };
> +
> +    for (PRUint32 i = 0; i < PR_ARRAY_SIZE(cpuPropItems); i++) {

Don't we usually use NS_ARRAY_LENGTH rather than PR_ARRAY_SIZE? (I think they are actually the same thing, but we usually prefer the NS_ version of the macros, see NS_MIN vs PR_MIN for example)

Comment 15

[Justin Lebar (not reading bugmail)]

> it's unclear if this shoud also report hasMMX and hasSSE, those have been added to nsSystemInfo but 
> not here. Was it requested for telemetry? (the question is probably for jlebar)

Yes, please include these.  I expect the vast majority of CPUs will have both, but knowing whether it's 98% or 99.99% is helpful.

Comment 16

[Sid Stamm [:geekboy or :sstamm]]

This seems reasonable from a privacy perspective.  The set of boolean flags for capabilities is not much more information than the manufacturer and name of the device, and these data points will help us correlate performance issues with hardware capabilities.

Clearing privacy-review-needed flag.

Comment 17

[Nathan Froyd [:froydnj]]

Attachment: adding cpuid telemetry, v3

Adding hasMMX and hasSSE to the ping as requested.  Carrying over r+ and feedback+.

I opted to keep PR_ARRAY_SIZE; it is at least consistent with other usages in xpcom/.

Comment 18

[Justin Lebar (not reading bugmail)]

I can check this in for you, but please upload a patch with the right commit message (hg qref -e).

Comment 19

[Marco Bonardo [:mak]]

(In reply to Nathan Froyd (:froydnj) from comment #17)
> I opted to keep PR_ARRAY_SIZE; it is at least consistent with other usages
> in xpcom/.

Not completely sure, in xpcom/ there are 2 PR_ARRAY_SIZE and 87 NS_ARRAY_LENGTH

Comment 20

[Nathan Froyd [:froydnj]]

Doh, I spazzed and thought it was NS_ARRAY_SIZE.  Updated patch coming up.

Comment 21

[Nathan Froyd [:froydnj]]

Attachment: adding cpuid telemetry, v4

Patch with NS_ARRAY_LENGTH and a fixed editor thinko in TelemetryPing.js.

I think the commit message is correct now.

Comment 22

[Justin Lebar (not reading bugmail)]

Yep; commit message looks good.  Thanks!

Inbound: https://hg.mozilla.org/integration/mozilla-inbound/rev/f8a8498fa7eb

Comment 23

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f8a8498fa7eb