Discussion:
[gem5-dev] Review Request: MEM: Add the port proxies to the source tree
(too old to reply)
Andreas Hansson
2011-11-28 18:16:33 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/916/
-----------------------------------------------------------

Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert.


Summary
-------

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.


Diffs
-----

src/mem/SConscript e70d031cb5f9
src/mem/fs_translating_proxy.hh PRE-CREATION
src/mem/fs_translating_proxy.cc PRE-CREATION
src/mem/port_proxy.hh PRE-CREATION
src/mem/port_proxy.cc PRE-CREATION
src/mem/se_translating_proxy.hh PRE-CREATION
src/mem/se_translating_proxy.cc PRE-CREATION

Diff: http://reviews.m5sim.org/r/916/diff


Testing
-------

util/regress passing all ignoring failing eio and t1000


Thanks,

Andreas
Nilay Vaish
2011-11-28 18:29:40 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/916/#review1693
-----------------------------------------------------------


Andreas, can you explain in more detail the functionality which is lacking
right now and how your series of patches address it?

- Nilay
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.m5sim.org/r/916/
-----------------------------------------------------------
(Updated 2011-11-28 10:16:33)
Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert.
Summary
-------
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.
FunctionalPort > PortProxy
TranslatingPort > SETranslatingProxy
VirtualPort > FSTranslatingProxy
This patch does not instantiate any of the aforementioned ports, it
merely adds the source files.
Diffs
-----
src/mem/SConscript e70d031cb5f9
src/mem/fs_translating_proxy.hh PRE-CREATION
src/mem/fs_translating_proxy.cc PRE-CREATION
src/mem/port_proxy.hh PRE-CREATION
src/mem/port_proxy.cc PRE-CREATION
src/mem/se_translating_proxy.hh PRE-CREATION
src/mem/se_translating_proxy.cc PRE-CREATION
Diff: http://reviews.m5sim.org/r/916/diff
Testing
-------
util/regress passing all ignoring failing eio and t1000
Thanks,
Andreas
Gabe Black
2011-11-28 21:28:25 UTC
Permalink
Post by Andreas Hansson
Post by Nilay Vaish
Andreas, can you explain in more detail the functionality which is lacking
right now and how your series of patches address it?
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.


- Gabe


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/916/#review1693
-----------------------------------------------------------
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.m5sim.org/r/916/
-----------------------------------------------------------
(Updated 2011-11-28 10:16:33)
Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert.
Summary
-------
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.
FunctionalPort > PortProxy
TranslatingPort > SETranslatingProxy
VirtualPort > FSTranslatingProxy
This patch does not instantiate any of the aforementioned ports, it
merely adds the source files.
Diffs
-----
src/mem/SConscript e70d031cb5f9
src/mem/fs_translating_proxy.hh PRE-CREATION
src/mem/fs_translating_proxy.cc PRE-CREATION
src/mem/port_proxy.hh PRE-CREATION
src/mem/port_proxy.cc PRE-CREATION
src/mem/se_translating_proxy.hh PRE-CREATION
src/mem/se_translating_proxy.cc PRE-CREATION
Diff: http://reviews.m5sim.org/r/916/diff
Testing
-------
util/regress passing all ignoring failing eio and t1000
Thanks,
Andreas
Ali Saidi
2011-11-28 21:35:51 UTC
Permalink
Post by Gabe Black
Post by Nilay Vaish
Andreas, can you explain in more detail the functionality which is lacking
right now and how your series of patches address it?
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.
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.


- Ali


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/916/#review1693
-----------------------------------------------------------
Post by Gabe Black
-----------------------------------------------------------
http://reviews.m5sim.org/r/916/
-----------------------------------------------------------
(Updated 2011-11-28 10:16:33)
Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert.
Summary
-------
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.
FunctionalPort > PortProxy
TranslatingPort > SETranslatingProxy
VirtualPort > FSTranslatingProxy
This patch does not instantiate any of the aforementioned ports, it
merely adds the source files.
Diffs
-----
src/mem/SConscript e70d031cb5f9
src/mem/fs_translating_proxy.hh PRE-CREATION
src/mem/fs_translating_proxy.cc PRE-CREATION
src/mem/port_proxy.hh PRE-CREATION
src/mem/port_proxy.cc PRE-CREATION
src/mem/se_translating_proxy.hh PRE-CREATION
src/mem/se_translating_proxy.cc PRE-CREATION
Diff: http://reviews.m5sim.org/r/916/diff
Testing
-------
util/regress passing all ignoring failing eio and t1000
Thanks,
Andreas
Ali Saidi
2011-11-28 21:42:21 UTC
Permalink
Post by Gabe Black
Post by Nilay Vaish
Andreas, can you explain in more detail the functionality which is lacking
right now and how your series of patches address it?
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.
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.
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.


- Ali


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/916/#review1693
-----------------------------------------------------------
Post by Gabe Black
-----------------------------------------------------------
http://reviews.m5sim.org/r/916/
-----------------------------------------------------------
(Updated 2011-11-28 10:16:33)
Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert.
Summary
-------
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.
FunctionalPort > PortProxy
TranslatingPort > SETranslatingProxy
VirtualPort > FSTranslatingProxy
This patch does not instantiate any of the aforementioned ports, it
merely adds the source files.
Diffs
-----
src/mem/SConscript e70d031cb5f9
src/mem/fs_translating_proxy.hh PRE-CREATION
src/mem/fs_translating_proxy.cc PRE-CREATION
src/mem/port_proxy.hh PRE-CREATION
src/mem/port_proxy.cc PRE-CREATION
src/mem/se_translating_proxy.hh PRE-CREATION
src/mem/se_translating_proxy.cc PRE-CREATION
Diff: http://reviews.m5sim.org/r/916/diff
Testing
-------
util/regress passing all ignoring failing eio and t1000
Thanks,
Andreas
Gabe Black
2011-11-28 21:56:54 UTC
Permalink
Post by Gabe Black
Post by Nilay Vaish
Andreas, can you explain in more detail the functionality which is lacking
right now and how your series of patches address it?
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.
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.
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.
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.


- Gabe


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/916/#review1693
-----------------------------------------------------------
Post by Gabe Black
-----------------------------------------------------------
http://reviews.m5sim.org/r/916/
-----------------------------------------------------------
(Updated 2011-11-28 10:16:33)
Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert.
Summary
-------
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.
FunctionalPort > PortProxy
TranslatingPort > SETranslatingProxy
VirtualPort > FSTranslatingProxy
This patch does not instantiate any of the aforementioned ports, it
merely adds the source files.
Diffs
-----
src/mem/SConscript e70d031cb5f9
src/mem/fs_translating_proxy.hh PRE-CREATION
src/mem/fs_translating_proxy.cc PRE-CREATION
src/mem/port_proxy.hh PRE-CREATION
src/mem/port_proxy.cc PRE-CREATION
src/mem/se_translating_proxy.hh PRE-CREATION
src/mem/se_translating_proxy.cc PRE-CREATION
Diff: http://reviews.m5sim.org/r/916/diff
Testing
-------
util/regress passing all ignoring failing eio and t1000
Thanks,
Andreas
Steve Reinhardt
2011-12-03 17:50:52 UTC
Permalink
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
http://reviews.m5sim.org/r/916/#review1718
-----------------------------------------------------------


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 dealin
g 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


- Steve
Post by Andreas Hansson
-----------------------------------------------------------
http://reviews.m5sim.org/r/916/
-----------------------------------------------------------
(Updated 2011-11-28 10:16:33)
Review request for Default, Ali Saidi, Gabe Black, Steve Reinhardt, and Nathan Binkert.
Summary
-------
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.
FunctionalPort > PortProxy
TranslatingPort > SETranslatingProxy
VirtualPort > FSTranslatingProxy
This patch does not instantiate any of the aforementioned ports, it
merely adds the source files.
Diffs
-----
src/mem/SConscript e70d031cb5f9
src/mem/fs_translating_proxy.hh PRE-CREATION
src/mem/fs_translating_proxy.cc PRE-CREATION
src/mem/port_proxy.hh PRE-CREATION
src/mem/port_proxy.cc PRE-CREATION
src/mem/se_translating_proxy.hh PRE-CREATION
src/mem/se_translating_proxy.cc PRE-CREATION
Diff: http://reviews.m5sim.org/r/916/diff
Testing
-------
util/regress passing all ignoring failing eio and t1000
Thanks,
Andreas
Gabriel Michael Black
2011-12-07 00:51:36 UTC
Permalink
Post by Steve Reinhardt
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?
With FULL_SYSTEM, we were choosing to include or exclude things but
generally not to choose between two implementations of the same thing.
With the ISAs things are significantly different. In my merged
repository, I have both translating ports and virtual ports at the
same time, although only one or the other will get used. If we're
going to go so far as to actually combine the two types of ports that
would be great, but we shouldn't use the preprocessor and FULL_SYSTEM
to decide which implementation to use since that gets ugly when the
preprocessor can't be used any more.

That being said, We should consider the names SE and FS
semideprecated. They're still pretty descriptive and are seen around
the simulator, but I don't want to use them in any formal way like in
the names of classes. It would make sense to use the original names
and change the implementation.

Gabe
Steve Reinhardt
2011-12-07 01:30:18 UTC
Permalink
Post by Gabriel Michael Black
Post by Steve Reinhardt
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?
With FULL_SYSTEM, we were choosing to include or exclude things but
generally not to choose between two implementations of the same thing. With
the ISAs things are significantly different. In my merged repository, I
have both translating ports and virtual ports at the same time, although
only one or the other will get used. If we're going to go so far as to
actually combine the two types of ports that would be great, but we
shouldn't use the preprocessor and FULL_SYSTEM to decide which
implementation to use since that gets ugly when the preprocessor can't be
used any more.
My goal is that we eventually actually combine the two types of ports. As
I think you have already pointed out, Gabe, there's no reason to have two
different ports that do effectively the same thing with different names. I
seem to recall this even being somewhat of a poster child for where SE and
FS had evolved to be gratuitously different that our SE/FS merger was going
to address.

I agree that as a general policy we shouldn't have two different objects
with the same name and use FULL_SYSTEM to select between them at compile
time, but I see this as an exception because this would be an intermediate
step toward the eventual goal of unifying them into a single object. In
particular, it would let us take some pieces of code (like that
remote_gdb.cc snippet) and have a single piece of code there that works in
both SE and FS modes.

The potential drawback is that it would force us to actually combine the
two classes before we're able to get rid of FULL_SYSTEM as a preprocessor
symbol. Depending on your perspective that might not be a bad thing,
though, as it would also prevent us from putting that piece of cleanup off
indefinitely. Or another way to look at it is that if we're going to do it
eventually, why not now. (Not as a part of this specific set of patches,
but perhaps soon afterward.)

Steve
Gabriel Michael Black
2011-12-07 06:25:43 UTC
Permalink
Post by Steve Reinhardt
Post by Gabriel Michael Black
Post by Steve Reinhardt
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?
With FULL_SYSTEM, we were choosing to include or exclude things but
generally not to choose between two implementations of the same thing. With
the ISAs things are significantly different. In my merged repository, I
have both translating ports and virtual ports at the same time, although
only one or the other will get used. If we're going to go so far as to
actually combine the two types of ports that would be great, but we
shouldn't use the preprocessor and FULL_SYSTEM to decide which
implementation to use since that gets ugly when the preprocessor can't be
used any more.
My goal is that we eventually actually combine the two types of ports. As
I think you have already pointed out, Gabe, there's no reason to have two
different ports that do effectively the same thing with different names. I
seem to recall this even being somewhat of a poster child for where SE and
FS had evolved to be gratuitously different that our SE/FS merger was going
to address.
I agree that as a general policy we shouldn't have two different objects
with the same name and use FULL_SYSTEM to select between them at compile
time, but I see this as an exception because this would be an intermediate
step toward the eventual goal of unifying them into a single object. In
particular, it would let us take some pieces of code (like that
remote_gdb.cc snippet) and have a single piece of code there that works in
both SE and FS modes.
The potential drawback is that it would force us to actually combine the
two classes before we're able to get rid of FULL_SYSTEM as a preprocessor
symbol. Depending on your perspective that might not be a bad thing,
though, as it would also prevent us from putting that piece of cleanup off
indefinitely. Or another way to look at it is that if we're going to do it
eventually, why not now. (Not as a part of this specific set of patches,
but perhaps soon afterward.)
Steve
I agree with what you've saying, but this will force us to merge the
two classes before I can move my code back into the main repository.
I've eliminated FULL_SYSTEM entirely and just have a run time
FullSystem (bad caps, I know) which chooses the behavior. As the merge
gets delayed, it will be harder and harder to keep things in sync.
This would be a good forcing function, except I'd absorb the force,
and I'm already sufficiently motivated :-).

This gets into the AddressSpace object idea I'd had before where the
port has an object that does translation for it through whatever means
is necessary. For FullSystem I think it uses a function (vtophys?) to
look through the guests paging structures to do the translation, and
in SE I think it uses the fake page table object we maintain. Since
these are somewhat different, it's not entirely trivial to merge them.
It's still not super hard, but it will take a little work. I was
hoping to do that after merging back in.

And actually speaking of merging back in, I should probably merge with
the new commits in the main repo and put the great big diff from the
merge commit up on review board.

I'd like to characterize how performance changes when FS/SE is a
runtime thing, but as I mentioned I'd have to run that on my laptop
and that would be slow and inconvenient since I'd have to leave it
alone to get reliable results. If somebody wanted to play around with
it for me, I can tell you what to do. There's basically just one patch
that tips it from one to the other, ignoring the follow on changes
that merge the build_opts and regression stuff.

Gabe

Continue reading on narkive:
Loading...