> On June 1, 2015, 5:14 p.m., Brad Beckmann wrote: > > src/mem/protocol/MESI_Two_Level-L1cache.sm, line 1227 > > <http://reviews.gem5.org/r/2842/diff/1/?file=45368#file45368line1227> > > > > Why introduce a new action here? It seems like the simplier way to > > handle this situation is insert the forward_eviction_to_cpu action after > > the hx_load_hit callback rather than introduce a new action and additional > > complexity in the sequencer. > > Marco Elver wrote: > Calling hx_load_hit and then forward_eviction_to_cpu would always have to > be seen by the LSQ in that order. To guarantee this is never an issue, both > should be sent as one. The main reason being that if the ReadResp arrives > after the InvalidateReq in the LSQ, the ReExec fault is lost in the LSQ. See > comment in LSQ::recvTimingResp. > > From what I can see, this order is currently not guaranteed (and I'm not > sure what the cost of guaranteeing it would be). In RubyPort, the > InvalidateReq is *not* sent with a schedTimingResp, yet any response via > hitCallback is. This means, that regardless of the calling (action) order, > the InvalidateReq always arrives before the ReadResp, and the fault is always > lost in the LSQ. > > There are likely other potential fixes (fix LSQ not to depend on order, > guarantee order from Ruby side), but this one seems the least intrusive and > most robust.
Ah, I see. Thanks for the explanation! Eventually we should remove all delay in the RubyPort interface. The complexity of this patch is good justification why we should check in this other patch: http://reviews.gem5.org/r/2784 as a step of removing unecessary delay between the core and the l1 cache. This patch looks good to me then. - Brad ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/2842/#review6447 ----------------------------------------------------------- On May 21, 2015, 7:27 p.m., Marco Elver wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/2842/ > ----------------------------------------------------------- > > (Updated May 21, 2015, 7:27 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 10839:2978f9ed5e7a > --------------------------- > ruby: Add ReadRespWithInvalidate support, fixes MESI consistency bug > > A sunk Inv in the IS state needs to be propagated to the LQ eventually. Thus > far, Ruby has no support for properly propagating an invalidate along with a > ReadResp. > > This patch adds support for ReadRespWithInvalidate upon a readCallback, so > that the LQ can properly deal with such a request. > > Note that we cannot just do a forward_eviction in the IS,Inv transition, as > other outstanding loads in the LQ after the one for which we get the Inv, may > be satisfied (with old values) inbetween the Inv and IS_I,Data transitions. > > The scenarios are the same as for the problem fixed in revision 10575 (which > addressed the LSQ side of things). > > > Diffs > ----- > > src/mem/protocol/MESI_Two_Level-L1cache.sm ecbab2522757 > src/mem/protocol/RubySlicc_Types.sm ecbab2522757 > src/mem/ruby/system/Sequencer.hh ecbab2522757 > src/mem/ruby/system/Sequencer.cc ecbab2522757 > > Diff: http://reviews.gem5.org/r/2842/diff/ > > > Testing > ------- > > Tester no longer finds bug. Note this was done with an (older) version of > gem5 that was still happy with Ruby+O3CPU. A quick run with ruby-tester > yielded no obvious issues. > > > *NOTE:* I'm not entirely happy with the "Hack?" code. If someone can possibly > suggest how we can add a nicer way to add support for ReadRespWithInvalidate > to Ruby, then that'd probably be better than what this patch does. > > > Thanks, > > Marco Elver > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
