From a23e914519de16061340db62d7a5dfc2cc7d027e Mon Sep 17 00:00:00 2001 From: Andreas Hansson Date: Fri, 12 Aug 2016 14:11:45 +0100 Subject: [PATCH] mem: Use FromCache attribute in snoop filter allocation This patch improves the snoop filter allocation decisions by not only looking at whether a port is snooping or not, but also if the packet actually came from a cache. The issue with only looking at isSnooping is that the CPU ports, for example, are snooping, but not actually caching. Previously we ended up incorrectly allocating entries in systems without caches (such as the atomic and timing quick regressions). Eventually these misguided allocations caused the snoop filter to panic due to an excessive size. On the request path we now include the fromCache check on the packet itself, and for responses we check if we actually have a snoop-filter entry. Change-Id: Idd2dbc4f00c7e07d331e9a02658aee30d0350d7e Reviewed-by: Nikos Nikoleris Reviewed-by: Stephan Diestelhorst Reviewed-by: Tony Gutierrez --- src/mem/snoop_filter.cc | 32 +++++++++++++++++++------------- 1 file changed, 19 insertions(+), 13 deletions(-) diff --git a/src/mem/snoop_filter.cc b/src/mem/snoop_filter.cc index 636861c2b..14cdc7e07 100755 --- a/src/mem/snoop_filter.cc +++ b/src/mem/snoop_filter.cc @@ -1,5 +1,5 @@ /* - * Copyright (c) 2013-2015 ARM Limited + * Copyright (c) 2013-2016 ARM Limited * All rights reserved * * The license below extends only to copyright in the software and shall @@ -65,9 +65,9 @@ SnoopFilter::lookupRequest(const Packet* cpkt, const SlavePort& slave_port) DPRINTF(SnoopFilter, "%s: packet src %s addr 0x%x cmd %s\n", __func__, slave_port.name(), cpkt->getAddr(), cpkt->cmdString()); - // Ultimately we should check if the packet came from an - // allocating source, not just if the port is snooping - bool allocate = !cpkt->req->isUncacheable() && slave_port.isSnooping(); + // check if the packet came from a cache + bool allocate = !cpkt->req->isUncacheable() && slave_port.isSnooping() && + cpkt->fromCache(); Addr line_addr = cpkt->getBlockAddr(linesize); SnoopMask req_port = portToMask(slave_port); reqLookupResult = cachedLocations.find(line_addr); @@ -235,11 +235,12 @@ SnoopFilter::updateSnoopResponse(const Packet* cpkt, assert(cpkt->isResponse()); assert(cpkt->cacheResponding()); - // Ultimately we should check if the packet came from an - // allocating source, not just if the port is snooping - bool allocate = !cpkt->req->isUncacheable() && req_port.isSnooping(); - if (!allocate) + // if this snoop response is due to an uncacheable request, or is + // being turned into a normal response, there is nothing more to + // do + if (cpkt->req->isUncacheable() || !req_port.isSnooping()) { return; + } Addr line_addr = cpkt->getBlockAddr(linesize); SnoopMask rsp_mask = portToMask(rsp_port); @@ -268,6 +269,7 @@ SnoopFilter::updateSnoopResponse(const Packet* cpkt, sf_item.holder = 0; } assert(!cpkt->isWriteback()); + // @todo Deal with invalidating responses sf_item.holder |= req_mask; sf_item.requested &= ~req_mask; assert(sf_item.requested | sf_item.holder); @@ -319,15 +321,19 @@ SnoopFilter::updateResponse(const Packet* cpkt, const SlavePort& slave_port) assert(cpkt->isResponse()); - // Ultimately we should check if the packet came from an - // allocating source, not just if the port is snooping - bool allocate = !cpkt->req->isUncacheable() && slave_port.isSnooping(); - if (!allocate) + // we only allocate if the packet actually came from a cache, but + // start by checking if the port is snooping + if (cpkt->req->isUncacheable() || !slave_port.isSnooping()) return; + // next check if we actually allocated an entry Addr line_addr = cpkt->getBlockAddr(linesize); + auto sf_it = cachedLocations.find(line_addr); + if (sf_it == cachedLocations.end()) + return; + SnoopMask slave_mask = portToMask(slave_port); - SnoopItem& sf_item = cachedLocations[line_addr]; + SnoopItem& sf_item = sf_it->second; DPRINTF(SnoopFilter, "%s: old SF value %x.%x\n", __func__, sf_item.requested, sf_item.holder); -- 2.30.2