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

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

[Sebastian Andrzej Siewior]

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>
---
 include/linux/module.h        |  2 +-
 include/trace/events/module.h |  2 +-
 kernel/module/main.c          | 51 ++++++++++++++++++-----------------
 3 files changed, 29 insertions(+), 26 deletions(-)

diff --git a/include/linux/module.h b/include/linux/module.h
index d9a5183a9fe71..b473a329f1927 100644
--- a/include/linux/module.h
+++ b/include/linux/module.h
@@ -581,7 +581,7 @@ struct module {
 	/* Destruction function. */
 	void (*exit)(void);
 
-	atomic_t refcnt;
+	rcuref_t refcnt;
 #endif
 
 #ifdef CONFIG_CONSTRUCTORS
diff --git a/include/trace/events/module.h b/include/trace/events/module.h
index e5a006be9dc66..cdb88bb4bc24a 100644
--- a/include/trace/events/module.h
+++ b/include/trace/events/module.h
@@ -81,7 +81,7 @@ DECLARE_EVENT_CLASS(module_refcnt,
 
 	TP_fast_assign(
 		__entry->ip	= ip;
-		__entry->refcnt	= atomic_read(&mod->refcnt);
+		__entry->refcnt	= rcuref_read(&mod->refcnt);
 		__assign_str(name);
 	),
 
diff --git a/kernel/module/main.c b/kernel/module/main.c
index 3dbe230984336..15b26e1431f59 100644
--- a/kernel/module/main.c
+++ b/kernel/module/main.c
@@ -570,17 +570,15 @@ EXPORT_TRACEPOINT_SYMBOL(module_get);
 /* Init the unload section of the module. */
 static int module_unload_init(struct module *mod)
 {
-	/*
-	 * Initialize reference counter to MODULE_REF_BASE.
-	 * refcnt == 0 means module is going.
-	 */
-	atomic_set(&mod->refcnt, MODULE_REF_BASE);
-
 	INIT_LIST_HEAD(&mod->source_list);
 	INIT_LIST_HEAD(&mod->target_list);
 
-	/* Hold reference count during initialization. */
-	atomic_inc(&mod->refcnt);
+	/*
+	 * Initialize reference counter to MODULE_REF_BASE.
+	 * refcnt == 0 means module is going.
+	 * Hold reference count during initialization.
+	 */
+	rcuref_init(&mod->refcnt, MODULE_REF_BASE + 1);
 
 	return 0;
 }
@@ -674,25 +672,33 @@ static inline int try_force_unload(unsigned int flags)
 }
 #endif /* CONFIG_MODULE_FORCE_UNLOAD */
 
-/* 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;
 }
 
 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) {
 		*forced = try_force_unload(flags);
 		if (!(*forced))
 			return -EWOULDBLOCK;
@@ -714,7 +720,7 @@ static int try_stop_module(struct module *mod, int flags, int *forced)
  */
 int module_refcount(struct module *mod)
 {
-	return atomic_read(&mod->refcnt) - MODULE_REF_BASE;
+	return rcuref_read(&mod->refcnt) - MODULE_REF_BASE;
 }
 EXPORT_SYMBOL(module_refcount);
 
@@ -844,7 +850,7 @@ static const struct module_attribute modinfo_refcnt =
 void __module_get(struct module *module)
 {
 	if (module) {
-		atomic_inc(&module->refcnt);
+		WARN_ON(!rcuref_get(&module->refcnt));
 		trace_module_get(module, _RET_IP_);
 	}
 }
@@ -857,7 +863,7 @@ bool try_module_get(struct module *module)
 	if (module) {
 		/* Note: here, we can fail to get a reference */
 		if (likely(module_is_live(module) &&
-			   atomic_inc_not_zero(&module->refcnt) != 0))
+			   rcuref_get(&module->refcnt)))
 			trace_module_get(module, _RET_IP_);
 		else
 			ret = false;
@@ -868,11 +874,8 @@ EXPORT_SYMBOL(try_module_get);
 
 void module_put(struct module *module)
 {
-	int ret;
-
 	if (module) {
-		ret = atomic_dec_if_positive(&module->refcnt);
-		WARN_ON(ret < 0);	/* Failed to put refcount */
+		WARN_ON(rcuref_put(&module->refcnt));
 		trace_module_put(module, _RET_IP_);
 	}
 }
-- 
2.47.2

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

[Petr Pavlu]

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?

> -/* 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.

>  }
>  
>  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)) {'.

-- 
Thanks,
Petr

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

[Sebastian Andrzej Siewior]

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

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

[Petr Pavlu]

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

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

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

[Sebastian Andrzej Siewior]

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