syscall_emul: [patch 4/22] remove redundant M5_pid field from process

Information
Submitter:	Brandon Potter
Repository:	gem5
Branch:	default
Bugs:
Depends On:
Reviewers
Groups:	Default
People:

Description

Changeset 11692:ba60e4ae816f
---------------------------
syscall_emul: [patch 4/22] remove redundant M5_pid field from process

Testing Done

Issue Summary

Description	From	Last Updated	Status
Same goes for unserialization.	Alexandru Dutu	Oct. 18, 2016, 11 a.m.	Dropped

With this patch, if you run multiple processes in SE mode they all have the same PID (100) by default. Will this cause issues? I haven't tested a pthreads application, but that seems like it might cause problems.

Would you just suggest that we make sure to assign different PIDs in the Python config file (where we create the Process object) to get around this issue? If so, we should have a fatal if there are multiple Processes with the same PID.

Brandon Potter Oct. 17, 2016, 7:35 a.m. (Oct. 17, 2016, 7:35 a.m.)

I have another patch that I haven't posted yet that used to rely on the incrementing PID that I deleted here. I haven't figured out how I want to address the issue yet. The two obvious ways are to make the changes in Python or make the changes in C++ and use the counter to increment the value when new Process classes are created. I think that I might have to use the incrementing version with clone and I might add that functionality back in with C++, but I'll increment the _pid field that you see here in this patch rather than having nextPID, M5_pid, and _pid. We don't need several versions of the same thing to make it work.

Jason Lowe-Power Oct. 17, 2016, 7:41 a.m. (Oct. 17, 2016, 7:41 a.m.)

One option from the Python side is to use a static class variable in Process.py to automatically assign it. I think there's an example of this in configs/topologies/Cluster.py (and other places, too).

Tony Gutierrez Oct. 17, 2016, 10:05 a.m. (Oct. 17, 2016, 10:05 a.m.)

If it is going to be auto-assigned, I think it would be better to add the static variable in the C++. I see no need to expose auto-assigned vars in the python configs, unless there are cases where users would need to assign a specific PID to a process?

Summary:

-	syscall_emul: remove redundant M5_pid field from process class
+	syscall_emul: [patch 4/22] remove redundant M5_pid field from process

Description:

~		Changeset 11692:b3d8b57ab6a7
	~	Changeset 11692:ba60e4ae816f

~		syscall_emul: remove redundant M5_pid field from process class
	~	syscall_emul: [patch 4/22] remove redundant M5_pid field from process

Diff:

Revision 2 (+35 -50)

Show changes

	src/arch/alpha/process.cc
	src/arch/sparc/faults.cc
	src/sim/Process.py
	src/sim/process.hh
	src/sim/process.cc
	src/sim/system.hh
	src/sim/system.cc

src/sim/process.cc (Diff revision 2)

Even if we are serializing nextPid in the System object, shouldn't we still serialize _pid? We might need to have the same pids restored and not reassign different ones.

src/sim/process.cc (Diff revision 2)

Same goes for unserialization.

Show all issues

Brandon Potter Oct. 18, 2016, 11:01 a.m. (Oct. 18, 2016, 11:01 a.m.)

Thanks for pointing this out, but there are more fields than this missing from the checkpoints. The checkpoint support has been broken in SE for a while now so I doubt that anyone actually uses it. If they have, I'd like to know about it. (As an aside, we need to bake the checkpoint testing into the regressions if we want to keep support for them.)

Checkpoints are hard to support with some of the stuff that's getting added. (I'd appreciate it if I could get the go ahead to completely break it (well, more so than already) in the near term.) Need to discuss it more generally with a separate thread on gem5-dev to cover the complications and get feedback on how to handle them.

I agree that it's dumb to have two PIDs in the Process object, but until we figure out how we really want to fix the auto-assignment thing, it's probably premature to push this patch IMO. Let's wait until we have a full solution.

I'd be fine with doing something in python rather than C++; the way I see it, we already assign default values in python, so adding code to assign unique PIDs is really just a "smart default". That said, strictly speaking the Process class is not the best place for this, since really you'd want separate next-PID counters per System object. Making that happen automagically would be tricky though, and even the tricky solutions I can think of would be fragile in terms of perhaps swithcing around PID assignments based on unrelated config script changes. So off the top of my head, here's a proposal:

We continue to provide a fixed default PID in python to keep things simple for single-process runs.
For configs where the user is creating multiple Process objects per System, it's up to the user to manually assign unique PIDs. I think this is a good solution because it's an easy thing for users to do (just implement their own local counter in the python script), and yet is trivial from our side too. If we really wanted to, we could provide a get_next_pid() method on the Python System object, but I think that's overkill.
The System object needs to track all the PIDs of all the Process objects that are assigned to it at config time, and (1) call fatal() if the user builds a config with non-unique PIDs (with a helpful error message of course), and (2) do something to figure out how it can continue to assign unique PIDs when clone() is called. It might be possible to do #1 in python, but it's probably easier and more robust to do it in C++. Part 2 definitely needs to be on the C++ side, and could be as complicated as keeping a map of which PIDs are used, or as simple as just tracking the max manually assigned PID and then incrementing from there.

Brandon Potter Jan. 16, 2017, 10:49 a.m. (Jan. 16, 2017, 10:49 a.m.)

I implemented Steve's recommendations. The implementation went into the clone/exec patch which I will repost with the update.

There is a default value for PID set to 1000. The configs allow a PID to be specified and there is a check inside the Process constructor which accesses the System object and into a PID set to check if the PID is already used; the constructor issues a fatal is there is a PID collision. Within the clone system call, there is a mechanism that incrementally checks through the PID set and increments through it until it finds a free PID.

Ship It!

Ship It!

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 11692:b3d8b57ab6a7

Changeset 11692:ba60e4ae816f

Diff:

Status: Closed (submitted)