dev: Add m5 op to toggle synchronization for dist-gem5.

Information
Submitter:	Michael LeBeane
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11561:1414a40eb1e2
---------------------------
dev: Add m5 op to toggle synchronization for dist-gem5.
This patch adds the ability for an application to request dist-gem5 to begin/
end synchronization using an m5 op.  When toggling on sync, all nodes agree on
the next sync point based on the maximum of all nodes' ticks.  CPUs are
suspended until the sync point to avoid sending network messages until sync has
been enabled.  Toggling off sync acts like a global execution barrier, where
all CPUs are disabled until every node reaches the toggle off point.  This
avoids tricky situations such as one node hitting a toggle off followed by a
toggle on before the other nodes hit the first toggle off.

Testing Done

Issue Summary

Description	From	Last Updated	Status
The type should be bool. Minor nit: the name could be changed to something like "syncStartOnPseudoOp" to reflect the meaning ...	Gabor Dozsa	Oct. 12, 2016, 8:50 a.m.	Resolved
Minor nit: could you rename this method to "toggleSync()" or similar? That would better describe the functionality. The word "switch" ...	Gabor Dozsa	Oct. 12, 2016, 8:50 a.m.	Resolved
This change effectively disables the use of the start_tick parameter. It is because 'nextAt' is overwritten in SyncEvent::start() by the ...	Gabor Dozsa	Oct. 12, 2016, 8:50 a.m.	Dropped
Cannot another active thread hit another stop-sync pesudo op while this thread is still suspended? Maybe all active threads should ...	Gabor Dozsa	Oct. 12, 2016, 8:50 a.m.	Resolved

Any comments on this patch?

Mohammad Alian Sept. 2, 2016, 9:27 p.m. (Sept. 2, 2016, 9:27 p.m.)

I just have some high level question before goring through the code:

Can you explain the use-case for this patch? Probably it means to speed up the simulation but why do we need to switch on/off synchronization at some points?

When you suspend CPUs, then how your simulation will gaurantee forward progress? Doesn't this lead to dead-lock in some situations?

Thanks.

Michael LeBeane Sept. 13, 2016, 11:48 a.m. (Sept. 13, 2016, 11:48 a.m.)

The use case is, like you inferred, to speed up simulation when you don't need to synchronize. There is currently a method to support this using ticks, but I prefer to use source code annotations.

The CPUs suspend until the largest tick of all participating CPUs is reached. This is to ensure that we really have started synchronizing after executing the pseudo-op, else we could start sending network messages before we are ready. I don't see a deadlock situation here, can you elaborate on your concern?

I like the idea of starting the sync on a pseudo op very much. I cannot see the usefulness of stopping the sync after it started though. Do you have a use case in mind?

I have a few comments below (mainly minor nits).

However, I think there is also a basic issue with the CPU suspend approach. A CPU can wake up whenever there is an interrupt or even a snoop request. That should be taken care of somehow I assume.

Michael LeBeane Sept. 13, 2016, 11:48 a.m. (Sept. 13, 2016, 11:48 a.m.)

The use case for switching off would be if you have multiple regions of interest in the code and no need to sync in between. You could also just run the simulation a bunch of times and move the start sync point, but I think it’s much cleaner to run the app once and toggle the sync.

I suppose a spurious wakeup is possible; I had not really thought about it. I haven't seen CPUs wakeup in practice, but I don't really have any devices that would generate an interrupt/snoop when all the cores are asleep.

One alternative way to do this would be to not suspend any of the cores and modify the sync to support all the devices being on different ticks when sync starts. We would need to remember what the start time was for all the participating nodes and use that time to enforce synchronization instead of assuming everyone is on the same tick. I avoided doing this since suspending the CPUs until we reach the max of all nodes ticks' was easier.

Michael LeBeane Oct. 3, 2016, 8:47 a.m. (Oct. 3, 2016, 8:47 a.m.)

Before I address your specific review comments, I think it would be good to resolve the high-level concern about suspending CPUs. Do you have any thoughts about the alternative approach I suggested? I'm not a huge fan of it since having all the nodes on different ticks makes correlating traces between nodes very difficult. Did you have a better approach in mind?

Gabor Dozsa Oct. 6, 2016, 3:04 a.m. (Oct. 6, 2016, 3:04 a.m.)

I agree that the alternative approach would make it difficult to correlate events between nodes. I would suggest to go ahead with your current CPU suspend/wakup approach for now. Would it be possible to issue a warning in case of a spurious wakeup event? If something goes wrong the warning could give a hint.

src/dev/net/dist_iface.hh (Diff revision 1)

The type should be bool.

Minor nit: the name could be changed to something like "syncStartOnPseudoOp" to reflect the meaning better.

Show all issues

src/dev/net/dist_iface.hh (Diff revision 1)

Minor nit: could you rename this method to "toggleSync()" or similar? That would better describe the functionality. The word "switch" ususally refers to the network switch model in the dist-gem5 context so it is a bit confusing here.

Show all issues

src/dev/net/dist_iface.cc (Diff revision 1)

This change effectively disables the use of the start_tick parameter. It is because 'nextAt' is overwritten in SyncEvent::start() by the max tick of all participating gem5 - which will be 0 at the first global barrier (if we do not start the sync on pseudo op).

However, I think the pseudo op is a much better way to defer the sync start. Could you just remove the 'start_tick' option altogether?

Show all issues

Michael LeBeane Oct. 12, 2016, 8:50 a.m. (Oct. 12, 2016, 8:50 a.m.)

I did a quick test and the start_tick approach still appears to work with this patch.

nextAt is assigned the value of start_tick for all the nodes and the switch in Synch::init(). SyncSwitch::progress() only updates nextAt(which determines the max tick of all paticipating processes) if it is less than send_tick. At time zero this is not true, so the first global barrier will be scheduled at start_tick.

src/dev/net/dist_iface.cc (Diff revision 1)

Cannot another active thread hit another stop-sync pesudo op while this thread is still suspended? Maybe all active threads should be suspended here?

Show all issues

Diff:

Revision 2 (+193 -37)

Show changes

	configs/common/Options.py
	src/arch/x86/isa/decoder/two_byte_opcodes.isa
	src/dev/net/Ethernet.py
	src/dev/net/dist_etherlink.cc
	src/dev/net/dist_iface.hh
	src/dev/net/dist_iface.cc
	src/dev/net/dist_packet.hh
	src/dev/net/tcp_iface.hh
	6 more

Ship it! Thanks for the fixes!

You have a pending review.

Review Board 2.0.15

This change has been marked as submitted.

Screenshots

Files

Issue Summary

Diff:

Status: Closed (submitted)