lkml.org 
[lkml]   [2012]   [Jun]   [23]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: deferring __fput()
    On Sat, Jun 23, 2012 at 08:45:05PM +0100, Al Viro wrote:
    > BTW, I really wonder why do we need to have that void *data in task_work; we can
    > always embed the sucker into a bigger struct (if nothing else, task_work +
    > void *data) and get to it via container_of(). And in quite a few cases we don't
    > want that data thing at all. Moreover, the reasons to use hlist_head instead of
    > a single forward pointer are very thin on the ground:
    > * task_work_add() adds to the beginning of the list
    > * task_work_cancel() walks the list to find the entry to remove
    > * trask_work_run() takes the list out, walks it to the end,
    > then walks it backwards via pprev. Could as well reverse the pointers
    > while walking forward...

    You know what... Let's dump that "reverse the list" thing completely. What we ought to
    do instead of that is honestly keeping both the head of the (single-linked) list and
    pointer to pointer to its last element. Sure, that'll eat one more word in task_struct.
    And it doesn't matter, since we'll be able to kill something else in there - namely,
    ->scm_work_list. The whole reason we have it is that we want to prevent recursion
    in __scm_destroy(); with __fput() getting done via task_work_add() this recursion
    will be gone automatically.

    So here's what I want to do:

    1) kill task_work->data; the only user that cares will allocate task_work + struct cred * and
    use container_of() to get to it.

    2) replace current->task_works with struct task_work *task_works, **last_work and replace
    hlist_node in task_work with struct task_work *next; task_work_add() would add to the tail,
    task_work_cancel() would do the obvious "scan the single-linked list, remove the element
    found" wihtout bothering with hlist and task_work_run() would just walk the list in direct
    order.

    3) at that point task_work is equal in size (and layout, BTW) to rcu_head. So we can add it
    into the same union in struct file where we already have list_head and rcu_head. No space
    eaten up. fput() would, once the counter reaches 0, remove the file from list (the only
    place walking that list skips the ones with zero refcount anyway) and, if we are in a normal
    process, use task_work_add() to have __fput() done to it. If we are in kernel thread or
    atomic context, just move the sucker to global list and use schedule_work() to have said
    list emptied and everything in it fed to __fput().

    4) kill ->scm_work_list, along with all contortions in __scm_destroy(); just have it do all
    the fput() (if any) it wants to do for this cookie.

    5) in fs/aio.c, replace
    if (unlikely(!fput_atomic(req->ki_filp))) {
    spin_lock(&fput_lock);
    list_add(&req->ki_list, &fput_head);
    spin_unlock(&fput_lock);
    schedule_work(&fput_work);
    } else {
    req->ki_filp = NULL;
    really_put_req(ctx, req);
    }
    with
    fput(req->ki_filp);
    req->ki_filp = NULL;
    really_put_req(ctx, req);
    Sure, we are not allowed to block there. And we won't. fput_head/fput_work/aio_fput_routine()
    goes to hell, and so does the whole "called_fput" mess in kiocb_batch_refill() in there - retry
    loop and all.

    Objections, anyone? Linus, Oleg, Dave?


    \
     
     \ /
      Last update: 2012-06-23 23:41    [W:2.213 / U:0.076 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site