src/mem/ruby/system/System.hh (Diff revision 1)

Rename to start_cycle.

Show all issues

Ship It!

Can you please expand on the commit message? I understand that these changes are to allow multiple RubySystem instances, but some detailed information about this should be available in the commit log.

Overall, it seems like there should be a better way to do this using gem5-principled stats. However, it seems pretty non-trivial to use gem5 stats to handle, for instance, the VC load average stats without significant changes.
Another option, which would clean things up significantly, would be to make start_cycle a private static member of RubySystem and provide a static accessor method. Just like warm-up and cool-down, stats reset is a simulation-wide operation and not system-instance-specific, so there only needs to be one start_cycle variable in the simulation. You're already resetting this variable like a static member, and you've already added the accessor function, so you'd just need to roll-out the RubySystem pointer changes to do it this way. This also removes the need to add parameters to the Throttle.

src/mem/ruby/network/Network.cc (Diff revision 2)

Why does this need to change? If it's for consistency, why did you choose to declare local pointers everywhere and access them through params()? It seems superfluous. Are there forthcoming changes that rely on this convention?

Show all issues

src/mem/ruby/network/simple/Throttle.hh (Diff revision 2)

I don't understand why this is inconsistent with the GarnetNetwork changes: Here, you've added a per-instance pointer to Ruby to achieve the same functionality as with the GarnetNetwork changes, but in the GarnetNetwork, you're accessing the RubySystem from the params() in collateStats.
I'd rather these be consistent.
NOTE: If you were to follow my overall suggestion of making the start_time static in RubySystem, this change would not need to happen, and there would be no consistency issues between here and the GarnetNetwork.

Show all issues

1. Brandon Potter June 26, 2015, 7:52 a.m. (June 26, 2015, 7:52 a.m.)

   Throttle is not derived from a clocked memory object, but it does have a clocked memory object that is passed into it.  Is it reasonable to assume that this clocked memory object will return the same cycle count as the RubySystem object?
   If so, I think that it would be possible to avoid passing in the pointer to RubySystem.  If not, the RubySystem pointer needs to be passed in to give the right cycle count with curCycle inside Throttle::collateStats().

2. Joel Hestness June 26, 2015, 8:18 a.m. (June 26, 2015, 8:18 a.m.)

   Ok, I understand that you need to get the cycle count from somewhere and that you've chosen the RubySystem for that. What I'm confused about is the inconsistency of whether you add a member variable to point to the RubySystem or access it from the params. Common gem5 convention is to add a member variable any time that a param variable is used outside the constructor. Your changes to Throttle follow this convention. I think it would be more consistent to add member variables that point to the RubySystem from the GarnetNetwork objects.

3. Brandon Potter June 26, 2015, 10:27 a.m. (June 26, 2015, 10:27 a.m.)

   Is it acceptable to leave the access through params() as is?  It does exactly the same thing, except the access is done through the params() field instead of explicitly adding another member to the object.  Feel like this could be argued for in both ways, but the difference for me is just the extra work in making the modifications.

4. Joel Hestness June 26, 2015, 10:43 a.m. (June 26, 2015, 10:43 a.m.)

   Fine, though I feel that consistency on small things like this should be considered before submitting a patch for review (e.g. your g_system_ptr patch has numerous inconsistencies like this as well). Substantial refactorings like these are opportunities to make code more consistent and easier to change in the future.

5. Brandon Potter June 26, 2015, 10:55 a.m. (June 26, 2015, 10:55 a.m.)

   I have addressed all of your comments except this one.  Personally, I feel that this one is a bit arbitrary given that the params are recorded in the class (what other purpose do they serve if the params are explicitly retained other than to use them?), but I will change it if you really want me to do so.
   I think the end result of your suggestions is that the code does look better and is a bit cleaner, but I disagree with this one case.
   Just to be thorough, what's bothering you about the g_system_ptr patch?

6. Joel Hestness June 26, 2015, 11:45 a.m. (June 26, 2015, 11:45 a.m.)

   It's alright. I'm realizing that many of the choices in these patch are a result of Ruby's poor organization, which makes it hard to decide on conventions. I have not opened any issues on the g_system_ptr patch, because I think it would take a lot of effort to fix things more appropriately there. In other words, you shouldn't worry about my thoughts for getting the g_system_ptr patch through (but please do expand a bit on the commit message).
   So, I apologize profusely for this, but I'm having second thoughts about making the start_cycle a static variable. The problem with that is that separate RubySystem instances should be allowed to have different frequencies, so their Cycle counts may differ (eventually, I think we should be calculating utilization stats from network frequency values, but until then, we'll probably need separate RubySystem start_cycle variables). So, you were right with the original patch that the start_cycle variable should be per-RubySystem-instance. I think it would be fine if you checked in the second version of this patch (link: http://reviews.gem5.org/r/2901/diff/2/raw/ ). Again, sorry about the extra effort.

7. Brandon Potter June 26, 2015, 12:49 p.m. (June 26, 2015, 12:49 p.m.)

   No worries.
   Vi + persistent_undo + https://github.com/mbbill/undotree == ezpz

src/mem/ruby/network/simple/Throttle.cc (Diff revision 2)

Why not just use ruby_system as the pointer instead of creating a local pointer, rs?

Show all issues

Ship It!