Add hint::spin_loop() into Atomic*::*update()

[im-0]

Atomic*::update() and Atomic*::try_update() are implemented using a CAS loop and thus spin loop hint seems appropriate here.

Question: are there any relevant benchmarks to run to check that this patch does not regress anything? For example, I see that try_update() is used to implement RwLock. Are there any benchmarks for RwLock?

[rustbot]

Thanks for the pull request, and welcome! The Rust Project is excited to review your changes, and you should hear from @aapoalas (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

• @rustbot author: the review is finished, PR author should check the comments and take action accordingly
• @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

Why was this reviewer chosen?

The reviewer was selected based on:

• Owners of files modified in this PR: libs
• libs expanded to 12 candidates
• Random selection from 6 candidates

[joboet]

Thanks for the pull request!

I'd argue against this: the CAS will only fail if the value is concurrently being changed but hint::spin_loop() is intended for cases where the value is repeatedly loaded to check whether some condition has been fulfilled, i.e. the thread is busy-waiting.

That's also why e.g. compare_exchange on targets that only have LL/SC instructions (like ARM) doesn't use the yield instruction, even though the value may be repeatedly loaded.

[im-0]

Ah... So hint::spin_loop() is supposed to be used inside loops where longer wait is expected and not inside loops where just a few iterations are expected worst case? Do I get it right?

[clarfonthey]

I was under the impression that spin_loop was explicitly for cases like this, so, I'm a little surprised it isn't used here. But I also don't know enough about the underlying hints to really be sure.

Either way, I would expect that if these shouldn't have spin-loop hints, there should be a comment explaining why.

[ChayimFriedman2]

Either way, I would expect that if these shouldn't have spin-loop hints, there should be a comment explaining why.

And probably a doc comment explaining that you should not use this if you do need a spin loop hint.

[aapoalas]

Hmm. To me this seems reasonable, but if it's also not normally used then it does sound like Rust shouldn't add it either...

I feel like I don't have the expertise to make this decision alone so I'll nominate for libs.

[hanna-kruppe]

I agree with @joboet and @ChayimFriedman2. hint::spin_loop() is for when you’re busy-waiting on another thread. In those cases, retrying immediately on CAS failure is unlikely to make progress, and as the hint’s docs explain, such busy-wait loops should always be limited and eventually yield control to the scheduler to avoid catastrophic failures.

The typical/expected use of Atomic*::update is not such busy-wait loops but self-contained critical sections, the Atomic* equivalent of let guard = mutex.lock(); *guard = update(*guard); drop(guard);. If the CAS fails due to contention, another thread succeeded instead and retrying immediately is likely to succeed, unless the location is constantly contended by many threads, in which case doing hint::spin_loop() in all the failing threads doesn’t help.

It’s theoretically possible to contort Atomic*::update() into something that is a busy-wait, which is e.g. susceptible to priority inversion issues. But in that case, adding the hint doesn’t fix the problems spinning causes. I agree with @ChayimFriedman2 that it makes sense to extend the docs of update and try_update to advice against doing anything that amounts to busy-waiting for another thread to complete something. The update operation should essentially be a pure function from old value to new value. In cases where you may have to wait on another thread, you should write the retry loop yourself to include the iteration limit and eventual scheduler interaction.

[programmerjake]

copied from the private zulip thread since I realized my comment would be more useful when it's public:

from what I understand, the spin loop hint is to tell the CPU that it'll be spinning waiting until the value in memory is changed by another CPU, so doing lots of loads really fast doesn't really help since it uses more power and won't make the other CPU change the value in memory any faster. so the hint will make the CPU slow down and check less often instead of e.g. checking the value 4-5 billion times a second.

for Atomic::update, slowing down is counterproductive since rather than waiting for some other CPU to change the value in memory, we're instead trying again when some other CPU changed the value out from under us and hoping we'll eventually get to where no other CPU tried to change the value, so we can perform our update. slowing down makes it more likely that some other CPU can interfere with our attempted update since there's a bigger gap for the other CPU to get into.

[Darksonn]

I agree, this is not a good idea. The spin loop hint is for the case where the loop fails because another thread has not taken a certain action yet. But for a CAS loop the success/failure scenario is reversed. It exits when no other thread taking an action during the loop.

[im-0]

Thank you for the explanation! Closing this because spin_loop is not really needed here.

[clarfonthey]

Opened #159679 with an attempt to make the docs more clear about this.