----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/552/#review936 -----------------------------------------------------------
Hi Somayeh, I have several comments and questions that pertain to both the design and the coding style. Please don't be discouraged by the number of my comments. Most people's first review tend to look like this. In general, I think you changes to the protocol are more complicated than they need to be. The flush request should be a blocking request, just like other requests. Returning to a base state when the flush request is still outstanding will cause you bunch of unnecessary pain, and you'll likely never get that to completely work. Let me know if you have any questions and once you address my comments please submit the next version of your patch for review. I'd rather provide you feedback sooner than have you work several weeks in isolation. src/cpu/testers/rubytest/Check.hh <http://reviews.m5sim.org/r/552/#comment1270> Don't add comments such as this. Mercurial provides us this information and adding explicit comments like this is redundant and will only clutter the code. I see you did this in several places. I didn't mark each one of them, but please correct them all. Thanks! src/cpu/testers/rubytest/Check.cc <http://reviews.m5sim.org/r/552/#comment1271> Minor comment: Please add a space between '==' and '0'. src/cpu/testers/rubytest/Check.cc <http://reviews.m5sim.org/r/552/#comment1281> Why are you setting the data pointer to a non-null value? It seesm that the flush request should be "dataless". src/mem/packet.cc <http://reviews.m5sim.org/r/552/#comment1272> Don't exceed 80 characters per line. src/mem/physical.cc <http://reviews.m5sim.org/r/552/#comment1282> This condition and the similar condition below are awkward and I don't think they are necessary. In general, I don't think this patch should impact physicalmemory at all. Instead, inside the function RubyPort::M5Port::hitCallback() cache flush requests should set the local variable accessPhysMem to false, similar to how failed SC request behave. src/mem/protocol/MOESI_hammer-cache.sm <http://reviews.m5sim.org/r/552/#comment1288> If you implement the flush mechanism in a more blocking fashion, I don't think you need as many events that just pertain to flushing. In particular, having to start a flush over is something you want to avoid at all costs. If implemented correctly, the initial flush request should eventually succeed. In general, nack and retry mechanisms are particularly tricky to test in ruby because the random tester tends to find the pathelogical deadlock case. src/mem/protocol/MOESI_hammer-cache.sm <http://reviews.m5sim.org/r/552/#comment1283> paranthesis alignment src/mem/protocol/MOESI_hammer-cache.sm <http://reviews.m5sim.org/r/552/#comment1289> Is there a reason why you need this action and the vt_ action for writing the tbe versus using the pre-existing actions? src/mem/protocol/MOESI_hammer-cache.sm <http://reviews.m5sim.org/r/552/#comment1285> Minor comment: No need to modify this line. src/mem/protocol/MOESI_hammer-cache.sm <http://reviews.m5sim.org/r/552/#comment1284> Why issue a db_issueBlock? Since the hammer protocol uses exclusive L1/L2 caches managed by a single controller, don't you know that this is the only cached copy of the block? Can't you just directly issue the PUTX request? src/mem/protocol/MOESI_hammer-cache.sm <http://reviews.m5sim.org/r/552/#comment1286> Do not transition to a base state here or in the following three transitions. Instead, you should either remain in the same transient state or transition to another transient state. In general, if you've issued a request to the directory, you what to remain in a transient state until your entire request has been satisfied. Going back to a base state and reissuing the flush or invalidate request creates a bunch of races and puts open ended requests in the system that will be nearly impossible to track down. Understand how the current protocol works when receives forwarded requests in transient states and implement a similar behavior for flushes. src/mem/protocol/MOESI_hammer-dir.sm <http://reviews.m5sim.org/r/552/#comment1287> If you implement the flush mechanism in a more blocking fashion, I don't believe you should have races between BLOCK and UnblockM. src/mem/ruby/system/RubyPort.cc <http://reviews.m5sim.org/r/552/#comment1280> indents should be four spaces wide. src/mem/ruby/system/Sequencer.hh <http://reviews.m5sim.org/r/552/#comment1279> The spacing seems slightly off here and the next function declaration. The parameters should line up to the right of the open parenthesis. src/mem/ruby/system/Sequencer.cc <http://reviews.m5sim.org/r/552/#comment1273> Again, do not exceed 80 characters per line. (Yes, slicc code is the only exception) src/mem/ruby/system/Sequencer.cc <http://reviews.m5sim.org/r/552/#comment1274> Again, 80 char. src/mem/ruby/system/Sequencer.cc <http://reviews.m5sim.org/r/552/#comment1275> 80 again here src/mem/ruby/system/Sequencer.cc <http://reviews.m5sim.org/r/552/#comment1276> and here src/mem/ruby/system/Sequencer.cc <http://reviews.m5sim.org/r/552/#comment1277> It seems that flush requests shouldn't even get to this condition. Shouldn't flush request data packets be set to NULL? src/mem/ruby/system/Sequencer.cc <http://reviews.m5sim.org/r/552/#comment1278> 80 chars - Brad On 2011-03-06 16:11:12, Somayeh Sardashti wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/552/ > ----------------------------------------------------------- > > (Updated 2011-03-06 16:11:12) > > > Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, Nathan > Binkert, and Brad Beckmann. > > > Summary > ------- > > MOESI_hammer: adding cache flushing > This patch adds cache flushing to MOESI_hammer. In order to flush a > cache line, a FLUSH request is issued. Flushing is basically a store to > a line (with no change to data) followed by a write back to the memory > (invalidation). > > At the L1 cache, up on a flush request, a GETF (similar to GETX) is > issued. When the directory processes this request, it gives the > exclusive permission, and blocks this line until the flush is done > (stalls other GETS/GETX/GETF requests). If the cache line is already in > a modified state (MM/M), the cache issues a BLOCK request to the > directory (instead of issuing GETF as the cache already has the line > exclusively), > which blocks the line until the flush is done. > > > Diffs > ----- > > src/cpu/testers/rubytest/Check.hh baf4b5f6782e > src/cpu/testers/rubytest/Check.cc baf4b5f6782e > src/mem/packet.hh baf4b5f6782e > src/mem/packet.cc baf4b5f6782e > src/mem/physical.cc baf4b5f6782e > src/mem/protocol/MOESI_hammer-cache.sm baf4b5f6782e > src/mem/protocol/MOESI_hammer-dir.sm baf4b5f6782e > src/mem/protocol/MOESI_hammer-msg.sm baf4b5f6782e > src/mem/protocol/RubySlicc_Exports.sm baf4b5f6782e > src/mem/protocol/RubySlicc_Types.sm baf4b5f6782e > src/mem/ruby/slicc_interface/RubyRequest.hh baf4b5f6782e > src/mem/ruby/slicc_interface/RubyRequest.cc baf4b5f6782e > src/mem/ruby/system/DMASequencer.cc baf4b5f6782e > src/mem/ruby/system/RubyPort.cc baf4b5f6782e > src/mem/ruby/system/Sequencer.hh baf4b5f6782e > src/mem/ruby/system/Sequencer.cc baf4b5f6782e > > Diff: http://reviews.m5sim.org/r/552/diff > > > Testing > ------- > > To test this implementation, I have changed the ruby tester (Check.cc). > It randomly creates flushes (instead of checks (loads)). In this > version, a flush request returns data, so it can be used like loads > after issuing some stores (actions). Currently, this implementation > passes more than 20000 ruby tests (including more than 1000 flushes), > however there are still bugs, which cause deadlocks. > > > Thanks, > > Somayeh > > _______________________________________________ m5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/m5-dev
