* Paul E. McKenney (paul...@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
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
