Oleg Nesterov <[email protected]> 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 [email protected]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
[Index of Archives]
[Kernel Newbies]
[Netfilter]
[Bugtraq]
[Photo]
[Stuff]
[Gimp]
[Yosemite News]
[MIPS Linux]
[ARM Linux]
[Linux Security]
[Linux RAID]
[Video 4 Linux]
[Linux for the blind]
[Linux Resources]
