linux-kernel - Re: [PATCH] rcu: Remove superfluous versions of rcu_read_lock_sched

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]

Date:	Wed, 23 Mar 2016 09:38:46 -0700
From:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
To:	Boqun Feng <boqun.feng@...il.com>
Cc:	linux-kernel@...r.kernel.org,
	Josh Triplett <josh@...htriplett.org>,
	Steven Rostedt <rostedt@...dmis.org>,
	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>,
	Lai Jiangshan <jiangshanlai@...il.com>,
	Frederic Weisbecker <fweisbec@...il.com>
Subject: Re: [PATCH] rcu: Remove superfluous versions of
 rcu_read_lock_sched_held()

On Wed, Mar 23, 2016 at 11:11:48PM +0800, Boqun Feng wrote:
> Currently, we have four versions of rcu_read_lock_sched_held(),
> depending on the combined choices on PREEMPT_COUNT and DEBUG_LOCK_ALLOC.
> But we actually don't need to specialize those for PREEMPT_COUNT=n
> kernel. Because:
> 
> 1.	For the implementations in DEBUG_LOCK_ALLOC=n kernel, we can use
> 	preemptible() to implement one rcu_read_lock_sched_held(), which
> 	gives us the same behavior as the current two.
> 
> 2.	For the implementations in DEBUG_LOCK_ALLOC=y kernel, even when
> 	PREEMPT_COUNT=n, one CPU may block no grace period because of
> 	the same reason for the PREEMPT_COUNT=y and !PREEMPT kernel.
> 	(e.g. dynticks or cpu hotplug)
> 
> So unify the implementations of rcu_read_lock_sched_held() by using
> macro preemptible() and tightening up the lock-held checking for
> PREEMPT_COUNT=n kernel. And this will improve the readability, make the
> debug checking as expected and save several lines of code.
> 
> Signed-off-by: Boqun Feng <boqun.feng@...il.com>

Looks like a nice consolidation!  I have queued this for review and
testing, updating the commit log as follows:

	Currently, we have four versions of rcu_read_lock_sched_held(),
	depending on the combined choices on PREEMPT_COUNT and
	DEBUG_LOCK_ALLOC.  However, there is an existing function
	preemptible() that already distinguishes between the
	PREEMPT_COUNT=y and PREEMPT_COUNT=n cases, and allows these four
	implementations to be consolidated down to two.
		    
	This commit therefore uses preemptible() to achieve this
	consolidation.	Note that there could be a small performance
	regression in the case of CONFIG_DEBUG_LOCK_ALLOC=y &&
	PREEMPT_COUNT=n.  However, given the overhead associated with
	CONFIG_DEBUG_LOCK_ALLOC=y, this should be down in the noise.

Does that capture it?

							Thanx, Paul

> ---
>  include/linux/rcupdate.h | 17 +----------------
>  kernel/rcu/update.c      |  4 ++--
>  2 files changed, 3 insertions(+), 18 deletions(-)
> 
> diff --git a/include/linux/rcupdate.h b/include/linux/rcupdate.h
> index b5d48bd..e3f845b 100644
> --- a/include/linux/rcupdate.h
> +++ b/include/linux/rcupdate.h
> @@ -510,14 +510,7 @@ int rcu_read_lock_bh_held(void);
>   * CONFIG_DEBUG_LOCK_ALLOC, this assumes we are in an RCU-sched read-side
>   * critical section unless it can prove otherwise.
>   */
> -#ifdef CONFIG_PREEMPT_COUNT
>  int rcu_read_lock_sched_held(void);
> -#else /* #ifdef CONFIG_PREEMPT_COUNT */
> -static inline int rcu_read_lock_sched_held(void)
> -{
> -	return 1;
> -}
> -#endif /* #else #ifdef CONFIG_PREEMPT_COUNT */
> 
>  #else /* #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> 
> @@ -534,18 +527,10 @@ static inline int rcu_read_lock_bh_held(void)
>  	return 1;
>  }
> 
> -#ifdef CONFIG_PREEMPT_COUNT
>  static inline int rcu_read_lock_sched_held(void)
>  {
> -	return preempt_count() != 0 || irqs_disabled();
> +	return !preemptible();
>  }
> -#else /* #ifdef CONFIG_PREEMPT_COUNT */
> -static inline int rcu_read_lock_sched_held(void)
> -{
> -	return 1;
> -}
> -#endif /* #else #ifdef CONFIG_PREEMPT_COUNT */
> -
>  #endif /* #else #ifdef CONFIG_DEBUG_LOCK_ALLOC */
> 
>  #ifdef CONFIG_PROVE_RCU
> diff --git a/kernel/rcu/update.c b/kernel/rcu/update.c
> index ca828b4..3ccdc8e 100644
> --- a/kernel/rcu/update.c
> +++ b/kernel/rcu/update.c
> @@ -67,7 +67,7 @@ static int rcu_normal_after_boot;
>  module_param(rcu_normal_after_boot, int, 0);
>  #endif /* #ifndef CONFIG_TINY_RCU */
> 
> -#if defined(CONFIG_DEBUG_LOCK_ALLOC) && defined(CONFIG_PREEMPT_COUNT)
> +#ifdef CONFIG_DEBUG_LOCK_ALLOC
>  /**
>   * rcu_read_lock_sched_held() - might we be in RCU-sched read-side critical section?
>   *
> @@ -111,7 +111,7 @@ int rcu_read_lock_sched_held(void)
>  		return 0;
>  	if (debug_locks)
>  		lockdep_opinion = lock_is_held(&rcu_sched_lock_map);
> -	return lockdep_opinion || preempt_count() != 0 || irqs_disabled();
> +	return lockdep_opinion || !preemptible();
>  }
>  EXPORT_SYMBOL(rcu_read_lock_sched_held);
>  #endif
> -- 
> 2.7.3
>