[PATCH] chardev: Simplify usage of try_module_get()

[PATCH] chardev: Simplify usage of try_module_get()

[Uwe Kleine-König]

try_module_get(NULL) is true, so there is no need to check owner being
NULL.

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
 fs/char_dev.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/fs/char_dev.c b/fs/char_dev.c
index 950b6919fb87..6ba032442b39 100644
--- a/fs/char_dev.c
+++ b/fs/char_dev.c
@@ -350,7 +350,7 @@ static struct kobject *cdev_get(struct cdev *p)
 	struct module *owner = p->owner;
 	struct kobject *kobj;
 
-	if (owner && !try_module_get(owner))
+	if (!try_module_get(owner))
 		return NULL;
 	kobj = kobject_get_unless_zero(&p->kobj);
 	if (!kobj)
-- 
2.42.0

Re: [PATCH] chardev: Simplify usage of try_module_get()

[Christian Brauner]

On Fri, 13 Oct 2023 15:24:42 +0200, Uwe Kleine-König wrote:
> try_module_get(NULL) is true, so there is no need to check owner being
> NULL.
> 
> 

Applied to the vfs.misc branch of the vfs/vfs.git tree.
Patches in the vfs.misc branch should appear in linux-next soon.

Please report any outstanding bugs that were missed during review in a
new review to the original patch series allowing us to drop it.

It's encouraged to provide Acked-bys and Reviewed-bys even though the
patch has now been applied. If possible patch trailers will be updated.

Note that commit hashes shown below are subject to change due to rebase,
trailer updates or similar. If in doubt, please check the listed branch.

tree:   https://git.kernel.org/pub/scm/linux/kernel/git/vfs/vfs.git
branch: vfs.misc

[1/1] chardev: Simplify usage of try_module_get()
      https://git.kernel.org/vfs/vfs/c/e8591fda1b0d

Re: [PATCH] chardev: Simplify usage of try_module_get()

[Al Viro]

On Fri, Oct 13, 2023 at 03:24:42PM +0200, Uwe Kleine-König wrote:
> try_module_get(NULL) is true, so there is no need to check owner being
> NULL.
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
>  fs/char_dev.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/fs/char_dev.c b/fs/char_dev.c
> index 950b6919fb87..6ba032442b39 100644
> --- a/fs/char_dev.c
> +++ b/fs/char_dev.c
> @@ -350,7 +350,7 @@ static struct kobject *cdev_get(struct cdev *p)
>  	struct module *owner = p->owner;
>  	struct kobject *kobj;
>  
> -	if (owner && !try_module_get(owner))
> +	if (!try_module_get(owner))
>  		return NULL;
>  	kobj = kobject_get_unless_zero(&p->kobj);
>  	if (!kobj)

I wouldn't mind that, if that logics in try_module_get() was inlined.
It isn't...

Re: [PATCH] chardev: Simplify usage of try_module_get()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1457 bytes --]

On Mon, Oct 16, 2023 at 11:43:11PM +0100, Al Viro wrote:
> On Fri, Oct 13, 2023 at 03:24:42PM +0200, Uwe Kleine-König wrote:
> > try_module_get(NULL) is true, so there is no need to check owner being
> > NULL.
> > 
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > ---
> >  fs/char_dev.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/fs/char_dev.c b/fs/char_dev.c
> > index 950b6919fb87..6ba032442b39 100644
> > --- a/fs/char_dev.c
> > +++ b/fs/char_dev.c
> > @@ -350,7 +350,7 @@ static struct kobject *cdev_get(struct cdev *p)
> >  	struct module *owner = p->owner;
> >  	struct kobject *kobj;
> >  
> > -	if (owner && !try_module_get(owner))
> > +	if (!try_module_get(owner))
> >  		return NULL;
> >  	kobj = kobject_get_unless_zero(&p->kobj);
> >  	if (!kobj)
> 
> I wouldn't mind that, if that logics in try_module_get() was inlined.
> It isn't...

I don't understand what you intend to say here. What is "that"? Are you
talking about

	owner && !try_module_get(owner)

vs

	!try_module_get(owner)

or the following line with kobject_get_unless_zero? Do you doubt the
validity of my patch, or is it about something try_module_get()
could/should do more than it currently does?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] chardev: Simplify usage of try_module_get()

[Al Viro]

On Tue, Oct 17, 2023 at 11:13:53AM +0200, Uwe Kleine-König wrote:

> I don't understand what you intend to say here. What is "that"? Are you
> talking about
> 
> 	owner && !try_module_get(owner)
> 
> vs
> 
> 	!try_module_get(owner)
> 
> or the following line with kobject_get_unless_zero? Do you doubt the
> validity of my patch, or is it about something try_module_get()
> could/should do more than it currently does?

I'm saying that it would be a good idea to turn try_module_get() into
an inline in all cases, including the CONFIG_MODULE_UNLOAD one.
Turning it into something like !module || __try_module_get(module),
with the latter being out of line.  With that done, your patch would be
entirely unobjectionable...

Re: [PATCH] chardev: Simplify usage of try_module_get()

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2330 bytes --]

On Tue, Oct 17, 2023 at 10:54:13AM +0100, Al Viro wrote:
> On Tue, Oct 17, 2023 at 11:13:53AM +0200, Uwe Kleine-König wrote:
> 
> > I don't understand what you intend to say here. What is "that"? Are you
> > talking about
> > 
> > 	owner && !try_module_get(owner)
> > 
> > vs
> > 
> > 	!try_module_get(owner)
> > 
> > or the following line with kobject_get_unless_zero? Do you doubt the
> > validity of my patch, or is it about something try_module_get()
> > could/should do more than it currently does?
> 
> I'm saying that it would be a good idea to turn try_module_get() into
> an inline in all cases, including the CONFIG_MODULE_UNLOAD one.
> Turning it into something like !module || __try_module_get(module),
> with the latter being out of line.  With that done, your patch would be
> entirely unobjectionable...

I looked into that suggestion, and I don't like it.

There are three definitions for try_module_get() (slightly simplified):

 - CONFIG_MODULES=y && CONFIG_MODULE_UNLOAD=y

 	return !module || (module_is_live(module) && atomic_inc_not_zero(&module->refcnt) != 0);

 - CONFIG_MODULES=y && CONFIG_MODULE_UNLOAD=n

   	return !module || module_is_live(module);

 - CONFIG_MODULES=n

 	return true;

So splitting all three into

	!module || __try_module_get(module)

adds an unnecessary check for the CONFIG_MODULES=n case. And only
consolidating the CONFIG_MODULES=y case would allow to go a bit further
and do:

	#ifdef CONFIG_MODULES

	# ifdef CONFIG_MODULE_UNLOAD=y

	/* maybe make this an non-inline */
	static inline bool module_incr_refcnt(struct module)
	{
		return atomic_inc_not_zero(&module->refcnt) != 0;
	}

	# else /* ifdef CONFIG_MODULE_UNLOAD=y */

	# define module_incr_refcnt(module) true

	# endif /* ifdef CONFIG_MODULE_UNLOAD=y / else */

	static inline bool try_module_get(struct module *module)
	{
		if (!module)
			return true;

		return module_is_live(module) && module_incr_refcnt(module);
	}

	#else

	static inline bool try_module_get(struct module *module)
	{
		return true;
	}

	#endif

I'm not convinced this is easier ...

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]