Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 21 Jul 2022 18:16:09 -0700
From:   "Paul E. McKenney" <paulmck@kernel.org>
To:     Zqiang <qiang1.zhang@intel.com>
Cc:     frederic@kernel.org, quic_neeraju@quicinc.com, rcu@vger.kernel.org,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rcu: Only check tasks blocked on leaf rcu_nodes for
 PREEMPT_RCU
Message-ID: <20220722011609.GD1790663@paulmck-ThinkPad-P17-Gen-1>
Reply-To: paulmck@kernel.org
References: <20220722005213.3511188-1-qiang1.zhang@intel.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220722005213.3511188-1-qiang1.zhang@intel.com>
Precedence: bulk

On Fri, Jul 22, 2022 at 08:52:13AM +0800, Zqiang wrote:
> In PREEMPT_RCU kernel, for multi-node rcu tree, if the RCU read
> critical section is preempted, the current task are only queued
> leaf rcu_node blkd list, for single-node rcu tree, the root node
> is also leaf node.
> 
> This commit add rcu_is_leaf_node() to filter out checks for non-leaf
> rcu_node.
> 
> Signed-off-by: Zqiang <qiang1.zhang@intel.com>

First, thank you for digging into this!

> ---
>  kernel/rcu/tree_plugin.h | 6 +++++-
>  1 file changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/kernel/rcu/tree_plugin.h b/kernel/rcu/tree_plugin.h
> index b2219577fbe2..a9df11ec65af 100644
> --- a/kernel/rcu/tree_plugin.h
> +++ b/kernel/rcu/tree_plugin.h
> @@ -693,6 +693,8 @@ static void rcu_preempt_check_blocked_tasks(struct rcu_node *rnp)
>  
>  	RCU_LOCKDEP_WARN(preemptible(), "rcu_preempt_check_blocked_tasks() invoked with preemption enabled!!!\n");
>  	raw_lockdep_assert_held_rcu_node(rnp);
> +	if (!rcu_is_leaf_node(rnp))
> +		goto end;
>  	if (WARN_ON_ONCE(rcu_preempt_blocked_readers_cgp(rnp)))
>  		dump_blkd_tasks(rnp, 10);
>  	if (rcu_preempt_has_tasks(rnp) &&

So you are adding a simple check for all rcu_node structures
that removes two simple checks for non-leaf rcu_node structures.

Assuming that the costs of all these checks is roughly the same (but
please feel free to actually measure them on real hardware), what what
fraction of the rcu_node structures must be non-leaf for this change to
be a win?  Then for what configurations using default fanouts is this
fraction exceeded?

The default fanout is 16 from non-leaf to leaf and 64 from non-leaf
to non-leaf (32 for non-leaf to non-leaf on 32-bit systems).

Hey, you wanted some algebra practice anyway.  ;-)

> @@ -703,6 +705,7 @@ static void rcu_preempt_check_blocked_tasks(struct rcu_node *rnp)
>  		trace_rcu_unlock_preempted_task(TPS("rcu_preempt-GPS"),
>  						rnp->gp_seq, t->pid);
>  	}
> +end:
>  	WARN_ON_ONCE(rnp->qsmask);
>  }
>  
> @@ -1178,7 +1181,8 @@ static void rcu_initiate_boost(struct rcu_node *rnp, unsigned long flags)
>   */
>  static void rcu_preempt_boost_start_gp(struct rcu_node *rnp)
>  {
> -	rnp->boost_time = jiffies + RCU_BOOST_DELAY_JIFFIES;
> +	if (rcu_is_leaf_node(rnp))
> +		rnp->boost_time = jiffies + RCU_BOOST_DELAY_JIFFIES;

And here you are adding a check on all nodes to eliminate an addition
and a store on non-leaf nodes.  Same questions as above, with the same
invitation to actually measure the costs of these operations.

						Thanx, Paul

>  }
>  
>  /*
> -- 
> 2.25.1
>