Analyze contiguous physical memory chunks that mmap system call gives

Analyze contiguous physical memory chunks that mmap system call gives

R=mseaborn@chromium.org

Committed: https://github.com/google/rowhammer-test/commit/6db5fa600bcf0c554e89e50d3ed431a412c65a08

[ruiq, 2015-06-30 22:39:26]

The CQ bit was checked by ruiq@google.com

[ruiq, 2015-06-30 22:39:26]

The CQ bit was unchecked by ruiq@google.com

[ruiq, 2015-06-30 22:43:24]

ruiq@google.com changed reviewers:
+ mseaborn@chromium.org

[Mark Seaborn, 2015-07-01 19:21:23]

Can you remove "BUG=" from the commit message?  It's a Chromium/NaCl style to
have a BUG= line, but it doesn't really apply for the rowhammer-test code.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
File physmem_alloc_analysis/README.md (right):

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/README.md:5: for mmap, and analyze how contiguous the
assigned physical frames are.
Can you also add a sentence or two on the motivation for doing this?

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/README.md:7: ## How to run the program:
Nit: omit trailing ":" (for consistency with other headings)

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
File physmem_alloc_analysis/gnuplot_script (right):

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/gnuplot_script:1: set style data histogram
Nit: I suspect this file ought to have a copyright notice too.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
File physmem_alloc_analysis/physmem_alloc_analyzer.py (right):

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:19: def ExtractBits(val,
offset_in_bits, size_in_bits):
Not used; can be removed.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:31: counter = 0
Not used

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:48: cont_fraction = {}
I don't think you need to build another dict for this, do you?

It would be clearer to compute this fraction in the last "for" loop.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:52: sorted_by_size =
sorted(cont_fraction.items(), key=operator.itemgetter(0))
Note that this is equivalent to:
sorted_by_size = sorted(cont_fraction.items())

because of how Python sorts tuples.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:56: print "%10d %10d %10d
%10.2f" % (size, count, size_total, fraction)
Nit: be consistent about whether you use ' or " for strings

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
File physmem_alloc_analysis/physmem_alloc_profiler.cc (right):

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_profiler.cc:58: argv[0]);
Nit: The usual Google style is for arguments to line up:

// good (aligned with "(")
SomeFunc(arg1, arg2,
         arg3,
         arg4, ...);

// good (4-space indent)
SomeFunc(
    arg1, arg2,
    arg3,
    arg4, ...);

// less good
SomeFunc(arg1, arg2,
    arg3,
    arg4, ...);

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_profiler.cc:83: size_t i;
This can go inside the "for"

[ruiq, 2015-07-01 20:42:59]

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
File physmem_alloc_analysis/README.md (right):

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/README.md:5: for mmap, and analyze how contiguous the
assigned physical frames are.
On 2015/07/01 19:21:22, Mark Seaborn wrote:
> Can you also add a sentence or two on the motivation for doing this?

Done.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/README.md:7: ## How to run the program:
On 2015/07/01 19:21:22, Mark Seaborn wrote:
> Nit: omit trailing ":" (for consistency with other headings)

Done.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
File physmem_alloc_analysis/gnuplot_script (right):

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/gnuplot_script:1: set style data histogram
On 2015/07/01 19:21:22, Mark Seaborn wrote:
> Nit: I suspect this file ought to have a copyright notice too.

Done.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
File physmem_alloc_analysis/physmem_alloc_analyzer.py (right):

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:19: def ExtractBits(val,
offset_in_bits, size_in_bits):
On 2015/07/01 19:21:23, Mark Seaborn wrote:
> Not used; can be removed.

Done.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:31: counter = 0
On 2015/07/01 19:21:23, Mark Seaborn wrote:
> Not used

Done.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:48: cont_fraction = {}
On 2015/07/01 19:21:22, Mark Seaborn wrote:
> I don't think you need to build another dict for this, do you?
> 
> It would be clearer to compute this fraction in the last "for" loop.

Done.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:52: sorted_by_size =
sorted(cont_fraction.items(), key=operator.itemgetter(0))
On 2015/07/01 19:21:22, Mark Seaborn wrote:
> Note that this is equivalent to:
> sorted_by_size = sorted(cont_fraction.items())
> 
> because of how Python sorts tuples.

Done.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:56: print "%10d %10d %10d
%10.2f" % (size, count, size_total, fraction)
On 2015/07/01 19:21:22, Mark Seaborn wrote:
> Nit: be consistent about whether you use ' or " for strings

Done.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
File physmem_alloc_analysis/physmem_alloc_profiler.cc (right):

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_profiler.cc:58: argv[0]);
On 2015/07/01 19:21:23, Mark Seaborn wrote:
> Nit: The usual Google style is for arguments to line up:
> 
> // good (aligned with "(")
> SomeFunc(arg1, arg2,
>          arg3,
>          arg4, ...);
> 
> // good (4-space indent)
> SomeFunc(
>     arg1, arg2,
>     arg3,
>     arg4, ...);
> 
> // less good
> SomeFunc(arg1, arg2,
>     arg3,
>     arg4, ...);

Done.

https://codereview.chromium.org/1217243006/diff/20001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_profiler.cc:83: size_t i;
On 2015/07/01 19:21:23, Mark Seaborn wrote:
> This can go inside the "for"

Done.

[Mark Seaborn, 2015-07-06 17:32:33]

LGTM, thanks

https://codereview.chromium.org/1217243006/diff/40001/physmem_alloc_analysis/...
File physmem_alloc_analysis/physmem_alloc_analyzer.py (right):

https://codereview.chromium.org/1217243006/diff/40001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:16: import operator
Not used now

https://codereview.chromium.org/1217243006/diff/40001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:43: sorted_by_size =
sorted(cont_count.items())
Nit: iteritems() should work here too (for consistency with the loop above)

[ruiq, 2015-07-07 01:12:50]

https://codereview.chromium.org/1217243006/diff/40001/physmem_alloc_analysis/...
File physmem_alloc_analysis/physmem_alloc_analyzer.py (right):

https://codereview.chromium.org/1217243006/diff/40001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:16: import operator
On 2015/07/06 17:32:33, Mark Seaborn wrote:
> Not used now

Done.

https://codereview.chromium.org/1217243006/diff/40001/physmem_alloc_analysis/...
physmem_alloc_analysis/physmem_alloc_analyzer.py:43: sorted_by_size =
sorted(cont_count.items())
On 2015/07/06 17:32:33, Mark Seaborn wrote:
> Nit: iteritems() should work here too (for consistency with the loop above)

Done.

[ruiq, 2015-07-07 02:40:24]

The CQ bit was checked by ruiq@google.com

[ruiq, 2015-07-07 02:40:25]

The patchset sent to the CQ was uploaded after l-g-t-m from
mseaborn@chromium.org
Link to the patchset: https://codereview.chromium.org/1217243006/#ps60001
(title: "More clean up")

[commit-bot: I haz the power, 2015-07-07 02:42:02]

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

[commit-bot: I haz the power, 2015-07-07 02:42:03]

Commit queue rejected this change because it did not recognize the base URL.
Please commit your change manually.

[ruiq, 2015-07-07 21:08:50]

Committed patchset #4 (id:60001) manually as
6db5fa600bcf0c554e89e50d3ed431a412c65a08 (presubmit successful).