lkml.org 
[lkml]   [2010]   [Mar]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [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


\
 
 \ /
  Last update: 2010-03-28 04:15    [W:0.099 / U:0.272 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site