Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.34 as permitted sender) client-ip=23.128.96.34;
Message-ID: <5144fd23-e737-f23c-ce66-58d58ba13757@gmail.com>
Date:   Tue, 10 Oct 2023 14:46:27 +0800
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:102.0)
 Gecko/20100101 Thunderbird/102.15.1
Subject: Re: [PATCH 2/5] srcu: Only accelerate on enqueue time
Content-Language: en-US
To:     Frederic Weisbecker <frederic@kernel.org>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Neeraj upadhyay <neeraj.iitr10@gmail.com>,
        Joel Fernandes <joel@joelfernandes.org>,
        Boqun Feng <boqun.feng@gmail.com>,
        Uladzislau Rezki <urezki@gmail.com>, RCU <rcu@vger.kernel.org>,
        "Paul E . McKenney" <paulmck@kernel.org>,
        Yong He <zhuangel570@gmail.com>
References: <20231003232903.7109-1-frederic@kernel.org>
 <20231003232903.7109-3-frederic@kernel.org>
From:   Like Xu <like.xu.linux@gmail.com>
In-Reply-To: <20231003232903.7109-3-frederic@kernel.org>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
Precedence: bulk

On 4/10/2023 7:29 am, Frederic Weisbecker wrote:
> Acceleration in SRCU happens on enqueue time for each new callback. This
> operation is expected not to fail and therefore any similar attempt
> from other places shouldn't find any remaining callbacks to accelerate.
> 
> Moreover accelerations performed beyond enqueue time are error prone
> because rcu_seq_snap() then may return the snapshot for a new grace
> period that is not going to be started.
> 
> Remove these dangerous and needless accelerations and introduce instead
> assertions reporting leaking unaccelerated callbacks beyond enqueue
> time.
> 
> Co-developed-by: Yong He <zhuangel570@gmail.com>
> Co-developed-by: Joel Fernandes <joel@joelfernandes.org>
> Co-developed-by: Neeraj upadhyay <neeraj.iitr10@gmail.com>
> Signed-off-by: Frederic Weisbecker <frederic@kernel.org>
> ---
>   kernel/rcu/srcutree.c | 6 ++----
>   1 file changed, 2 insertions(+), 4 deletions(-)
> 
> diff --git a/kernel/rcu/srcutree.c b/kernel/rcu/srcutree.c
> index 9fab9ac36996..560e99ec5333 100644
> --- a/kernel/rcu/srcutree.c
> +++ b/kernel/rcu/srcutree.c
> @@ -784,8 +784,7 @@ static void srcu_gp_start(struct srcu_struct *ssp)
>   	spin_lock_rcu_node(sdp);  /* Interrupts already disabled. */
>   	rcu_segcblist_advance(&sdp->srcu_cblist,
>   			      rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
> -	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
> -				       rcu_seq_snap(&ssp->srcu_sup->srcu_gp_seq));
> +	WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
>   	spin_unlock_rcu_node(sdp);  /* Interrupts remain disabled. */
>   	WRITE_ONCE(ssp->srcu_sup->srcu_gp_start, jiffies);
>   	WRITE_ONCE(ssp->srcu_sup->srcu_n_exp_nodelay, 0);
> @@ -1721,6 +1720,7 @@ static void srcu_invoke_callbacks(struct work_struct *work)
>   	ssp = sdp->ssp;
>   	rcu_cblist_init(&ready_cbs);
>   	spin_lock_irq_rcu_node(sdp);
> +	WARN_ON_ONCE(!rcu_segcblist_segempty(&sdp->srcu_cblist, RCU_NEXT_TAIL));
>   	rcu_segcblist_advance(&sdp->srcu_cblist,
>   			      rcu_seq_current(&ssp->srcu_sup->srcu_gp_seq));
>   	if (sdp->srcu_cblist_invoking ||
> @@ -1750,8 +1750,6 @@ static void srcu_invoke_callbacks(struct work_struct *work)
>   	 */
>   	spin_lock_irq_rcu_node(sdp);
>   	rcu_segcblist_add_len(&sdp->srcu_cblist, -len);
> -	(void)rcu_segcblist_accelerate(&sdp->srcu_cblist,
> -				       rcu_seq_snap(&ssp->srcu_sup->srcu_gp_seq));

We did observe such issues at our farm and the same diff was applied,
thus this fix is especially appreciated. Thanks.

Reviewed-by: Like Xu <likexu@tencent.com>

>   	sdp->srcu_cblist_invoking = false;
>   	more = rcu_segcblist_ready_cbs(&sdp->srcu_cblist);
>   	spin_unlock_irq_rcu_node(sdp);