Received-SPF: pass (google.com: domain of linux-kernel+bounces-130854-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:4601:e00::3 as permitted sender) client-ip=2604:1380:4601:e00::3;
Date: Wed, 3 Apr 2024 20:19:33 -0700
From: "Paul E. McKenney" <paulmck@kernel.org>
To: Z qiang <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: <9120fc5d-4385-4151-a30c-734c61b0685d@paulmck-laptop>
Reply-To: paulmck@kernel.org
References: <20240324124224.615-1-qiang.zhang1211@gmail.com>
 <fded324e-19bd-47a0-bd92-f25aaeddfc1f@paulmck-laptop>
 <CALm+0cUiXDY3n6dvOWSmBqr3MTpAsxO+uwFzMvXySUbw1tBkZQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CALm+0cUiXDY3n6dvOWSmBqr3MTpAsxO+uwFzMvXySUbw1tBkZQ@mail.gmail.com>

On Mon, Mar 25, 2024 at 01:26:19PM +0800, Z qiang wrote:
> >
> > 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()".
> 
> Hi, Paul
> 
> I mean that
> call_srcu()
> ->srcu_gp_start_if_needed
>     ->spin_lock_irqsave_sdp_contention
>          -> spin_trylock_irqsave_rcu_node     (may be return false)
>           ->spin_lock_irqsave_rcu_node(ssp->srcu_sup, *flags);   <---spinlock
> 
> >
> > > 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?
> 
> WARN_ON_ONCE(!cur_ops->debug_objects) ?

If the RCU flavor asked for debug-objects testing, but didn't provide
the necessary functions to carry it out.  Maybe something like this
between the "if" and the "return"?

	WARN_ON_ONCE(cur_ops->debug_objects &&
		     (!cur_ops->call || !cur_ops->cb_barrier))

> > 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?
> 
> the cur_ops->readlock/unlock() can guarantee that the callback will
> not be called
> when in the readlock/unlock() critical section.
> Besides, for srcu, if invoke preempt_disable(), and the call_srcu()
> internally calls
> spinlock, which will trigger a lockdep warning in RT-kernels.

Very good!

> > > -     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().
> 
> mean that IS_ENABLED(CONFIG_DEBUG_OBJECTS_RCU_HEAD)?

That is what I was thinking of.  Does that work in this case?

							Thanx, Paul

> Thanks
> Zqiang
> 
> >
> >                                                         Thanx, Paul
> >
> > >  }
> > >
> > > --
> > > 2.17.1
> > >