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

Remove wrong advice about spin locks from spin_loop_hint docs #67798

Merged
merged 2 commits into from
Jan 8, 2020

Conversation

matklad
Copy link
Member

@matklad matklad commented Jan 2, 2020

Using a pure spin lock for a critical section in a preemptable thread
is always wrong, however short the critical section may be. The thread
might be preempted, which will cause all other threads to hammer
busily at the core for the whole quant. Moreover, if threads have
different priorities, this might lead to a priority inversion problem
and a deadlock. More generally, a spinlock is not more efficient than
a well-written mutex, which typically does several spin iterations at
the start anyway.

The advise about UP vs SMP is also irrelevant in the context of
preemptive threads.

See also accompanying piece: https://matklad.github.io/2020/01/02/spinlocs-considered-harmful.html

And another, independent piece: https://probablydance.com/2019/12/30/measuring-mutexes-spinlocks-and-how-bad-the-linux-scheduler-really-is

EDIT: obligatory disclosure that I am not an expert in these things, and might be terribly wrong :)

Using a pure spin lock for a critical section in a preemptable thread
is always wrong, however short the critical section may be. The thread
might be preempted, which will cause all other threads to hammer
busily at the core for the whole quant. Moreover, if threads have
different priorities, this might lead to a priority inversion problem
and a deadlock. More generally, a spinlock is not more efficient than
a well-written mutex, which typically does several spin iterations at
the start anyway.

The advise about UP vs SMP is also irrelevant in the context of
preemptive threads.
@rust-highfive
Copy link
Collaborator

r? @KodrAus

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 2, 2020
Copy link
Member

@steveklabnik steveklabnik left a comment

Choose a reason for hiding this comment

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

IMHO, this is a @rust-lang/libs decision; the docs formatting looks fine, but it's up to them for the content.

@Amanieu
Copy link
Member

Amanieu commented Jan 2, 2020

I don't think the advice is wrong. However we could put more emphasis on the underlying assumption that the other thread is running on a different CPU: this is not the case if the other thread has been preempted or interrupted while holding the lock.

@matklad
Copy link
Member Author

matklad commented Jan 5, 2020

I don't think the advice is wrong.

I think it depends on the specific reading of the given docs. One reading is

when implementing locks, it is generally faster to spin for a bit, before making a blocking syscall

This is, in my understanding, 100% correct.

The other plausible reading is

If you can use either std::sync::Mutex or a spinlock, and the critical section is short, a pure spin lock will be more efficient than a sleeping mutex.

I strongly feel that this advise is very wrong. I also feel that the original wording likely implies this reading for a user who is not already familiar with the pitfalls of pure spinlocks. To clarify, there are very specific narrow circumstances where this advice is right (for example, pinned thread per CPU situation with realtime priorities), but in other, more common situations the advice is harmful.

I changed the wording to make it unambiguously refer to the first case. I've also removed the bit about busy polling. It is another plausible use-case for spin_loop_hint, but is more esoteric and doesn't fit into the flow with the new wording. Additionally, the advice to use yield_now or sleep also seems suspicious: it looks like if you want to block in a busy-polling loop, you should just make a blocking syscall like poll/read? (for the curious reader: Linus makes a compelling argumnet that using yield should be avoided: if you are going to call into kernel to sleep, you might as well inform the kernel why you are sleeping (FUTEX_WAIT, poll, etc)).

@matklad
Copy link
Member Author

matklad commented Jan 5, 2020

Oh, I think it also makes sense to r? @Amanieu as well :-)

@rust-highfive rust-highfive assigned Amanieu and unassigned KodrAus Jan 5, 2020
src/libcore/sync/atomic.rs Outdated Show resolved Hide resolved
@Amanieu
Copy link
Member

Amanieu commented Jan 8, 2020

I guess there's always more information we can add about spinlock design, but in the end this doc comment isn't the right place for it.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 8, 2020

📌 Commit b25eeef has been approved by Amanieu

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 8, 2020
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Jan 8, 2020
Remove wrong advice about spin locks from `spin_loop_hint` docs

Using a pure spin lock for a critical section in a preemptable thread
is always wrong, however short the critical section may be. The thread
might be preempted, which will cause all other threads to hammer
busily at the core for the whole quant. Moreover, if threads have
different priorities, this might lead to a priority inversion problem
and a deadlock. More generally, a spinlock is not more efficient than
a well-written mutex, which typically does several spin iterations at
the start anyway.

The advise about UP vs SMP is also irrelevant in the context of
preemptive threads.

See also accompanying piece: https://matklad.github.io/2020/01/02/spinlocs-considered-harmful.html

And another, independent piece: https://probablydance.com/2019/12/30/measuring-mutexes-spinlocks-and-how-bad-the-linux-scheduler-really-is

EDIT: obligatory disclosure that I am not an expert in these things, and might be terribly wrong :)
bors added a commit that referenced this pull request Jan 8, 2020
Rollup of 10 pull requests

Successful merges:

 - #67774 (Try statx for all linux-gnu target.)
 - #67781 (Move `is_min_const_fn` query to librustc_mir.)
 - #67798 (Remove wrong advice about spin locks from `spin_loop_hint` docs)
 - #67849 (Add a check for swapped words when we can't find an identifier)
 - #67875 (Distinguish between private items and hidden items in rustdoc)
 - #67887 (`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]`)
 - #67955 (rustdoc: Remove more `#[doc(cfg(..))]` duplicates)
 - #67977 (Updates for VxWorks)
 - #67985 (Remove insignificant notes from CStr documentation)
 - #68003 (ci: fix wrong shared.sh import for publish_toolstate)

Failed merges:

 - #67820 (Parse the syntax described in RFC 2632)
 - #67979 (Move `intravisit` => `rustc_hir` + misc cleanup)

r? @ghost
bors added a commit that referenced this pull request Jan 8, 2020
Rollup of 10 pull requests

Successful merges:

 - #67774 (Try statx for all linux-gnu target.)
 - #67781 (Move `is_min_const_fn` query to librustc_mir.)
 - #67798 (Remove wrong advice about spin locks from `spin_loop_hint` docs)
 - #67849 (Add a check for swapped words when we can't find an identifier)
 - #67875 (Distinguish between private items and hidden items in rustdoc)
 - #67887 (`Option::{expect,unwrap}` and `Result::{expect, expect_err, unwrap, unwrap_err}` have `#[track_caller]`)
 - #67955 (rustdoc: Remove more `#[doc(cfg(..))]` duplicates)
 - #67977 (Updates for VxWorks)
 - #67985 (Remove insignificant notes from CStr documentation)
 - #68003 (ci: fix wrong shared.sh import for publish_toolstate)

Failed merges:

 - #67820 (Parse the syntax described in RFC 2632)
 - #67979 (Move `intravisit` => `rustc_hir` + misc cleanup)

r? @ghost
@bors bors merged commit b25eeef into rust-lang:master Jan 8, 2020
@matklad matklad deleted the spin-thouse-docs branch July 7, 2020 16:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants