Page MenuHomeFreeBSD

Add support for blocking waits to refcounts.
ClosedPublic

Authored by jeff on Aug 13 2019, 8:02 PM.

Details

Summary

This adds a wait bit to refcount with an atomic sleep. Consumers that use this facility need to be aware that specific count values can not be examined directly.

When a counter is saturated this will block forever.

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

jeff created this revision.Aug 13 2019, 8:02 PM
jeff edited the summary of this revision. (Show Details)Aug 13 2019, 8:09 PM
jeff added reviewers: jhb, markj.
markj added a comment.Aug 13 2019, 8:54 PM

I'm not really crazy about this to be honest. refcount_release() gets inlined hundreds if not thousands of times into various functions. Now all of those callers are polluted with extra dead branch to call wakeup(). The fences in refcount_release() also don't make sense when refcount_wait() is used since a refcount of 0 doesn't mean that the protected object is unreachable, the way you'd normally expect a refcount to work. I would argue that this use-case would be better served by a new KPI or something open-coded.

That said, I don't see any technical issues aside from the nits I pointed out.

sys/sys/refcount.h
45 ↗(On Diff #60756)

The use of bit 31 meant that we could detect saturation with a single js instruction after the acquire, but that's no longer true with this change.

85 ↗(On Diff #60756)

Extra newline.

88 ↗(On Diff #60756)

We should assert some bound on n, probably 2^30?

jeff updated this revision to Diff 60763.Aug 13 2019, 9:56 PM

Address some of Markj's feedback. Move the last ref drop into a dedicated function to lessen text bloat.

markj added inline comments.Aug 13 2019, 10:09 PM
sys/sys/refcount.h
118 ↗(On Diff #60763)

On underflow, we want to call refcount_release_last() to reset the counter value. But now that won't happen since this is an unsigned comparison.

119 ↗(On Diff #60763)

Style: missing parens.

jeff updated this revision to Diff 60809.Aug 14 2019, 10:35 PM

Address review feedback

markj added inline comments.Aug 15 2019, 7:31 PM
sys/kern/kern_synch.c
55 ↗(On Diff #60809)

This sorts before resourcevar.h.

363 ↗(On Diff #60809)

Can't the cmpset fail spuriously on LL/SC platforms if the cache line is concurrently modified?

markj accepted this revision.Aug 15 2019, 7:34 PM
markj added inline comments.
sys/kern/kern_synch.c
363 ↗(On Diff #60809)

Never mind, that only applies to fcmpset.

This revision is now accepted and ready to land.Aug 15 2019, 7:34 PM
kib added a subscriber: kib.Aug 17 2019, 6:11 PM
kib added inline comments.
sys/kern/kern_synch.c
370 ↗(On Diff #60809)

There is no _rel at the beginning of the function. You need to point to the refcount_releasen().