> 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. > > Nilay Vaish wrote: > 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. > > Brandon Potter wrote: > We are doing device-device task enqueueing. The method that we're using > to do the task enqueue is to write to a device memory location through the > MMIO/PIO functionality. Since this device memory range exists outside of the > physical address range, we need the sequencer for the dma device to be able > to write outside of the physical address range. The method that this was done > through before was a check on the physical address range for the system. If > the address lies outside of the physical address range, then it must be a PIO > address and it routes it through to the PIO bus. Previously, the PIO address > checks were taken care of in that DMASequencers derived from RubyPorts as the > base class. When you refactored the DMASequencer to remove the RubyPort as > the base class that functionality went away. (There's also more code > duplication now since code from RubyPort was duplicated in the DMASequencer > code.) It might be worthwhile to revert the patch which removed the RubyPort > as the base class unless there's a compelling reason not to do so. (We have a > patch internal patch which does this.)
Thanks for the explaining your use case. I never had it on my mind. You can revert 10518. - 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
