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]<mailto:[email protected]>> Date: Thursday, 13 June 2013 20:10 To: Andreas Hansson <[email protected]<mailto:[email protected]>> Cc: Default <[email protected]<mailto:[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]<mailto:[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
