Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

i#4881: Fix LRU counter bug in drcachesim #4883

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
27 changes: 25 additions & 2 deletions clients/drcachesim/simulator/cache_lru.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,31 @@
// The count value 0 means the most recent access, and the cache line with the
// highest counter value will be picked for replacement in replace_which_way.

bool
cache_lru_t::init(int associativity, int block_size, int total_size,
caching_device_t *parent, caching_device_stats_t *stats,
prefetcher_t *prefetcher, bool inclusive, bool coherent_cache, int id,
snoop_filter_t *snoop_filter,
const std::vector<caching_device_t *> &children)
{
// Works in the same way as the base class,
// except that the counters are initialized in a different way.

bool ret_val =
cache_t::init(associativity, block_size, total_size, parent, stats, prefetcher,
inclusive, coherent_cache, id, snoop_filter, children);
if (ret_val == false)
return false;

// Initialize line counters with 0, 1, 2, ..., associativity - 1.
for (int i = 0; i < blocks_per_set_; i++) {
for (int way = 0; way < associativity_; ++way) {
get_caching_device_block(i << assoc_bits_, way).counter_ = way;
}
}
return true;
}

void
cache_lru_t::access_update(int line_idx, int way)
{
Expand Down Expand Up @@ -69,7 +94,5 @@ cache_lru_t::replace_which_way(int line_idx)
max_way = way;
}
}
// Set to non-zero for later access_update optimization on repeated access
get_caching_device_block(line_idx, max_way).counter_ = 1;
derekbruening marked this conversation as resolved.
Show resolved Hide resolved
return max_way;
}
8 changes: 8 additions & 0 deletions clients/drcachesim/simulator/cache_lru.h
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,14 @@
#include "cache.h"

class cache_lru_t : public cache_t {
public:
bool
init(int associativity, int line_size, int total_size, caching_device_t *parent,
caching_device_stats_t *stats, prefetcher_t *prefetcher, bool inclusive = false,
bool coherent_cache = false, int id_ = -1,
snoop_filter_t *snoop_filter_ = nullptr,
const std::vector<caching_device_t *> &children = {}) override;

protected:
void
access_update(int line_idx, int way) override;
Expand Down
28 changes: 14 additions & 14 deletions clients/drcachesim/tests/offline-legacy.templatex
Original file line number Diff line number Diff line change
@@ -1,17 +1,17 @@
Cache simulation results:
Core #0 \(1 thread\(s\)\)
L1I stats:
Hits: *61,?531
Misses: 731
Hits: *61,?538
Misses: 724
Invalidations: 0
Miss rate: 1[,\.]?17%
Miss rate: 1[,\.]?16%
L1D stats:
Hits: *21,?155
Misses: 808
Hits: *21,?189
Misses: 774
Invalidations: 0
Prefetch hits: 182
Prefetch misses: 626
Miss rate: 3[,\.]?68%
Prefetch hits: 151
Prefetch misses: 623
Miss rate: 3[,\.]?52%
Core #1 \(4 thread\(s\)\)
L1I stats:
Hits: *19,?428
Expand All @@ -28,11 +28,11 @@ Core #1 \(4 thread\(s\)\)
Core #2 \(0 thread\(s\)\)
Core #3 \(0 thread\(s\)\)
LL stats:
Hits: 385
Misses: *1,?542
Hits: 342
Misses: *1,?544
Invalidations: 0
Prefetch hits: 143
Prefetch misses: 675
Local miss rate: 80[,\.]?02%
Child hits: *122,?839
Prefetch hits: 141
Prefetch misses: 674
Local miss rate: 81[,\.]?87%
Child hits: *122,?849
Total miss rate: 1[,\.]?24%
2 changes: 1 addition & 1 deletion clients/drcachesim/tests/offline-snappy-serial.templatex
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
Cache simulation results:
.*
LL stats:
Hits: 58,699
Hits: 78,312
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Wow this is a non-trivial change. The other "legacy" test had very minor changes. Wondering if this LRU issue caused other noticeable result differences on other workloads.)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep. When I looked at this, I was thinking I made something wrong... But it really changes a lot. (BTW, I think this bug in LRU really could result in great difference.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh it failed again

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I think that is an unrelated flaky test...checking

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is #3213 which unfortunately has happened before but nobody has reproduced locally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read log and see some timeout.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Read log and see some timeout.

If you could paste a link to what you are seeing? Only the Windows test was red.

.*