> 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. > > Joel Hestness 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. > > 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, if you decide to keep this inside insertRequest() then you might move it up two lines so that it is called before the default_entry is created. It's really minor, but it doesn't make sense to create the default_entry before the check. - Brandon ----------------------------------------------------------- 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 gem5-dev@gem5.org http://m5sim.org/mailman/listinfo/gem5-dev
