> On April 8, 2012, 9:22 p.m., Steve Reinhardt wrote: > > src/mem/bus.cc, line 74 > > <http://reviews.gem5.org/r/1118/diff/3/?file=25562#file25562line74> > > > > This master/slave thing (where the bus masters connect to BusSlavePorts > > and slaves connect to BusMasterPorts) still confuses me... at the very > > least, let's be clear in the comments that that's what is happening. Also, > > I would say there's no such thing as a "default master". There's a default > > slave device, which (by our twisted logic) has to connect to a master port, > > but that doesn't make it a "default master". > > > > I think these comments really refer more to changes that happened in > > the previous patch, but I was asleep at the wheel then or something. > > Andreas Hansson wrote: > I'm happy to add more descriptions and explanations, and could also refer > to all existing documentation for TLM-2, and/or any protocol like AMBA, OCP, > VCI, etc. just to have more background info that uses the same terminology. > > The notion of master and slave and that a master always connects to a > slave will be on the Wiki as well. The bus has a master port that is the > default port...hence default master from the perspective of the bus. The > master port that connects to the default slave. > > > Steve Reinhardt wrote: > There are two parts: understanding it (which only needs to happen once) > and getting used to it (which may never happen). The former is important, > but I'm past that; it's the latter that bugs me. > > It's like when you give a talk and you want to point out something on > your slides, and you have to remember that what's on your left as you face > the audience is on the audience's right, unless you're looking at your slides > on your laptop and not on the screen, in which case your left is the same as > the audience's left. The concept is straightforward, and it's easy to figure > out when you stop and think about it, but it doesn't readily become > intuitive, as you can tell by the number of people that mess it up. > > So mostly what I'm talking about here is just being generous and > redundant with casual reminders in the comments that when we're accessing > master ports it is so that we can talk to slave devices, and vice versa. I > see that you've already done some of that, which is great. > > As far as "default master" specifically, I figured out why it bugged me > so much... I see that you've already addressed this too, but I'll explain > anyway just in case it helps keep you from thinking I'm crazy. (Or maybe it > will further convince you that I am crazy, but I'll take that risk.) > > Basically when you use "master" or "slave" as a stand-alone noun, to me > that implicitly refers to a device. So "default master" sounds like "default > master device", which makes no sense. Saying that the port that's used to > talk to the default slave is the "default master port" is conceptually OK > (even if it takes getting used to as discussed above), but abbreviating > "default master port" as "default master" is where it breaks for me. > > I think it's also true that any use of "master" or "slave" as an > adjective modifying anything other than "port" inclines me to think of the > device and not the port, and is at best ambiguous. Thus I would recommend > staying away from terms like "master id" and "slave id". It looks like > "master id" is not used, and "slave id" is used only in the forward*() > functions, where of course it identifies the master device to exclude ;-).
I agree with your comments, and will be even better at clarifying "master port" and "slave port" when talking about ports. Consider the excluded_slave_id parameter changed to a slightly lengthier excluded_slave_port_id :) In general, a "master" should refer to a module with only "master ports", and slave to a module with only "slave ports". If you have e.g. a bridge or a DmaDevice, it becomes more difficult as it has both master ports and slave ports. I will make sure I'm consistent with this terminology in this and future patches. ...and don't worry, I don't think you're crazy. > On April 8, 2012, 9:22 p.m., Steve Reinhardt wrote: > > src/mem/bus.cc, line 352 > > <http://reviews.gem5.org/r/1118/diff/3/?file=25562#file25562line352> > > > > There seems to be quite a bit of duplicated code here with recvTiming() > > (and similarly recvAtomicSnoop() appears to duplicate a lot of code with > > recvAtomic()). > > > > Can we leave recvTiming and recvAtomic as integrated functions, but add > > an isSnoop flag to distinguish snoops from non-snoops? E.g., the > > BusMasterPort and BusSlavePort methods would look like this: > > > > recvTiming(PacketPtr pkt) { > > bus->recvTiming(pkt, this, false); > > } > > > > recvTimingSnoop(PacketPtr pkt) { > > bus->recvTiming(pkt, this, true); > > } > > > > I'd think this would still allow cleanly distinguishing the snoop and > > non-snoop cases while avoiding so much code duplication. But I'm just > > eyeballing it... > > > > Andreas Hansson wrote: > I can break out additional code into new member functions rather. The > next few patches will introduce the master and slave interfaces, and in doing > so also split the recvTiming and recvTimingSnoop into a request and response > part that then get moved from the Port class to the appropriate master/slave > subclass. The end result is a non-ambiguous interface that is unique to each > port. The concept of the parameter would make this impossible, as it by > construction would have to be shared between both. > > I'll factor out more of the code in members. > > Steve Reinhardt wrote: > Thanks for the update, but I'm still not thrilled that, for example, > recvAtomic() and recvAtomicSnoop() are over thirty lines each of which > approximately two are different. It sounds like the direction you're headed > is not actually going to solve this, but rather make it impossible (or more > difficult) to solve. I guess I can withhold judgment awhile longer, but on > the face of it this seems like a step backwards. I'll simplify recvAtomicSnoop similar to what is done for recvTimingSnoop and post a new version of the patch. I hope that is more in line with what you were hoping for. If there is more I can do please let me know. The direction I'm heading in is to separate out the different protocol phases into separate functions, with very clear semantics (they are there at the moment, just hidden in a complex control flow). The assert statements that currently "guard" these assumptions can eventually go away once all modules are implementing these interfaces (as exemplified by removing the broadcast flag). - Andreas ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: http://reviews.gem5.org/r/1118/#review2512 ----------------------------------------------------------- On April 9, 2012, 8:44 a.m., Andreas Hansson wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > http://reviews.gem5.org/r/1118/ > ----------------------------------------------------------- > > (Updated April 9, 2012, 8:44 a.m.) > > > Review request for Default. > > > Description > ------- > > MEM: Separate snoops and normal memory requests/responses > > This patch introduces port access methods that separates snoop > request/responses from normal memory request/responses. The > differentiation is made for functional, atomic and timing accesses and > builds on the introduction of master and slave ports. > > Before the introduction of this patch, the packets belonging to the > different phases of the protocol (request -> [forwarded snoop request > -> snoop response]* -> response) all use the same port access > functions, even though the snoop packets flow in the opposite > direction to the normal packet. That is, a coherent master sends > normal request and receives responses, but receives snoop requests and > sends snoop responses (vice versa for the slave). These two distinct > phases now use different access functions, as described below. > > Starting with the functional access, a master sends a request to a > slave through sendFunctional, and the request packet is turned into a > response before the call returns. In a system without cache coherence, > this is all that is needed from the functional interface. For the > cache-coherent scenario, a slave also sends snoop requests to coherent > masters through sendFunctionalSnoop, with responses returned within > the same packet pointer. This is currently used by the bus and caches, > and the LSQ of the O3 CPU. The send/recvFunctional and > send/recvFunctionalSnoop are moved from the Port super class to the > appropriate subclass. > > Atomic accesses follow the same flow as functional accesses, with > request being sent from master to slave through sendAtomic. In the > case of cache-coherent ports, a slave can send snoop requests to a > master through sendAtomicSnoop. Just as for the functional access > methods, the atomic send and receive member functions are moved to the > appropriate subclasses. > > The timing access methods are different from the functional and atomic > in that requests and responses are separated in time and > send/recvTiming are used for both directions. Hence, a master uses > sendTiming to send a request to a slave, and a slave uses sendTiming > to send a response back to a master, at a later point in time. Snoop > requests and responses travel in the opposite direction, similar to > what happens in functional and atomic accesses. With the introduction > of this patch, it is possible to determine the direction of packets in > the bus, and no longer necessary to look for both a master and a slave > port with the requested port id. > > In contrast to the normal recvFunctional, recvAtomic and recvTiming > that are pure virtual functions, the recvFunctionalSnoop, > recvAtomicSnoop and recvTimingSnoop have a default implementation that > calls panic. This is to allow non-coherent master and slave ports to > not implement these functions. > > > Diffs > ----- > > src/arch/x86/pagetable_walker.hh a47fd7c2d44e > src/arch/x86/pagetable_walker.cc a47fd7c2d44e > src/cpu/base.hh a47fd7c2d44e > src/cpu/base.cc a47fd7c2d44e > src/cpu/inorder/cpu.hh a47fd7c2d44e > src/cpu/inorder/cpu.cc a47fd7c2d44e > src/cpu/o3/cpu.hh a47fd7c2d44e > src/cpu/o3/cpu.cc a47fd7c2d44e > src/cpu/o3/lsq.hh a47fd7c2d44e > src/cpu/o3/lsq_impl.hh a47fd7c2d44e > src/cpu/simple/atomic.hh a47fd7c2d44e > src/cpu/simple/timing.hh a47fd7c2d44e > src/cpu/simple/timing.cc a47fd7c2d44e > src/cpu/testers/directedtest/RubyDirectedTester.hh a47fd7c2d44e > src/cpu/testers/directedtest/RubyDirectedTester.cc a47fd7c2d44e > src/cpu/testers/memtest/memtest.hh a47fd7c2d44e > src/cpu/testers/memtest/memtest.cc a47fd7c2d44e > src/cpu/testers/networktest/networktest.hh a47fd7c2d44e > src/cpu/testers/networktest/networktest.cc a47fd7c2d44e > src/cpu/testers/rubytest/RubyTester.hh a47fd7c2d44e > src/cpu/testers/rubytest/RubyTester.cc a47fd7c2d44e > src/dev/io_device.hh a47fd7c2d44e > src/dev/io_device.cc a47fd7c2d44e > src/mem/bridge.hh a47fd7c2d44e > src/mem/bridge.cc a47fd7c2d44e > src/mem/bus.hh a47fd7c2d44e > src/mem/bus.cc a47fd7c2d44e > src/mem/cache/base.hh a47fd7c2d44e > src/mem/cache/cache.hh a47fd7c2d44e > src/mem/cache/cache_impl.hh a47fd7c2d44e > src/mem/mport.hh a47fd7c2d44e > src/mem/mport.cc a47fd7c2d44e > src/mem/packet_queue.hh a47fd7c2d44e > src/mem/packet_queue.cc a47fd7c2d44e > src/mem/port.hh a47fd7c2d44e > src/mem/port.cc a47fd7c2d44e > src/mem/ruby/system/RubyPort.hh a47fd7c2d44e > src/mem/ruby/system/RubyPort.cc a47fd7c2d44e > src/sim/system.hh a47fd7c2d44e > > Diff: http://reviews.gem5.org/r/1118/diff/ > > > Testing > ------- > > util/regress all passing (disregarding t1000 and eio) > > > Thanks, > > Andreas Hansson > > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
