Clock: Move the clock and related functions to ClockedObject

Information
Submitter:	Andreas Hansson
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 9127:f00508d2c790
---------------------------
Clock: Move the clock and related functions to ClockedObject

This patch moves the clock of the CPU, bus, and numerous devices to
the new class ClockedObject, that sits in between the SimObject and
MemObject in the class hierarchy. Although there are currently a fair
amount of MemObjects that do not make use of the clock, they
potentially should do so, e.g. the caches should at some point have
the same clock as the CPU, potentially with a 1:n ratio. This patch
does not introduce any new clock objects or object hierarchies
(clusters, clock domains etc), but is still a step in the direction of
having a more structured approach clock domains.

The most contentious part of this patch is the serialisation of clocks
that some of the modules (but not all) did previously. This
serialisation should not be needed as the clock is set through the
parameters even when restoring from the checkpoint. In other words,
the state is "stored" in the Python code that creates the modules.

The nextCycle methods are also simplified and the clock phase
parameter of the CPU is removed (this could be part of a clock object
once they are introduced).

Testing Done

util/regress all passing (disregarding t1000 and eio)

Issue Summary

Description	From	Last Updated	Status
Is this really necessary? As you say in the other comment, this should all be done in python eventually anyway...	Steve Reinhardt	July 30, 2012, 4:19 a.m.	Dropped
Do we want a clock phase? I certainly wanted that when I did a similar thing. I'll send you some ...	Nathan Binkert	July 30, 2012, 4:19 a.m.	Dropped
This is likely premature optimization, but sticking a div in every clock schedule seems scary. The trick that I've used ...	Nathan Binkert	July 30, 2012, 4:19 a.m.	Resolved
So I didn't notice this until I was looking at the more recent patch with the 'tick' and 'clock' vars, ...	Steve Reinhardt	Aug. 1, 2012, 8:14 p.m.	Dropped

I love the idea of this, but I still think that this does not belong in MemObject.  I think that we should really have a ClockedObject class that derives from SimObject and that MemObject should derive from that.  There are a number of things that use a clock that aren't necessarily MemObjects that could be moved over to the ClockedObject.  Also, I have a bunch of random event queue diffs that you might be interested in.  Let me know if you want me to send them your way.

Andreas Hansson July 18, 2012, 2:32 a.m. (July 18, 2012, 2:32 a.m.)

I'm happy to add another level in the class hierarchy. It would also match closer with a future use where a ClockedObject could be e.g. wrapping a few cores and caches and merely be a level in the hierarchy.

Any objections to ClockedObject? If not I'll get to it...

Summary:

-	Clock: Move the clock and related functions to MemObject
+	Clock: Move the clock and related functions to ClockedObject

Description:

~		Changeset 9120:da3f3c760407
	~	Changeset 9127:89d2b9da0a82

~		Clock: Move the clock and related functions to MemObject
	~	Clock: Move the clock and related functions to ClockedObject

		This patch moves the clock of the CPU, bus, and numerous devices to
~		the MemObject base class. Although there are currently a fair amount
~		of MemObjects that do not make use of the clock, they potentially
~		should do so, e.g. the caches should at some point have the same clock
~		as the CPU, potentially with a 1:n ratio. This patch does not
~		introduce any new clock objects or object hierarchies, but is still a
~		step in the direction of having a more structured approach clock
~		domains.
	~	the new class ClockedObject, that sits in between the SimObject and
	~	MemObject in the class hierarchy. Although there are currently a fair
	~	amount of MemObjects that do not make use of the clock, they
	~	potentially should do so, e.g. the caches should at some point have
	~	the same clock as the CPU, potentially with a 1:n ratio. This patch
	~	does not introduce any new clock objects or object hierarchies
	~	(clusters, clock domains etc), but is still a step in the direction of
	+	having a more structured approach clock domains.

		The most contentious part of this patch is the serialisation of clocks
		that some of the modules (but not all) did previously. This
		serialisation should not be needed as the clock is set through the
		parameters even when restoring from the checkpoint. In other words,
		the state is "stored" in the Python code that creates the modules.

		The nextCycle methods are also simplified and the clock phase
		parameter of the CPU is removed (this could be part of a clock object
		once they are introduced).

Diff:

Revision 2 (+34 -120)

Show changes

	src/arch/x86/interrupts.hh
	src/arch/x86/interrupts.cc
	src/arch/x86/utility.cc
	src/cpu/BaseCPU.py
	src/cpu/base.hh
	src/cpu/base.cc
	src/cpu/testers/memtest/memtest.hh
	src/cpu/testers/networktest/networktest.hh
	19 more

You forgot to add ClockedObject.py.

Description:

~		Changeset 9127:89d2b9da0a82
	~	Changeset 9127:f00508d2c790

		Clock: Move the clock and related functions to ClockedObject

		This patch moves the clock of the CPU, bus, and numerous devices to
		the new class ClockedObject, that sits in between the SimObject and
		MemObject in the class hierarchy. Although there are currently a fair
		amount of MemObjects that do not make use of the clock, they
		potentially should do so, e.g. the caches should at some point have
		the same clock as the CPU, potentially with a 1:n ratio. This patch
		does not introduce any new clock objects or object hierarchies
		(clusters, clock domains etc), but is still a step in the direction of
		having a more structured approach clock domains.

		The most contentious part of this patch is the serialisation of clocks
		that some of the modules (but not all) did previously. This
		serialisation should not be needed as the clock is set through the
		parameters even when restoring from the checkpoint. In other words,
		the state is "stored" in the Python code that creates the modules.

		The nextCycle methods are also simplified and the clock phase
		parameter of the CPU is removed (this could be part of a clock object
		once they are introduced).

Diff:

Revision 3 (+197 -120)

Show changes

	src/arch/x86/interrupts.hh
	src/arch/x86/interrupts.cc
	src/arch/x86/utility.cc
	src/cpu/BaseCPU.py
	src/cpu/base.hh
	src/cpu/base.cc
	src/cpu/testers/memtest/memtest.hh
	src/cpu/testers/networktest/networktest.hh
	21 more

I agree with your removal of the serialization code, but I think there are important caveats.  On one hand, it makes sense that I should be able to take a checkpoint with a 1GHz CPU and restore that to a 2GHz CPU (just like I can transition from a simple CPU to O3 via a checkpoint).  On the other hand, we will need to be very careful that state derived from the clock (e.g., the next event time, or some latency values) is also recalculated and not serialized.  I don't know that there's anything to do about it at this point; this is just yet another category of potential bugs to keep an eye out for.

Nathan Binkert July 19, 2012, 4:51 a.m. (July 19, 2012, 4:51 a.m.)

Along those lines, what about dynamic frequency scaling and such?  It's a rare use case, but certainly will become more common.  Seems like we need an option to checkpoint values, but be able to override them in the configuration.  Then again, perhaps nobody will be doing DVFS before the checkpoint is dropped.

Steve Reinhardt July 19, 2012, 5:47 a.m. (July 19, 2012, 5:47 a.m.)

Actually I think DVFS will make checkpointing clock values less necessary... basically as the rest of the code gets used to the idea of having the clock change out from under it, then it will naturally also deal with having the clock change because you unserialized from a checkpoint that was taken with different clock values.

The only thing to remember is that if we end up having a method that needs to be called when the clock changes to keep other state in sync, then we should also call those after an unserialization.  (Or if we want to be really conservative, we could serialize the clock values in ClockedObject, and only call the method if the configure value is different from the serialized value.)  But this is all future work...

Nathan Binkert July 19, 2012, 5:55 a.m. (July 19, 2012, 5:55 a.m.)

Well, the kernel needs to know (since it controls the frequency and thinks it knows what's going on.)  Being in the situation where the kernel thinks you're at one frequency, but in reality, you're running at another can cause problems if you're not careful.

Steve Reinhardt July 19, 2012, 6:12 a.m. (July 19, 2012, 6:12 a.m.)

Does the kernel get notified when the frequency changes, or does it read it out of a register when it needs to know?  The latter is easy, of course.

Nathan Binkert July 19, 2012, 6:28 a.m. (July 19, 2012, 6:28 a.m.)

Well, in normal operation (i.e. when you're not overheating), the kernel tells the processor when to slow down.  This, I think, would be the typical DVS scenario.  In the exceptional case, I think that the hardware will drop the clock (but not the voltage?) and then interrupt the processor which will then check a register.  It's been a long time and I'm pretty fuzzy, but I think this is how it all works.  In my case, I was actually using Alpha and it was definitely kernel telling system what to do through a special instruction.  I can't seem to find the diffs though.  It was all a big hack as I recall.

src/arch/x86/interrupts.cc (Diff revision 3)

Is this really necessary?  As you say in the other comment, this should all be done in python eventually anyway...

Show all issues

Andreas Hansson July 19, 2012, 6:52 a.m. (July 19, 2012, 6:52 a.m.)

I am just making it equal to the behaviour it had previously. There are plenty places where the code checks if clock == 0, and I don't know: 1) When that would happen, 2) what the impact is.

At this point I did not want to change any behaviour.

src/sim/ClockedObject.py (Diff revision 3)

Do we want a clock phase?  I certainly wanted that when I did a similar thing.  I'll send you some code that manages that.  It could be pushed off to a future changeset.

Show all issues

Steve Reinhardt July 19, 2012, 6:11 a.m. (July 19, 2012, 6:11 a.m.)

It looked like the phase code was there in the CPU (but not in other objects) and was unused.  So it seemed reasonable to me to leave it out until someone really needed it again.  Can you elaborate on why you found it necessary?

Andreas Hansson July 19, 2012, 6:51 a.m. (July 19, 2012, 6:51 a.m.)

My observation was also: not used anywhere, easy to add back if we want, so prune

Nathan Binkert July 19, 2012, 7:39 a.m. (July 19, 2012, 7:39 a.m.)

Hmmm.  It was pretty critical for me when doing network simulations, but perhaps that is just and oddball use case.

src/sim/clocked_object.hh (Diff revision 3)

This is likely premature optimization, but sticking a div in every clock schedule seems scary.  The trick that I've used in the past was to save the clock tick and check to see if curtick is == to the saved tick.  If so, you can just do an add and almost all the time, the condition will pass.  Again, perhaps over optimized.  I'll send you the code.

Show all issues

Andreas Hansson July 19, 2012, 6:51 a.m. (July 19, 2012, 6:51 a.m.)

I guess it all boils down to how it is used. Ultimately gem5 should be event based, not a collection of cycle callable models. As such, I would hope it doesn't get called every cycle. I have not stats to back that up though, so it could be that you're absolutely right.

At this point, I merely wanted to tidy up what was there...that did a modulo operation for every call.

Nathan Binkert July 19, 2012, 7:24 a.m. (July 19, 2012, 7:24 a.m.)

Sure it's event based, but most things get clocked every cycle for many cycles and then go idle for a period of time and then do it again.  (Think CPUs, caches).  In this case, the addition works except from waking after idle.  When that happens, the condition fails, and then you do the divide.

Andreas Hansson July 19, 2012, 7:34 a.m. (July 19, 2012, 7:34 a.m.)

I can give it a bash and compare the performance. Perhaps as a follow on patch or you want to see it as part of this one?

Nathan Binkert July 19, 2012, 7:39 a.m. (July 19, 2012, 7:39 a.m.)

As a follow-on is fine with me.  I just get concerned with event queue performance because in certain situations the performance can be particularly bad.  As I recall, AMD had some use of a really old version of m5 (that ali did for them) that really stressed the event queue and ran into similar problems.  If we ever get the network model in ruby moved over to our event queue, and people run network only experiments, you'll certainly see performance issues.

Andreas Hansson July 19, 2012, 7:57 a.m. (July 19, 2012, 7:57 a.m.)

Are you concerned how this performs compared to what it used to be (see below)? If so I can change it back. If the issue is more forward looking I'd prefer to keep it for a future patch.

Tick	
BaseCPU::nextCycle(Tick begin_tick)	
{
    Tick next_tick = begin_tick;	
    if (next_tick % clock != 0)
        next_tick = next_tick - (next_tick % clock) + clock;
    next_tick += phase;
    assert(next_tick >= curTick());
    return next_tick;
}

Nathan Binkert July 19, 2012, 8:04 a.m. (July 19, 2012, 8:04 a.m.)

Oh, I didn't realize that the current version had a modulo.  Yeah, you can do it in a future patch.  I'm mostly concerned that this starts getting used for objects/events that are much shorter lived such that the event queue time starts to dominate.  I thought that there was an example of what I was talking about in the tree, but after looking, it must have been in my HP stuff or got removed.

src/sim/clocked_object.hh (Diff revision 3)

I'm ok with this, but it would be nice to know if we're leaving some performance on the table by doing this. I imagine atomic simple cpu would be the best test case.

Andreas Hansson July 24, 2012, 5:47 p.m. (July 24, 2012, 5:47 p.m.)

In what sense, compared to what exactly? This should already be an improvement compared to the CPU code that was there (at least so I would imagine).

src/sim/clocked_object.hh (Diff revision 3)

So I didn't notice this until I was looking at the more recent patch with the 'tick' and 'clock' vars, but doesn't it seem like, if curTick() is currently on a clock edge, that nextCycle() would return the curTick()+clock and not curTick()?

I'm assuming that it's doing this for compatibility with the existing CPU nextCycle() methods, but is this a good time to address this (if people agree that it's a bug, or at least confusing)?

Show all issues

Andreas Hansson July 29, 2012, 5:58 p.m. (July 29, 2012, 5:58 p.m.)

It is indeed kept this way for backwards compatibility. I'll put it on the list of clock-related issues to fix :). My hope was to first tidy all these things up, and then have one patch that actually sorts out the oddities and bumps the stats.

Hence, I'd like to make this change, and then in a later patch change the behaviour and then bump the stats. Sounds reasonable?

Steve Reinhardt July 30, 2012, 4:08 a.m. (July 30, 2012, 4:08 a.m.)

I agree with making it a two-step transition.  I was thinking of whether there was something that should be done here in terms of naming to make that transition easier, but looking more closely, I agree that this is the best first step.

Ship It!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Summary:

Description:

Changeset 9120:da3f3c760407

Changeset 9127:89d2b9da0a82

Diff:

Description:

Changeset 9127:89d2b9da0a82

Changeset 9127:f00508d2c790

Diff:

Status: Closed (submitted)