Messages in this thread | | | Subject | Re: [PATCH 01/23] tref: Implement task references. | From | (Eric W. Biederman) | Date | Sat, 04 Mar 2006 04:16:19 -0700 |
| |
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/
| |