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@xxxxxxxxxxxxx>
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
