Imperial-MEng/pintos_22
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Implement process_wait #31

Merged
gk1623 merged 9 commits from process-wait into master 2024-11-11 22:56:29 +00:00
Conversation 16 Commits 9 Files Changed 6
+127 -9
gk1623 commented 2024-11-08 09:24:20 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

Changes:

  • Separate struct process_result allows a thread's page to be freed while keeping the result available for the parent (if still alive).
    • This then needs to be freed by either parent or child, whoever dies last (in process_exit).
  • Implement syscall_exec to make some tests work.

TODO:

  • process_result.sema and process_result.exit_status are accessed by potentially multiple threads. We need to synchronise this; however, I don't think locks nor semaphores would work for this due to being too high-level. Lmk any ideas (other than disabling interrupts).
  • I need a sanity check about my "child_results is guaranteed to be in-order" comment please 😅 .
  • There's a deadlock in at least exec-multiple.
Changes: - Separate `struct process_result` allows a thread's page to be freed while keeping the result available for the parent (if still alive). - This then needs to be freed by either parent or child, whoever dies last (in `process_exit`). - Implement `syscall_exec` to make some tests work. TODO: - [x] `process_result.sema` and `process_result.exit_status` are accessed by potentially multiple threads. We need to synchronise this; however, I don't think locks nor semaphores would work for this due to being too high-level. Lmk any ideas (other than disabling interrupts). - [x] I need a sanity check about my "child_results is guaranteed to be in-order" comment please :sweat_smile: . - [x] There's a deadlock in _at least_ `exec-multiple`.
gk1623 commented 2024-11-08 09:24:21 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

requested review from @td1223

requested review from @td1223
gk1623 commented 2024-11-08 09:24:37 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

changed the description

changed the description
gk1623 commented 2024-11-08 10:50:17 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

added 1 commit

  • 6ed1ccd2 - Fix process_result locking by acquiring in process_wait as well to prevent freeing memory too early

Compare with previous version

added 1 commit <ul><li>6ed1ccd2 - Fix process_result locking by acquiring in process_wait as well to prevent freeing memory too early</li></ul> [Compare with previous version](/lab2425_autumn/pintos_22/-/merge_requests/31/diffs?diff_id=137979&start_sha=84fe637c7e284dd8dab7e2a7ce036857900e1926)
gk1623 commented 2024-11-08 10:50:57 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

marked the checklist item process_result.sema and process_result.exit_status are accessed by potentially multiple threads. We need to synchronise this; however, I don't think locks nor semaphores would work for this due to being too high-level. Lmk any ideas (other than disabling interrupts). as completed

marked the checklist item **`process_result.sema` and `process_result.exit_status` are accessed by potentially multiple threads. We need to synchronise this; however, I don't think locks nor semaphores would work for this due to being too high-level. Lmk any ideas (other than disabling interrupts).** as completed
gk1623 commented 2024-11-08 10:52:23 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

Okay so I've managed to get locks almost to work. Turns out I just needed them in process_wait to make sure I don't free memory too early.

However, I am now getting a deadlock in one of the tests (exec-multiple). I suspect this might have something to do with priority donation not working 100% properly, but still need to debug further.

Okay so I've managed to get locks almost to work. Turns out I just needed them in `process_wait` to make sure I don't free memory too early. However, I am now getting a deadlock in one of the tests (exec-multiple). I suspect this might have something to do with priority donation not working 100% properly, but still need to debug further.
gk1623 commented 2024-11-08 13:06:51 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

changed the description

changed the description
sb3923 commented 2024-11-08 13:32:34 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

Feel free to revert back to the first few commits of master and try it out. I can't see any deadlock scenario but it is possible.

Feel free to revert back to the first few commits of master and try it out. I can't see any deadlock scenario but it is possible.
gk1623 commented 2024-11-08 14:00:57 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

I’ll give it ago with latest master when I get home, but notice the timeout in the exec-multiple test.

I’ll give it ago with latest master when I get home, but notice the timeout in the exec-multiple test.
gk1623 commented 2024-11-09 11:06:44 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

added 1 commit

  • 7778e05a - Fix deadlock by release of lock and semaphore in the wrong order

Compare with previous version

added 1 commit <ul><li>7778e05a - Fix deadlock by release of lock and semaphore in the wrong order</li></ul> [Compare with previous version](/lab2425_autumn/pintos_22/-/merge_requests/31/diffs?diff_id=138169&start_sha=6ed1ccd21ea012ffb37d8c8d47e87b5f90979fc8)
gk1623 commented 2024-11-09 11:06:55 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

marked the checklist item There's a deadlock in at least exec-multiple. as completed

marked the checklist item **There's a deadlock in _at least_ `exec-multiple`.** as completed
gk1623 commented 2024-11-09 11:07:54 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

I haven't quite figured out exactly why this is a problem, but turns out releasing the lock AFTER sema_up in one of the if branches was problamatic, so I switched the order.

I haven't quite figured out exactly why this is a problem, but turns out releasing the lock AFTER `sema_up` in one of the `if` branches was problamatic, so I switched the order.
gk1623 commented 2024-11-09 11:07:56 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

resolved all threads

resolved all threads
gk1623 commented 2024-11-09 11:09:07 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

marked this merge request as ready

marked this merge request as **ready**
gk1623 commented 2024-11-11 17:36:09 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

added 1 commit

  • 18c88423 - Fix race-condition in process result (memory leak), fix infinite loop in donors_list

Compare with previous version

added 1 commit <ul><li>18c88423 - Fix race-condition in process result (memory leak), fix infinite loop in donors_list</li></ul> [Compare with previous version](/lab2425_autumn/pintos_22/-/merge_requests/31/diffs?diff_id=138519&start_sha=7778e05aa42e7fa8c6f0e2aee1924f3c2df64ec8)
gk1623 commented 2024-11-11 17:38:38 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

Deleted previous comment because it was completely wrong.

So the "working" implementation had a race-condition where if a parent thread dies after the child releases its result lock, but BEFORE it calls sema_up, then neither thread will free the result, resulting in a memory leak.

So revert back to the previous implementation which had a "deadlock". Turns out it wasn't a deadlock, but an infinite loop in the donors_list caused by a bug in the priority donation implementation that resulted in threads being part of their own donors_list.

Deleted previous comment because it was completely wrong. So the "working" implementation had a race-condition where if a parent thread dies after the child releases its result lock, but BEFORE it calls `sema_up`, then neither thread will free the result, resulting in a memory leak. So revert back to the previous implementation which had a "deadlock". Turns out it wasn't a deadlock, but an infinite loop in the `donors_list` caused by a bug in the priority donation implementation that resulted in threads being part of their own `donors_list`.
td1223 commented 2024-11-11 20:31:01 +00:00 (Migrated from gitlab.doc.ic.ac.uk)
Copy Link
Copy Source

marked the checklist item I need a sanity check about my "child_results is guaranteed to be in-order" comment please 😅 . as completed

marked the checklist item **I need a sanity check about my "child_results is guaranteed to be in-order" comment please :sweat_smile: .** as completed
td1223 (Migrated from gitlab.doc.ic.ac.uk) merged commit f194fa1fa8 into master 2024-11-11 22:56:29 +00:00
Sign in to join this conversation.
Reviewers
No Reviewers
Labels
No items
No labels
Milestone
No items
No Milestone Task 2: User Programs
Projects
Clear projects
No projects
Assignees
Clear assignees
cyclane (Gleb Koval)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Imperial-MEng/pintos_22#31