> On Oct. 11, 2012, 12:50 a.m., Andreas Hansson wrote: > > src/mem/protocol/MESI_CMP_directory-msg.sm, line 83 > > <http://reviews.gem5.org/r/1443/diff/3/?file=30215#file30215line83> > > > > Why is it not symmetric with the reads? > > Nilay Vaish wrote: > While a check can be added in functionalWrite(), it is not necessary. The > cases in which > the data block may not exist (from an actual physical implementation > prespective), the > protocol will not read the data that a functionalWrite() may write to the > block. > > But that's not true for a functionalRead(). The data block is present in > all the > messages, but only a few of them hold the actual data. Hence, the check > is required.
Could you add a few lines along these lines in the code? In short: Why is the check for PUTX and nothing else for the read, and why no check for the write. > On Oct. 11, 2012, 12:50 a.m., Andreas Hansson wrote: > > src/mem/protocol/MESI_CMP_directory-msg.sm, line 98 > > <http://reviews.gem5.org/r/1443/diff/3/?file=30215#file30215line98> > > > > Same as previously, a short comment on why Data dataExclusive and > > memory data (what is data/mem data difference?). > > Nilay Vaish wrote: > There should be comments about these message types where they are defined. > But those comments are not helpful. I am editing those so that > differentiation > can be made between types. I was more asking for a line or two in the code describing why these are the ones in the if statement. Could you add this? > On Oct. 11, 2012, 12:50 a.m., Andreas Hansson wrote: > > src/mem/ruby/buffers/MessageBuffer.cc, line 451 > > <http://reviews.gem5.org/r/1443/diff/3/?file=30233#file30233line451> > > > > Could this be iterators as well? Feels a bit off to mix counter based > > and iterator based iteration. > > Nilay Vaish wrote: > I am not a fan of iterators. But my knowledge of C++ tells me that map > and list > datatypes do not support counter based traversal. With vectors, I tend to > do > counter based traversal only. I'll live with it :) until we go c++11 and can make it a for (auto s: m_prio_heap) > On Oct. 11, 2012, 12:50 a.m., Andreas Hansson wrote: > > src/mem/ruby/buffers/MessageBuffer.cc, line 496 > > <http://reviews.gem5.org/r/1443/diff/3/?file=30233#file30233line496> > > > > Could there be order dependent behaviour here or somewhere else? The > > iteration over a map scares me quite a bit! > > > > In general, we should avoid it as the order is not defined. > > Nilay Vaish wrote: > In case of functional write, the order does not matter since all the > messages in > the memory system are updated. > > For functional reads, any packet that holds the data should be holding > the correct > data since we allow only exclusive writes. Ok, perhaps I just don't understand the full implication of what is happening. I would imagine that the messages would hold transient states, and that the controllers would resolve them. Perhaps I'm just missing something here. > On Oct. 11, 2012, 12:50 a.m., Andreas Hansson wrote: > > src/mem/ruby/slicc_interface/RubyRequest.cc, line 28 > > <http://reviews.gem5.org/r/1443/diff/3/?file=30245#file30245line28> > > > > Just a note on the memtester...it is as far as I am concerned broken in > > its current implementation. The way it interacts with the shadow memory is > > too restrictive. The ordering model allows many more legal permutations > > than what we currently do by comparing with the functional memory. > > > > I'm not sure I would design for a broken memtest to pass. > > Nilay Vaish wrote: > I would not say memtest is broken, but I agree that it is restrictive. > Since one of the purposes of the patch is to clear the regression tests, > I have to produce code that would satisfy the tester. As and when the > tester is changed, I would redo the implementation to take care of the > changes. I still think there is a danger designing for a test which is not really testing what it should be testing (i.m.h.o), but I'm happy to go ahead as you suggest. - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1443/#review3561 ----------------------------------------------------------- On Oct. 11, 2012, 3:47 a.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1443/ > ----------------------------------------------------------- > > (Updated Oct. 11, 2012, 3:47 a.m.) > > > Review request for Default. > > > Description > ------- > > Changeset 9283:a755377671a5 > --------------------------- > ruby: improved support for functional accesses > This patch adds support to different entities in the ruby memory system > for more reliable functional read/write accesses. Only the simple network > has been augmented as of now. Later on Garnet will also support functional > accesses. > The patch adds functional access code to all the different types of messages > that protocols can send around. These messages are functionally accessed > by going through the buffers maintained by the network entities. > The patch also rectifies some of the bugs found in coherence protocols while > testing the patch. > > With this patch applied, functional writes always succeed. But functional > reads can still fail. > > > Diffs > ----- > > configs/example/ruby_mem_test.py a5ede748a1d9 > src/mem/protocol/MESI_CMP_directory-L1cache.sm a5ede748a1d9 > src/mem/protocol/MESI_CMP_directory-L2cache.sm a5ede748a1d9 > src/mem/protocol/MESI_CMP_directory-dir.sm a5ede748a1d9 > src/mem/protocol/MESI_CMP_directory-msg.sm a5ede748a1d9 > src/mem/protocol/MI_example-cache.sm a5ede748a1d9 > src/mem/protocol/MI_example-dir.sm a5ede748a1d9 > src/mem/protocol/MI_example-msg.sm a5ede748a1d9 > src/mem/protocol/MOESI_CMP_directory-L1cache.sm a5ede748a1d9 > src/mem/protocol/MOESI_CMP_directory-L2cache.sm a5ede748a1d9 > src/mem/protocol/MOESI_CMP_directory-dir.sm a5ede748a1d9 > src/mem/protocol/MOESI_CMP_directory-msg.sm a5ede748a1d9 > src/mem/protocol/MOESI_CMP_token-L1cache.sm a5ede748a1d9 > src/mem/protocol/MOESI_CMP_token-dir.sm a5ede748a1d9 > src/mem/protocol/MOESI_CMP_token-msg.sm a5ede748a1d9 > src/mem/protocol/MOESI_hammer-cache.sm a5ede748a1d9 > src/mem/protocol/MOESI_hammer-dir.sm a5ede748a1d9 > src/mem/protocol/MOESI_hammer-msg.sm a5ede748a1d9 > src/mem/protocol/Network_test-msg.sm a5ede748a1d9 > src/mem/protocol/RubySlicc_Exports.sm a5ede748a1d9 > src/mem/protocol/RubySlicc_MemControl.sm a5ede748a1d9 > src/mem/ruby/buffers/MessageBuffer.hh a5ede748a1d9 > src/mem/ruby/buffers/MessageBuffer.cc a5ede748a1d9 > src/mem/ruby/buffers/MessageBufferNode.hh a5ede748a1d9 > src/mem/ruby/network/Network.hh a5ede748a1d9 > src/mem/ruby/network/simple/PerfectSwitch.cc a5ede748a1d9 > src/mem/ruby/network/simple/SimpleNetwork.hh a5ede748a1d9 > src/mem/ruby/network/simple/SimpleNetwork.cc a5ede748a1d9 > src/mem/ruby/network/simple/Switch.hh a5ede748a1d9 > src/mem/ruby/network/simple/Switch.cc a5ede748a1d9 > src/mem/ruby/slicc_interface/AbstractController.hh a5ede748a1d9 > src/mem/ruby/slicc_interface/Message.hh a5ede748a1d9 > src/mem/ruby/slicc_interface/NetworkMessage.hh a5ede748a1d9 > src/mem/ruby/slicc_interface/RubyRequest.hh a5ede748a1d9 > src/mem/ruby/slicc_interface/RubyRequest.cc a5ede748a1d9 > src/mem/ruby/slicc_interface/RubySlicc_Util.hh a5ede748a1d9 > src/mem/ruby/system/MemoryControl.hh a5ede748a1d9 > src/mem/ruby/system/MemoryControl.cc a5ede748a1d9 > src/mem/ruby/system/RubyMemoryControl.hh a5ede748a1d9 > src/mem/ruby/system/RubyMemoryControl.cc a5ede748a1d9 > src/mem/ruby/system/System.cc a5ede748a1d9 > src/mem/slicc/ast/TypeDeclAST.py a5ede748a1d9 > src/mem/slicc/symbols/StateMachine.py a5ede748a1d9 > src/mem/slicc/symbols/SymbolTable.py a5ede748a1d9 > src/mem/slicc/symbols/Type.py a5ede748a1d9 > > Diff: http://reviews.gem5.org/r/1443/diff/ > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
