On 3/10/25 22:24, Sebastian Andrzej Siewior wrote: > On 2025-03-10 15:28:23 [+0100], Petr Pavlu wrote: >> On 3/9/25 13:19, Sebastian Andrzej Siewior wrote: >>> By using rcuref_t the atomic_inc_not_zero() and atomic_dec_if_positive() >>> can disappear. By design rcuref_t does not allow decrementing past the >>> "0" reference and increment once it has been released. It will spill >>> warnings if there are more puts than initial gets. >>> This also makes the put/ get attempt in try_release_module_ref() a >>> little simpler. Should the get in try_release_module_ref() fail then >>> there should be another warning originating from module_put() which is >>> the only race I can imagine. >>> >>> Use rcuref_t for module::refcnt. >>> >>> Signed-off-by: Sebastian Andrzej Siewior <sebast...@breakpoint.cc> >> >> 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. > 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? 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. -- Thanks, Petr
