----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.m5sim.org/r/611/#review1054 -----------------------------------------------------------
Hi Nilay, First, thanks for your patience. Sorry I wasn't able to provide you comments sooner. Overall, this patch is definitely what I had in mind and is a good starting point. I have several comments/questions below. Please let me know if you need me to clarify anything. src/mem/ruby/system/AbstractMemory.hh <http://reviews.m5sim.org/r/611/#comment1425> Instead of having canMakeFunctionalAccess do some internal analysis and return a boolean result, why not simply pass back the AccessPermission? The status of the address for a particular memory object does not indicate whether a functional access is possible. Rather it is the collective status across all memory objects which determine whether the functional access is possible. src/mem/ruby/system/CacheMemory.cc <http://reviews.m5sim.org/r/611/#comment1426> Again, I think this type of analysis should be moved the RubyPort, but I do want to point out that functional accesses have different constraints than real accesses. Even if the AccessPermission is Read_Only, a functional access can succeed and should update the block. src/mem/ruby/system/RubyPort.cc <http://reviews.m5sim.org/r/611/#comment1427> Eventually, we'll need to change recvFunctional to return a bool indicating whether the access was successful. I suspect the ramifications of the change will be rather significant across gem5, so for now it is fine to just return void. However, eventually we will need to address this larger issue. src/mem/ruby/system/RubyPort.cc <http://reviews.m5sim.org/r/611/#comment1421> Are you just pushing a lable here for debug purposes? Does this have any impact on the actual functionality? src/mem/ruby/system/RubyPort.cc <http://reviews.m5sim.org/r/611/#comment1428> This loop is probably the most complicated and important part of this patch. It might be easiest if we move this functionality into two different functions, one for reads and one for writes. The read scan just needs to ensure that at least one memory says that it has the address in Read_Only or ReadWrite state. We may also want to doublecheck that multiple memories say they have the address in ReadWrite state. The write scan is more complicated. If one memory says that it has the address in ReadWrite state, then I don't think it matters what state the other memories are in (except of course if another memory says it also has the address in ReadWrite), the write should succeed. Also if the write scans says that all copies are in Read_Only or Invalid/NotPresent state and no copies are Busy, the write should succeed. However, writes should fail if either no Read_Only or ReadWrite copies are found, or if a Busy copy is found and no ReadWrite copy is found. The latter situation will likely indicate the functional write is racing with a timing write. There is no easy, protocol-agnostic way to resolve such a race in the current infrastructure. Make sense? src/mem/ruby/system/System.hh <http://reviews.m5sim.org/r/611/#comment1423> Why is this a static function? It seems unecessary and it prevents us from using ruby in multiple systems. - Brad On 2011-03-30 16:19:26, Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.m5sim.org/r/611/ > ----------------------------------------------------------- > > (Updated 2011-03-30 16:19:26) > > > Review request for Default. > > > Summary > ------- > > Ruby: Add support for functional accesses > This patch is meant for aiding discussions on implementation of functional > access support in Ruby. > > > Diffs > ----- > > configs/ruby/MESI_CMP_directory.py d54b7775a6b0 > configs/ruby/Ruby.py d54b7775a6b0 > src/mem/ruby/network/Network.cc d54b7775a6b0 > src/mem/ruby/network/Network.py d54b7775a6b0 > src/mem/ruby/profiler/Profiler.cc d54b7775a6b0 > src/mem/ruby/profiler/Profiler.py d54b7775a6b0 > src/mem/ruby/recorder/Tracer.cc d54b7775a6b0 > src/mem/ruby/recorder/Tracer.py d54b7775a6b0 > src/mem/ruby/system/AbstractMemory.hh PRE-CREATION > src/mem/ruby/system/AbstractMemory.cc PRE-CREATION > src/mem/ruby/system/Cache.py d54b7775a6b0 > src/mem/ruby/system/CacheMemory.hh d54b7775a6b0 > src/mem/ruby/system/CacheMemory.cc d54b7775a6b0 > src/mem/ruby/system/DirectoryMemory.hh d54b7775a6b0 > src/mem/ruby/system/DirectoryMemory.cc d54b7775a6b0 > src/mem/ruby/system/DirectoryMemory.py d54b7775a6b0 > src/mem/ruby/system/RubyPort.hh d54b7775a6b0 > src/mem/ruby/system/RubyPort.cc d54b7775a6b0 > src/mem/ruby/system/RubySystem.py d54b7775a6b0 > src/mem/ruby/system/SConscript d54b7775a6b0 > src/mem/ruby/system/Sequencer.py d54b7775a6b0 > src/mem/ruby/system/System.hh d54b7775a6b0 > src/mem/ruby/system/System.cc d54b7775a6b0 > > Diff: http://reviews.m5sim.org/r/611/diff > > > Testing > ------- > > > Thanks, > > Nilay > > _______________________________________________ m5-dev mailing list m5-dev@m5sim.org http://m5sim.org/mailman/listinfo/m5-dev
