Received-SPF: pass (google.com: domain of linux-kernel+bounces-115263-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Sun, 24 Mar 2024 16:57:52 -0700
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Zqiang <qiang.zhang1211@gmail.com>
Cc: frederic@kernel.org, neeraj.upadhyay@kernel.org, joel@joelfernandes.org,
	rcu@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH] rcutorture: Make rcutorture support srcu double call test
Message-ID: <fded324e-19bd-47a0-bd92-f25aaeddfc1f@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <20240324124224.615-1-qiang.zhang1211@gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240324124224.615-1-qiang.zhang1211@gmail.com>

On Sun, Mar 24, 2024 at 08:42:24PM +0800, Zqiang wrote:
> This commit also allows rcutorture to support srcu double call test
> with CONFIG_DEBUG_OBJECTS_RCU_HEAD option enabled. since the spinlock

						   ^ Comma ","?

> will be called in call_srcu(), in RT-kernel, the spinlock is sleepable,

You lost me on "the spinlock will be called in call_srcu()".

> therefore remove disable-irq and disable-preempt protection.
> 
> Signed-off-by: Zqiang <qiang.zhang1211@gmail.com>

Nice!  A question below.

> ---
>  kernel/rcu/rcutorture.c | 36 +++++++++++++++++++++---------------
>  1 file changed, 21 insertions(+), 15 deletions(-)
> 
> diff --git a/kernel/rcu/rcutorture.c b/kernel/rcu/rcutorture.c
> index 3f9c3766f52b..6571a69142f8 100644
> --- a/kernel/rcu/rcutorture.c
> +++ b/kernel/rcu/rcutorture.c
> @@ -388,6 +388,7 @@ struct rcu_torture_ops {
>  	int extendables;
>  	int slow_gps;
>  	int no_pi_lock;
> +	int debug_objects;
>  	const char *name;
>  };
>  
> @@ -573,6 +574,7 @@ static struct rcu_torture_ops rcu_ops = {
>  	.irq_capable		= 1,
>  	.can_boost		= IS_ENABLED(CONFIG_RCU_BOOST),
>  	.extendables		= RCUTORTURE_MAX_EXTEND,
> +	.debug_objects		= 1,
>  	.name			= "rcu"
>  };
>  
> @@ -743,6 +745,7 @@ static struct rcu_torture_ops srcu_ops = {
>  	.cbflood_max	= 50000,
>  	.irq_capable	= 1,
>  	.no_pi_lock	= IS_ENABLED(CONFIG_TINY_SRCU),
> +	.debug_objects	= 1,
>  	.name		= "srcu"
>  };
>  
> @@ -782,6 +785,7 @@ static struct rcu_torture_ops srcud_ops = {
>  	.cbflood_max	= 50000,
>  	.irq_capable	= 1,
>  	.no_pi_lock	= IS_ENABLED(CONFIG_TINY_SRCU),
> +	.debug_objects	= 1,
>  	.name		= "srcud"
>  };
>  
> @@ -3481,35 +3485,37 @@ static void rcu_test_debug_objects(void)
>  #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
>  	struct rcu_head rh1;
>  	struct rcu_head rh2;
> +	int idx;
> +
> +	if (!cur_ops->debug_objects || !cur_ops->call ||
> +			!cur_ops->cb_barrier)

If this is built-in, could we please WARN if there is a conflict?
Otherwise, it looks like the test succeeded.

> +		return;
> +
>  	struct rcu_head *rhp = kmalloc(sizeof(*rhp), GFP_KERNEL);
>  
>  	init_rcu_head_on_stack(&rh1);
>  	init_rcu_head_on_stack(&rh2);
> -	pr_alert("%s: WARN: Duplicate call_rcu() test starting.\n", KBUILD_MODNAME);
> +	pr_alert("%s: WARN: Duplicate call_%s() test starting.\n", KBUILD_MODNAME, cur_ops->name);
>  
>  	/* Try to queue the rh2 pair of callbacks for the same grace period. */
> -	preempt_disable(); /* Prevent preemption from interrupting test. */

What makes us not need this preempt_disable() in the RCU case?

> -	rcu_read_lock(); /* Make it impossible to finish a grace period. */
> -	call_rcu_hurry(&rh1, rcu_torture_leak_cb); /* Start grace period. */
> -	local_irq_disable(); /* Make it harder to start a new grace period. */

Same question for the local_irq_disable()?

> -	call_rcu_hurry(&rh2, rcu_torture_leak_cb);
> -	call_rcu_hurry(&rh2, rcu_torture_err_cb); /* Duplicate callback. */
> +	idx = cur_ops->readlock(); /* Make it impossible to finish a grace period. */
> +	cur_ops->call(&rh1, rcu_torture_leak_cb); /* Start grace period. */
> +	cur_ops->call(&rh2, rcu_torture_leak_cb);
> +	cur_ops->call(&rh2, rcu_torture_err_cb); /* Duplicate callback. */
>  	if (rhp) {
> -		call_rcu_hurry(rhp, rcu_torture_leak_cb);
> -		call_rcu_hurry(rhp, rcu_torture_err_cb); /* Another duplicate callback. */
> +		cur_ops->call(rhp, rcu_torture_leak_cb);
> +		cur_ops->call(rhp, rcu_torture_err_cb); /* Another duplicate callback. */
>  	}
> -	local_irq_enable();
> -	rcu_read_unlock();
> -	preempt_enable();
> +	cur_ops->readunlock(idx);
>  
>  	/* Wait for them all to get done so we can safely return. */
> -	rcu_barrier();
> -	pr_alert("%s: WARN: Duplicate call_rcu() test complete.\n", KBUILD_MODNAME);
> +	cur_ops->cb_barrier();
> +	pr_alert("%s: WARN: Duplicate call_%s() test complete.\n", KBUILD_MODNAME, cur_ops->name);
>  	destroy_rcu_head_on_stack(&rh1);
>  	destroy_rcu_head_on_stack(&rh2);
>  	kfree(rhp);
>  #else /* #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD */
> -	pr_alert("%s: !CONFIG_DEBUG_OBJECTS_RCU_HEAD, not testing duplicate call_rcu()\n", KBUILD_MODNAME);
> +	pr_alert("%s: !CONFIG_DEBUG_OBJECTS_RCU_HEAD, not testing duplicate call_%s()\n", KBUILD_MODNAME, cur_ops->name);
>  #endif /* #else #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD */

It might be possible to simplify the code by turning this #ifdef into
IS_ENABLED().

							Thanx, Paul

>  }
>  
> -- 
> 2.17.1
>