linux-kernel - Re: [PATCH] rcu: Avoid unneeded function call in rcu_read

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20210827183455.GP4156@paulmck-ThinkPad-P17-Gen-1>
Date:   Fri, 27 Aug 2021 11:34:55 -0700
From:   "Paul E. McKenney" <paulmck@...nel.org>
To:     Waiman Long <longman@...hat.com>
Cc:     Josh Triplett <josh@...htriplett.org>,
        Steven Rostedt <rostedt@...dmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
        Lai Jiangshan <jiangshanlai@...il.com>,
        Joel Fernandes <joel@...lfernandes.org>, rcu@...r.kernel.org,
        linux-kernel@...r.kernel.org, alexei.starovoitov@...il.com,
        andrii@...nel.org
Subject: Re: [PATCH] rcu: Avoid unneeded function call in rcu_read_unlock()

On Thu, Aug 26, 2021 at 10:21:22PM -0400, Waiman Long wrote:
> Since commit aa40c138cc8f3 ("rcu: Report QS for outermost
> PREEMPT=n rcu_read_unlock() for strict GPs"). A real function call
> rcu_read_unlock_strict() is added to the inlined rcu_read_unlock().
> The rcu_read_unlock_strict() call is only needed if the performance
> sagging CONFIG_RCU_STRICT_GRACE_PERIOD option is set. This config
> option isn't set for most production kernels while the function call
> overhead remains.
> 
> To provide a slight performance improvement, the
> CONFIG_RCU_STRICT_GRACE_PERIOD config check is moved from
> rcu_read_unlock_strict() to __rcu_read_unlock() so that the function
> call can be compiled out in most cases.
> 
> Besides, the GPL exported rcu_read_unlock_strict() also impact the
> the compilation of non-GPL kernel modules as rcu_read_unlock() is a
> frequently used kernel API.
> 
> Signed-off-by: Waiman Long <longman@...hat.com>

Nice, and good eyes!!!

I have queued this for v5.16, that is, not the upcoming merge window
but the one after that.

I did my usual wordsmithing, so please check the following in case I
messed something up.  I intentionally omitted the EXPORT_SYMBOL_GPL()
discussion because:

1.	Kernels built with CONFIG_PREEMPT=y have the same issue
	with the __rcu_read_lock() and __rcu_read_unlock() functions.

2.	Many other RCU functions are EXPORT_SYMBOL_GPL() and have
	been for almost two decades.

But if someone does use RCU readers within CONFIG_PREEMPT=n kernels from
a binary module, I will happily refer them to you for any RCU issues
that they encounter.  ;-)

I am also CCing the BPF guys in case my interpretation of the code in
the BPF verifier is incorrect.

							Thanx, Paul

------------------------------------------------------------------------

commit 4a9f53b997b809c0256838e31c604aeeded2345a
Author: Waiman Long <longman@...hat.com>
Date:   Thu Aug 26 22:21:22 2021 -0400

    rcu: Avoid unneeded function call in rcu_read_unlock()
    
    Since commit aa40c138cc8f3 ("rcu: Report QS for outermost PREEMPT=n
    rcu_read_unlock() for strict GPs") the function rcu_read_unlock_strict()
    is invoked by the inlined rcu_read_unlock() function.  However,
    rcu_read_unlock_strict() is an empty function in production kernels,
    which are built with CONFIG_RCU_STRICT_GRACE_PERIOD=n.
    
    There is a mention of rcu_read_unlock_strict() in the BPF verifier,
    but this is in a deny-list, meaning that BPF does not care whether
    rcu_read_unlock_strict() is ever called.
    
    This commit therefore provides a slight performance improvement
    by hoisting the check of CONFIG_RCU_STRICT_GRACE_PERIOD from
    rcu_read_unlock_strict() into rcu_read_unlock(), thus avoiding the
    pointless call to an empty function.
    
    Cc: Alexei Starovoitov <alexei.starovoitov@...il.com>
    Cc: Andrii Nakryiko <andrii@...nel.org>
    Signed-off-by: Waiman Long <longman@...hat.com>
    Signed-off-by: Paul E. McKenney <paulmck@...nel.org>

diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
index 434d12fe2d4f..5e0beb5c5659 100644
--- a/include/linux/rcupdate.h
+++ b/include/linux/rcupdate.h
@@ -71,7 +71,8 @@ static inline void __rcu_read_lock(void)
 static inline void __rcu_read_unlock(void)
 {
 	preempt_enable();
-	rcu_read_unlock_strict();
+	if (IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD))
+		rcu_read_unlock_strict();
 }
 
 static inline int rcu_preempt_depth(void)
diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
index 7a4876a3a882..0b55c647ab80 100644
--- a/kernel/rcu/tree_plugin.h
+++ b/kernel/rcu/tree_plugin.h
@@ -814,8 +814,7 @@ void rcu_read_unlock_strict(void)
 {
 	struct rcu_data *rdp;
 
-	if (!IS_ENABLED(CONFIG_RCU_STRICT_GRACE_PERIOD) ||
-	   irqs_disabled() || preempt_count() || !rcu_state.gp_kthread)
+	if (irqs_disabled() || preempt_count() || !rcu_state.gp_kthread)
 		return;
 	rdp = this_cpu_ptr(&rcu_data);
 	rcu_report_qs_rdp(rdp);