Daniel Carvalho has submitted this change and it was merged. (
https://gem5-review.googlesource.com/c/public/gem5/+/17530 )
Change subject: mem-cache: Add match functions to QueueEntry
......................................................................
mem-cache: Add match functions to QueueEntry
Having the caller decide the matching logic is error-prone, and
frequently ends up with the secure bit being forgotten. This
change adds matching functions to the QueueEntry to avoid this
problem.
As a side effect the signature of findPending has been changed.
Change-Id: I6e494a821c1e6e841ab103ec69632c0e1b269a08
Signed-off-by: Daniel R. Carvalho <[email protected]>
Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/17530
Tested-by: kokoro <[email protected]>
Reviewed-by: Nikos Nikoleris <[email protected]>
Maintainer: Nikos Nikoleris <[email protected]>
---
M src/mem/cache/base.cc
M src/mem/cache/mshr.cc
M src/mem/cache/mshr.hh
M src/mem/cache/queue.hh
M src/mem/cache/queue_entry.hh
M src/mem/cache/write_queue_entry.cc
M src/mem/cache/write_queue_entry.hh
7 files changed, 100 insertions(+), 19 deletions(-)
Approvals:
Nikos Nikoleris: Looks good to me, approved; Looks good to me, approved
kokoro: Statistics mismatch
diff --git a/src/mem/cache/base.cc b/src/mem/cache/base.cc
index ddcfd80..2a6bc2a 100644
--- a/src/mem/cache/base.cc
+++ b/src/mem/cache/base.cc
@@ -729,9 +729,7 @@
// full write buffer, otherwise we favour the miss requests
if (wq_entry && (writeBuffer.isFull() || !miss_mshr)) {
// need to search MSHR queue for conflicting earlier miss.
- MSHR *conflict_mshr =
- mshrQueue.findPending(wq_entry->blkAddr,
- wq_entry->isSecure);
+ MSHR *conflict_mshr = mshrQueue.findPending(wq_entry);
if (conflict_mshr && conflict_mshr->order < wq_entry->order) {
// Service misses in order until conflict is cleared.
@@ -744,9 +742,7 @@
return wq_entry;
} else if (miss_mshr) {
// need to check for conflicting earlier writeback
- WriteQueueEntry *conflict_mshr =
- writeBuffer.findPending(miss_mshr->blkAddr,
- miss_mshr->isSecure);
+ WriteQueueEntry *conflict_mshr =
writeBuffer.findPending(miss_mshr);
if (conflict_mshr) {
// not sure why we don't check order here... it was in the
// original code but commented out.
diff --git a/src/mem/cache/mshr.cc b/src/mem/cache/mshr.cc
index 8db7c29..13a5016 100644
--- a/src/mem/cache/mshr.cc
+++ b/src/mem/cache/mshr.cc
@@ -273,6 +273,9 @@
Target::Source source = (target->cmd == MemCmd::HardPFReq) ?
Target::FromPrefetcher : Target::FromCPU;
targets.add(target, when_ready, _order, source, true, alloc_on_fill);
+
+ // All targets must refer to the same block
+ assert(target->matchBlockAddr(targets.front().pkt, blkSize));
}
@@ -685,3 +688,24 @@
print(str);
return str.str();
}
+
+bool
+MSHR::matchBlockAddr(const Addr addr, const bool is_secure) const
+{
+ assert(hasTargets());
+ return (blkAddr == addr) && (isSecure == is_secure);
+}
+
+bool
+MSHR::matchBlockAddr(const PacketPtr pkt) const
+{
+ assert(hasTargets());
+ return pkt->matchBlockAddr(blkAddr, isSecure, blkSize);
+}
+
+bool
+MSHR::conflictAddr(const QueueEntry* entry) const
+{
+ assert(hasTargets());
+ return entry->matchBlockAddr(blkAddr, isSecure);
+}
diff --git a/src/mem/cache/mshr.hh b/src/mem/cache/mshr.hh
index e9505eb..4b05489 100644
--- a/src/mem/cache/mshr.hh
+++ b/src/mem/cache/mshr.hh
@@ -531,6 +531,10 @@
* @return string with mshr fields + [deferred]targets
*/
std::string print() const;
+
+ bool matchBlockAddr(const Addr addr, const bool is_secure) const
override;
+ bool matchBlockAddr(const PacketPtr pkt) const override;
+ bool conflictAddr(const QueueEntry* entry) const override;
};
#endif // __MEM_CACHE_MSHR_HH__
diff --git a/src/mem/cache/queue.hh b/src/mem/cache/queue.hh
index 6c8a192..30fe4ba 100644
--- a/src/mem/cache/queue.hh
+++ b/src/mem/cache/queue.hh
@@ -174,7 +174,7 @@
// cacheable accesses being added to an WriteQueueEntry
// serving an uncacheable access
if (!(ignore_uncacheable && entry->isUncacheable()) &&
- entry->blkAddr == blk_addr && entry->isSecure ==
is_secure) {
+ entry->matchBlockAddr(blk_addr, is_secure)) {
return entry;
}
}
@@ -185,7 +185,8 @@
{
pkt->pushLabel(label);
for (const auto& entry : allocatedList) {
- if (entry->blkAddr == blk_addr &&
entry->trySatisfyFunctional(pkt)) {
+ if (entry->matchBlockAddr(blk_addr, pkt->isSecure()) &&
+ entry->trySatisfyFunctional(pkt)) {
pkt->popLabel();
return true;
}
@@ -195,16 +196,17 @@
}
/**
- * Find any pending requests that overlap the given request.
- * @param blk_addr Block address.
- * @param is_secure True if the target memory space is secure.
- * @return A pointer to the earliest matching WriteQueueEntry.
+ * Find any pending requests that overlap the given request of a
+ * different queue.
+ *
+ * @param entry The entry to be compared against.
+ * @return A pointer to the earliest matching entry.
*/
- Entry* findPending(Addr blk_addr, bool is_secure) const
+ Entry* findPending(const QueueEntry* entry) const
{
- for (const auto& entry : readyList) {
- if (entry->blkAddr == blk_addr && entry->isSecure ==
is_secure) {
- return entry;
+ for (const auto& ready_entry : readyList) {
+ if (ready_entry->conflictAddr(entry)) {
+ return ready_entry;
}
}
return nullptr;
diff --git a/src/mem/cache/queue_entry.hh b/src/mem/cache/queue_entry.hh
index 39ee0a0..61c898d 100644
--- a/src/mem/cache/queue_entry.hh
+++ b/src/mem/cache/queue_entry.hh
@@ -119,14 +119,41 @@
/** True if the entry targets the secure memory space. */
bool isSecure;
- QueueEntry() : readyTime(0), _isUncacheable(false),
- inService(false), order(0), blkAddr(0), blkSize(0),
- isSecure(false)
+ QueueEntry()
+ : readyTime(0), _isUncacheable(false),
+ inService(false), order(0), blkAddr(0), blkSize(0),
isSecure(false)
{}
bool isUncacheable() const { return _isUncacheable; }
/**
+ * Check if entry corresponds to the one being looked for.
+ *
+ * @param addr Address to match against.
+ * @param is_secure Whether the target should be in secure space or
not.
+ * @return True if entry matches given information.
+ */
+ virtual bool matchBlockAddr(const Addr addr, const bool is_secure)
+ const = 0;
+
+ /**
+ * Check if entry contains a packet that corresponds to the one being
+ * looked for.
+ *
+ * @param pkt The packet to search for.
+ * @return True if any of its targets' packets matches the given one.
+ */
+ virtual bool matchBlockAddr(const PacketPtr pkt) const = 0;
+
+ /**
+ * Check if given entry's packets conflict with this' entries packets.
+ *
+ * @param entry Other entry to compare against.
+ * @return True if entry matches given information.
+ */
+ virtual bool conflictAddr(const QueueEntry* entry) const = 0;
+
+ /**
* Send this queue entry as a downstream packet, with the exact
* behaviour depending on the specific entry type.
*/
diff --git a/src/mem/cache/write_queue_entry.cc
b/src/mem/cache/write_queue_entry.cc
index 6c8387b..f3106e4 100644
--- a/src/mem/cache/write_queue_entry.cc
+++ b/src/mem/cache/write_queue_entry.cc
@@ -112,6 +112,9 @@
"a cacheable eviction or a writeclean");
targets.add(target, when_ready, _order);
+
+ // All targets must refer to the same block
+ assert(target->matchBlockAddr(targets.front().pkt, blkSize));
}
void
@@ -141,6 +144,27 @@
return cache.sendWriteQueuePacket(this);
}
+bool
+WriteQueueEntry::matchBlockAddr(const Addr addr, const bool is_secure)
const
+{
+ assert(hasTargets());
+ return (blkAddr == addr) && (isSecure == is_secure);
+}
+
+bool
+WriteQueueEntry::matchBlockAddr(const PacketPtr pkt) const
+{
+ assert(hasTargets());
+ return pkt->matchBlockAddr(blkAddr, isSecure, blkSize);
+}
+
+bool
+WriteQueueEntry::conflictAddr(const QueueEntry* entry) const
+{
+ assert(hasTargets());
+ return entry->matchBlockAddr(blkAddr, isSecure);
+}
+
void
WriteQueueEntry::print(std::ostream &os, int verbosity,
const std::string &prefix) const
diff --git a/src/mem/cache/write_queue_entry.hh
b/src/mem/cache/write_queue_entry.hh
index 59714d9..9aaac49 100644
--- a/src/mem/cache/write_queue_entry.hh
+++ b/src/mem/cache/write_queue_entry.hh
@@ -179,6 +179,10 @@
* @return string with mshr fields
*/
std::string print() const;
+
+ bool matchBlockAddr(const Addr addr, const bool is_secure) const
override;
+ bool matchBlockAddr(const PacketPtr pkt) const override;
+ bool conflictAddr(const QueueEntry* entry) const override;
};
#endif // __MEM_CACHE_WRITE_QUEUE_ENTRY_HH__
--
To view, visit https://gem5-review.googlesource.com/c/public/gem5/+/17530
To unsubscribe, or for help writing mail filters, visit
https://gem5-review.googlesource.com/settings
Gerrit-Project: public/gem5
Gerrit-Branch: master
Gerrit-Change-Id: I6e494a821c1e6e841ab103ec69632c0e1b269a08
Gerrit-Change-Number: 17530
Gerrit-PatchSet: 6
Gerrit-Owner: Daniel Carvalho <[email protected]>
Gerrit-Reviewer: Daniel Carvalho <[email protected]>
Gerrit-Reviewer: Nikos Nikoleris <[email protected]>
Gerrit-Reviewer: kokoro <[email protected]>
Gerrit-MessageType: merged
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev
