Page MenuHomeFreeBSD

mips: fcmpset: do not spin on sc failure
ClosedPublic

Authored by kevans on Sep 29 2019, 2:57 PM.

Details

Summary

For ll/sc architectures, atomic(9) allows failure modes where *old == val due to write failure and callers should compensate for this. Do not retry on failure, just leave 0 in ret and fail the operation if we couldn't sc it.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

kevans created this revision.Sep 29 2019, 2:57 PM
kib added inline comments.Sep 29 2019, 3:19 PM
sys/mips/include/atomic.h
405 ↗(On Diff #62703)

exit on failure (or abort)

406 ↗(On Diff #62703)

This label should be '1' etc.

kevans updated this revision to Diff 62709.Sep 29 2019, 3:25 PM

Re-label jumps and revise comment to be more specific

kib accepted this revision.Sep 29 2019, 4:29 PM
kib added inline comments.
sys/mips/include/atomic.h
405 ↗(On Diff #62709)

So this uses the delay slot to provide the old value, right ? I think it deserves a comment. (Not that the old code did explained it).

This revision is now accepted and ready to land.Sep 29 2019, 4:29 PM
kevans marked 2 inline comments as done.Sep 29 2019, 4:45 PM
kevans added inline comments.
sys/mips/include/atomic.h
405 ↗(On Diff #62709)

I do not believe so- in the comparison failure case, we've jumped to the sw below and provided. If the comparison didn't fail, we've left *cmpval untouched.

imp added inline comments.Sep 30 2019, 1:49 AM
sys/mips/include/atomic.h
405 ↗(On Diff #62709)

Then you need a nop here. Otherwise I think the following instruction is used...
unless there's a special rule that a delay slot won't cross a label...

514 ↗(On Diff #62709)

Same here.

kevans added inline comments.Sep 30 2019, 1:58 AM
sys/mips/include/atomic.h
405 ↗(On Diff #62709)

Regardless of whether it can cross a label or not (which would seem odd, my impression was labels aren't actually compiled in but rather just the location noted for replacement in the underlying asm... but I haven't bothered checking this), it would likely be better to provide nop to be explicit.

imp added inline comments.Sep 30 2019, 2:03 AM
sys/mips/include/atomic.h
405 ↗(On Diff #62709)

yea... The assembler is 'smart' and helps one manage the delay slots and sometimes inserts NOPs and other times doesn't. It's been 20ish years since I did MIPS assembler enough to have that swapped into my brain all the special times things would be written, and when they wouldn't... explicit nop would make it clear.

kevans updated this revision to Diff 62735.Sep 30 2019, 2:20 AM

Add nops to document that we're expecting no delay slot interactions to be happening here...

I would assume the assembler's implicitly adding this nop anyways based on the old version that we're replacing, but explicit is better than implicit.

This revision now requires review to proceed.Sep 30 2019, 2:20 AM
kib added inline comments.Sep 30 2019, 2:25 PM
sys/mips/include/atomic.h
405 ↗(On Diff #62735)

Actually this is an exit not only on failure.

kevans updated this revision to Diff 62756.Sep 30 2019, 3:52 PM

Revise incorrect comment, drop a more complete comment describing that it's intentionally leaving *cmpval untouched and a pointer to the documentation that says it's ok.

kib accepted this revision.Sep 30 2019, 9:07 PM
This revision is now accepted and ready to land.Sep 30 2019, 9:07 PM
This revision was automatically updated to reflect the committed changes.