syscall_emul: [PATCH 15/22] add clone/execve for threading and multiprocess simulations
Review Request #3696 - Created Nov. 7, 2016 and submitted
Information | |
---|---|
Brandon Potter | |
gem5 | |
default | |
Reviewers | |
Default | |
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.
Description: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+750 -249) |
-
src/mem/page_table.cc (Diff revision 2) -
Ranged for can be used here.
-
src/sim/process.hh (Diff revision 2) -
These are not following the naming conventions, they should not contain underscore.
-
src/sim/process.cc (Diff revision 2) -
Does memState need to be dynamically allocated?
-
src/sim/process.cc (Diff revision 2) -
Ranged for can be used here.
-
src/sim/syscall_emul.hh (Diff revision 2) -
Should these commented lines be removed?
-
src/sim/syscall_emul.hh (Diff revision 2) -
Ranged for will work here as well.
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.
-
src/arch/x86/process.cc (Diff revision 2) -
Same here.
-
src/arch/x86/process.cc (Diff revision 2) -
Same here.
-
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.
-
src/sim/Process.py (Diff revision 2) -
Why change this?
-
src/sim/process.hh (Diff revision 2) -
Seems like this should be an assert() instead of a silent return.
-
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.
-
src/sim/process.cc (Diff revision 2) -
Unnecessary.
-
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();
-
src/sim/process.cc (Diff revision 2) -
np->argv.insert(np->argv.end(), argv.begin(), argv.end());
Seems cleaner. Same for envp.
-
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.
-
src/sim/syscall_desc.hh (Diff revision 2) -
const
-
src/sim/syscall_emul.hh (Diff revision 2) -
Seems like there should be a cleaner way to do this with a copy 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.
Description: |
|
|||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 3 (+828 -280) |
Ship It!
Ship It!