Re: [PATCH tip/core/rcu 1/5] rcu: Add duplicate-callback tests to rcutorture

[Mathieu Desnoyers <mathieu.desnoyers@efficios.com>]

* Paul E. McKenney (paulmck@linux.vnet.ibm.com) wrote:
[...]
> The result is as follows.  Better?

Hi Paul,

Pitching in late in the thread, so that I can get a share of the fun ;-)

> 							Thanx, Paul
> 
> #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
> static void rcu_torture_leak_cb(struct rcu_head *rhp)
> {
> }
> 
> static void rcu_torture_err_cb(struct rcu_head *rhp)
> {
> 	/*
> 	 * This -might- happen due to race conditions, but is unlikely.
> 	 * The scenario that leads to this happening is that the
> 	 * first of the pair of duplicate callbacks is queued,
> 	 * someone else starts a grace period that includes that
> 	 * callback, then the second of the pair must wait for the
> 	 * next grace period.  Unlikely, but can happen.  If it
> 	 * does happen, the debug-objects subsystem won't have splatted.
> 	 */
> 	pr_alert("rcutorture: duplicated callback was invoked.\n");
> }
> #endif /* #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD */
> 

Hrm. Putting an #ifdef within a function when not utterly needed is
usually a bad idea. How about:

/*
 * Verify that double-free causes debug-objects to complain, but only
 * if CONFIG_DEBUG_OBJECTS_RCU_HEAD=y.  Otherwise, say that the test
 * cannot be carried out.
 */
#ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
static void rcu_test_debug_objects(void)
{
 	struct rcu_head rh1;
 	struct rcu_head rh2;
 
 	init_rcu_head_on_stack(&rh1);
 	init_rcu_head_on_stack(&rh2);
 	pr_alert("rcutorture: WARN: Duplicate call_rcu() test starting.\n");
 	preempt_disable(); /* Prevent preemption from interrupting test. */
 	rcu_read_lock(); /* Make it impossible to finish a grace period. */
 	call_rcu(&rh1, rcu_torture_leak_cb); /* Start grace period. */
 	local_irq_disable(); /* Make it harder to start a new grace period. */
 	call_rcu(&rh2, rcu_torture_leak_cb);
 	call_rcu(&rh2, rcu_torture_err_cb); /* Duplicate callback. */
 	local_irq_enable();
 	rcu_read_unlock();
 	preempt_enable();
 	rcu_barrier();
 	pr_alert("rcutorture: WARN: Duplicate call_rcu() test complete.\n");
 	destroy_rcu_head_on_stack(&rh1);
 	destroy_rcu_head_on_stack(&rh2);
}
#else /* #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD */
static void rcu_test_debug_objects(void)
{
 	pr_alert("rcutorture: !CONFIG_DEBUG_OBJECTS_RCU_HEAD, not testing duplicate call_rcu()\n");
}
#endif /* #else #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD */


More comments inlined in the code below,

> /*
>  * Verify that double-free causes debug-objects to complain, but only
>  * if CONFIG_DEBUG_OBJECTS_RCU_HEAD=y.  Otherwise, say that the test
>  * cannot be carried out.
>  */
> static void rcu_test_debug_objects(void)
> {
> #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD
> 	struct rcu_head rh1;
> 	struct rcu_head rh2;
> 
> 	init_rcu_head_on_stack(&rh1);
> 	init_rcu_head_on_stack(&rh2);
> 	pr_alert("rcutorture: WARN: Duplicate call_rcu() test starting.\n");
> 	preempt_disable(); /* Prevent preemption from interrupting test. */
> 	rcu_read_lock(); /* Make it impossible to finish a grace period. */
> 	call_rcu(&rh1, rcu_torture_leak_cb); /* Start grace period. */

Are we really "starting" a grace period ? If rcu_test_debug_objects() is
executed after some callbacks are already queued, are we, by definition,
"starting" the grace period ?

Also, I find it weird to have, in that order:

1) preempt_disable()
2) rcu_read_lock()
3) local_irq_disable()

I would rather expect:

1) rcu_read_lock()
2) preempt_disable()
3) local_irq_disable()

So they come in increasing order of impact on the system: with
non-preemptable RCU, the read-lock and preempt disable mean the same
thing, however, with preemptable RCU, the impact of preempt disable
seems larger than the impact of RCU read lock: preemption is still
enabled when within a RCU critical section. Both will work, but I find
this call order slightly weird.

Also, if your goal is to increase the chances that call_rcu() enqueues
both callbacks into the same grace period, you might want to issue a
rcu_barrier() early in this function, so that call_rcu() has even more
chances to enqueue the callbacks into the same grace period.

However, if you care about testing enqueue into same _and_ different
grace periods, you might want to turn this single-shot test into a
stress-test by calling it repeatedly.

Thanks!

Mathieu

> 	local_irq_disable(); /* Make it harder to start a new grace period. */
> 	call_rcu(&rh2, rcu_torture_leak_cb);
> 	call_rcu(&rh2, rcu_torture_err_cb); /* Duplicate callback. */
> 	local_irq_enable();
> 	rcu_read_unlock();
> 	preempt_enable();
> 	rcu_barrier();
> 	pr_alert("rcutorture: WARN: Duplicate call_rcu() test complete.\n");
> 	destroy_rcu_head_on_stack(&rh1);
> 	destroy_rcu_head_on_stack(&rh2);
> #else /* #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD */
> 	pr_alert("rcutorture: !CONFIG_DEBUG_OBJECTS_RCU_HEAD, not testing duplicate call_rcu()\n");
> #endif /* #else #ifdef CONFIG_DEBUG_OBJECTS_RCU_HEAD */
> }
> 
> > > +
> > >  static int __init
> > >  rcu_torture_init(void)
> > >  {
> > > @@ -2163,6 +2206,8 @@ rcu_torture_init(void)
> > >  		firsterr = retval;
> > >  		goto unwind;
> > >  	}
> > > +	if (object_debug)
> > > +		rcu_test_debug_objects();
> > >  	rcutorture_record_test_transition();
> > >  	mutex_unlock(&fullstop_mutex);
> > >  	return 0;
> > 
> 

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com