Post by Mateusz GuzikCan I get some flames on this one?
:-).
Post by Mateusz GuzikPost by Mateusz GuzikI replaced them with refcount_acquire_if_not_zero and
refcount_release_if_not_last.
I dislike the length of the names. Can you propose something shorter ?
Unfortunately the original API is alreday quite verbose and I don't have
anything readable which would retain "refcount_acquire" (instead of a
"ref_get" or "ref_acq"). Adding "_nz" as a suffix does not look good
("refcount_acquire_if_nz").
refcount -> rc
acquire -> acq
The names are still verbose, yet not descriptive. refcount -> ref loses
more than refcount -> rc.
Post by Mateusz GuzikPost by Mateusz GuzikThe "acq" abbreviation is already used a lot for atomic ops.
How about refcount_acquire_gt_0() and refcount_release_gt_1()1?
Still verbose, and I prefer the _if_[n]z suffix. The gt suffix is only
shorter since you left out the 'if' part.
Post by Mateusz GuzikPost by Mateusz GuzikCurrent refcount(9) api is not only quite limited, but it also operates
on u_int instead of a type opaque to its consumers. This gives fewer
places which can assert counter sanity and increases chances of an
accidental abuse/type mismatch.
As such, how about deprecating current refcount_* funcs and introducing
a new family instead.
Say funcs would be prefixed with ref_. Consumers would use a ref_t type
(the obvious refcount_t is stolen by zfs).
Bug in zfs. rc_t might be OK.
Post by Mateusz GuzikPost by Mateusz GuzikApart from aforementioned 0->1 and 1->0 funcs, this introduces ref_read
to obtain the state of passed counter. A cast to volatile pointer +
dereference of that guarantees us a read at that point, so we don't have
to put the qualifier in the struct.
Finally, a blessed type is provided for use by all consumers so that the
correct type is enforced at compile time.
diff --git a/sys/sys/refcount.h b/sys/sys/refcount.h
index 4611664..c7c3732 100644
--- a/sys/sys/refcount.h
+++ b/sys/sys/refcount.h
@@ -38,6 +38,11 @@
#define KASSERT(exp, msg) /* */
#endif
+struct ref {
+ u_int r_count;
+};
+typedef struct ref ref_t;
+
style(9) forbids "typedef struct" since it is less opaque than a pointer
to an opaque struct. The contents of an opaque struct should not be
exposed, but "typedef struct" requires exposing them.
Post by Mateusz GuzikPost by Mateusz Guzikstatic __inline void
refcount_init(volatile u_int *count, u_int value)
{
This still hard-codes u_int in the API.
In fact, it is impossible for the refcount type to be fully opaque, since
to initialize it you need to pass a count. Callers must know that this
count is an integer and more about the type of this integer.
Post by Mateusz GuzikPost by Mateusz Guzik@@ -64,4 +69,64 @@ refcount_release(volatile u_int *count)
return (old == 1);
}
As above.
Post by Mateusz GuzikPost by Mateusz Guzik+static __inline void
+ref_init(ref_t *ref, u_int value)
+{
+
+ refcount_init(&ref->r_count, value);
+}
As above, now in a new API.
Post by Mateusz GuzikPost by Mateusz Guzik+
+static __inline u_int
+ref_read(ref_t *ref)
+{
+ u_int count;
+
+ count = *(volatile u_int *)&(ref->r_count);
+ KASSERT(count < UINT_MAX, ("refcount %p overflowed", ref));
+
+ return (count);
+}
As above, but more so. Most initializations will be to 0, but return
values may be large. struct ref could have other stuff in it, but the
counter part must be non-opaque so that clients can know the count.
I seee that this just duplicates refcount_read().
Post by Mateusz GuzikPost by Mateusz Guzik+
+static __inline void
+ref_acq(ref_t *ref)
+{
+
+ refcount_acquire(&ref->r_count);
+}
+
+static __inline int
+ref_rel(ref_t *ref)
+{
+
+ return (refcount_release(&ref->r_count));
+}
More obvious duplication.
I don't like anything in refcount.h. It just obfuscates adding or
subtracting 1 and adds some debugging code. refcount_init() doesn't
use an atomic op, so it wouldn't work for resetting an active counter.
The atomic ops that add and subtract 1 have severely limited types in
MI code. They wouldn't be able to use anything except u_int without
large overheads or complications on some arches. (I also don't like
counter64. It hard-codes 64 instead of u_int, and has complications
on all arches, but is not complicated enough to minimize the overheads.)
Post by Mateusz GuzikPost by Mateusz Guzik+
+static __inline int
+ref_acq_gt_0(ref_t *ref)
+{
+ u_int old;
+
+ for (;;) {
+ old = ref_read(ref);
+ if (old == 0)
+ return (0);
+ if (atomic_cmpset_int(&ref->r_count, old, old + 1))
+ return (1);
+ }
+}
+
+static __inline int
+ref_rel_gt_1(ref_t *ref)
+{
+ u_int old;
+
+ for (;;) {
+ old = ref_read(ref);
+ if (old == 1)
+ return (0);
+ if (atomic_cmpset_int(&ref->r_count, old, old - 1))
+ return (1);
This does bad things if old == 0.
These are complicated enough to be more than obfuscations of adding and
subtracting 1.
Acquire/release in the names de-emphasizes that reference counters are
_counters_, and refcount -> ref goes further in this direction. This
can be made further obfscated/opaque by not having public APIs to
return the count (so the problems with the type of this count go
away). Initializations would then have to be to the fixed state of
"not held", and queries could only return held/not held. But these
functions and their 0/1 suffixes go in the opposite direction, and seem
to requires callers to know more than the held/not held state to work.
Old style bugs:
- tab instead of space after #endif
- space after '!'
- excessive underscores
See an old header like stdio.h for the normal style of this endif and its
macro name.
Bruce