linux-kernel - Re: [patch 0/6] rcu head debugobjects

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Sun, 28 Mar 2010 20:45:13 -0400
From:	Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	"Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>,
	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, 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

* Thomas Gleixner (tglx@...utronix.de) wrote:
> B1;2005;0cOn Sat, 27 Mar 2010, Paul E. McKenney wrote:
> > 
> > o	Patch 4/6 looks good to me, and given that Thomas hasn't
> > 	complained, I am guessing that he is OK with it.
> 
> Looks sane at the first glance. Will go over it in detail tomorrow.
>  
> > 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.
> 
> Well, I'm a bit wary about that. The reason is that we really want
> the annotation of:
> 
>        init_on_stack();
>        ....
>        destroy_on_stack();
> 
> instead of the confusing:
> 
>        init();
>        ....
>        destroy_on_stack();
> 
> So having an automatism in the bdi_work_init() function will people
> make forget to put the destroy_on_stack() annotation into it.
> 
> The flag is horrible as well. How about this:
> 
> /* helper function, do not use in code ! */
> __bdi_work_init(....., onstack)
> {
> 	....
> 	if (on_stack) {
> 	   work.state |= WS_ONSTACK;
> 	   init_rcu_head_on_stack(&work.rcu_head);
> 	} else {
> 	....
> }
> 
> See, how this moves also the "work.state |= WS_ONSTACK;" line out of
> the calling code.
> 
> bdi_work_init(...)
> {
> 	 __bdi_work_init(...., false);
> }
> 
> bdi_work_init_on_stack(...)
> {
> 	 __bdi_work_init(...., true);
> }
> 
> 
> out of the code.
> 
> To make it complete, please do not use the asymmetric:
> 
>    destroy_rcu_head_on_stack(&work.rcu_head);
> 
> Create a helper function:
> 
> bdi_destroy_work_on_stack(...)
> {
>    destroy_rcu_head_on_stack(work->rcu_head);
> }    
> 
> That makes it way more readable and we did that with the other on
> stack initializers as well.

These changes are queued for v3. Thanks Thomas!

Mathieu

> 
> Thanks,
> 
> 	tglx

-- 
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/