lkml.org 
[lkml]   [2010]   [Mar]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [patch 0/6] rcu head debugobjects
    On Sun, Mar 28, 2010 at 04:07:10AM +0200, Thomas Gleixner 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.

    Hello, Thomas,

    I must defer to you on this one. ;-)

    Thanx, Paul


    \
     
     \ /
      Last update: 2010-03-28 06:33    [W:0.025 / U:90.132 seconds]
    ©2003-2016 Jasper Spaans. hosted at Digital OceanAdvertise on this site