Page MenuHomeFreeBSD

refcount(9): Add refcount_release_if_last() and refcount_load()
ClosedPublic

Authored by markj on Nov 2 2020, 5:57 PM.

Details

Summary

The former is intended for use in vmspace_exit(). The latter is to
encourage use of explicit loads rather than relying on the volatile
qualifier. This works better with kernel sanitizers, which can
intercept atomic(9) calls, and makes tricky lockless code easier to read
by not forcing the reader to remember which variables are declared
volatile.

Diff Detail

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

Event Timeline

markj requested review of this revision.Nov 2 2020, 5:57 PM
markj created this revision.

I remember trying to add something of the sort and running into a conflict with linuxkpi. Have you compiled the entire kernel + modules with it? There is definitely a refcount_* namespace conflict, but I don't remember if they have read or load.

Also there are not that many consumers of the refcount API. Perhaps we can commit this as the first step, when patch every refcount* user to stop immediate reats and finally add a refcount_t type and convert everyone in one go. I can provide diffs for vfs and all the struct file handling. Step of that sort is necessary and the earlier the better.

kib added inline comments.
sys/sys/refcount.h
183 ↗(On Diff #79076)

'Invalid zero decrement' ?

212 ↗(On Diff #79076)

else is not needed.

This revision is now accepted and ready to land.Nov 2 2020, 6:15 PM
markj marked 2 inline comments as done.Nov 2 2020, 6:34 PM
In D27056#603776, @mjg wrote:

I remember trying to add something of the sort and running into a conflict with linuxkpi. Have you compiled the entire kernel + modules with it? There is definitely a refcount_* namespace conflict, but I don't remember if they have read or load.

I did compile and boot GENERIC. I think you are thinking of refcount_read().

Also there are not that many consumers of the refcount API. Perhaps we can commit this as the first step, when patch every refcount* user to stop immediate reats and finally add a refcount_t type and convert everyone in one go. I can provide diffs for vfs and all the struct file handling. Step of that sort is necessary and the earlier the better.

Yes I would eventually like to have a refcount_t. My aim here is to convert vmspace's custom reference counter to use refcount(9), and there are several naked loads of vm_refcnt. The next step would be to start converting refcount(9) consumers to use refcount_load() and MFC as much as possible before making any KPI changes.

sys/sys/refcount.h
183 ↗(On Diff #79076)

Ah right, n is unsigned.

markj marked an inline comment as done.

Handle feedback.

This revision now requires review to proceed.Nov 2 2020, 6:36 PM

Well the question is what about the clash then. It is a problem in waiting, but it does not have to be immediately addressed.

This revision is now accepted and ready to land.Nov 2 2020, 6:41 PM
mmel added a subscriber: mmel.

Tested on ARM and ARM64

sys/sys/refcount.h
183 ↗(On Diff #79082)

n is not used in format string