| Message ID | 20240324124224.615-1-qiang.zhang1211@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | rcutorture: Make rcutorture support srcu double call test | expand |
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 >
> > 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) ? > 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. > > > - 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)? Thanks Zqiang > > Thanx, Paul > > > } > > > > -- > > 2.17.1 > >
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 > > >
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) + 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. */ - 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. */ - 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 */ }
This commit also allows rcutorture to support srcu double call test with CONFIG_DEBUG_OBJECTS_RCU_HEAD option enabled. since the spinlock will be called in call_srcu(), in RT-kernel, the spinlock is sleepable, therefore remove disable-irq and disable-preempt protection. Signed-off-by: Zqiang <qiang.zhang1211@gmail.com> --- kernel/rcu/rcutorture.c | 36 +++++++++++++++++++++--------------- 1 file changed, 21 insertions(+), 15 deletions(-)