Merge 'task1/priority-donation' into 'master' #14

Merged

sb3923 merged 66 commits from task1/priority-donation into master 2024-10-23 16:15:45 +00:00

Conversation 35 Commits 66 Files Changed 4 +276 -16

sb3923 commented 2024-10-23 16:01:59 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Submitting a working implementation of priority scheduling supporting priority donation. All test case passes. All race conditions have been handled. Code was reviewed for style. - Saleh and Themis.

Submitting a working implementation of priority scheduling supporting priority donation. All test case passes. All race conditions have been handled. Code was reviewed for style. - Saleh and Themis.

sb3923 commented 2024-10-23 16:01:59 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

requested review from @gk1623

requested review from @gk1623

sb3923 commented 2024-10-23 16:03:12 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

assigned to @sb3923

assigned to @sb3923

sb3923 commented 2024-10-23 16:03:17 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

assigned to @td1223 and unassigned @sb3923

assigned to @td1223 and unassigned @sb3923

sb3923 commented 2024-10-23 16:03:21 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

assigned to @sb3923 and unassigned @td1223

assigned to @sb3923 and unassigned @td1223

ed1223 commented 2024-10-23 16:15:43 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

approved this merge request

approved this merge request

ed1223 commented 2024-10-23 16:15:45 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

mentioned in commit 0cbae2a2e5

mentioned in commit 0cbae2a2e512b00526c3cdf748add1f670e532e9

ed1223 (Migrated from gitlab.doc.ic.ac.uk) merged commit 0cbae2a2e5 into master 2024-10-23 16:15:45 +00:00

gk1623 commented 2024-10-25 07:56:37 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

For the future, lets try to not do such changes

For the future, lets try to not do such changes

gk1623 commented 2024-10-25 07:56:37 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

117-123 (with curlies) should be indented (pintos style).

117-123 (with curlies) should be indented (pintos style).

gk1623 commented 2024-10-25 07:56:37 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Imo the underscore is too vague about what it means, perhaps something like this would be better?

priority_more (const struct list_elem *a, const struct list_elem *b,
               void *aux UNUSED)
{
  struct thread *at = list_entry (a, struct thread, elem);
  struct thread *bt = list_entry (b, struct thread, elem);

  return at->priority > bt->priority;

Imo the underscore is too vague about what it means, perhaps something like this would be better? ```suggestion:-6+0 priority_more (const struct list_elem *a, const struct list_elem *b, void *aux UNUSED) { struct thread *at = list_entry (a, struct thread, elem); struct thread *bt = list_entry (b, struct thread, elem); return at->priority > bt->priority; ```

gk1623 commented 2024-10-25 07:56:38 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

I see this is used only for a list_max (...) to get the maximum priority, so afaik order of same priority threads does not matter. So, can we not just instead do list_min (...) using priority_more, and remove this function entirely?

I see this is used only for a `list_max (...)` to get the maximum priority, so afaik order of same priority threads does not matter. So, can we not just instead do `list_min (...)` using `priority_more`, and remove this function entirely?

gk1623 commented 2024-10-25 07:56:38 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Why?

Why?

gk1623 commented 2024-10-25 07:56:38 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Can a thread be in multiple donor lists? If yes, then we can't do this.... one struct list_elem can only be a member of one list at a single time.

Can a thread be in multiple donor lists? If yes, then we can't do this.... one `struct list_elem` can only be a member of one list at a single time.

gk1623 commented 2024-10-25 08:02:25 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Other than the comments, looks very good, thanks guys!

I've tried to think about how we can reduce disabling interrupts, but couldn't come up with anything. You've probably thought this aspect through a lot as well, right?

Other than the comments, looks very good, thanks guys! I've tried to think about how we can reduce disabling interrupts, but couldn't come up with anything. You've probably thought this aspect through a lot as well, right?

sb3923 commented 2024-10-25 08:36:32 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

We did indeed. Technically it is possible in certain cases, but it would be extremely ugly (module separation / editing the code for other structs / unnecessary fields in struct thread etc).

Also, the design document only asks about one case in particular and whether it can be done without disable interrupts. So this also sort of gave us a bit of confidence that this is probably the only place where using them may have been an option.

We did indeed. Technically it is possible in certain cases, but it would be extremely ugly (module separation / editing the code for other structs / unnecessary fields in struct thread etc). Also, the design document only asks about one case in particular and whether it can be done without disable interrupts. So this also sort of gave us a bit of confidence that this is probably the only place where using them may have been an option.

td1223 commented 2024-10-25 08:36:39 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

saleh claimed this is the style they use in other list comparison functions/elsewhere in the codebase, so I guess it's good (?)

saleh claimed this is the style they use in other list comparison functions/elsewhere in the codebase, so I guess it's good (?)

sb3923 commented 2024-10-25 08:36:59 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Agreed.

Agreed.

sb3923 commented 2024-10-25 08:38:05 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Yeah you're right. I swear we fixed this one but probably never pushed it. To Be Fixed later today.

Yeah you're right. I swear we fixed this one but probably never pushed it. To Be Fixed later today.

td1223 commented 2024-10-25 08:38:45 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

we did this originally but felt we might get marked down for lack of clarity/ abusing functions. it's probably easier to understand that max gives maximum, min gives minimum, especially since the functions say the expect a 'list_less' function, but i dont mind reverting

we did this originally but felt we might get marked down for lack of clarity/ abusing functions. it's probably easier to understand that max gives maximum, min gives minimum, especially since the functions say the expect a 'list_less' function, but i dont mind reverting

sb3923 commented 2024-10-25 08:38:53 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Yes it is the style they follow hence the naming. So this is consistent with PintOS.

Yes it is the style they follow hence the naming. So this is consistent with PintOS.

sb3923 commented 2024-10-25 08:41:28 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Yeah we had list_min (..., priority_more, ...) in the entire codebase before. But it was really confusing and led to many errors that were only caught when debugging later on in the project. I feel it is better to keep both. It is a private function anyway.
I believe we are more likely to be marked down for abusing the naming, and using min when want the max (ugly code) than we are if we keep both (if this is even considered an instance of duplication).

Yeah we had `list_min (..., priority_more, ...)` in the entire codebase before. But it was really confusing and led to many errors that were only caught when debugging later on in the project. I feel it is better to keep both. It is a private function anyway. I believe we are more likely to be marked down for abusing the naming, and using min when want the max (ugly code) than we are if we keep both (if this is even considered an instance of duplication).

td1223 commented 2024-10-25 08:41:41 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

this isn't our code iirc, so best left alone

this isn't our code iirc, so best left alone

gk1623 commented 2024-10-25 08:44:16 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Perhaps we could at least re-used priority_more within donor_priority_less, rather than rewriting pretty much exactly the same code?

ie. return !priority_more(a_, b_);

Perhaps we could at least re-used `priority_more` within `donor_priority_less`, rather than rewriting pretty much exactly the same code? ie. `return !priority_more(a_, b_);`

gk1623 commented 2024-10-25 08:44:43 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

224 is an addition from this PR, so this is definitely our code

224 is an addition from this PR, so this is definitely our code

td1223 commented 2024-10-25 08:44:43 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

shouldn't be possible. a thread is only a donor if it waits for the lock of the donee. but you can only be waiting for one lock at a time.

shouldn't be possible. a thread is only a donor if it waits for the lock of the donee. but you can only be waiting for one lock at a time.

sb3923 commented 2024-10-25 08:45:18 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

This was added in the final code base.

I think it should be there yes! It is also possible to add a simple check of the effective priority (which is the same as the base one in this case).

One thing worth considering is whether it can be called from within an interrupt handler? I dont think so but maybe worth just double checking.

This was added in the final code base. I think it should be there yes! It is also possible to add a simple check of the effective priority (which is the same as the base one in this case). One thing worth considering is whether it can be called from within an interrupt handler? I dont think so but maybe worth just double checking.

gk1623 commented 2024-10-25 08:46:50 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

I'm convinced that it's safe, I just don't really understand why you added it.

I'm convinced that it's safe, I just don't really understand why you added it.

sb3923 commented 2024-10-25 08:46:56 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

No we can't unfourtnately. We basically did this initially which was only found after hours of debugging.

This is because the elem they use is different! So the list_max and list_min do not give the expected values but seemingly random when used with the wrong comparison function

No we can't unfourtnately. We basically did this initially which was only found after hours of debugging. This is because the `elem` they use is different! So the list_max and list_min do not give the expected values but seemingly random when used with the wrong comparison function

gk1623 commented 2024-10-25 08:48:07 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Oh right I see, makes sense.

Oh right I see, makes sense.

sb3923 commented 2024-10-25 08:48:08 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

If the newly created thread has a higher priority then the current running thread should yield the processor to that thread. At least this is what were thinking.

If the newly created thread has a higher priority then the current running thread should yield the processor to that thread. At least this is what were thinking.

sb3923 commented 2024-10-25 08:49:35 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Exactly what @td1223 said. The donors are removed when a lock is released (possibly moved), so yes they can always be in one donors list at a time.

Exactly what @td1223 said. The donors are removed when a lock is released (possibly moved), so yes they can always be in one donors list at a time.

td1223 commented 2024-10-25 08:50:29 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

yeah my bad I forgot I added this as one of the first commits and that's the reasoning. this aspect is tested in the suite too

yeah my bad I forgot I added this as one of the first commits and that's the reasoning. this aspect is tested in the suite too

gk1623 commented 2024-10-25 08:52:06 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Since we have both the parent_thread and t(hread) anyway, I think we should do a check for that, so that we're not yielding for no reason, what do you think?

ie.

+  if (parent_thread->priority < t->priority)
+    thread_yield ();

Since we have both the `parent_thread` and `t`(hread) anyway, I think we should do a check for that, so that we're not yielding for no reason, what do you think? ie. ```suggestion:-0+0 + if (parent_thread->priority < t->priority) + thread_yield (); ```

sb3923 commented 2024-10-25 08:53:00 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

Yeah I am happy with adding this simple check 😄
I shall add it later on with the other fix.

Yeah I am happy with adding this simple check :smile: I shall add it later on with the other fix.

sb3923 commented 2024-10-25 15:06:15 +00:00 (Migrated from gitlab.doc.ic.ac.uk)

Copy Link

mentioned in merge request !20

mentioned in merge request !20

ed1223 (Migrated from gitlab.doc.ic.ac.uk) approved these changes 2025-10-16 16:43:33 +00:00

Sign in to join this conversation.

Reviewers

No Reviewers

ed1223

Labels

No items

No Label

Milestone

No items

No Milestone Task 1: Scheduling

Projects

Clear projects

No project

Assignees

Clear assignees

cyclane

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: Imperial-MEng/pintos_22#14

No description provided.