Messages in this thread | | | From | Linus Torvalds <> | Date | Tue, 28 Feb 2023 16:42:23 -0800 | Subject | Re: [patch 2/3] atomics: Provide rcuref - scalable reference counting |
| |
On Tue, Feb 28, 2023 at 6:33 AM Thomas Gleixner <tglx@linutronix.de> wrote: > > This unconditional increment avoids the inc_not_zero() problem, but > requires a more complex implementation on the put() side when the count > drops from 1 to 0. > > When this transition is detected then it is attempted to mark the reference > count dead, by setting it to the midpoint of the dead zone with a single > atomic_cmpxchg_release() operation. This operation can fail due to a > concurrent rcuref_get() elevating the reference count from 0 to 1.
This looks sane to me, however it does look like the code is not really optimal.
This is supposed to be a critical function, and is inlined:
> +static inline __must_check bool rcuref_get(rcuref_t *ref) > +{ > + unsigned int old = atomic_fetch_add_relaxed(1, &ref->refcnt); > + > + if (likely(old < RCUREF_MAXREF)) > + return true;
but that comparison would be much better if RCUREF_MAXREF was 0x80000000 and you'd end up just checking the sign of the result, instead of checking big numbers.
Also, this optimal value choice ends up being architecture-specific, since some do the "fetch_add", and others tend to prefer "add_return", and so the point that is cheapest to check ends up depending on which architecture it is.
This may seem like nit-picking, but I absolutely *HATE* our current refcount interface for how absolutely horrid the code generation ends up being. It's gotten better, but it's still not great.
So if we're introducing yet another refcount interface, and it's done in the name of efficiency, I would *really* want it to actually be exactly that: efficient. Not some half-way thing.
And yes, that may mean that it should have some architecture-specific code (with fallback defaults for the generic case).
Linus
| |