Hello Anouk Van Laer, Curtis Dunham,

I'd like you to do a code review. Please visit

    https://gem5-review.googlesource.com/5046

to review the following change.


Change subject: mem: Support for specifying the destination of a WriteClean
......................................................................

mem: Support for specifying the destination of a WriteClean

Previously, WriteClean packets would always write to the first memory
below unless the memory was unable to allocate in which case it would
be forwarded further below.

This change adds support for specifying the destination of a
WriteClean packet. The cache annotates the request with the specified
destination and marks the packet as write-through upon its
creation. The coherent xbar checks packets for their destination and
resets the write-through flag when necessary e.g., the coherent xbar
that is set as the PoC will reset the write-through flag for packets
to the PoC.

Change-Id: I84b653f5cb6e46e97e09508649a3725d72d94606
Reviewed-by: Curtis Dunham <[email protected]>
Reviewed-by: Anouk Van Laer <[email protected]>
---
M src/mem/cache/cache.cc
M src/mem/cache/cache.hh
M src/mem/coherent_xbar.cc
M src/mem/coherent_xbar.hh
M src/mem/packet.cc
M src/mem/packet.hh
M src/mem/request.hh
7 files changed, 136 insertions(+), 23 deletions(-)



diff --git a/src/mem/cache/cache.cc b/src/mem/cache/cache.cc
index 467bc04..4d3b590 100644
--- a/src/mem/cache/cache.cc
+++ b/src/mem/cache/cache.cc
@@ -430,20 +430,26 @@
         assert(blkSize == pkt->getSize());

         if (!blk) {
-            // a writeback that misses needs to allocate a new block
-            blk = allocateBlock(pkt->getAddr(), pkt->isSecure(),
-                                writebacks);
-            if (!blk) {
-                // no replaceable block available: give up, fwd to
-                // next level.
-                incMissCount(pkt);
+            if (pkt->writeThrough()) {
+                // if this is a write through packet, we don't try to
+                // allocate if the block is not present
                 return false;
-            }
-            tags->insertBlock(pkt, blk);
+            } else {
+                // a writeback that misses needs to allocate a new block
+                blk = allocateBlock(pkt->getAddr(), pkt->isSecure(),
+                                    writebacks);
+                if (!blk) {
+                    // no replaceable block available: give up, fwd to
+                    // next level.
+                    incMissCount(pkt);
+                    return false;
+                }
+                tags->insertBlock(pkt, blk);

-            blk->status = (BlkValid | BlkReadable);
-            if (pkt->isSecure()) {
-                blk->status |= BlkSecure;
+                blk->status = (BlkValid | BlkReadable);
+                if (pkt->isSecure()) {
+                    blk->status |= BlkSecure;
+                }
             }
         }

@@ -451,7 +457,9 @@
         // write clean operation and the block is already in this
         // cache, we need to update the data and the block flags
         assert(blk);
-        blk->status |= BlkDirty;
+        if (!pkt->writeThrough()) {
+            blk->status |= BlkDirty;
+        }
         // nothing else to do; writeback doesn't expect response
         assert(!pkt->needsResponse());
         std::memcpy(blk->data, pkt->getConstPtr<uint8_t>(), blkSize);
@@ -461,7 +469,9 @@
         // populate the time when the block will be ready to access.
         blk->whenReady = clockEdge(fillLatency) + pkt->headerDelay +
             pkt->payloadDelay;
-        return true;
+        // if this a write-through packet it will be sent to cache
+        // below
+        return !pkt->writeThrough();
     } else if (blk && (pkt->needsWritable() ? blk->isWritable() :
                        blk->isReadable())) {
         // OK to satisfy access
@@ -1623,7 +1633,7 @@
 }

 PacketPtr
-Cache::writecleanBlk(CacheBlk *blk)
+Cache::writecleanBlk(CacheBlk *blk, Request::Flags dest)
 {
     Request *req = new Request(tags->regenerateBlkAddr(blk->tag, blk->set),
                                blkSize, 0, Request::wbMasterId);
@@ -1641,6 +1651,10 @@
     // We inform the cache below that the block has sharers in the
     // system as we retain our copy.
     pkt->setHasSharers();
+    if (dest) {
+        req->setFlags(dest);
+        pkt->setWriteThrough();
+    }
     std::memcpy(pkt->getPtr<uint8_t>(), blk->data, blkSize);
     return pkt;
 }
diff --git a/src/mem/cache/cache.hh b/src/mem/cache/cache.hh
index 94dbf56..8e20e91 100644
--- a/src/mem/cache/cache.hh
+++ b/src/mem/cache/cache.hh
@@ -455,9 +455,10 @@
     /**
      * Create a writeclean request for the given block.
      * @param blk The block to write clean
+     * @param dest The destination of this clean operation
      * @return The write clean packet for the block.
      */
-    PacketPtr writecleanBlk(CacheBlk *blk);
+    PacketPtr writecleanBlk(CacheBlk *blk, Request::Flags dest = 0);

     /**
      * Create a CleanEvict request for the given block.
diff --git a/src/mem/coherent_xbar.cc b/src/mem/coherent_xbar.cc
index c46660c..da253b3 100644
--- a/src/mem/coherent_xbar.cc
+++ b/src/mem/coherent_xbar.cc
@@ -183,6 +183,12 @@
     // determine how long to be crossbar layer is busy
     Tick packetFinishTime = clockEdge(Cycles(1)) + pkt->payloadDelay;

+    // is this the destination point for this packet? (e.g. true if
+    // this xbar is the PoC for a cache maintenance operation to the
+    // PoC) otherwise the destination is any cache that can satisfy
+    // the request
+    const bool is_destination = isDestination(pkt);
+
     const bool snoop_caches = !system->bypassCaches() &&
         pkt->cmd != MemCmd::WriteClean;
     if (snoop_caches) {
@@ -243,7 +249,7 @@
     } else {
         // determine if we are forwarding the packet, or responding to
         // it
-        if (!pointOfCoherency || pkt->isRead() || pkt->isWrite()) {
+        if (forwardPacket(pkt)) {
             // if we are passing on, rather than sinking, a packet to
             // which an upstream cache has committed to responding,
             // the line was needs writable, and the responding only
@@ -253,6 +259,13 @@
                 pkt->setExpressSnoop();
             }

+            // make sure that the write request (e.g., WriteClean)
+            // will stop at the memory below if this crossbar is its
+            // destination
+            if (pkt->isWrite() && is_destination) {
+                pkt->clearWriteThrough();
+            }
+
             // since it is a normal request, attempt to send the packet
             success = masterPorts[master_port_id]->sendTimingReq(pkt);
         } else {
@@ -646,6 +659,12 @@
     MemCmd snoop_response_cmd = MemCmd::InvalidCmd;
     Tick snoop_response_latency = 0;

+    // is this the destination point for this packet? (e.g. true if
+    // this xbar is the PoC for a cache maintenance operation to the
+    // PoC) otherwise the destination is any cache that can satisfy
+    // the request
+    const bool is_destination = isDestination(pkt);
+
     const bool snoop_caches = !system->bypassCaches() &&
         pkt->cmd != MemCmd::WriteClean;
     if (snoop_caches) {
@@ -698,7 +717,14 @@
         DPRINTF(CoherentXBar, "%s: Not forwarding %s\n", __func__,
                 pkt->print());
     } else {
-        if (!pointOfCoherency || pkt->isRead() || pkt->isWrite()) {
+        if (forwardPacket(pkt)) {
+            // make sure that the write request (e.g., WriteClean)
+            // will stop at the memory below if this crossbar is its
+            // destination
+            if (pkt->isWrite() && is_destination) {
+                pkt->clearWriteThrough();
+            }
+
             // forward the request to the appropriate destination
response_latency = masterPorts[master_port_id]->sendAtomic(pkt);
         } else {
@@ -958,6 +984,16 @@
          (!pkt->needsWritable() || pkt->responderHadWritable()));
 }

+bool
+CoherentXBar::forwardPacket(const PacketPtr pkt)
+{
+    // we are forwarding the packet if:
+    // 1) this is a read or a write
+    // 2) this crossbar is above the point of coherency
+    return pkt->isRead() || pkt->isWrite() || !pointOfCoherency;
+}
+
+
 void
 CoherentXBar::regStats()
 {
diff --git a/src/mem/coherent_xbar.hh b/src/mem/coherent_xbar.hh
index 214a290..0c2907f 100644
--- a/src/mem/coherent_xbar.hh
+++ b/src/mem/coherent_xbar.hh
@@ -398,6 +398,29 @@
      */
     bool sinkPacket(const PacketPtr pkt) const;

+    /**
+     * Determine if the crossbar should forward the packet, as opposed to
+     * responding to it.
+     */
+    bool forwardPacket(const PacketPtr pkt);
+
+    /**
+     * Determine if the packet's destination is the memory below
+     *
+     * The memory below is the destination for a cache mainteance
+     * operation to the Point of Coherence/Unification if this is the
+     * Point of Coherence/Unification.
+     *
+     * @param pkt The processed packet
+     *
+     * @return Whether the memory below is the destination for the packet
+     */
+    bool isDestination(const PacketPtr pkt) const
+    {
+        return (pkt->req->isToPOC() && pointOfCoherency) ||
+            (pkt->req->isToPOU() && pointOfUnification);
+    }
+
     Stats::Scalar snoops;
     Stats::Scalar snoopTraffic;
     Stats::Distribution snoopFanout;
diff --git a/src/mem/packet.cc b/src/mem/packet.cc
index 4e1b8e7..444561e 100644
--- a/src/mem/packet.cc
+++ b/src/mem/packet.cc
@@ -349,12 +349,14 @@
 void
 Packet::print(ostream &o, const int verbosity, const string &prefix) const
 {
-    ccprintf(o, "%s%s [%x:%x]%s%s%s%s", prefix, cmdString(),
+    ccprintf(o, "%s%s [%x:%x]%s%s%s%s%s%s", prefix, cmdString(),
              getAddr(), getAddr() + getSize() - 1,
              req->isSecure() ? " (s)" : "",
              req->isInstFetch() ? " IF" : "",
              req->isUncacheable() ? " UC" : "",
-             isExpressSnoop() ? " ES" : "");
+             isExpressSnoop() ? " ES" : "",
+             req->isToPOC() ? " PoC" : "",
+             req->isToPOU() ? " PoU" : "");
 }

 std::string
diff --git a/src/mem/packet.hh b/src/mem/packet.hh
index b317723..34735ed 100644
--- a/src/mem/packet.hh
+++ b/src/mem/packet.hh
@@ -253,7 +253,7 @@

     enum : FlagsType {
         // Flags to transfer across when copying a packet
-        COPY_FLAGS             = 0x0000000F,
+        COPY_FLAGS             = 0x0000001F,

         // Does this packet have sharers (which means it should not be
         // considered writable) or not. See setHasSharers below.
@@ -272,6 +272,10 @@
         // responding to a snoop. See setCacheResponding below.
         CACHE_RESPONDING       = 0x00000008,

+        // The writeback/writeclean should be propagated further
+        // downstream by the receiver
+        WRITE_THROUGH          = 0x00000010,
+
         /// Are the 'addr' and 'size' fields valid?
         VALID_ADDR             = 0x00000100,
         VALID_SIZE             = 0x00000200,
@@ -619,6 +623,19 @@
     bool responderHadWritable() const
     { return flags.isSet(RESPONDER_HAD_WRITABLE); }

+    /**
+     * A writeback/writeclean cmd gets propagated further downstream
+     * by the receiver when the flag is set.
+     */
+    void setWriteThrough()
+    {
+        assert(cmd.isWrite() &&
+               (cmd.isEviction() || cmd == MemCmd::WriteClean));
+        flags.set(WRITE_THROUGH);
+    }
+    void clearWriteThrough() { flags.clear(WRITE_THROUGH); }
+    bool writeThrough() const { return flags.isSet(WRITE_THROUGH); }
+
     void setSuppressFuncError()     { flags.set(SUPPRESS_FUNC_ERROR); }
bool suppressFuncError() const { return flags.isSet(SUPPRESS_FUNC_ERROR); }
     void setBlockCached()          { flags.set(BLOCK_CACHED); }
diff --git a/src/mem/request.hh b/src/mem/request.hh
index d9f58d2..ff01cca 100644
--- a/src/mem/request.hh
+++ b/src/mem/request.hh
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2012-2013 ARM Limited
+ * Copyright (c) 2012-2013,2017 ARM Limited
  * All rights reserved
  *
  * The license below extends only to copyright in the software and shall
@@ -87,7 +87,7 @@
 class Request
 {
   public:
-    typedef uint32_t FlagsType;
+    typedef uint64_t FlagsType;
     typedef uint8_t ArchFlagsType;
     typedef ::Flags<FlagsType> Flags;

@@ -182,6 +182,15 @@
         /** The request is a page table walk */
         PT_WALK                     = 0x20000000,

+        /** The request targets the point of unification */
+        DST_POU                     = 0x0000001000000000,
+
+        /** The request targets the point of coherence */
+        DST_POC                     = 0x0000002000000000,
+
+        /** Bits to define the destination of a request */
+        DST_BITS                    = 0x0000003000000000,
+
         /**
          * These flags are *not* cleared when a Request object is
          * reused (assigned a new address).
@@ -790,6 +799,17 @@
     }

     /**
+     * Accessor functions for the destination of a memory request. The
+     * destination flag can specify a point of reference for the
+     * operation (e.g. a cache block clean to the the point of
+     * unification). At the moment the destination is only used by the
+     * cache maintenance operations.
+     */
+    bool isToPOU() const { return _flags.isSet(DST_POU); }
+    bool isToPOC() const { return _flags.isSet(DST_POC); }
+    Flags getDest() const { return _flags & DST_BITS; }
+
+    /**
* Accessor functions for the memory space configuration flags and used by * GPU ISAs such as the Heterogeneous System Architecture (HSA). Note that
      * these are for testing only; setting extraFlags should be done via

--
To view, visit https://gem5-review.googlesource.com/5046
To unsubscribe, or for help writing mail filters, visit https://gem5-review.googlesource.com/settings

Gerrit-Project: public/gem5
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I84b653f5cb6e46e97e09508649a3725d72d94606
Gerrit-Change-Number: 5046
Gerrit-PatchSet: 1
Gerrit-Owner: Nikos Nikoleris <[email protected]>
Gerrit-Reviewer: Anouk Van Laer <[email protected]>
Gerrit-Reviewer: Curtis Dunham <[email protected]>
_______________________________________________
gem5-dev mailing list
[email protected]
http://m5sim.org/mailman/listinfo/gem5-dev

Reply via email to