> On Sept. 24, 2015, 11:32 p.m., Brad Beckmann wrote: > > Do not check this in. I suggest you stop going in this direction. Any > > performance benefits provided by this patch do not justify the resulting > > duplicate code and downstream re-correlation and development efforts. > > There are very good reasons for the separation of RubyPort and the > > sequencers. When we post our GPU patches, we post more child objects of > > RubyPort that are GPU specific. > > > > RubyPort encapsulates the generic interfaces needed to connect m5ports with > > Ruby controllers, while the child objects contain the device specific logic > > that wants to be shared across multiple protocols. For instance, the > > Sequencer contains logic for handling CPU synchronization and coalescing > > that assumes a RFO coherence protocol. The DMA sequencer is a simple > > interface that handles large, multi-line access. We will be posting the > > GPU coalescer that assumes weaker GPU coherence and coalesces memory > > requests across a wavefront. > > Nilay Vaish wrote: > I'll retain RubyPort so that you can inherit from it. The code for ports > has to be written separately for > each type of controller, as is the case all over gem5. DMA and CPU > sequencers do not share RubyPort, only CPU > sequencer inherits from RubyPort. > > Brad Beckmann wrote: > Please do not change the Sequencer and replace it with the > FirstLevelController and please do not replace the DMASequencer with the > AbstractDMAController. A lot of code exists that depends on the current > interfaces. Also please do not make any changes to the sm files until after > we post our GPU patches. In general, slow down proposing huge changes like > this. A lot of people depend on the current code. If we were even to agree > to these types of changes (and so far we do not), changes like this one need > to be made slowly. > > Also from now on, if you claim a patch improves simulation performance, > you need provide absolute numbers. Many of us are not convinced the > performance benefit justifies the development cost. > > Finally, *if* (and I emphasize if) we were to make a change like this, I > suggest we focus on eliminating the m5port->mandatory queue two-step process > for communicating requests to the L1 cache logic. Currently this patch > merges the objects, but it still relies on the two-step communication process > within the merged object. Perhaps eliminating that unecessary timing is > where you are planning to go with this, but it is not clear. If that is your > intention, please communicate the details of your plan before posting more > patches. > > Brad Beckmann wrote: > By the way, the DMA sequencers used to inherit from RubyPort, but late > last year you changed that. That is unfortunate because now DMASequencer and > RubyPort duplicate a lot of code. For instance, their > MemSlavePort::recvTimingReq look almost identical, but not quite. Thus > current Ruby dma devices cannot issue PIO requests. This is disappointing > and right now is causing us to do more development to work around it. > > Given our current problems with the DMA sequencer not inheriting from > RubyPort, I am very leery of allowing any more of these types of changes. > Instead, I'd like to see us revert 10518:30e3715c9405. > > Nilay Vaish wrote: > Why would a DMA controller issue PIO requests? > > Nilay Vaish wrote: > One more thing, there is a line in fs.py that attaches piobus to a ruby > controller. It might be that that is what you are missing. > > Michael LeBeane wrote: > We have internal DMA device models at AMD that talk to other devices via > PIO requests in addition to regular memory accesses. This functionality was > removed in 10518:30e3715c9405. We had to make the DMASequencer inherit from > RubyPort again to make this work. > > Nilay Vaish wrote: > Does DMA controller's behavior depend on whether the request was MMIO or > PIO? > > Michael LeBeane wrote: > I am not sure what you mean here, I don't know if there is a functional > distinction between the two. Can you elaborate on your concern? > > Nilay Vaish wrote: > If the DMA controller (or DMA sequencer, whatever you like to call it) is > not going to > distinguish between the two types of requests, then it does not need > separate ports > for them. So there is no need for a separate PIO port. > > Michael LeBeane wrote: > When you say "two types of requests" are you talking abour "MMIO vs PIO" > or "DMA vs MMIO/PIO"? The DMA Controller needs to distinguish between the > latter to determine whether the request goes to memory or to a device. There > isn't a distinction for the former as far as I understand it.
So you agree that MMIO and PIO requests are not going be differentiated at the DMA controller. In my understanding, a DMA controller only does the following: a. receive request to access memory from some requestor device. The device could be in communication with cpu through mmio or pio. b. go through the coherence mechanism and fulfill the request c. respond back to the requestor. No where in this flow I see any need for a PIO port. Does the question of DMA vs MMIO/PIO even arise? Those are supposed to be orthogonal concepts. The CPU is using MMIO or PIO does not seem to have any relation to whether the CPU is going to hand-off data transfer responsibility to some other entity (DMA controller) or not. Overall you still need to explain to me why the DMA controller needs a PIO port. Which communication pattern, that cannot happen right now, would having a PIO port enable? May be you should give an example to illustrate your use case. - Nilay ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/3134/#review7279 ----------------------------------------------------------- On Sept. 24, 2015, 2:57 p.m., Nilay Vaish wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/3134/ > ----------------------------------------------------------- > > (Updated Sept. 24, 2015, 2:57 p.m.) > > > Review request for Default. > > > Repository: gem5 > > > Description > ------- > > Changeset 11139:6433ce0b920e > --------------------------- > ruby: combine RubyPort and Sequencer and rename as FirstLevelController > > This patch move the file RubyPort.* to FirstLevelController.*. Then, it > copies > the contents of Sequencer.* to FirstLevelController.*. Finally, it renames > RubyPort and Sequencer to FirstLevelController and adjusts line lengths where > ever execeeded. There are two reasons for this change: > > * I find this FirstLevelController abstraction better than having separate > RubyPort and Sequencer. With the FirstLevelController class, we are trying to > imply the functionality is common to all controllers that are directly > attached > to the cores. Earlier, it appeared as if there are two components between the > core and the actual first level controller. Now the first level controllers > in > slicc would inherit from this new FirstLevelController class and we would > directly connect the controller to the core, instead of going through the > sequencer. > > * The combined entity would allow us to make some changes that would > ultimately > improve ruby's performance significantly. > > > Diffs > ----- > > src/mem/protocol/MESI_Two_Level-L1cache.sm 5d38dc2f7d66 > src/mem/protocol/MESI_Two_Level-dma.sm 5d38dc2f7d66 > src/mem/protocol/MI_example-cache.sm 5d38dc2f7d66 > src/mem/protocol/MI_example-dma.sm 5d38dc2f7d66 > src/mem/protocol/MOESI_CMP_directory-L1cache.sm 5d38dc2f7d66 > src/mem/protocol/MOESI_CMP_directory-dma.sm 5d38dc2f7d66 > src/mem/protocol/MOESI_CMP_token-L1cache.sm 5d38dc2f7d66 > src/mem/protocol/MOESI_CMP_token-dma.sm 5d38dc2f7d66 > src/mem/protocol/MOESI_hammer-cache.sm 5d38dc2f7d66 > src/mem/protocol/MOESI_hammer-dma.sm 5d38dc2f7d66 > src/mem/protocol/Network_test-cache.sm 5d38dc2f7d66 > src/mem/protocol/RubySlicc_Exports.sm 5d38dc2f7d66 > src/mem/protocol/RubySlicc_Types.sm 5d38dc2f7d66 > src/mem/ruby/SConscript 5d38dc2f7d66 > src/mem/ruby/profiler/Profiler.cc 5d38dc2f7d66 > src/mem/ruby/slicc_interface/AbstractController.hh 5d38dc2f7d66 > src/mem/ruby/slicc_interface/AbstractController.cc 5d38dc2f7d66 > src/mem/ruby/slicc_interface/AbstractDMAController.hh PRE-CREATION > src/mem/ruby/slicc_interface/AbstractDMAController.cc PRE-CREATION > src/mem/ruby/slicc_interface/Controller.py 5d38dc2f7d66 > src/mem/ruby/slicc_interface/FirstLevelController.hh PRE-CREATION > src/mem/ruby/slicc_interface/FirstLevelController.cc PRE-CREATION > src/mem/ruby/slicc_interface/SConscript 5d38dc2f7d66 > src/mem/ruby/system/CacheRecorder.hh 5d38dc2f7d66 > src/mem/ruby/system/CacheRecorder.cc 5d38dc2f7d66 > src/mem/ruby/system/DMASequencer.hh 5d38dc2f7d66 > src/mem/ruby/system/DMASequencer.cc 5d38dc2f7d66 > src/mem/ruby/system/RubyPort.hh 5d38dc2f7d66 > src/mem/ruby/system/RubyPort.cc 5d38dc2f7d66 > src/mem/ruby/system/RubyPortProxy.hh 5d38dc2f7d66 > src/mem/ruby/system/RubySystem.cc 5d38dc2f7d66 > src/mem/ruby/system/RubySystem.py 5d38dc2f7d66 > src/mem/ruby/system/SConscript 5d38dc2f7d66 > src/mem/ruby/system/Sequencer.hh 5d38dc2f7d66 > src/mem/ruby/system/Sequencer.cc 5d38dc2f7d66 > src/mem/ruby/system/Sequencer.py 5d38dc2f7d66 > src/mem/slicc/symbols/StateMachine.py 5d38dc2f7d66 > tests/configs/pc-simple-timing-ruby.py 5d38dc2f7d66 > configs/example/fs.py 5d38dc2f7d66 > configs/ruby/MESI_Three_Level.py 5d38dc2f7d66 > configs/ruby/MESI_Two_Level.py 5d38dc2f7d66 > configs/ruby/MI_example.py 5d38dc2f7d66 > configs/ruby/MOESI_CMP_directory.py 5d38dc2f7d66 > configs/ruby/MOESI_CMP_token.py 5d38dc2f7d66 > configs/ruby/MOESI_hammer.py 5d38dc2f7d66 > configs/ruby/Network_test.py 5d38dc2f7d66 > configs/ruby/Ruby.py 5d38dc2f7d66 > src/mem/protocol/MESI_Three_Level-L0cache.sm 5d38dc2f7d66 > > Diff: http://reviews.gem5.org/r/3134/diff/ > > > Testing > ------- > > > Thanks, > > Nilay Vaish > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
