Hi guys, I do not think the solution is to remove delays (as Brad suggested). Rather, I suggest we do the same as in the classic memory system, which is to enforce order for all responses and snoop requests sent towards the CPU.
Andreas On 01/06/2015 13:13, "Brad Beckmann" <[email protected]> wrote: > > >> 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 -- IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you. ARM Limited, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2557590 ARM Holdings plc, Registered office 110 Fulbourn Road, Cambridge CB1 9NJ, Registered in England & Wales, Company No: 2548782 _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
