Re: [PATCH v2 19/28] LoongArch: ftrace: Use RCU in all users of __module_text_address().

[Steven Rostedt <rostedt@goodmis.org>]

On Fri, 20 Dec 2024 18:41:33 +0100
Sebastian Andrzej Siewior <bigeasy@linutronix.de> wrote:

> --- a/arch/loongarch/kernel/ftrace_dyn.c
> +++ b/arch/loongarch/kernel/ftrace_dyn.c
> @@ -85,14 +85,13 @@ static bool ftrace_find_callable_addr(struct dyn_ftrace *rec, struct module *mod
>  	 * dealing with an out-of-range condition, we can assume it
>  	 * is due to a module being loaded far away from the kernel.
>  	 *
> -	 * NOTE: __module_text_address() must be called with preemption
> -	 * disabled, but we can rely on ftrace_lock to ensure that 'mod'
> +	 * NOTE: __module_text_address() must be called within a RCU read
> +	 * section, but we can rely on ftrace_lock to ensure that 'mod'
>  	 * retains its validity throughout the remainder of this code.
>  	 */
>  	if (!mod) {
> -		preempt_disable();
> +		guard(rcu)();
>  		mod = __module_text_address(pc);
> -		preempt_enable();
>  	}
>  
>  	if (WARN_ON(!mod))
> -- 

I personally dislike swapping one line of protection for the guard() code.

Although, I do think scoped_guard() can work.

That is:

	if (!mod) {
		read_rcu_lock();
		mod = __module_text_address(pc);
		read_rcu_unlock();
	}

Is easier to understand than:

	if (!mod) {
		guard(rcu)();
		mod = __module_text_address(pc);
	}

Because it makes me wonder, why use a guard() for a one liner?

But, when I saw your other patch, if we had:

	if (!mod) {
		scoped_guard(rcu)()
			mod = __module_text_address(pc);
	}

To me, hat looks much better than the guard() as it is obvious to what the
code is protecting. Even though, I still prefer the explicit, lock/unlock.
Maybe, just because I'm more used to it.

IMHO, guard() is for complex functions that are error prone. A single line
is not something that is error prone (unless you don't match the lock and
unlock properly, but that's pretty obvious when that happens).

But this is just my own opinion.

-- Steve