memHierarchy: serialize reads behind in-flight dirty writebacks in incoherent noninclusive caches - #2709
Open
jake-ke wants to merge 112 commits into
Open
memHierarchy: serialize reads behind in-flight dirty writebacks in incoherent noninclusive caches#2709jake-ke wants to merge 112 commits into
jake-ke wants to merge 112 commits into
Conversation
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Automatically Merged using SST Master Branch Merger
Under coherence_protocol=none (Incoherent / Incoherent_L1) with a noninclusive
lower level, a cache evicting a dirty (M) line sent the PutM writeback
fire-and-forget and immediately dropped the line without tracking it (no
mshr_->insertWriteback). A concurrent read to the same line then missed, raced
the writeback down to memory, and could read the pre-writeback (stale) value.
Because incoherent caches never invalidate, the stale value was then cached
permanently -- silent data corruption. The coherent managers avoid this via the
existing insertWriteback / AckPut MSHR-blocking machinery, which the incoherent
path never wired up (and the MemController never acked PutM).
Wire up the existing writeback-ack serialization for the incoherent path:
- Incoherent / Incoherent_L1: on M eviction, insert an MSHR writeback entry
when recv_writeback_ack_ is set; advertise recv/send WB acks in
getInitCoherenceEvent; add handleAckPut (clears the writeback entry and
replays the stalled request); accept Command::AckPut.
- MemController: add a default-off 'writeback_acks' param. When enabled, leave
the PutM response in place (AckPut) instead of F_NORESPONSE, and advertise
sendWBAck during init. Default-off keeps every existing (coherent) config
byte-identical; only a noninclusive incoherent hierarchy opts in.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
MSHR::insertEviction appended newAddr to an Evict entry's pointer list
unconditionally. A request that is retried while already parked behind that
same eviction re-enters processCacheMiss -> allocateLine, fails the eviction
again, and re-registers, so the list ends up holding the same waiter twice.
The incoherent L1 is the exposed case: unlike Incoherent (L2) and the MESI
managers, IncoherentL1 never sets or checks the MSHR's stalledForEvict flag,
so nothing suppresses the re-registration.
The duplicate is then fatal. cleanUpAfterRequest/Response/retry emit one
NULLCMD per pointer, so a duplicated waiter produces two NULLCMDs for the same
(oldAddr, newAddr). The first one's removeEvictPointer clears *all* matching
pointers (std::list::remove) and, if that was oldAddr's last entry, erases
oldAddr's MSHR register; the second then dereferences the dead register and
fatals in MSHR::getFrontType ("Address doesn't exist in MSHR").
An eviction-pointer list is semantically a set -- each waiter needs exactly one
wake-up -- so only append a waiter that is not already present. This cannot drop
a wake-up (the surviving entry still retries the waiter) and cannot deadlock (it
only skips a redundant append).
This companions the incoherent dirty-eviction writeback-ack fix, which creates
far more "cannot evict yet, park the request" windows and so makes the
re-registration reachable in practice.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
The writeback-ack handshake introduced MSHREntryType::Writeback into the
incoherent managers for the first time, and MSHR::insertWriteback push_fronts it,
so a Writeback entry can sit ahead of anything already queued for that address.
Three sites still assumed an MSHR register's front entry is only ever an Event or
an Evict. They mask each other, so they surface one at a time.
1. Incoherent::retry() lacked the pendingWriteback guard that IncoherentL1::retry
and all four MESI managers have. A retry on an address with an in-flight
writeback therefore fell into the eviction-pointer branch and fataled in
MSHR::getEvictPointers ("Entry type is not Evict"). This is the crash seen on
long runs: the L2 evicts a dirty line, and the L1 writes the same line back
inside the memory ack round trip.
2. handlePutM/handlePutE serviced a writeback from the level above without
consulting the MSHR -- in state I they call allocateLine directly. That both
reaches (1) and re-installs the line while our own PutM for the same address is
still in flight, so a later eviction can put a second PutM for that address on
the wire and the level below may absorb the two out of order: exactly the
stale-data race the handshake exists to prevent.
3. handleEviction push_fronted a Writeback ahead of a queued request, displacing
the front entry that handleNULLCMD replays and that carries the
stalled-for-evict and in-progress state. The request was then woken twice (once
by the AckPut's retry, once by its own NULLCMD), re-parked on a second victim
and completed, leaving eviction pointers aimed at a register that no longer
exists (fatal in MSHR::getFrontType) or a null in the retry buffer (segfault in
Cache::processEvent).
Fixes: guard Incoherent::retry with pendingWriteback; stall PutM/PutE behind a
pending writeback for the same address; check the front entry type before
replaying a waiter in both handleNULLCMDs; follow eviction pointers only from an
Evict front in IncoherentL1::cleanUpAfterResponse; and refuse a tracked dirty
eviction while a request is queued for that line, so a Writeback never displaces
a queued entry (this removes the cause of 2 and 3 rather than tolerating it).
Validated: four standalone configurations (incoherent L1 -> noninclusive
incoherent L2 -> memory with writeback_acks=1 and a long backend latency to widen
the ack window) reproduce all three failures before the fix and run clean after;
138 swept configurations across cache size, associativity, MSHR depth, write mix,
memory latency and a bus topology with a second L1 beside the L2 all complete;
SAT workloads return correct results with bit-identical cycle counts before and
after, so the fix costs nothing outside the race window.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Contributor
|
Status Flag 'Pre-Test Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2708.
Problem
Under
coherence_protocol=none(Incoherent/Incoherent_L1) above a noninclusive level, a cache evicting a dirty (M) line sends thePutMwriteback fire-and-forget and deallocates the line without tracking the in-flight writeback (nomshr_->insertWriteback). A concurrent same-line read can then race the writeback down to the next level and read the pre-writeback (stale) value; since incoherent caches never invalidate, the stale value is cached and returned to the CPU. The MSHR writeback-blocking machinery (insertWriteback/handleAckPut/AckPut) already exists and is used by the coherent (MESI) managers — the incoherent path just never wired it up, and theMemControllernever acksPutM.Fix
Wire up the existing writeback-ack serialization on the incoherent path:
coherencemgr/Incoherent_L1.{cc,h},coherencemgr/Incoherent.{cc,h}: on M eviction insert an MSHR writeback entry whenrecv_writeback_ack_; advertiserecvWBAck(L1) /sendWBAck(L2) ingetInitCoherenceEvent; addhandleAckPut; acceptCommand::AckPutingetValidReceiveEvents.memoryController.{cc,h}: add a default-offwriteback_acksparameter. When enabled, the controller leaves thePutMresponse in place (→AckPut) instead ofF_NORESPONSE, and advertisessendWBAckduring init. A noninclusive incoherent hierarchy opts in via this param.With the param off (the default), behavior is byte-identical for every existing configuration — the change only takes effect when a config explicitly sets
writeback_acks=1.Testing
MemControllerparam defaults off, so all coherent (MSI/MESI, inclusive/noninclusive) configs are byte-identical.coherence_protocol=nonehierarchy from Silent data corruption: incoherent (coherence_protocol=none) cache serves a stale line when a read races a dirty-eviction writeback #2708 with and without the fix; no regression with the fix applied.Note for reviewers
Instead of the explicit
writeback_acksparam, theMemControllercould auto-ackPutMwhen the connected cache advertisedrecvWBAck=trueduring init (captured inprocessInitEvent) — cleaner and avoids a user-facing param. The param version is offered as the safe, explicit default; happy to switch to auto-negotiation if preferred.PR checklist
develbranchdevel