Skip to content

Conversation

@geoweb999
Copy link

@geoweb999 geoweb999 commented Sep 24, 2024

Issue Link #51

Implemented:

1. kernel level changes

  • sys_getprocs -- new system call to support ps command
  • getprocs(pd, max) -- new system call to support ps command where pd is a proc_data array and max is number of processes to report (see man page)

2. user level changes

  • ps -- new user space command that implements a traditional unix process status of running processes
  • usertests -- added 4 test cases to xv6 regression test suite

3. documentation changes

  • docs/ps.md -- man page in markdown format
  • docs/getprocs.md -- man page in markdown format

Implementation details

  • getprocs is implemented as system call 22 in the appropriate kernel and user files
  • the only args support in this implementation is "-h" which provides minimal help

How to test
Note: automated testing is incomplete due to lack of shell scripting support in this release of xv6

  1. run "ps" in xv6: Note, repeated executions of 'ps' should result in the 3rd PID increasing:
$ ps
3 processes are running
PID	PPID	STATE	SIZE	NAME
1	0	sleep 	16384	init
2	1	sleep 	20480	sh
3	2	run   	16384	ps
$ ps
3 processes are running
PID	PPID	STATE	SIZE	NAME
1	0	sleep 	16384	init
2	1	sleep 	20480	sh
4	2	run   	16384	ps
  1. run the regression suite: /usertests. Note this process runs somewhat slowly.
$ /usertests getprocstest
usertests starting
test getprocstest: getprocstest: getprocs test1 returned 4 procs
getprocstest: getprocs test2 returned 1 procs
getprocstest: getprocs test3 returned 4 procs
getprocstest: getprocs test OK
OK
ALL TESTS PASSED
  • test1: run getprocs with default params: expect 4 procs
  • test2: run getprocs with max=1: expect 1 proc
  • test3: run getprocs with max=NPROC+1 (exceed max value): expect 4 procs
  • test4: run getprocs with invalid max, expect error

@mikesmith-23
Copy link

I would like to code review this.

@abkslm
Copy link

abkslm commented Sep 27, 2024

I'll review this :)

@mikesmith-23
Copy link

mikesmith-23 commented Oct 3, 2024

High-Level Checks:

[✅] Does the PR fully resolve the task/issue?
Yes! This project aligns perfectly with the initial proposal. It successfully implements the getprocs system call and the ps command, including support for the -h flag, as specified.

[✅] Are there tests included?
Yes! In addition to allowing the ps command to be tested manually in the OS, the PR includes a built-in regression test suite to automate testing of the getprocs system call. This provides thorough coverage and ensures the system functions as expected.

[✅] Does the code follow project formatting standards?
Yes! The modified and added files maintain cohesive and legible formatting. Comments are provided throughout, making the code easier to understand. However, I did notice that the Javadoc-style header for the sys_getprocs function is placed within the function body—it might be better positioned above the function for consistency with traditional formatting.

[✅] Does the OS compile and run successfully?
Yes! The OS boots up without issues after running make qemu, and everything functions as expected, without any errors or crashes during testing.

[✅] Is documentation provided?
Yes! The PR provides a concise summary of the code’s functionality, and there are two well-written man pages (getprocs.md and ps.md) in the documentation folder, which explain the commands in detail.

Code Checks:

[✅] Does the code accomplish its intended purpose?
Yes! The ps command provides an accurate and clear view of the active processes, displaying information such as PID, parent PID, state, memory size, and the name of the process, as intended. It’s a useful addition to the OS.

[✅] Were all edge cases considered?
Yes! The project feels thorough and well-rounded. The system call and command handle various scenarios effectively, though one potential improvement could be adding more interactivity. It would be great if users had more control over how information is displayed or filtered (e.g., more flags for sorting or filtering processes).

[✅] Is the code well-organized and maintainable?
Yes! The changes are well-structured, and the code integrates seamlessly with the existing OS framework. The code is well-documented, making it easy to follow, and the file organization is intuitive. Overall, it demonstrates good maintainability.

[✅] Does the code maintain performance?
Yes! While there is a minor delay when running the automated tests (as Geoff mentioned), the wait time is quite reasonable. During my own tests, the provided test cases ran in about 23 seconds on my laptop, which seems acceptable given the scope of the project.

@malensek
Copy link
Contributor

Great idea for a P1-scope project, excellent execution. Your documentation is well-written, code is clean, concise, and well-documented, and overall... it's hard to find anything to complain about. Printing to stderr for error messages? (I guess I could complain about readability with respect to using the ternary operator, but you could probably just tell me to "get good" and that'd be enough). Great work!

You acknowledge this in your code doc, but one small issue is the amount of data that will need to be transferred out to user space due to allocating space for the entire process table (mostly empty in our use cases). On Linux, this is a non-issue because this information is reported via procfs instead. On the BSDs, they usually have a system call similar to yours but dynamically resize the process, copy out the exact amount of necessary data, copy out its size (as an "out arg"), and then update the pointer it was given to point at the freshly-allocated data. Too much work for P1. :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants