Message-ID: <20100327231421.GA4685@Krystal>
Date:	Sat, 27 Mar 2010 19:14:21 -0400
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:	akpm@...ux-foundation.org, Ingo Molnar <mingo@...e.hu>,
	linux-kernel@...r.kernel.org, laijs@...fujitsu.com,
	dipankar@...ibm.com, josh@...htriplett.org, dvhltc@...ibm.com,
	niv@...ibm.com, tglx@...utronix.de, peterz@...radead.org,
	rostedt@...dmis.org, Valdis.Kletnieks@...edu, dhowells@...hat.com,
	eric.dumazet@...il.com, adobriyan@...il.com
Subject: Re: [patch 0/6] rcu head debugobjects

* Paul E. McKenney (paulmck@...ux.vnet.ibm.com) wrote:
> On Sat, Mar 27, 2010 at 11:32:33AM -0400, Mathieu Desnoyers wrote:
> > Hi,
> > 
> > Here is an updated version of the rcu head debugobjects, following the comments
> > I received in the last rounds.
> > 
> > It applies on top of -tip, based on 2.6.34-rc2, commit
> > 2e958f219d2b8d192d44e2472a214b3a93c44673
> > 
> > Unless people have any objection, it should be ready to be merged. I think the
> > appropriate maintainer to perform this merge would be Paul E. McKenney, because
> > this patchset is RCU-related.
> 
> This should be very helpful in tracking down otherwise-painful bugs!!!
> Thank you, Mathieu!!!  I am happy to apply this, especially given Dave
> Miller's Acked-by.
> 
> A few questions and comments:
> 
> o	Patches 1/6, 2/6, 3/6: Was the intent for the three Subject
> 	lines to read as follows?
> 
> 	[patch 1/6] Revert "net: remove INIT_RCU_HEAD() usage"
> 	[patch 2/6] Revert "netfilter: don't use INIT_RCU_HEAD()"
> 	[patch 3/6] Revert "net: don't use INIT_RCU_HEAD"

Yep. Oops, got burnt by git show > patches/patchname.patch. Will fix and
re-send.

> 
> o	Patch 4/6 looks good to me, and given that Thomas hasn't
> 	complained, I am guessing that he is OK with it.

OK.

> 
> o	Would it be possible to make this bisectable as follows?
> 
> 	a.	Insert a new patch after current patch 4/6 that
> 		defines destroy_rcu_head_on_stack(),
> 		rcu_head_init_on_stack(), and rcu_head_init() with
> 		their !CONFIG_DEBUG_OBJECTS_RCU_HEAD definitions.
> 
> 	b.	Move current patch 6/6 to this position.
> 
> 	c.	Move current patch 5/6 to this position, updating
> 		to reflect the new patch added in (a) above.
> 

Sure. Will do.

> o	Patch 6/6: Would it be possible to use the object_is_on_stack()
> 	function defined in include/linux/sched.h instead of passing
> 	in the flag on_stack to bdi_work_init()?  It looks like
> 	fs/fs-writeback.c already includes include/linux/sched.h, so
> 	shouldn't be a problem from a #include-hell viewpoint.

Wow, that's cool! We learn about exciting internal API functions every day,
isn't life great ? I will definitely change the fs-writeback.c code to make use
of it.

We might event want to go further. A similar scheme could be used for the
rcu_head debugobject activation fixup. Basically, I need a way to distinguish
between:

A) objects on stack and allocated objects
and
B) objects statically initialized

So either we use something resembling:

  if (object_is_on_stack() || object_is_allocated())

or

  if (object_is_static())

I am not aware of the proper API members to do that though.

> 
> Please let me know if I am missing something on any of the above.  If
> these changes seem reasonable to you, please either submit a new patch
> set or let me know that you are OK with me making these changes.

As soon as I get the information about the static vs stack/alloc, I'll complete
the update and re-send the updated patchset.

Thanks,

Mathieu


> 
> 							Thanx, Paul

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists