lkml.org 
[lkml]   [2006]   [Mar]   [4]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    SubjectRe: [PATCH 01/23] tref: Implement task references.
    From
    Date
    Oleg Nesterov <oleg@tv-sign.ru> writes:

    > Ok. I missed the virtualization/pspace discussion completely, so you are
    > very probably right.

    So I think the pid_ref could is likely still short several helper functions,
    but is probably usable. Using it is slightly more costly but I doubt the
    pid hash table has any significant performance penalties.

    The important property to preserve from a maintenance standpoint is
    that the helper functions take enough information that when I go back
    and implement pid spaces I will need to at most tweak the pid_ref
    implementation, and the pid_ref helper functions and not need to
    go back through and change all of the users (again).

    > Oleg.
    >
    > struct pid_ref
    > {
    > pid_t pid;
    > int count;
    > struct hlist_node chain;
    > };
    >
    > // allocated in pidhash_init()
    > static struct hlist_head *ref_hash;
    >
    > static struct pid_ref *find_pid_ref(pid_t pid)
    > {
    > struct hlist_node *elem;
    > struct pid_ref *ref;
    >
    > hlist_for_each_entry(ref, elem, &ref_hash[pid_hashfn(pid)], chain)
    > if (ref->pid == pid)
    > return ref;
    >
    > return NULL;
    > }
    >
    > // This is the only function modified.
    > fastcall void free_pidmap(int pid)
    > {
    > pidmap_t *map = pidmap_array + pid / BITS_PER_PAGE;
    > int offset = pid & BITS_PER_PAGE_MASK;
    > struct pid_ref *ref;
    >
    > clear_bit(offset, map->page);
    > atomic_inc(&map->nr_free);
    >
    > ref = find_pid_ref(pid);
    > if (unlikely(ref != NULL)) {
    > hlist_del_init(&ref->chain);
    > ref->pid = 0;
    > }
    > }

    Ouch! I believe free_pidmap now needs the tasklist_lock so
    we can free the pid and kill the pid_ref atomically. Otherwise
    the pid could potentially get reused before we free the pid reference.
    I think that means ensuring all of the callers take tasklist_lock.

    > static inline int pid_inuse(pid_t pid)
    > {
    > pidmap_t *map = pidmap_array + pid / BITS_PER_PAGE;
    > int offset = pid & BITS_PER_PAGE_MASK;
    >
    > return test_bit(offset, map->page);
    > }
    >
    > struct pid_ref *alloc_pid_ref(pid_t pid)
    > {
    > struct pid_ref *ref;
    >
    > write_lock_irq(&tasklist_lock);
    > ref = find_pid_ref(pid);
    > if (ref)
    > ref->count++;
    > else if (pid_inuse(pid)) {
    > ref = kmalloc(sizeof(*ref), GFP_ATOMIC);
    > if (ref) {
    > ref->pid = pid;
    > ref->count = 1;
    > hlist_add_head(&ref->chain,
    > &ref_hash[pid_hashfn(pid)]);
    > }
    > }
    > write_unlock_irq(&tasklist_lock);
    >
    > return ref;
    > }

    I need a helper that does this from a task structure but that
    is simple enough.

    > void free_pid_ref(struct pid_ref *ref)
    > {
    > if (!ref)
    > return;
    >
    > write_lock_irq(&tasklist_lock);
    > if (!--ref->count) {
    > hlist_del_init(&ref->chain);
    > kfree(ref);
    > }
    > write_unlock_irq(&tasklist_lock);
    > }

    I think calling this put_pid_ref instead of free_pid_ref
    is more accurate. The whole alloc/free _pid_ref instead
    of the more traditional get/put kind of throws me. Since
    an allocation/free is possible I can see where this comes from
    but I don't feel right about those names.

    Eric

    -
    To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
    the body of a message to majordomo@vger.kernel.org
    More majordomo info at http://vger.kernel.org/majordomo-info.html
    Please read the FAQ at http://www.tux.org/lkml/

    \
     
     \ /
      Last update: 2006-03-04 12:19    [W:4.064 / U:0.236 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site