lkml.org 
[lkml]   [2013]   [Aug]   [24]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
Patch in this message
/
Date
From
SubjectRe: [PATCH tip/core/rcu 1/5] rcu: Add duplicate-callback tests to rcutorture
* 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


\
 
 \ /
  Last update: 2013-08-24 21:41    [W:0.163 / U:0.160 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site