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

Can we not get this through the sequencer rather?

Show all issues

1. Jason Lowe-Power Feb. 5, 2015, 7:38 a.m. (Feb. 5, 2015, 7:38 a.m.)

   I can make this change, if you want. But I was following the same format as is in the other Ruby ports (i.e. line 82 in RubyPort.hh). IMO, it's better to have consistency across similar objects than minimizing the code.

2. Andreas Hansson Feb. 5, 2015, 7:44 a.m. (Feb. 5, 2015, 7:44 a.m.)

   Don't worry if it is too invasive. It merely seems unnecessary to pass the pointer around everywhere if it is already in the "bigger" objects.

src/mem/ruby/system/DMASequencer.cc (Diff revision 1)

Should this really be functional, or should it just be "access"?

Show all issues

1. Jason Lowe-Power Feb. 5, 2015, 7:38 a.m. (Feb. 5, 2015, 7:38 a.m.)

   I can make this change. What is the difference between access() and functionalAccess()? When should one be used and not the other? I might have missed it, but I can't find any guide to this on the wiki or in the code.
   
   Also, do you want me to go back through and make this update in the RubyPort as well?

2. Andreas Hansson Feb. 5, 2015, 7:44 a.m. (Feb. 5, 2015, 7:44 a.m.)

   functional -> do not change any state (only read/write and update data), should only be used by send/recvFunctional imho
   
   normal -> anything from atomic or timing where we also need to deal with Swap, LLSC etc and the state (in addition to data) changes
   
   I think in all these cases we should use access()

3. Jason Lowe-Power Feb. 5, 2015, 8:02 a.m. (Feb. 5, 2015, 8:02 a.m.)

   Gotcha. Fixed.

src/mem/ruby/system/RubySystem.py (Diff revision 1)

I find the description rather misleading.

In essence it is not the case of bypassing the interconnect, it is more a case of duplicating the memory, and ignoring the "normal" one.

Thinking about it, why do we not just use the actual memory, but do so by faking it through system->getPhysMem().access(...)

Show all issues

1. Jason Lowe-Power Feb. 5, 2015, 7:38 a.m. (Feb. 5, 2015, 7:38 a.m.)

   I've updated this description. I don't follow your last statement, though.

2. Andreas Hansson Feb. 5, 2015, 7:44 a.m. (Feb. 5, 2015, 7:44 a.m.)

   Why do we need this additional backing store, and why do we not simply bypass the interconnect and use C++ magic, going through the system->getPhysMem(), but to the same backing store we would have used had we gone through the interconnect. Perhaps I am missing something...

3. Jason Lowe-Power Feb. 5, 2015, 8:02 a.m. (Feb. 5, 2015, 8:02 a.m.)

   Well, on the one hand, this patch is just fixing the support Nilay added in patch 10525:77787650cbb (http://reviews.gem5.org/r/2466/). IIRC, that patch was added after Nilay removed this support although both gem5-gpu and AMD depends on it. It turns out that patch was totally broken, and without this patch that support didn't work. I'd like to get this patch pushed in since it's fixing something that we depend on.
   
   On the other hand, if I follow what you're saying, yes, there is a much better way to provide this support. At the high level all we want is to use Ruby for timing and a backing store for the data. It's very hard to implement a cache coherence protocol that deals with data correctly, and the backing store is useful both in debugging protocols while building them and for protocols with subtle bugs in how data is handled.
   
   Does this make sense?

4. Andreas Hansson Feb. 5, 2015, 8:05 a.m. (Feb. 5, 2015, 8:05 a.m.)

   Yup, makes sense.
   
   Let's save the overhaul for another patch (or perhaps a new spin on Nilay's previous patch).

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

const

Show all issues

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

const

Show all issues

LGTM

LGTM, also.

I don't think it should be addressed in this patch, but I'm making notes here as a vision for how I think we should proceed with these backing store and protocol developments:

Really what we're capturing with the backing store option is whether a Ruby coherence protocol requires the backing store, but *not* that the RubySystem needs it for any fundamental reason. In other words, requiring a backing store is a property of a particular coherence protocol implementation rather than the overall RubySystem. We've noted previously that protocol implementations can require backing store if they are not known to correctly buffer data in the cache controllers, or if they implement some functionality that cannot be easily modeled without a backing store (e.g. slim GPU atomics). The last couple changes to backing stores are major steps in the right direction: we've reduced the backing store specifier to a single option, rather than the prior structure, which disaggregated the variable and required that the programmer set the option on each of the RubySequencers (previously 'access_phys_mem'). However, the ultimate solution *should* be that each coherence protocol implementation specifies whether it requires the backing store.

To this end, I propose that the next step for development on this should be to create an abstract Python wrapper object for Ruby protocol implementations that embeds this "requires backing store" property as a variable. All Ruby protocols could descend from that wrapper object, and we could handle the protocol as a Python object in config files rather than using the ugly Python exec calling convention we have in Ruby.py currently. If the protocol requires the backing store, Ruby should check for the requirement (e.g. accessor function in the abstract Python protocol object) and set up the backing store without the need for any command line parameters. If a developer decides to validate a protocol as "data correct", s/he would simply set the "requires backing store" variable to False for that protocol when the validation is completed.

src/mem/ruby/system/System.cc (Diff revision 4)

This doesn't seem right. m_phys_mem is SimpleMemory type, which inherits empty serialize()/unserialize() functions from the Serializable class. Thus, this code doesn't do anything except add the fake backing store header to the checkpoint.

Can you elaborate on the situation that suggested you needed this?

Show all issues

1. Jason Lowe-Power Feb. 9, 2015, 1:58 p.m. (Feb. 9, 2015, 1:58 p.m.)

   This check is required since m_phys_mem is null when m_access_backing_store is false.

2. Joel Hestness Feb. 9, 2015, 2:36 p.m. (Feb. 9, 2015, 2:36 p.m.)

   Sorry, the issue I opened may have been unclear... I do not feel that this has been resolved.
   
   First, this code was added between revisions 3 and 4 of this review request and relates to checkpointing testing that maybe hadn't been completed for the original request posted here. So, this appears to be additional functionality beyond what I've already say "Ship it!" for. If you'd like to address the checkpointing issues separately from the backing store fixes in this patch, then this particular change to this revision (lines 156-158 and 278-280) should be moved a separate review request. I'd be fine with addressing this checkpointing issue here or in another review.
   
   If you'd like this change to be considered as part of this review request, then my issue needs to be resolved here. That issue more specifically: It doesn't appear that you should need to serialize or unserialize the Ruby m_phys_mem, whether or not you are using it in either the checkpointed or restored simulation. This is suggested by the fact that checkpointing/restoring the fake backing store was not needed in previous versions of gem5. Unless I'm overlooking something, it doesn't seem correct to checkpoint m_phys_mem here: SimpleMemory doesn't appear to serialize anything when you dump it while taking a checkpoint (its serialize() function is empty - inherited from Serializable), so there's nothing to unserialize when restoring m_phys_mem. Thus, it appears that this new code in revision 4 does nothing (lines 156-158, and 278-280). If this code was added to address a bug with checkpointing, it seems very unlikely that this is a correct way to fix the bug. Can you clarify this (either here or separate review if you go that route)?

src/mem/ruby/system/System.cc (Diff revision 4)

This is a problematic way to do this. If the checkpoint was taken with a coherence protocol that did not require the fake functional backing store, but you're restoring into a protocol that requires it, then m_phys_mem would not have been serialized and cannot be unserialized here. You'd run into the panic that the fake functional store do not exist in the checkpoint.

To resolve this: If something has changed so that we actually do need to serialize the fake functional store (again, I don't understand why), you should probably just serialize out the m_access_backing_store variable and unserialize it to check whether a fake functional store was saved when taking the checkpoint. If it wasn't serialized out, you can't restore it.

Show all issues

1. Jason Lowe-Power Feb. 9, 2015, 1:56 p.m. (Feb. 9, 2015, 1:56 p.m.)

   I think this is adding too much complication to this feature. There are a number of possibilities between restoring and checkpointing with and without the backing store. I don't think that we need to add the complications of checking for each of them. Additionally, as was pointed out earlier in this review, this patch is a fix for the (very) broken current state, and the current backing store design needs to be changed. Thus, I think what we have here is good enough for now.
   
   As it is, this code requires taking and restoring checkpoints with the same backing store option. This is in line with the other idiosyncrasies of taking checkpoints with Ruby (e.g. you have to be very careful with the number of controllers and the order the controllers are created in). I'll add a note to the commit message with this info.
   
   I would like to see this bug-fix patch go in as is and later the backing store logic can be redesigned the "right" way if someone is so inclined.