Messages in this thread | | | Date | Sun, 28 Mar 2010 04:07:10 +0200 (CEST) | From | Thomas Gleixner <> | Subject | Re: [patch 0/6] rcu head debugobjects |
| |
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.
Thanks,
tglx
| |