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

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

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 <sebastian@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().
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.

> > -/* Try to release refcount of module, 0 means success. */
> > -static int try_release_module_ref(struct module *mod)
> > +/* Try to release the initial reference of module, true means success. */
> > +static bool try_release_module_ref(struct module *mod)
> >  {
> > -	int ret;
> > +	bool ret;
> >  
> >  	/* Try to decrement refcnt which we set at loading */
> > -	ret = atomic_sub_return(MODULE_REF_BASE, &mod->refcnt);
> > -	BUG_ON(ret < 0);
> > +	ret = rcuref_put(&mod->refcnt);
> >  	if (ret)
> > -		/* Someone can put this right now, recover with checking */
> > -		ret = atomic_add_unless(&mod->refcnt, MODULE_REF_BASE, 0);
> > +		/* Last reference put, module can go */
> > +		return true;
> >  
> > -	return ret;
> > +	ret = rcuref_get(&mod->refcnt);
> > +	if (!ret)
> > +		/*
> > +		 * Last put failed but can't acquire a reference. This means
> > +		 * the previous owner has put the reference.
> > +		 */
> > +		return true;
> > +
> > +	/* There is still a reference on the module */
> > +	return false;
> 
> The updated version of try_release_module_ref() no longer uses the
> MODULE_REF_BASE constant and silently expects that it is equal to 1. We
> could trivially get rid of MODULE_REF_BASE and similarly hardcode it
> as 1 throughout kernel/module/main.c, but I assume the purpose of it
> being a #define constant is for explicitness to make the code clearer.
> 
> I realize the new code cannot use MODULE_REF_BASE because rcuref_t
> currently doesn't have functions to add/subtract an arbitrary value from
> the reference counter. I guess this goes back to my first question about
> why rcuref_t is preferred over refcount_t.

The idea is to start with a refcount of two. I don't if having the
define as MODULE_REF_BASE or simply use 1 + 1 to make it obvious that 1
additional reference is held which is dropped in
try_release_module_ref() it an attempt to release the module.
rcuref does not provide add/ inc but allows to keep one extra refence.

> >  }
> >  
> >  static int try_stop_module(struct module *mod, int flags, int *forced)
> >  {
> >  	/* If it's not unused, quit unless we're forcing. */
> > -	if (try_release_module_ref(mod) != 0) {
> > +	if (try_release_module_ref(mod) != true) {
> 
> Nit: -> 'if (!try_release_module_ref(mod)) {'.

Noted.

Sebastian