i#3068: API for accessing cache model metrics #5020

mat-kalinowski · 2021-07-20T14:09:34Z

This patch contains implementation of the API for accessing separate
metrics gathered during the simulation.

Functions for accessing metrics were added for all caches in the
default configuration: L1I, L1D and LL. Also additional functions
for getting configuration options of the cache_simulator_t.

Issue: #3068

This patch contains implementation of the API for accessing separate metrics gathered during the simulation. Functions for accessing metrics were added for all caches in the default configuration: L1I, L1D and LL. Also additional functions for getting configuration options of the cache_simulator_t. Issue: #3068 Change-Id: I2970ca57886d96c68c92d7c17f0c9df920163152

This commit contains fixes for minor formatting errors which caused clang format check failure.

This commit contains fix for additional formatting error that wasn't spotted in the previous commit.

derekbruening

Thank you for the contribution. I have a few suggestions.

derekbruening · 2021-07-20T16:48:55Z

clients/drcachesim/simulator/cache_simulator.h

+    int_least64_t
+    get_l1d_metric(metric_name_t metric, unsigned int core) const;
+    int_least64_t
+    get_ll_metric(metric_name_t metric) const;


In our usage we routinely have a 3-level cache with a per-core L2 in between L1{I,D} and LLC (created with a config file). Possibilities:

Add a get_l2_metric(). To take in a core index, it would have to only support direct parents of L1* and so would not support arbitrary cache layouts.

Export an iterator over all caches (split into l1, llc, and "other"==l2 separate iterators?) which returns some kind of key (the cache name?) to then pass back to query a metric.

Take in the cache by name (the name from the config file) -- less usable by itself without an iterator.

Thanks for the suggestions! Sorry for the delay, I had a few days off. I like the flexibility of accessing stats in arbitrary cache model and also having really simple interface. I thought about having common function for all caches, something like int_least64_t get_cache_metric(unsigned level, unsigned core, metric_t metric)
Does separation for instruction and data caches makes sense only for L1 cache in any model ?

I think the current code only supports i vs d at L1 but if it's easy not to assume that here that might help future support.

something like int_least64_t get_cache_metric(unsigned level, unsigned core, metric_t metric)

Would this support asymmetric layouts? I guess it would if you always walk from the core up level levels.

I tried to make this code as flexible as possible. Current approach should work with split cache of the higher level in simple models, when given higher level split cache is an ancestor of L1 (for example L1i -> L2i).

Can you provide example of some simple asymmetric layout ? My current approach is to walk up from L1 cache and I think it should work properly.

Can you provide example of some simple asymmetric layout ? My current approach is to walk up from L1 cache and I think it should work properly.

Never used or seen anything too weird with dr$sim, was just speculating about possible corner cases. Such a walk should work with any layout though it seems.

derekbruening · 2021-07-20T16:52:05Z

clients/drcachesim/simulator/cache_simulator.h

    // Exposed to make it easy to test
    bool
    check_warmed_up();
    uint64_t
    remaining_sim_refs() const;

+    unsigned int
+    get_core_num() const;


Since cache_simulator_knobs_t is already public -- should the knobs be returned? IIRC a config file setup does set the key knobs fields?

Yes, knobs fields are set by config file setup. I have added suitable method.

derekbruening · 2021-07-20T16:53:54Z

clients/drcachesim/simulator/caching_device_stats.h

+    // References to the properties with statistics are held in the map with the
+    // statistic name as the key. Sample map element: {HITS, num_hits_}
+    std::map<metric_name_t, int_least64_t &> stats_map_;
+


Please add some kind of test. Maybe in drcachesim_unit_tests or a new similar test? Xref #4842.

I have added unit test in the drcachesim_unit_tests. I couldn't test Invalidates metric properly because of the bug/issue that I described here: #5031

OK, best to put a XXX i#5031: or TODO i#5031: comment in the test about adding that metric.

AssadHashmi · 2021-07-20T18:04:28Z

add to whitelist

derekbruening · 2021-07-20T18:55:36Z

Looks like #4954 was hit on A64 Jenkins -- maybe should be added to the known-flaky list.

derekbruening · 2021-07-20T18:55:41Z

run arm tests

This patch contains implementation of the unit test and improvements to the code structure pointed out in the review comments. Change-Id: I19bc8a28c614219b2cb6d14f4445d23504a74815

This commit contains fixes for a few formatting errors which caused clang format test failure.

mat-kalinowski · 2021-08-03T11:41:59Z

Right now I am handling errors like this:

if (parent == NULL) {
    ERRMSG("Wrong cache level: %u.\n", core);
    return 0;
}

I thought also about returning -1 without printing error message. Which approach you think is better? Also do you have some comments about newest commit with changes ?

derekbruening · 2021-08-03T16:06:33Z

Right now I am handling errors like this:
if (parent == NULL) {
    ERRMSG("Wrong cache level: %u.\n", core);
    return 0;
}
I thought also about returning -1 without printing error message. Which approach you think is better?

For get_cache_metric() I would think of it as a library routine likely to be used by third-party code and as such I'd return an error rather than printing an error.

Also do you have some comments about newest commit with changes ?

Oh, was waiting for a (re-)review request (https://dynamorio.org/page_code_reviews.html#autotoc_md114 "When the requested changes have been pushed, you can ask for a re-review from the reviewers. This can be done by clicking the re-review button next to the reviewers' name on the right sidebar on the pull request page.")

derekbruening

Thank you for adding the test. Just a few comments mostly about names and then should be good to go.

derekbruening · 2021-08-03T16:10:01Z

clients/drcachesim/simulator/cache_simulator.h

@@ -43,6 +43,8 @@
 #include "cache.h"
 #include "snoop_filter.h"

+enum cache_split_t { DATA, INSTRUCTION };


Safer to put inside the class and not in the global namespace (xref #4343). Or see other suggestions for the enum below.

derekbruening · 2021-08-03T16:11:14Z

clients/drcachesim/simulator/cache_simulator.h

@@ -60,12 +62,19 @@ class cache_simulator_t : public simulator_t {
    bool
    print_results() override;

+    int_least64_t
+    get_cache_metric(unsigned level, metric_name_t metric, unsigned core = 0,


I would re-order the parameters with the metric first and grouping the 3 identifiers of which cache (level, core, split) together.

derekbruening · 2021-08-03T16:20:10Z

clients/drcachesim/simulator/caching_device_stats.h

@@ -49,6 +50,20 @@ enum invalidation_type_t {
    INVALIDATION_COHERENCE,
 };

+enum metric_name_t {


I know the existing code has global scope enums but we're trying to avoid that going forward (xref #4343), and here we have very simple names that could conflict. We do require C++11 so this could be a scoped enum (typedef enum class metric_name_t) where it would require the prefix metric_name_t::HITS; or at least use a prefix like the existing enums so METRIC_HITS; or something, to avoid global symbols with very simple names like HITS which could conflict (this code is statically linked into some very large code bases).

derekbruening · 2021-08-03T16:23:26Z

clients/drcachesim/simulator/cache_simulator.cpp

+
+    if (core >= knobs_.num_cores) {
+        ERRMSG("Wrong core number: %u.\n", core);
+        return 0;


Xref the top-level thread: I would vote for returning an error. Could go so far as error enum types; or just return -1 (document that's the error return code) either w/ or w/o the print (could see the print useful if there's no errno; could see it annoying linked into a large system that maybe doesn't know the cache layout and is passing all possible levels and using return code to find the bounds of the layout -- there an errno would be nice).

Ditto below.

derekbruening · 2021-08-03T16:25:13Z

clients/drcachesim/simulator/caching_device_stats.h

+    // References to the properties with statistics are held in the map with the
+    // statistic name as the key. Sample map element: {HITS, num_hits_}
+    std::map<metric_name_t, int_least64_t &> stats_map_;
+


OK, best to put a XXX i#5031: or TODO i#5031: comment in the test about adding that metric.

This patch contains improvements to the functionality and style of the code. Enums were adjusted to prevent polluting global namespace and approach to report errors was changed.

This patch fixes enum names in the metrics API unit tests. After changing enum types in the simulator code old names were left in the test code by mistake.

This patch fixes one formatting error in the unit test. Function call was split into two lines in the wrong way.

mat-kalinowski · 2021-08-04T14:24:28Z

Thanks for your comments! I've changed enum types to the enum class to prevent polluting global namespaces. I've implemented stats_error_t as a plain enum and added STATS_ prefix because I needed to return it as an int from the get_cache_metric function.

derekbruening

I just have some comment suggestions, otherwise looks good. Thank you for the contribution.

clients/drcachesim/simulator/cache_simulator.h

This patch adds one comment and improves existing one.

mat-kalinowski · 2021-08-05T13:00:18Z

run arm tests

derekbruening · 2021-08-05T15:28:51Z

Thanks again for the contribution.

One nit: For future merges please remove the duplicate title line (xref https://dynamorio.org/page_code_reviews.html#autotoc_md117):

commit 9574fca502ee03661dfd788f8744a8881f19eb63

    i#3068: API for accessing cache model metrics (#5020)
    
    i#3068: API for accessing cache model metrics
    
    This patch contains implementation of the API for accessing separate
    metrics gathered during the simulation.
    
    Functions for accessing metrics were added for all caches in the
    default configuration: L1I, L1D and LL. Also additional functions
    for getting configuration options of the cache_simulator_t.
    
    Issue: #3068

mat-kalinowski added 3 commits July 20, 2021 14:32

Fixing formatting errors

334395a

This commit contains fixes for minor formatting errors which caused clang format check failure.

Fixing additional formatting error

f5c83c6

This commit contains fix for additional formatting error that wasn't spotted in the previous commit.

derekbruening reviewed Jul 20, 2021

View reviewed changes

AssadHashmi mentioned this pull request Jul 20, 2021

Fixing support for inject_x64 injection inside WOW64 processes #4989

Merged

mat-kalinowski added 2 commits August 2, 2021 16:55

Adding unit test and code structure improvements

343d5bf

This patch contains implementation of the unit test and improvements to the code structure pointed out in the review comments. Change-Id: I19bc8a28c614219b2cb6d14f4445d23504a74815

Fixing formatting errors

49b4030

This commit contains fixes for a few formatting errors which caused clang format test failure.

mat-kalinowski requested a review from derekbruening August 3, 2021 16:10

derekbruening reviewed Aug 3, 2021

View reviewed changes

mat-kalinowski added 3 commits August 4, 2021 12:11

Code style and functionality improvements

a609f67

This patch contains improvements to the functionality and style of the code. Enums were adjusted to prevent polluting global namespace and approach to report errors was changed.

Fixing enum names in the unit test

38011c0

This patch fixes enum names in the metrics API unit tests. After changing enum types in the simulator code old names were left in the test code by mistake.

Fixing formatting error in the unit test

333eec1

This patch fixes one formatting error in the unit test. Function call was split into two lines in the wrong way.

mat-kalinowski requested a review from derekbruening August 4, 2021 14:33

derekbruening approved these changes Aug 4, 2021

View reviewed changes

clients/drcachesim/simulator/cache_simulator.h Show resolved Hide resolved

clients/drcachesim/simulator/cache_simulator.h Outdated Show resolved Hide resolved

mat-kalinowski added 2 commits August 5, 2021 12:03

Adding and improving comments

81db1b8

This patch adds one comment and improves existing one.

Merge branch 'master' into i3068-API-for-accessing-cache-metrics

1e08634

mat-kalinowski merged commit 9574fca into master Aug 5, 2021

mat-kalinowski deleted the i3068-API-for-accessing-cache-metrics branch August 5, 2021 13:14

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

i#3068: API for accessing cache model metrics #5020

i#3068: API for accessing cache model metrics #5020

mat-kalinowski commented Jul 20, 2021

derekbruening left a comment

derekbruening Jul 20, 2021

mat-kalinowski Jul 27, 2021 •

edited

Loading

derekbruening Jul 27, 2021

mat-kalinowski Aug 2, 2021

mat-kalinowski Aug 2, 2021

derekbruening Aug 2, 2021

derekbruening Jul 20, 2021

mat-kalinowski Aug 2, 2021

derekbruening Jul 20, 2021

mat-kalinowski Aug 2, 2021

derekbruening Aug 3, 2021

AssadHashmi commented Jul 20, 2021

derekbruening commented Jul 20, 2021

derekbruening commented Jul 20, 2021

mat-kalinowski commented Aug 3, 2021

derekbruening commented Aug 3, 2021

derekbruening left a comment

derekbruening Aug 3, 2021

derekbruening Aug 3, 2021

derekbruening Aug 3, 2021

derekbruening Aug 3, 2021

derekbruening Aug 3, 2021

mat-kalinowski commented Aug 4, 2021 •

edited

Loading

derekbruening left a comment

mat-kalinowski commented Aug 5, 2021

derekbruening commented Aug 5, 2021

i#3068: API for accessing cache model metrics #5020

i#3068: API for accessing cache model metrics #5020

Conversation

mat-kalinowski commented Jul 20, 2021

derekbruening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mat-kalinowski Jul 27, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AssadHashmi commented Jul 20, 2021

derekbruening commented Jul 20, 2021

derekbruening commented Jul 20, 2021

mat-kalinowski commented Aug 3, 2021

derekbruening commented Aug 3, 2021

derekbruening left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mat-kalinowski commented Aug 4, 2021 • edited Loading

derekbruening left a comment

Choose a reason for hiding this comment

mat-kalinowski commented Aug 5, 2021

derekbruening commented Aug 5, 2021

mat-kalinowski Jul 27, 2021 •

edited

Loading

mat-kalinowski commented Aug 4, 2021 •

edited

Loading