linux-kernel - Re: [PATCH tip v4 4/5] rcu: Do not call rcu_nocb_gp

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: <20151124155212.GA1165@fixme-laptop.cn.ibm.com>
Date:	Tue, 24 Nov 2015 23:52:12 +0800
From:	Boqun Feng <boqun.feng@...il.com>
To:	Daniel Wagner <daniel.wagner@...-carit.de>
Cc:	linux-kernel@...r.kernel.org, linux-rt-users@...r.kernel.org,
	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	Peter Zijlstra <peterz@...radead.org>,
	Thomas Gleixner <tglx@...utronix.de>,
	Marcelo Tosatti <mtosatti@...hat.com>,
	Paolo Bonzini <pbonzini@...hat.com>,
	Paul Gortmaker <paul.gortmaker@...driver.com>
Subject: Re: [PATCH tip v4 4/5] rcu: Do not call rcu_nocb_gp_cleanup() while
 holding rnp->lock

Hi Daniel,

On Tue, Nov 24, 2015 at 02:03:06PM +0100, Daniel Wagner wrote:
> rcu_nocb_gp_cleanup() is called while holding rnp->lock. Currently,
> this is okay because the wake_up_all() in rcu_nocb_gp_cleanup() will
> not enable the IRQs. lockdep is happy.
> 
> By switching over using swait this is not true anymore. swake_up_all()
> enables the IRQs while processing the waiters. __do_softirq() can now
> run and will eventually call rcu_process_callbacks() which wants to
> grap nrp->lock.
> 
> Let's move the rcu_nocb_gp_cleanup() call outside the lock before we
> switch over to swait.
> 

But you did introduce swait in this patch ;-)

[snip]

> 
> Signed-off-by: Daniel Wagner <daniel.wagner@...-carit.de>
> Cc: "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
> Cc: Peter Zijlstra <peterz@...radead.org>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: linux-kernel@...r.kernel.org
> ---
>  kernel/rcu/tree.c        |  4 +++-
>  kernel/rcu/tree.h        |  3 ++-
>  kernel/rcu/tree_plugin.h | 16 +++++++++++++---
>  3 files changed, 18 insertions(+), 5 deletions(-)
> 

So I tried to build this patch with a config having RCU_EXPERT=y and
RCU_NOCB_CPU=y, but I got:

In file included from include/linux/completion.h:11:0,
                 from include/linux/rcupdate.h:43,
                 from include/linux/sysctl.h:25,
                 from include/linux/timer.h:242,
                 from include/linux/workqueue.h:8,
                 from include/linux/pm.h:25,
                 from ./arch/x86/include/asm/apic.h:5,
                 from ./arch/x86/include/asm/smp.h:12,
                 from include/linux/smp.h:59,
                 from kernel/rcu/tree.c:34:
kernel/rcu/tree_plugin.h: In function ‘rcu_nocb_gp_cleanup’:
kernel/rcu/tree_plugin.h:1782:14: warning: passing argument 1 of ‘__wake_up’ from incompatible pointer type [-Wincompatible-pointer-types]
  wake_up_all(sq);
              ^
include/linux/wait.h:168:36: note: in definition of macro ‘wake_up_all’
 #define wake_up_all(x)   __wake_up(x, TASK_NORMAL, 0, NULL)


(I also attach the configure file in case you need it)

I think the reason is that you introduced swait in this patch, but
you didn't use the proper function, e.g. swake_up_all(), and also you
didn't #include <linux/swait.h> ;-)

Regards,
Boqun

> diff --git a/kernel/rcu/tree.c b/kernel/rcu/tree.c
> index 775d36c..952536d 100644
> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1568,7 +1568,6 @@ static int rcu_future_gp_cleanup(struct rcu_state *rsp, struct rcu_node *rnp)
>  	int needmore;
>  	struct rcu_data *rdp = this_cpu_ptr(rsp->rda);
>  
> -	rcu_nocb_gp_cleanup(rsp, rnp);
>  	rnp->need_future_gp[c & 0x1] = 0;
>  	needmore = rnp->need_future_gp[(c + 1) & 0x1];
>  	trace_rcu_future_gp(rnp, rdp, c,
> @@ -1972,6 +1971,7 @@ static void rcu_gp_cleanup(struct rcu_state *rsp)
>  	int nocb = 0;
>  	struct rcu_data *rdp;
>  	struct rcu_node *rnp = rcu_get_root(rsp);
> +	struct swait_queue_head *sq;
>  
>  	WRITE_ONCE(rsp->gp_activity, jiffies);
>  	raw_spin_lock_irq(&rnp->lock);
> @@ -2010,7 +2010,9 @@ static void rcu_gp_cleanup(struct rcu_state *rsp)
>  			needgp = __note_gp_changes(rsp, rnp, rdp) || needgp;
>  		/* smp_mb() provided by prior unlock-lock pair. */
>  		nocb += rcu_future_gp_cleanup(rsp, rnp);
> +		sq = rcu_nocb_gp_get(rnp);
>  		raw_spin_unlock_irq(&rnp->lock);
> +		rcu_nocb_gp_cleanup(sq);
>  		cond_resched_rcu_qs();
>  		WRITE_ONCE(rsp->gp_activity, jiffies);
>  		rcu_gp_slow(rsp, gp_cleanup_delay);
> diff --git a/kernel/rcu/tree.h b/kernel/rcu/tree.h
> index 2e991f8..3dcf6368 100644
> --- a/kernel/rcu/tree.h
> +++ b/kernel/rcu/tree.h
> @@ -608,7 +608,8 @@ static void zero_cpu_stall_ticks(struct rcu_data *rdp);
>  static void increment_cpu_stall_ticks(void);
>  static bool rcu_nocb_cpu_needs_barrier(struct rcu_state *rsp, int cpu);
>  static void rcu_nocb_gp_set(struct rcu_node *rnp, int nrq);
> -static void rcu_nocb_gp_cleanup(struct rcu_state *rsp, struct rcu_node *rnp);
> +static struct swait_queue_head *rcu_nocb_gp_get(struct rcu_node *rnp);
> +static void rcu_nocb_gp_cleanup(struct swait_queue_head *sq);
>  static void rcu_init_one_nocb(struct rcu_node *rnp);
>  static bool __call_rcu_nocb(struct rcu_data *rdp, struct rcu_head *rhp,
>  			    bool lazy, unsigned long flags);
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index b2bf396..db4f357 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -1777,9 +1777,9 @@ early_param("rcu_nocb_poll", parse_rcu_nocb_poll);
>   * Wake up any no-CBs CPUs' kthreads that were waiting on the just-ended
>   * grace period.
>   */
> -static void rcu_nocb_gp_cleanup(struct rcu_state *rsp, struct rcu_node *rnp)
> +static void rcu_nocb_gp_cleanup(struct swait_queue_head *sq)
>  {
> -	wake_up_all(&rnp->nocb_gp_wq[rnp->completed & 0x1]);
> +	wake_up_all(sq);
>  }
>  
>  /*
> @@ -1795,6 +1795,11 @@ static void rcu_nocb_gp_set(struct rcu_node *rnp, int nrq)
>  	rnp->need_future_gp[(rnp->completed + 1) & 0x1] += nrq;
>  }
>  
> +static struct swait_queue_head *rcu_nocb_gp_get(struct rcu_node *rnp)
> +{
> +	return &rnp->nocb_gp_wq[rnp->completed & 0x1];
> +}
> +
>  static void rcu_init_one_nocb(struct rcu_node *rnp)
>  {
>  	init_waitqueue_head(&rnp->nocb_gp_wq[0]);
> @@ -2469,7 +2474,7 @@ static bool rcu_nocb_cpu_needs_barrier(struct rcu_state *rsp, int cpu)
>  	return false;
>  }
>  
> -static void rcu_nocb_gp_cleanup(struct rcu_state *rsp, struct rcu_node *rnp)
> +static void rcu_nocb_gp_cleanup(struct swait_queue_head *sq)
>  {
>  }
>  
> @@ -2477,6 +2482,11 @@ static void rcu_nocb_gp_set(struct rcu_node *rnp, int nrq)
>  {
>  }
>  
> +static struct swait_queue_head *rcu_nocb_gp_get(struct rcu_node *rnp)
> +{
> +	return NULL;
> +}
> +
>  static void rcu_init_one_nocb(struct rcu_node *rnp)
>  {
>  }
> -- 
> 2.4.3
> 

View attachment "swait.config" of type "text/plain" (101926 bytes)

Download attachment "signature.asc" of type "application/pgp-signature" (474 bytes)