> On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/mem/protocol/MOESI_hammer-cache.sm, line 1096 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10718#file10718line1096> > > > > Is there a reason why you need this action and the vt_ action for > > writing the tbe versus using the pre-existing actions?
In this case, the TBE entry may not have the data. The added uf_ action stores data in TBE. The reason I added this action is that I do not want to allocate the cache line in the cache when I want to flush it, so I keep/update it in TBE during the flush. This action is similar to u_writeDataToCache, but it only updates TBE entry. > On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/cpu/testers/rubytest/Check.hh, line 64 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10713#file10713line64> > > > > 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! I applied it in the new version. > On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/cpu/testers/rubytest/Check.cc, line 65 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10714#file10714line65> > > > > Minor comment: Please add a space between '==' and '0'. I applied it in the new version. > On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/cpu/testers/rubytest/Check.cc, line 218 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10714#file10714line218> > > > > Why are you setting the data pointer to a non-null value? It seesm > > that the flush request should be "dataless". In this version, to make testing easier, flush requests return data, and are randomly used instead of loads. I will fix it in future patches. > On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/mem/packet.cc, line 153 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10716#file10716line153> > > > > Don't exceed 80 characters per line. applied! > On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/mem/physical.cc, line 325 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10717#file10717line325> > > > > 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. applied! > On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/mem/protocol/MOESI_hammer-cache.sm, line 133 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10718#file10718line133> > > > > 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. This event and related states are removed in the new implementation. > On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/mem/protocol/MOESI_hammer-cache.sm, line 1518 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10718#file10718line1518> > > > > 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? It has been removed in the new implementation. However, even if the cache has the line exclusively, it should block the directory before directly issuing PUTX. In this way, the directory stalls new requests for this line until the flush is done. > On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/mem/protocol/MOESI_hammer-dir.sm, line 1838 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10719#file10719line1838> > > > > If you implement the flush mechanism in a more blocking fashion, I > > don't believe you should have races between BLOCK and UnblockM. I am not sure if I need this any more in the new implementation. I will check it. > On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/mem/ruby/system/RubyPort.cc, line 248 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10726#file10726line248> > > > > indents should be four spaces wide. done! > On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/mem/ruby/system/Sequencer.hh, line 106 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10727#file10727line106> > > > > The spacing seems slightly off here and the next function declaration. > > The parameters should line up to the right of the open parenthesis. done! > On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/mem/ruby/system/Sequencer.cc, line 259 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10728#file10728line259> > > > > Again, do not exceed 80 characters per line. (Yes, slicc code is the > > only exception) done! > On 2011-03-08 11:53:38, Brad Beckmann wrote: > > src/mem/ruby/system/Sequencer.cc, line 610 > > <http://reviews.m5sim.org/r/552/diff/1/?file=10728#file10728line610> > > > > It seems that flush requests shouldn't even get to this condition. > > Shouldn't flush request data packets be set to NULL? As mentioned before, I will keep it now for the purpose of testing till I am sure about flushing. - Somayeh ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/552/#review936 ----------------------------------------------------------- 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
