Hi Tony,

This looks pretty good. Do you have an idea how to fix the other issues? Is this useful enough to get some things done, or is there more to do? How have you tested it? I'd like to see us run some benchmarks switching back and forth as part of a regression test to make sure that this isn't broken again.

Thanks agani,

Ali

src/cpu/base.cc (Diff revision 2)

This will not work correctly with x86. Take a look at the line src/arch/x86/interrupts.cc, line 301.

Show all issues

1. Tony Gutierrez May 25, 2012, 9:03 a.m. (May 25, 2012, 9:03 a.m.)

   Can you expound on why this is a problem? From the point of view of the interrupt controller the CPU switchout should be transparent. Essentially the id should remain constant regardless of which CPU model is currently active.

2. Nilay Vaish May 25, 2012, 9:46 a.m. (May 25, 2012, 9:46 a.m.)

   Because the check "cpu != NULL && cpu->cpuId() != newCPU->cpuId()" will return true
   and hence panic() will get triggered. Clearly cpu is not NULL. With your change,
   cpu and newCPU will have different Ids. This can be fixed by setting the cpuId of
   of the oldCPU after the interrupt controller has been.
   
   Can you explain why do you need to make this change?

Are you still working on this?

src/arch/arm/table_walker.cc (Diff revision 7)

Looks like this should be stateQueueL1

Show all issues

1. Tony Gutierrez Aug. 1, 2012, 9:10 a.m. (Aug. 1, 2012, 9:10 a.m.)

   Fixed, don't know how that changed from the previous version of the patch.

src/arch/arm/table_walker.cc (Diff revision 7)

you could do count + 1 here along with a comment that we're returning the count of the port + 1 because we still have stuff to do. 

Show all issues

1. Tony Gutierrez Aug. 1, 2012, 9:10 a.m. (Aug. 1, 2012, 9:10 a.m.)

   Also, recently I was thinking should we do something like:
   
   else {
       if (count) {
           changeState(Draining);
           return count;
       }
       changeState(Drained);
       return 0;
   }
   
   In the case the table walker itself is drained, but its ports aren't? Should it be "drained" if its ports are not?

2. Ali Saidi Aug. 1, 2012, 10:25 a.m. (Aug. 1, 2012, 10:25 a.m.)

   no it shouldn't be... if there is something in the port still, it's going to be getting a callback to deal with it.

3. Tony Gutierrez Aug. 1, 2012, 11:06 a.m. (Aug. 1, 2012, 11:06 a.m.)

   Ok. My reasoning for this was that I think any object that is responsible for calling drain() on other objects, should first call drain() on the objects it's responsible for again before it processes its drain event. E.g., for the table walker we should do for completeDrain():
   
   unsigned int count = port.drain(de);
   
   if (drainEvent && stateQueueL1.empty() && stateQueueL2.empty() && pendingQueue.empty() && !count) {
       // continue to set Drained and process drain event
   
   I tried this and it didn't seem to get me any further or fix any bugs, but I still think it's correct.
   
   Also the cache and DmaDevice set their state to Draining if their ports aren't drained, even though they themselves never contribute to count. Caches also set the drainEvent when they're draining and never set it to NULL, i.e., there is nothing like a completeDrain() for them. Either they don't need to do that, or they themselves should have to drain somehow and not just their ports. Currently, caches and DmaDevices are always drained if their ports are drained.

4. Ali Saidi Aug. 5, 2012, 2:02 a.m. (Aug. 5, 2012, 2:02 a.m.)

   It isn't doing that? I think it probably should.
   The caches don't do anything because they're not responsible for originating any requests (although caches with a prefetcher might violate this assumption). The device or a cpu is asking the cache to do a request on behalf of it, so if the device or cpu aren't drained the cache can't be.
   

src/cpu/o3/fetch_impl.hh (Diff revision 7)

that is a nice bug.. good catch

Show all issues

What happened to the 65 char description limit? :)

As a side note: with this patch, and using the config from my other patch, I can switch repeatedly with Alpha and haven't run into any problems. Switching back-and-forth every 5 billion ticks and every 1 million ticks I can run hello world entirely with O3 CPUs in FS. This includes kernel boot.

Tony... this looks great, thanks!  I didn't scrutinize it as carefully as Ali did, but it looks like a lot of good fixes.  Just a couple minor things I noticed before committing.

src/cpu/base.cc (Diff revision 9)

This fancy footwork with the cpuId value deserves a comment, I think...

1. Tony Gutierrez Aug. 8, 2012, 11:05 a.m. (Aug. 8, 2012, 11:05 a.m.)

   Ok, I can add a comment.
   
   But, I did have a question about the need for copying over the cpuId. Formerly it was simply copied over, meaning after one switch both CPUs would always have the same ID. And to me this makes sense, since from an OS perspective they are essentially the same CPU. From the simulator's view there may be reasons for the CPU and its switched-out equivalent to have unique IDs but I don't know of any. Would it be better to just have any CPU and its equivalent have the same ID?

2. Steve Reinhardt Aug. 8, 2012, 11:55 a.m. (Aug. 8, 2012, 11:55 a.m.)

   I don't know either... I assumed that you did it this way because there was some bug you ran into that required you to do this (which is why I was looking for a comment to explain what it was).  I think you should do the simplest thing that works.  In the long run, it would be good to have the python config script just set all the cpus that we'll be switching in and out of the same spot to have the same cpuId in the first place, then we cold just have an assertion in the C++ to guarantee that the configuration isn't messed up.  I don't know if it's worth trying to do that now or if it's best left for a later patch.

3. Tony Gutierrez Aug. 8, 2012, 2:18 p.m. (Aug. 8, 2012, 2:18 p.m.)

   I removed the CPU id copying. I also updated my config patch to set all "switched out" CPUs to share the id of their active equivalent CPU.

src/python/m5/simulate.py (Diff revision 9)

This looks valuable for debugging, but is it output we always want to leave on?  Don't we have a DPRINTF equivalent in Python?

Show all issues

1. Tony Gutierrez Aug. 8, 2012, 11:06 a.m. (Aug. 8, 2012, 11:06 a.m.)

   I'll look into this.

2. Tony Gutierrez Aug. 8, 2012, 2:17 p.m. (Aug. 8, 2012, 2:17 p.m.)

   I just removed it.

Looks good, assuming it still works!  Thanks!

src/cpu/base.cc (Diff revision 13)

I suggest adding this:

assert(_cpuId == oldCpu->cpuId());

1. Tony Gutierrez Aug. 9, 2012, 12:08 a.m. (Aug. 9, 2012, 12:08 a.m.)

   I added this. It still works as well as it did before (with my Simulation.py patch), i.e., I can boot the ALPHA kernel with a switch frequency of 5 billion ticks. There are still kernel panics occasionally for all ISAs. But, it's much more stable than it was before this patch.

2. Nilay Vaish Aug. 9, 2012, 2:36 a.m. (Aug. 9, 2012, 2:36 a.m.)

   Can you try with x86 as well?

3. Tony Gutierrez Aug. 9, 2012, 3:02 a.m. (Aug. 9, 2012, 3:02 a.m.)

   It was tested on x86. Doesn't break anything and makes switching more stable. The only x86 specific problem I encountered (actually the only non-ARM ISA specific problem) was the one I addressed in this changeset:
   
   http://repo.gem5.org/gem5/rev/cc47e11ccec1

Great, thanks!

If there are no contentions I think it would be good to ship this and the config patch. It makes switching somewhat usable. With this and the config that does switching out-of-the-box I think more people would start looking at this; the more people using this the better chance the remaining bugs will be fixed.