> On Oct. 10, 2015, 8:30 p.m., Nilay Vaish wrote: > > src/mem/ruby/system/Sequencer.cc, lines 180-184 > > <http://reviews.gem5.org/r/3149/diff/1/?file=50270#file50270line180> > > > > I prefer this check being done just before insertRequest() is called. > > Joel Hestness wrote: > I considered your request, but I don't understand why this check should > be moved. To check whether the line is blocked requires making a line address > out of the request address, which happens just before this, and this check is > consistent with other checks in insertRequest. Namely, this checks if there > is an outstanding operation for the line. Sequencer::makeRequest() just does > the translation of the packet to a RubyRequest, so returning from there with > RequestStatus_Aliased is inconsistent with the functionionality of that > method. Can you clarify why you think it should be moved? > > Nilay Vaish wrote: > insertRequest() is meant for deciding whether to put this request into > either of the read / write tables. And I think that's what it should be > doing and nothing more. It should not be the responsibility of the > insertRequest() > to check if there is some RMW operation going on the address. > > > I considered your request, but I don't understand why this check should > be moved. To check whether the line is blocked requires making a > line > address out of the request address, which happens just before this, > > Making line address is just one operation, other than the fact that we > use a global pointer to get the block size. > > > and this check is consistent with other checks in insertRequest. > > The only check in insertRequest() is which table to put the request into. > > > > Namely, this checks if there is an outstanding operation for the line. > Sequencer::makeRequest() just does the translation > > of the packet to a RubyRequest, so returning from there with > RequestStatus_Aliased is inconsistent with the functionionality of that > > method. Can you clarify why you think it should be moved? > > If you are worried about the return type, add a new one.
> insertRequest() is meant for deciding whether to put this request into either > of the read / write tables. And I think that's what it should be doing and > nothing more. It should not be the responsibility of the insertRequest() to > check if there is some RMW operation going on the address. Sorry, I'm still not clear about what you're arguing. This new isBlocked check is also checking whether to put the request into the read or write tables. It does so based on what accesses are currently in progress (i.e. an in-flight Locked_RMW), so it is consistent with the checks already in insertRequest. As I said, makeRequest only does translation from Packets to RubyRequests, so it has very different functionality than this new isBlocked check. To me, it doesn't make sense to move the isBlocked check into makeRequest. Also of note, the isBlocked condition should only evaluate to true very infrequently, so there isn't any need to optimize the check's location for performance. - Joel ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3149/#review7347 ----------------------------------------------------------- On Oct. 9, 2015, 3:15 p.m., Joel Hestness wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3149/ > ----------------------------------------------------------- > > (Updated Oct. 9, 2015, 3:15 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11157:65d87638830f > --------------------------- > ruby: Fix block_on behavior > > Ruby's controller block_on behavior aimed to block MessageBuffer requests into > SLICC controllers when a Locked_RMW was in flight. Unfortunately, this > functionality only partially works: When non-Locked_RMW memory accesses are > issued to the sequencer to an address with an in-flight Locked_RMW, the > sequencer may pass those accesses through to the controller. At the > controller, > a number of incorrect activities can occur depending on the protocol. In > MOESI_hammer, for example, an intermediate IFETCH will cause an L1D to L2 > transfer, which cannot be serviced, because the block_on functionality blocks > the trigger queue, resulting in a deadlock. Further, if an intermediate store > arrives (e.g. from a separate SMT thread), the sequencer allows the request > through to the controller, and the atomicity of the Locked_RMW may be broken. > > To avoid these problems, disallow the Sequencer from passing any memory > accesses to the controller besides Locked_RMW_Write when a Locked_RMW is in- > flight. > > > Diffs > ----- > > src/mem/ruby/slicc_interface/AbstractController.hh bd894d2bdd7c > src/mem/ruby/slicc_interface/AbstractController.cc bd894d2bdd7c > src/mem/ruby/system/Sequencer.cc bd894d2bdd7c > > Diff: http://reviews.gem5.org/r/3149/diff/ > > > Testing > ------- > > Considered many other potential solutions on gem5-gpu email list, which seem > unlikely to function as desired: > https://groups.google.com/forum/#!topic/gem5-gpu-dev/RQv4SxIKv7g > > Found reproducible version of the IFETCH bug with gem5 11139:bd894d2bdd7c > (using the xsave disable patch in this email thread: > http://comments.gmane.org/gmane.comp.emulators.m5.devel/24558 ) > > Reproducible command line: ../build/X86_MOESI_hammer/gem5.opt > --outdir=$outdir ../configs/example/fs.py --ruby --cpu-type=detailed --caches > --num-cpus=4 --script ../configs/boot/hack_back_ckpt.rcS --kernel > ../../full_system_files/binaries/x86_64-vmlinux-2.6.28.4-smp > > Verified that the patch fixes the reproducible bug and tested that booting > Linux works with O3CPU and numerous other system configurations. > > > Thanks, > > Joel Hestness > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
