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

[Uwe Kleine-König <u.kleine-koenig@xxxxxxxxxxxxxx>]

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: signature.asc
Description: PGP signature