Review Board 2.0.15


MEM: Add the port proxies to the source tree

Review Request #916 - Created Nov. 28, 2011 and discarded

Information
Andreas Hansson
gem5
default
Reviewers
Default
ali, gblack, nate, stever 
MEM: Add the port proxies to the source tree

Port proxies are used to replace non-structural ports, and thus enable
all ports in the system to correspond to a structural entity.

The following replacements are going to follow in the next patches:
FunctionalPort      > PortProxy
TranslatingPort     > SETranslatingProxy
VirtualPort         > FSTranslatingProxy

This patch does not instantiate any of the aforementioned ports, it
merely adds the source files.
util/regress passing all ignoring failing eio and t1000
Nilay Vaish
Posted (Nov. 28, 2011, 2:29 a.m.)
Andreas, can you explain in more detail the functionality which is lacking
right now and how your series of patches address it?
  1. Yes, this is a major change and it hasn't even been mentioned before. What does it do, how does it do it, and why is it necessary? I don't know what you mean by "non-structural ports". Looking at your patch briefly, you may be reinventing functional accesses. Also, in the relatively near future I'm probably going to replace Translating and Virtual ports entirely with a combined version that will be shared between SE and FS modes. The distinction between those modes is on its way out, and it would be a bad idea to bake it into class names, let alone their functionality. Without a lot of strong justification I don't think this or it's follow on patches should be committed.
  2. We've been discussing this for 6 months now with the changes various changes Andreas has been posting.

15-July 	Changes to the gem5 memory-system (release-0.1)
5-Aug  	Changes to the gem5 memory-system (release-0.2)

At that time everyone (most notable Steve) was happy with these changes. We've been addressing some of the comments that Steve had and have been otherwise busy, but please don't have a knee jerk reaction to a patch until you understand what it's trying to do.

  3. A non-structure port is a port that doesn't exist for some real reason in gem5 (e.g. a cache port), but it used for some sort of binary loading or the like. The change removes the all of these and replaces it with a single port. This way you can be certain that every port has a peer (one-to-one) relationship unlike the (one-to-N) you can have now. 
  4. I'm not going to remember a few emails from July, and I'm not just going to assume everything is fine when a changes come in that modify fundamental functionality across dozens of files with very little explanation attached. If something looks wrong and I don't know otherwise, I'm going to say something. Then you can take all the time in the world to convince me otherwise, and the repository will be safe.

I also still have no idea what a non-structure port is. If it doesn't exist, why does it matter? It doesn't look like anything is being removed, just turned into a different type of port with a new name. I still don't see what this is doing that's better than what's already there. Maybe once I understand that this will all make sense and be fine, but in the mean time these changes make me really uncomfortable.

Since this is turning into it's own discussion, lets move over to email. Once we hash everything out we can update the commit message so people can know what happened and why.
Steve Reinhardt
Posted (Dec. 3, 2011, 1:50 a.m.)
Overall this looks great.  Thanks to Andreas and all the other ARM folks that have worked on this.  There are definitely some tricky issues coming up with the larger port reworking, but this set of patches doesn't touch on any of them, so it's a good place to start!

I have three main areas of concern:

1. Procedural issues.  I already pointed out the couple of files that should really be treated as renames and not deletes and additions.  Also I'm not sure about merging these into a single changeset for commit.  Splitting it up definitely made it easier to review (thanks!) but led to some anomalies that I've pointed out in the detailed reviews, including some places where the final code is suboptimal.  I can also see where doing the file renames I mentioned and also making each incremental patch compilable would be in conflict, which also argues for a single changeset.  However it would be a big one, and you'd lose the "understandability" of the incremental changes.  Have you guys thought about this?  Any other opinions?

2. SE/FS convergence.  I generally hesitate to push people to incorporate orthogonal code cleanup into unrelated changes, but this seems like a really great opportunity to push toward unifying the two TranslatingPort classes.  At the very least I think it would be good to rename both SETranslatingPort and FSTranslatingPort to just TranslatingPort, with the one you get depending on your compilation mode.  Then in other parts of the code you wouldn't really have a distinction... for example, see my comment on that snippet in remote_gdb.cc.  Also renaming the SE-mode getMemProxy() to getVirtProxy() to be parallel with FS mode would help.  Since you're renaming things anyway it seems like we should do it right.  Of course this needs to be coordinated with what Gabe is doing; how are you dealing with the VirtualPort/TranslatingPort issue, Gabe?

3. Giving access to the System port.  If you just have more objects that need a System pointer so they can find the System proxy port, then we should do that using existing techniques in Python.  If you really need to expose the configuration hierarchy in C++ (which is probably a good idea, though I don't think it's appropriate for the current purpose), then we should do that in a cleaner fashion than what the current code does.

Let me know if you have any questions...

Steve