Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 9 Sep 2020 04:22:41 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     "Zhang, Qiang" <Qiang.Zhang@windriver.com>
Cc:     Joel Fernandes <joel@joelfernandes.org>,
        Uladzislau Rezki <urezki@gmail.com>,
        Josh Triplett <josh@joshtriplett.org>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        rcu <rcu@vger.kernel.org>, LKML <linux-kernel@vger.kernel.org>
Subject: Re: RCU:  Question   rcu_preempt_blocked_readers_cgp  in
  rcu_gp_fqs_loop func
Message-ID: <20200909112241.GE29330@paulmck-ThinkPad-P72>
Reply-To: paulmck@kernel.org
References: <20200818220245.GO27891@paulmck-ThinkPad-P72>
 <CAEXW_YRZ6RM90+aYA0JQ1war0n-D0M4peXJZE2_Uqf07xvF+5g@mail.gmail.com>
 <BYAPR11MB26323E6D956BA22DFE610A13FF5D0@BYAPR11MB2632.namprd11.prod.outlook.com>
 <20200819135654.GB3875610@google.com>
 <20200819152159.GX27891@paulmck-ThinkPad-P72>
 <20200819155808.GA8817@pc636>
 <20200820223957.GB120898@google.com>
 <20200821153328.GH2855@paulmck-ThinkPad-P72>
 <BYAPR11MB263285B19261BE8096D87F65FF260@BYAPR11MB2632.namprd11.prod.outlook.com>
 <BYAPR11MB2632FD34F68A89CCE039018BFF260@BYAPR11MB2632.namprd11.prod.outlook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <BYAPR11MB2632FD34F68A89CCE039018BFF260@BYAPR11MB2632.namprd11.prod.outlook.com>
User-Agent: Mutt/1.9.4 (2018-02-28)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Wed, Sep 09, 2020 at 07:03:39AM +0000, Zhang, Qiang wrote:
> 
> When config preempt RCU,  and then  there are multiple levels  node,  the current task is preempted  in rcu  read critical region.
> the current task be add to "rnp->blkd_tasks" link list,  and the "rnp->gp_tasks"  may be assigned a value .  these rnp is leaf node in RCU tree.
> 
> But in "rcu_gp_fqs_loop" func, we check blocked readers in root node. 
> 
> static void rcu_gp_fqs_loop(void)
>  {
>             .....
>             struct rcu_node *rnp = rcu_get_root();
>             .....
>             if (!READ_ONCE(rnp->qsmask) &&
>                                !rcu_preempt_blocked_readers_cgp(rnp))    ------> rnp is root node
>                      break;
>             ....
> }
> 
> the root node's blkd_tasks never add task, the "rnp->gp_tasks" is never be assigned value,  this check is invailed.
>  Should we check leaf nodes like this 

There are two cases:

1.	There is only a single rcu_node structure, which is both root
	and leaf.  In this case, the current check is required:  Both
	->qsmask and the ->blkd_tasks list must be checked.  Your
	rcu_preempt_blocked_readers() would work in this case, but
	the current code is a bit faster because it does not need
	to acquire the ->lock nor does it need the loop overhead.

2.	There are multiple levels.  In this case, as you say, the root
	rcu_node structure's ->blkd_tasks list will always be empty.
	But also in this case, the root rcu_node structure's ->qsmask
	cannot be zero until all the leaf rcu_node structures' ->qsmask
	fields are zero and their ->blkd_tasks lists no longer have
	tasks blocking the current grace period.  This means that your
	rcu_preempt_blocked_readers() function would never return
	true in this case.

So the current code is fine.

Are you seeing failures on mainline kernels?  If so, what is the failure
mode?

							Thanx, Paul

> --- a/kernel/rcu/tree.c
> +++ b/kernel/rcu/tree.c
> @@ -1846,6 +1846,25 @@ static bool rcu_gp_init(void)
>  	return true;
>  }
>  
> +static bool rcu_preempt_blocked_readers(void)
> +{
> +	struct rcu_node *rnp;
> +	unsigned long flags;
> +	bool ret = false;
> +
> +	rcu_for_each_leaf_node(rnp) {
> +		raw_spin_lock_irqsave_rcu_node(rnp, flags);
> +		if (rcu_preempt_blocked_readers_cgp(rnp)) {
> +			ret = true;
> +			raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> +			break;
> +		}
> +		raw_spin_unlock_irqrestore_rcu_node(rnp, flags);
> +	}
> +
> +	return ret;
> +}
> +
>  /*
>   * Helper function for swait_event_idle_exclusive() wakeup at force-quiescent-state
>   * time.
> @@ -1864,7 +1883,7 @@ static bool rcu_gp_fqs_check_wake(int *gfp)
>  		return true;
>  
>  	// The current grace period has completed.
> -	if (!READ_ONCE(rnp->qsmask) && !rcu_preempt_blocked_readers_cgp(rnp))
> +	if (!READ_ONCE(rnp->qsmask) && !rcu_preempt_blocked_readers())
>  		return true;
>  
>  	return false;
> @@ -1927,7 +1946,7 @@ static void rcu_gp_fqs_loop(void)
>  		/* Locking provides needed memory barriers. */
>  		/* If grace period done, leave loop. */
>  		if (!READ_ONCE(rnp->qsmask) &&
> -		    !rcu_preempt_blocked_readers_cgp(rnp))
> +		    !rcu_preempt_blocked_readers())
>  			break;
>  		/* If time for quiescent-state forcing, do it. */
>  		if (!time_after(rcu_state.jiffies_force_qs, jiffies) ||
> -- 
> 
> 
> thanks
> Qiang