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:   Tue, 29 Aug 2023 16:45:08 +0200
From:   Frederic Weisbecker <frederic@kernel.org>
To:     "Joel Fernandes (Google)" <joel@joelfernandes.org>
Cc:     linux-kernel@vger.kernel.org, rcu@vger.kernel.org,
        paulmck@kernel.org, Neeraj Upadhyay <quic_neeraju@quicinc.com>,
        Josh Triplett <josh@joshtriplett.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        Steven Rostedt <rostedt@goodmis.org>,
        Mathieu Desnoyers <mathieu.desnoyers@efficios.com>,
        Lai Jiangshan <jiangshanlai@gmail.com>,
        Zqiang <qiang.zhang1211@gmail.com>
Subject: Re: [PATCH v2 4/5] tree/nocb: Improve readability of nocb_gp_wait()
Message-ID: <ZO4EdIM/JtelKHFH@lothringen>
References: <20230729142738.222208-1-joel@joelfernandes.org>
 <20230729142738.222208-5-joel@joelfernandes.org>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230729142738.222208-5-joel@joelfernandes.org>
Precedence: bulk

On Sat, Jul 29, 2023 at 02:27:34PM +0000, Joel Fernandes (Google) wrote:
>  /*
>   * No-CBs GP kthreads come here to wait for additional callbacks to show up
>   * or for grace periods to end.
>   */
>  static void nocb_gp_wait(struct rcu_data *my_rdp)
>  {
> -	bool bypass = false;
>  	int __maybe_unused cpu = my_rdp->cpu;
>  	unsigned long cur_gp_seq;
>  	unsigned long flags;
>  	bool gotcbs = false;
> -	unsigned long j = jiffies;
> -	bool lazy = false;
>  	bool needwait_gp = false; // This prevents actual uninitialized use.
>  	bool needwake;
>  	bool needwake_gp;
> +	int defer_wake_type = RCU_NOCB_WAKE_NOT;
>  	struct rcu_data *rdp, *rdp_toggling = NULL;
>  	struct rcu_node *rnp;
>  	unsigned long wait_gp_seq = 0; // Suppress "use uninitialized" warning.
> @@ -712,44 +758,24 @@ static void nocb_gp_wait(struct rcu_data *my_rdp)
>  	 * won't be ignored for long.
>  	 */
>  	list_for_each_entry(rdp, &my_rdp->nocb_head_rdp, nocb_entry_rdp) {
> -		long bypass_ncbs;
> -		bool flush_bypass = false;
> -		long lazy_ncbs;
> +		int defer_wake_type_one = RCU_NOCB_WAKE_NOT;

No need to initialize it, nocb_gp_flush_wake() always returns a value, and
it will avoid mistakes in the future if nocb_gp_flush_wake() is moved and
accidentally not called.

> +		bool flushed;
> +		bool empty;
>  
> -		trace_rcu_nocb_wake(rcu_state.name, rdp->cpu, TPS("Check"));
>  		rcu_nocb_lock_irqsave(rdp, flags);
> -		lockdep_assert_held(&rdp->nocb_lock);
> -		bypass_ncbs = rcu_cblist_n_cbs(&rdp->nocb_bypass);
> -		lazy_ncbs = READ_ONCE(rdp->lazy_len);
> +		defer_wake_type_one = nocb_gp_flush_wake(rdp, &empty, &flushed);
>  
> -		if (bypass_ncbs && (lazy_ncbs == bypass_ncbs) &&
> -		    (time_after(j, READ_ONCE(rdp->nocb_bypass_first) + jiffies_till_flush) ||
> -		     bypass_ncbs > 2 * qhimark)) {
> -			flush_bypass = true;
> -		} else if (bypass_ncbs && (lazy_ncbs != bypass_ncbs) &&
> -		    (time_after(j, READ_ONCE(rdp->nocb_bypass_first) + 1) ||
> -		     bypass_ncbs > 2 * qhimark)) {
> -			flush_bypass = true;
> -		} else if (!bypass_ncbs && rcu_segcblist_empty(&rdp->cblist)) {
> -			rcu_nocb_unlock_irqrestore(rdp, flags);
> -			continue; /* No callbacks here, try next. */
> -		}
> +		// We may need to do a deferred wakeup later for bypass/lazy
> +		// So note down what we learnt from the rdp.
> +		defer_wake_type = max(defer_wake_type_one, defer_wake_type);
>  
> -		if (flush_bypass) {
> -			// Bypass full or old, so flush it.
> -			(void)rcu_nocb_try_flush_bypass(rdp, j);
> -			bypass_ncbs = rcu_cblist_n_cbs(&rdp->nocb_bypass);
> -			lazy_ncbs = READ_ONCE(rdp->lazy_len);
> +		// Did we make any updates to main cblist? If not, no
> +		// non-deferred wake up to do for this rdp.
> +		if (!flushed && empty) {

Can you ever have (flushed && empty) ? If not you should be able to remove the
flushed parameter.

Thanks.

> +			rcu_nocb_unlock_irqrestore(rdp, flags);
> +			continue;
>  		}
>  
> -		if (bypass_ncbs) {
> -			trace_rcu_nocb_wake(rcu_state.name, rdp->cpu,
> -					    bypass_ncbs == lazy_ncbs ? TPS("Lazy") : TPS("Bypass"));
> -			if (bypass_ncbs == lazy_ncbs)
> -				lazy = true;
> -			else
> -				bypass = true;
> -		}
>  		rnp = rdp->mynode;
>