diff -r 6ed5ea26d10b -r ca0ba71c37f9 src/mem/cache/cache_impl.hh --- a/src/mem/cache/cache_impl.hh Wed Nov 26 09:56:26 2014 +0000 +++ b/src/mem/cache/cache_impl.hh Wed Nov 26 10:13:03 2014 +0000 @@ -487,17 +487,24 @@ // that have shared copies. Not necessary if we know that // supplier had exclusive copy to begin with. if (pkt->needsExclusive() && !pkt->isSupplyExclusive()) { - Packet *snoopPkt = new Packet(pkt, true); // clear flags + // create a downstream express snoop with cleared packet + // flags, there is no need to allocate any data as the + // packet is merely used to co-ordinate state transitions + Packet *snoop_pkt = new Packet(pkt, true, false); + // also reset the bus time that the original packet has // not yet paid for - snoopPkt->firstWordDelay = snoopPkt->lastWordDelay = 0; - snoopPkt->setExpressSnoop(); - snoopPkt->assertMemInhibit(); - bool M5_VAR_USED success = memSidePort->sendTimingReq(snoopPkt); - // the packet is marked inhibited and will thus bypass any - // flow control + snoop_pkt->firstWordDelay = snoop_pkt->lastWordDelay = 0; + + // make this an instantaneous express snoop, and let the + // other caches in the system know that the packet is + // inhibited + snoop_pkt->setExpressSnoop(); + snoop_pkt->assertMemInhibit(); + bool M5_VAR_USED success = memSidePort->sendTimingReq(snoop_pkt); + // express snoops always succeed assert(success); - // main memory will delete snoopPkt + // main memory will delete the packet } // since we're the official target but we aren't responding, // delete the packet now. @@ -1578,12 +1585,19 @@ { // sanity check assert(req_pkt->isRequest()); + assert(req_pkt->needsResponse()); DPRINTF(Cache, "%s for %s address %x size %d\n", __func__, req_pkt->cmdString(), req_pkt->getAddr(), req_pkt->getSize()); // timing-mode snoop responses require a new packet, unless we // already made a copy... - PacketPtr pkt = already_copied ? req_pkt : new Packet(req_pkt); + PacketPtr pkt = req_pkt; + if (!already_copied) + // do not clear flags, and allocate space for data if the + // packet needs it (the only packets that carry data are read + // responses) + pkt = new Packet(req_pkt, false, req_pkt->isRead()); + assert(req_pkt->isInvalidate() || pkt->sharedAsserted()); pkt->makeTimingResponse(); // @todo Make someone pay for this @@ -1632,7 +1646,7 @@ // rewritten to be relative to cpu-side bus (if any) bool alreadyResponded = pkt->memInhibitAsserted(); if (is_timing) { - Packet snoopPkt(pkt, true); // clear flags + Packet snoopPkt(pkt, true, false); // clear flags, no allocation snoopPkt.setExpressSnoop(); snoopPkt.pushSenderState(new ForwardResponseRecord(pkt->getSrc())); // the snoop packet does not need to wait any additional @@ -2004,7 +2018,7 @@ // at the moment. Without this check we could get a stale // copy from memory that might get used in place of the // dirty one. - Packet snoop_pkt(tgt_pkt, true); + Packet snoop_pkt(tgt_pkt, true, false); snoop_pkt.setExpressSnoop(); snoop_pkt.senderState = mshr; cpuSidePort->sendTimingSnoopReq(&snoop_pkt); @@ -2040,7 +2054,7 @@ // not a cache block request, but a response is expected // make copy of current packet to forward, keep current // copy for response handling - pkt = new Packet(tgt_pkt); + pkt = new Packet(tgt_pkt, false, true); if (pkt->isWrite()) { pkt->setData(tgt_pkt->getConstPtr()); } diff -r 6ed5ea26d10b -r ca0ba71c37f9 src/mem/cache/mshr.cc --- a/src/mem/cache/mshr.cc Wed Nov 26 09:56:26 2014 +0000 +++ b/src/mem/cache/mshr.cc Wed Nov 26 10:13:03 2014 +0000 @@ -366,7 +366,10 @@ // Actual target device (typ. a memory) will delete the // packet on reception, so we need to save a copy here. - PacketPtr cp_pkt = new Packet(pkt, true); + PacketPtr cp_pkt = new Packet(pkt, true, true); + // clear flags and also allocate new data as the original + // packet data storage may have been deleted by the time we + // get to send this packet targets.add(cp_pkt, curTick(), _order, Target::FromSnoop, downstreamPending && targets.needsExclusive); diff -r 6ed5ea26d10b -r ca0ba71c37f9 src/mem/packet.hh --- a/src/mem/packet.hh Wed Nov 26 09:56:26 2014 +0000 +++ b/src/mem/packet.hh Wed Nov 26 10:13:03 2014 +0000 @@ -652,9 +652,9 @@ * less than that of the original packet. In this case the new * packet should allocate its own data. */ - Packet(PacketPtr pkt, bool clearFlags = false) + Packet(PacketPtr pkt, bool clear_flags, bool alloc_data) : cmd(pkt->cmd), req(pkt->req), - data(pkt->flags.isSet(STATIC_DATA) ? pkt->data : NULL), + data(nullptr), addr(pkt->addr), _isSecure(pkt->_isSecure), size(pkt->size), src(pkt->src), dest(pkt->dest), bytesValidStart(pkt->bytesValidStart), @@ -663,16 +663,26 @@ lastWordDelay(pkt->lastWordDelay), senderState(pkt->senderState) { - if (!clearFlags) + if (!clear_flags) flags.set(pkt->flags & COPY_FLAGS); flags.set(pkt->flags & (VALID_ADDR|VALID_SIZE)); - flags.set(pkt->flags & STATIC_DATA); - // if we did not copy the static data pointer, allocate data - // dynamically instead - if (!data) - allocate(); + // should we allocate space for data, or not, the express + // snoops do not need to carry any data as they only serve to + // co-ordinate state changes + if (alloc_data) { + // even if asked to allocate data, if the original packet + // holds static data, then the sender will not be doing + // any memcpy on receiving the response, thus we simply + // carry the pointer forward + if (pkt->flags.isSet(STATIC_DATA)) { + data = pkt->data; + flags.set(STATIC_DATA); + } else { + allocate(); + } + } } /** @@ -789,7 +799,14 @@ /** * Set the data pointer to the following value that should not be - * freed. + * freed. Static data allows us to do a single memcpy even if + * multiple packets are required to get from source to destination + * and back. In essence the pointer is set calling dataStatic on + * the original packet, and when ever this packet is copied and + * forwarded the same pointer is passed on. When a packet + * eventually reaches the destination holding the data, it is + * copied once into the location originally set. On the way back + * to the source, no copies are necessary. */ template void @@ -819,7 +836,15 @@ /** * Set the data pointer to a value that should have delete [] - * called on it. + * called on it. Dynamic data is local to this packet, and as the + * packet travels from source to destination, forwarded packets + * will allocated their own data. When a packet reaches the final + * destination it will populate the dynamic data of that specific + * packet, and on the way back towards the source, memcpy will be + * invoked in every step where a new packet was created e.g. in + * the caches. Ultimately when the response reaches the source a + * final memcpy is needed to extract the data from the packet + * before it is deallocated. */ template void @@ -867,7 +892,14 @@ void setData(const uint8_t *p) { + // we should never be copying data onto itself, which means we + // must idenfity packets with static data, as they carry the + // same pointer from source to destination and back + assert(p != getPtr() || flags.isSet(STATIC_DATA)); + if (p != getPtr()) + // for packet with allocated dynamic data, we copy data from + // one to the other, e.g. a forwarded response to a response std::memcpy(getPtr(), p, getSize()); }