O3CPU: Idle CPU status logic revised

Information
Submitter:	Alexandru Dutu
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 10652:1dd33bc912af
---------------------------
O3CPU: Idle CPU status logic revised
This patch sets the CPU status to idle when the last active thread gets
suspended.

Testing Done

Tested ALPHA and X86 quick SE regressions. Also, tested ALPHA multi-program SMT with small applications.

It looks sensible, but I wonder if we could not somehow move all this to the BaseCPU?

Also, what do these states mean for an SMT? Is there a need for threadIdle and coreIdle?

Alexandru Dutu Jan. 21, 2015, 8:42 a.m. (Jan. 21, 2015, 8:42 a.m.)

It seems that this is a special case for cores that are supporting SMT (or codewise, have activeThreads). It also doesn't seem a noticeable performance improvement to remove 1 vtable lookup for O3CPU and InorderCPU.

With SMT we can run into contention scenarios where one of the threads is suspended and the entire CPU is put to idle. This results in deadlock, as some other threads is suppose to wake up the suspended one. I am not sure if threadIdle is descriptive enough in SMT as the pipeline can be used by a different thread. I think the current terminology of active and inactive thread is much better. The term idle for a core is relevant as it means the pipeline is not executing any instruction.

Looks fine to me, too.  However, when following the logic here, I found that this is a generally messy part of the O3 core.  I think it would make sense to (1) restructure similar to the Minor CPU core (where there is a state per context) and then (2) unify this whole handling in BaseCPU and pull it out of the respective cores.  AFAICS, this (active / idle state vs threading) should be generic enough to be pulled out, or at least be similar across the cores.

Steve Reinhardt Feb. 3, 2015, 12:22 p.m. (Feb. 3, 2015, 12:22 p.m.)

Hi Stephan, I agree with you and Andreas that (1) this whole part of the code is overly complicated and (2) it would be good to abstract as much as possible and move it to BaseCPU.  However, this is just a simple bug fix, so it would be nice to get it committed and move on, and defer the restructuring to some future date.  Does that sound OK?

Andreas Hansson Feb. 5, 2015, 12:30 a.m. (Feb. 5, 2015, 12:30 a.m.)

I'd say the current patch is fine and good to go (just update the summary keyword to cpu and make sure all regressions pass).

That said, it would be good to see a few follow-up patches addressing the aforementioned issues. Since you started down this path it seems like a natural progression.

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Status: Closed (submitted)