Thanks for fixing this kyle! I discovered the same bug a few days ago but didn't add a stall for when a DMA arrives while the cache is blocking. This should fix it.
Best, Dan On Thu, Aug 13, 2020 at 3:05 PM Kyle Roarty (Gerrit) via gem5-dev < gem5-dev@gem5.org> wrote: > Kyle Roarty *submitted* this change. > > View Change <https://gem5-review.googlesource.com/c/public/gem5/+/31996> > Approvals: Bradford Beckmann: Looks good to me, approved; Looks good to > me, approved Matt Sinclair: Looks good to me, but someone else must approve > kokoro: Regressions pass > > mem-ruby: fix races between data and DMA in MOESI_AMD_Base-dir > > There are race conditions while running several benchmarks, where > the DMA engine and the CorePair simultaneously send requests for the > same block. This patch fixes two scenarios > (a) If the request from the DMA engine arrives before the one from the > CorePair, the directory controller records it as a pending request. > However, once the DMA request is serviced, the directory doesn't check > for pending requests. The CorePair, consequently, never sees a response > to its request and this results in a Deadlock. > > Added call to wakeUpDependents in the transition from BDR_Pm to U > Added call to wakeUpDependents in the transition from BDW_P to U > > (b) If the request from the CorePair is being serviced by the directory > and the DMA requests for the same block, this causes an invalid > transition because the current coherence doesn't take care of this > scenario. > > Added transition state where the requests from DMA are added to the > stall buffer. > > Updated B to U CoreUnblock transition to check all buffers, as the DMA > requests were being placed later in the stall buffer than was being checked > > Change-Id: I5a76efef97723bc53cf239ea7e112f84fc874ef8 > Reviewed-on: https://gem5-review.googlesource.com/c/public/gem5/+/31996 > Reviewed-by: Matt Sinclair <mattdsincl...@gmail.com> > Reviewed-by: Bradford Beckmann <brad.beckm...@amd.com> > Maintainer: Bradford Beckmann <brad.beckm...@amd.com> > Tested-by: kokoro <noreply+kok...@google.com> > --- > M src/mem/ruby/protocol/MOESI_AMD_Base-dir.sm > M src/mem/ruby/slicc_interface/AbstractController.cc > 2 files changed, 22 insertions(+), 3 deletions(-) > > diff --git a/src/mem/ruby/protocol/MOESI_AMD_Base-dir.sm > b/src/mem/ruby/protocol/MOESI_AMD_Base-dir.sm > index c8dafd5..f1bc637 100644 > --- a/src/mem/ruby/protocol/MOESI_AMD_Base-dir.sm > +++ b/src/mem/ruby/protocol/MOESI_AMD_Base-dir.sm > @@ -180,6 +180,7 @@ > void set_tbe(TBE a); > void unset_tbe(); > void wakeUpAllBuffers(); > + void wakeUpAllBuffers(Addr a); > void wakeUpBuffers(Addr a); > Cycles curCycle(); > > @@ -1069,6 +1070,10 @@ > stall_and_wait(requestNetwork_in, address); > } > > + action(sd_stallAndWaitRequest, "sd", desc="Stall and wait on the address") > { > + stall_and_wait(dmaRequestQueue_in, address); > + } > + > action(wa_wakeUpDependents, "wa", desc="Wake up any requests waiting for > this address") { > wakeUpBuffers(address); > } > @@ -1077,6 +1082,10 @@ > wakeUpAllBuffers(); > } > > + action(wa_wakeUpAllDependentsAddr, "waaa", desc="Wake up any requests > waiting for this address") { > + wakeUpAllBuffers(address); > + } > + > action(z_stall, "z", desc="...") { > } > > @@ -1090,6 +1099,11 @@ > st_stallAndWaitRequest; > } > > + // The exit state is always going to be U, so wakeUpDependents logic > should be covered in all the > + // transitions which are flowing into U. > + transition({BL, BS_M, BM_M, B_M, BP, BDW_P, BS_PM, BM_PM, B_PM, BS_Pm, > BM_Pm, B_Pm, B}, {DmaRead,DmaWrite}){ > + sd_stallAndWaitRequest; > + } > > // transitions from U > transition(U, DmaRead, BDR_PM) {L3TagArrayRead} { > @@ -1193,7 +1207,7 @@ > } > > transition({B}, CoreUnblock, U) { > - wa_wakeUpDependents; > + wa_wakeUpAllDependentsAddr; > pu_popUnblockQueue; > } > > @@ -1323,12 +1337,18 @@ > } > > transition(BDW_P, ProbeAcksComplete, U) { > + // Check for pending requests from the core we put to sleep while waiting > + // for a response > + wa_wakeUpAllDependentsAddr; > dt_deallocateTBE; > pt_popTriggerQueue; > } > > transition(BDR_Pm, ProbeAcksComplete, U) { > dd_sendResponseDmaData; > + // Check for pending requests from the core we put to sleep while waiting > + // for a response > + wa_wakeUpDependents; > dt_deallocateTBE; > pt_popTriggerQueue; > } > diff --git a/src/mem/ruby/slicc_interface/AbstractController.cc > b/src/mem/ruby/slicc_interface/AbstractController.cc > index 9da8727..d2b3370 100644 > --- a/src/mem/ruby/slicc_interface/AbstractController.cc > +++ b/src/mem/ruby/slicc_interface/AbstractController.cc > @@ -149,8 +149,7 @@ > { > if (m_waiting_buffers.count(addr) > 0) { > // > - // Wake up all possible lower rank (i.e. lower priority) buffers > that could > - // be waiting on this message. > + // Wake up all possible buffers that could be waiting on this > message. > // > for (int in_port_rank = m_in_ports - 1; > in_port_rank >= 0; > > To view, visit change 31996 > <https://gem5-review.googlesource.com/c/public/gem5/+/31996>. To > unsubscribe, or for help writing mail filters, visit settings > <https://gem5-review.googlesource.com/settings>. > Gerrit-Project: public/gem5 > Gerrit-Branch: develop > Gerrit-Change-Id: I5a76efef97723bc53cf239ea7e112f84fc874ef8 > Gerrit-Change-Number: 31996 > Gerrit-PatchSet: 3 > Gerrit-Owner: Kyle Roarty <kyleroarty1...@gmail.com> > Gerrit-Reviewer: Bradford Beckmann <brad.beckm...@amd.com> > Gerrit-Reviewer: Jason Lowe-Power <power...@gmail.com> > Gerrit-Reviewer: Kyle Roarty <kyleroarty1...@gmail.com> > Gerrit-Reviewer: Matt Sinclair <mattdsincl...@gmail.com> > Gerrit-Reviewer: kokoro <noreply+kok...@google.com> > Gerrit-CC: Alexandru Duțu <alexandru.d...@amd.com> > Gerrit-CC: Anthony Gutierrez <anthony.gutier...@amd.com> > Gerrit-CC: GAURAV JAIN <gja...@wisc.edu> > Gerrit-CC: John Alsop <johnathan.al...@amd.com> > Gerrit-CC: Matthew Poremba <matthew.pore...@amd.com> > Gerrit-CC: Pouya Fotouhi <pfoto...@ucdavis.edu> > Gerrit-MessageType: merged > _______________________________________________ > gem5-dev mailing list -- gem5-dev@gem5.org > To unsubscribe send an email to gem5-dev-le...@gem5.org > %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
_______________________________________________ gem5-dev mailing list -- gem5-dev@gem5.org To unsubscribe send an email to gem5-dev-le...@gem5.org %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
