syscall_emul: [PATCH 15/22] add clone/execve for threading and multiprocess simulations

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

Description

Changeset 11879:5e2d3ce3a08c
---------------------------
syscall_emul: [PATCH 15/22] add clone/execve for threading and multiprocess simulations

Modifies the clone system call and adds execve system call. Requires allowing
processes to steal thread contexts from other processes in the same system
object and the ability to detach pieces of process state (such as MemState)
to allow dynamic sharing.

Testing Done

Issue Summary

Description	From	Last Updated	Status
Ranged for can be used here.	Alexandru Dutu	Nov. 21, 2016, 1:11 p.m.	Resolved
These are not following the naming conventions, they should not contain underscore.	Alexandru Dutu	Nov. 21, 2016, 3:30 p.m.	Resolved
Does memState need to be dynamically allocated?	Alexandru Dutu	Nov. 23, 2016, 9:56 a.m.	Dropped
Ranged for can be used here.	Alexandru Dutu	Nov. 23, 2016, 11:16 a.m.	Resolved
Should these commented lines be removed?	Alexandru Dutu	Nov. 23, 2016, 11:16 a.m.	Dropped
Ranged for will work here as well.	Alexandru Dutu	Nov. 23, 2016, 11:23 a.m.	Dropped
Implementing assigment operator would be nice.	Michael LeBeane	Jan. 23, 2017, 1:20 p.m.	Resolved
Same here.	Michael LeBeane	Jan. 23, 2017, 1:20 p.m.	Resolved
Same here.	Michael LeBeane	Jan. 23, 2017, 1:28 p.m.	Resolved
Name is a bit weird IMO. getMappings make it seem like this is a getter, which it isn't. Maybe something ...	Michael LeBeane	Jan. 25, 2017, 9:38 a.m.	Dropped
Why change this?	Michael LeBeane	Feb. 2, 2017, 10:09 a.m.	Resolved
Seems like this should be an assert() instead of a silent return.	Michael LeBeane	Jan. 23, 2017, 11:15 a.m.	Resolved
I prefer shared_pointer here. Also could use some comments explaining what these fields are and that they can be shared ...	Michael LeBeane	Jan. 23, 2017, 11:48 a.m.	Dropped
Unnecessary.	Michael LeBeane	Jan. 23, 2017, 11:41 a.m.	Resolved
Looking at how you use this, maybe the right thing to do is to just make getMappings an actual getter: ...	Michael LeBeane	Jan. 25, 2017, 9:38 a.m.	Dropped
np->argv.insert(np->argv.end(), argv.begin(), argv.end()); Seems cleaner. Same for envp.	Michael LeBeane	Jan. 25, 2017, 9:42 a.m.	Resolved
What about the other variables you introduce, such as exitGroup, sigchld, maxStackSize, etc? Does checkpointing even work? If not, there ...	Michael LeBeane	Jan. 23, 2017, 11:44 a.m.	Dropped
const	Michael LeBeane	Jan. 23, 2017, 11:45 a.m.	Resolved
Seems like there should be a cleaner way to do this with a copy constructor.	Michael LeBeane	Feb. 17, 2017, 6:55 p.m.	Dropped
In general, there is too much duplication between this function and cloneFunc. For example, some the ProcessParams stuff seems to ...	Michael LeBeane	Feb. 17, 2017, 6:55 p.m.	Dropped

Description:

~		Changeset 11704:00370b5d6887
	~	Changeset 11722:6fea3eedd891

		syscall_emul: [PATCH 15/22] add clone/execve for threading and multiprocess simulations

		Modifies the clone system call and adds execve system call. Requires allowing
		processes to steal thread contexts from other processes in the same system
		object and the ability to detach pieces of process state (such as MemState)
		to allow dynamic sharing.

Diff:

Revision 2 (+750 -249)

Show changes

	src/arch/arm/process.cc
	src/arch/arm/linux/process.cc
	src/arch/generic/types.hh
	src/arch/mips/process.cc
	src/arch/power/process.cc
	src/arch/sparc/process.hh
	src/arch/sparc/process.cc
	src/arch/sparc/linux/syscalls.cc
	19 more

src/mem/page_table.cc (Diff revision 2)

Ranged for can be used here.

Show all issues

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

These are not following the naming conventions, they should not contain underscore.

Show all issues

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

Does memState need to be dynamically allocated?

Show all issues

Brandon Potter Nov. 23, 2016, 11:23 a.m. (Nov. 23, 2016, 11:23 a.m.)

Yes, the memState need to be able to be dynamically created and shared between Processes to simulate threads.

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

Ranged for can be used here.

Show all issues

src/sim/syscall_emul.hh (Diff revision 2)

Should these commented lines be removed?

Show all issues

Brandon Potter Nov. 23, 2016, 11:23 a.m. (Nov. 23, 2016, 11:23 a.m.)

I'm leaving them in place in case someone wants to implement this for sparc. The registers don't show up in the header file for sparc and I had a bit of trouble the compiler to not complain about them.

Also, I only put effort in getting the pthread support to work for x86. The other ISAs still need work, but the code should compile for all of them now.

src/sim/syscall_emul.hh (Diff revision 2)

Ranged for will work here as well.

Show all issues

Brandon Potter Nov. 23, 2016, 11:23 a.m. (Nov. 23, 2016, 11:23 a.m.)
```
fds is not an iterable class
```

In general, I think this patch could be improved by making better use of copy constructors and overloaded assignment operators. I can review again in more detail once that has been addressed.

src/arch/x86/process.cc (Diff revision 2)

Implementing assigment operator would be nice.

Show all issues

src/arch/x86/process.cc (Diff revision 2)

Same here.

Show all issues

Brandon Potter Feb. 17, 2017, 6:58 p.m. (Feb. 17, 2017, 6:58 p.m.)

This isn't really related to this patch so I'll create a new patch for this copy constructor.

src/arch/x86/process.cc (Diff revision 2)

Same here.

Show all issues

src/cpu/thread_state.hh (Diff revision 2)

What is this proxy stuff? Some comments would help.

src/mem/page_table.cc (Diff revision 2)

Name is a bit weird IMO. getMappings make it seem like this is a getter, which it isn't. Maybe something like appendMappings? Also should be const.

Show all issues

src/sim/Process.py (Diff revision 2)

Why change this?

Show all issues

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

Seems like this should be an assert() instead of a silent return.

Show all issues

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

I prefer shared_pointer here. Also could use some comments explaining what these fields are and that they can be shared accross different processes.

Show all issues

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

Unnecessary.

Show all issues

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

Looking at how you use this, maybe the right thing to do is to just make getMappings an actual getter:
MapVec mappings = pTable->getMappings();

Show all issues

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

np->argv.insert(np->argv.end(), argv.begin(), argv.end());
Seems cleaner.  Same for envp.

Show all issues

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

Should we panic here?

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

What about the other variables you introduce, such as exitGroup, sigchld, maxStackSize, etc? Does checkpointing even work? If not, there should be a warning stating what is not supported.

Show all issues

src/sim/syscall_desc.hh (Diff revision 2)

const

Show all issues

src/sim/syscall_emul.hh (Diff revision 2)

Seems like there should be a cleaner way to do this with a copy constructor.

Show all issues

Brandon Potter Feb. 17, 2017, 6:58 p.m. (Feb. 17, 2017, 6:58 p.m.)

ProcessParams is defined by swig and I do not know how to generate a create function with parameters. I looked through the other created Params inside build/ARCH/params and I didn't see any instance where the params were created with anything other than an empty constructor.

src/sim/syscall_emul.hh (Diff revision 2)

In general, there is too much duplication between this function and cloneFunc.  For example, some the ProcessParams stuff seems to be copied.  This should be factored out a bit better.

Show all issues

Description:

~		Changeset 11722:6fea3eedd891
	~	Changeset 11879:5e2d3ce3a08c

		syscall_emul: [PATCH 15/22] add clone/execve for threading and multiprocess simulations

		Modifies the clone system call and adds execve system call. Requires allowing
		processes to steal thread contexts from other processes in the same system
		object and the ability to detach pieces of process state (such as MemState)
		to allow dynamic sharing.

Diff:

Revision 3 (+828 -280)

Show changes

	src/arch/alpha/process.cc
	src/arch/alpha/linux/process.cc
	src/arch/arm/process.cc
	src/arch/arm/linux/process.cc
	src/arch/generic/types.hh
	src/arch/mips/process.cc
	src/arch/power/process.cc
	src/arch/riscv/process.cc
	24 more

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

Description:

Changeset 11704:00370b5d6887

Changeset 11722:6fea3eedd891

Diff:

Description:

Changeset 11722:6fea3eedd891

Changeset 11879:5e2d3ce3a08c

Diff:

Status: Closed (submitted)