Re: [PATCH] module: Use rcuref_t for module::refcnt.

[Sebastian Andrzej Siewior <sebastian@breakpoint.cc>]

On 2025-03-17 17:33:58 [+0100], Petr Pavlu wrote:
> >> I'd understand changing module::refcnt from atomic_t to refcount_t, but
> >> it isn't clear to me from the above description what using rcuref_t
> >> actually gains. Could you please explain why you think it is more
> >> appropriate over refcount_t here?
> > 
> > I seems easier to handle without the atomic_inc_not_zero() and
> > atomic_dec_if_positive().
> 
> I think the use of atomic_inc_not_zero()/refcount_inc_not_zero() is
> a common pattern. The call to atomic_dec_if_positive() would be with
> refcount_t in this case replaced by refcount_dec(). That looks fairly
> comparable to me to the rcuref_t version.

It is a common pattern. The difference is that atomic_inc_not_zero() is
implemented as cmpxchg loop while rcuref_get() is implemented as an
unconditional get. Now: The cmpxchg loop might need a retry if there are
two simultaneous gets while rcuref_get() does always the increment. So
if you have simultaneous gets you can see a performance improvement with
rcuref_t, see for instance
	https://lore.kernel.org/all/202504221604.38512645-lkp@intel.com/

> > rcuref_get() is implemented as an implicit inc and succeeds always as
> > long as the counter is not negative. Negative means the counter has been
> > probably released and the slowpath decides if it is released or not.
> > Eitherway you get rid of all the WARN_ON()s and the dec/ inc dance in
> > try_release_module_ref() where you simply attempt the final "put" and if
> > this one fails (because a refence is still held) you attempt to get the
> > inital reference and can decice if it was successfull or not.
> > If the puts outweight the gets then you see a warning from the rcuref()
> > code itself.
> 
> Sure, but having these warnings would be the case also with refcount_t,
> no?

The first put will fail, so it will attempt a get. If the get succeeds
then the initial reference has been obtained and the object will remain.
If the get fails, then the object has been properly released (there was
a put in between). So the state is eitehr/ or.

> I see that rcuref_t is so far used by dst_entry::__rcuref, for which it
> was originally added, and k_itimer::rcuref. I'm not sure if there's any
> guidance or prior consensus on when to use refcount_t vs rcuref_t.
> I understand some of the performance advantage of rcuref_t, but I wonder
> if code that doesn't substantially benefit from that, such
> module::refcnt, should now use it, or if it should stick to the more
> common refcount_t.

It is kind of new, yes. The big performance benefit comes when you have
multiple puts/gets in parallel. But if you don't you don't lose
anything. There is also file_ref_t which is the same as rcuref_t from
the principle of operating but different in terms of counter size and
memory barriers.

Sebastian