Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Wed, 28 Jun 2023 12:30:35 +0200
From:   Michal Hocko <mhocko@suse.com>
To:     Jean Delvare <jdelvare@suse.de>
Cc:     Luis Chamberlain <mcgrof@kernel.org>,
        linux-modules@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] module: print module name on refcount error
Message-ID: <ZJwLy5anSgFzbTUP@dhcp22.suse.cz>
References: <20230626123252.73dbc139@endymion.delvare>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230626123252.73dbc139@endymion.delvare>
Precedence: bulk

On Mon 26-06-23 12:32:52, Jean Delvare wrote:
> If module_put() triggers a refcount error, include the culprit
> module name in the warning message, to easy further investigation of
> the issue.
> 
> Signed-off-by: Jean Delvare <jdelvare@suse.de>
> Suggested-by: Michal Hocko <mhocko@suse.com>
> Cc: Luis Chamberlain <mcgrof@kernel.org>
> ---
>  kernel/module/main.c |    4 +++-
>  1 file changed, 3 insertions(+), 1 deletion(-)
> 
> --- linux-6.3.orig/kernel/module/main.c
> +++ linux-6.3/kernel/module/main.c
> @@ -850,7 +850,9 @@ void module_put(struct module *module)
>  	if (module) {
>  		preempt_disable();
>  		ret = atomic_dec_if_positive(&module->refcnt);
> -		WARN_ON(ret < 0);	/* Failed to put refcount */
> +		WARN(ret < 0,
> +		     KERN_WARNING "Failed to put refcount for module %s\n",
> +		     module->name);

Would it make sense to also print the refcnt here? In our internal bug
report it has turned out that this was an overflow (put missing) rather
than an underflow (too many put calls). Seeing the value could give a
clue about that. We had to configure panic_on_warn to capture a dump to
learn more which is rather impractical.

Other than that the module information on its own is an improvement
because one knows where to start looking or to reduce the tracing data
collected.

In any case
Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

>  		trace_module_put(module, _RET_IP_);
>  		preempt_enable();
>  	}
> 
> 
> -- 
> Jean Delvare
> SUSE L3 Support

-- 
Michal Hocko
SUSE Labs