Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

scx_lavd: mitigate the lock holder preemption problem #779

Merged
merged 4 commits into from
Oct 12, 2024

Conversation

multics69
Copy link
Contributor

@multics69 multics69 commented Oct 11, 2024

If a task holding a lock (i.e., lock holder) is preempted for some reason, it will hinder the forward progress of a system and cause unnecessary task switches. To mitigate the lock holder preemption problem, this PR does the following:

  • Track all blocking locks in kernel and userspace, so the scheduler can easily decide if a task holds a lock of not.
  • When a task holds a lock, the lock holder task should be not preempted or should not yield its execution to other tasks.
  • When a lock holder exhausts its time slice, it should be scheduled as soon as possible, so system-wide progress can be made.

In addition to the above major changes, this PR includes the bug fix in the preeemptability test (#773)

Overall this brings more consistent results with less preemption.

Changwoo Min added 3 commits October 11, 2024 17:03
Trace the acquisition and release of blocking locks for kernel and
fuxtexes for user-space. This is necessary to boost a lock holder
task in terms of latency and time slice. We do not boost shared
lock holders (e.g., read lock in rw_semaphore) since the kernel
already prioritizes the readers over writers.

Signed-off-by: Changwoo Min <[email protected]>
When a lock holder exhausts its time slide, it will be re-enqueued
to a DSQ waiting for shceduling while holding a lock. In this case,
prioritize its latency criticality proportionally, so a lock holder
would be not stuck in a DSQ for a long time, improving system-wide
progress.

Signed-off-by: Changwoo Min <[email protected]>
When a task holds a lock, it should not yield its time slice or it
should not be preempted out. In this way, we can mitigate harmful
preemption of lock holders and reduce the total preemption counts.

Signed-off-by: Changwoo Min <[email protected]>
@multics69 multics69 requested a review from htejun October 11, 2024 09:56
Copy link
Contributor

@arighi arighi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look great! Thanks for doing this, especially for all the locking investigation. I'm really curious to test this out.

{
if (taskc && cpuc) {
taskc->lock_boost++;
cpuc->lock_holder = is_lock_holder(taskc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we assume this is always true, since taskc->lock_boost has been just incremented?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

taskc can be NULL if a task is not under scx, so it is necessary. Also, it is necessary to satisfy the verifier.

* Reset task's lock and futex boost count
* for a lock holder to be boosted only once.
*/
reset_lock_futex_boost(taskc);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So if we reset the counter here, when the task releases the lock its counter will become negative? I'm wondering if a boolean logic would be simpler and accomplish the same, like acquire a lock => boost = true, release a lock => boost = false, and after using the boost => boost = false.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, the counter could become negative so the decrement logic test if the counter is positive before decrementing it.
I think using counter is necessary because a task can hold multiple locks.

@multics69
Copy link
Contributor Author

Thanks @arighi for the review!

@multics69 multics69 added this pull request to the merge queue Oct 12, 2024
Merged via the queue into sched-ext:main with commit 836cf9f Oct 12, 2024
25 checks passed
@multics69 multics69 deleted the lavd-futex-v2 branch October 12, 2024 02:56
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.

2 participants