| Message ID | 20250309121914.pN6eBMvI@breakpoint.cc (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | module: Use rcuref_t for module::refcnt. | expand |
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)) {'.
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
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_); } }
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(-)