[I'm getting around to revisiting this port binding code, and was quite surprised when I searched for this thread in my gmail account to find the unsent draft message below. I must have composed it way back in June, and probably didn't send it because I had more to say, but at this point I have no idea what that would have been. I'm sending it now, and if Andreas or anyone else has new thoughts on the matter since six months ago, please speak up. In particular, is there anything new wrt Andreas's port/interface separation patch?]
Hi Andreas, Thanks for the reply. I'm surprised the port/interface change has such a significant performance impact; I haven't given it much thought, but I'd think with all the templates you could get rid of a lot of the overhead at compile time. Do you know where the overhead is coming from? I didn't think the LHS/RHS separation was too bad, but I think we could simplify it further if we require that objects cannot have two memory ports with identical names, one a master and one a slave. I don't know if this ever happens currently; it sounds like it would be confusing. Anyway, if we impose that restriction, then we could have a single getPort() method, and leave the master/slave checking to the bind() method. All the bind method would need to do is verify that the other port type is compatible, and the only difference between master/slave ports and symmetric ports is that in the former case the compatible type is not identical to the port type, while in the latter case it is. The distinction would be even less if we didn't enforce that LHS=master and RHS=slave. Steve On Fri, Jun 14, 2013 at 12:59 AM, Andreas Hansson <[email protected]>wrote: > Hi Steve, > > Thanks for taking time to think this through. > > I agree that the most tempting solution here would be to build it on top > of the port/interface separation, and we could also use this for other > "links" between objects. The one point where it gets tricky is the > master/slave separation in the memory system. An ethernet link ultimately > has a tx/rx notion, but it is symmetric. I am not quite sure how we would > deal with that. For other connections, e.g. Interrupts, TLB management etc > there is a clear direction and I see how it could be used. The RHS and LHS > divide you made is not something I'm immediately fond of, but I have no > better suggestions. > > How do you propose we proceed? The reason I haven't pushed that patch is > that it does affect performance. I cannot remember exactly, but I think > it's in the order of 5% which is far from negligible. I will try and > resurrect it and give it a spin. > > Andreas > > From: Steve Reinhardt <[email protected]> > Date: Thursday, 13 June 2013 20:10 > To: Andreas Hansson <[email protected]> > Cc: Default <[email protected]> > Subject: Re: Review Request: sim: unify memory & ethernet port binding > mechanisms. > > (Switching from reviewboard to email as I fear this might be a more > extended discussion. Not that I mind, if it helps us to do the right > thing.) > > Thanks for pointing out your port/interface separation patch... I had > forgotten about that since it was so long ago that you posted it. I agree > that my patch conflicts with what you did there, but I think more because > we are both trying to do similar things (i.e., make ports more generic and > independent of the things they connect) than really going in different > directions... though we do go about it in different ways. There are a few > additional details to work out, but based on your patch, it should be > possible to implement Ethernet connections using Ports by doing something > along the lines of IfaceMasterPort<EtherHandler, EtherHandler>. > > So I will play devil's advocate a bit and say that the problem with your > patch is that it doesn't go far enough; it makes the concept of Port more > generic, but unnecessarily constrains it to be used only in the memory > system by leaving it tied to MemObject. If we push the concept of Port up > to be something that's shared by all SimObjects, then we can use it for > Ethernet and other types of connections as well as memory system > connections, and then it would encompass a superset of what my patch > enables as well. > > Thoughts? > > Steve > > > > On Thu, Jun 13, 2013 at 11:05 AM, Andreas Hansson <[email protected] > > wrote: > >> This is an automatically generated e-mail. To reply, visit: >> http://reviews.gem5.org/r/1922/ >> >> On June 13th, 2013, 8:33 a.m., *Andreas Hansson* wrote: >> >> You want to remove some ugly code in a portion that is only really done once >> for most systems, and the "price" is a more complex object structure for the >> ports. My gut feeling is that these different ports have little in common. >> For example, their actual interfaces are very different. I also think this >> patch makes the port/interface separation much more difficult. >> >> >> On June 13th, 2013, 10:59 a.m., *Steve Reinhardt* wrote: >> >> Actually my motivation for doing this is that I want to introduce a third >> type of port connection (for internal use only, for now), and scaling the >> current approach to a third type really compounds the ugliness, particularly >> because it requires modifying pyobject.cc directly (i.e., it can't be done >> via EXTRAS). The advantage of this change is that it makes the port >> connection paradigm independent of the type of ports, meaning it's easy to >> add more types. Sorry for not explaining that up front. >> >> The different ports do indeed have little in common other than their method >> of connection, but I contend that if they are going to share the same method >> of connection, the new code is a much better way of doing that. Another >> approach that I considered would be to forget using python assignment to >> bind Ethernet devices and links. If we add the constraints that (1) all >> Ethernet connections are made via point-to-point links and (2) those links >> are always symmetric in delay and bandwidth, then we could just pass in an >> EtherLink pointer as a param for each object that wants a connection. While >> I would be fine with that, particularly if we had started out that way, >> switching to that approach now both introduces more constraints and would >> require modifications to the (admittedly few) scripts that use Ethernet >> links, so I thought this approach was preferable. >> >> I'm not sure what you mean by "makes the port/interface separation much more >> difficult"; to me, it makes the port concept more abstract, which should >> make it more separable from other issues. >> >> The last statement was referring to the changes needed to support: >> http://reviews.gem5.org/r/1301/. I fear this additional level of Port makes >> that binding process more challenging. >> >> I definitely support the approach you mention with the EtherLink pointers. >> Supposedly that also avoids the (in my opinion rather confusing) binding by >> =. >> >> >> - Andreas >> >> On June 13th, 2013, 7:58 a.m., Steve Reinhardt wrote: >> Review request for Default. >> By Steve Reinhardt. >> >> *Updated June 13, 2013, 7:58 a.m.* >> Description >> >> Changeset 9757:071a2d97327a >> --------------------------- >> sim: unify memory & ethernet port binding mechanisms. >> >> Get rid of the ugly special-casing and dynamic casting going >> on in connectPorts(), and make the port binding mechanism >> more extensible. This is done by moving the port lookup >> methods to SimObject and creating a common base Port class >> from which both the old Port (now MemPort) and EtherInt >> (which perhaps should be renamed EtherPort) derive. >> >> Diffs >> >> - src/arch/x86/bios/acpi.hh (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/dev/etherdevice.hh (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/dev/etherdevice.cc (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/dev/etherint.hh (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/dev/etherint.cc (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/dev/etherlink.hh (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/dev/etherobject.hh (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/mem/mem_object.hh (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/mem/mem_object.cc (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/mem/port.hh (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/mem/port.cc (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/python/swig/pyobject.cc >> (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/sim/port.hh (PRE-CREATION) >> - src/sim/sim_object.hh (0b4a08751b42ac522e296bd1a12408b816068fa1) >> - src/sim/sim_object.cc (0b4a08751b42ac522e296bd1a12408b816068fa1) >> >> View Diff <http://reviews.gem5.org/r/1922/diff/> >> > > > -- IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy the > information in any medium. Thank you. > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
