Messages in this thread | | | Date | Sat, 04 Mar 2006 15:31:08 +0300 | From | Oleg Nesterov <> | Subject | Re: [PATCH 01/23] tref: Implement task references. |
| |
"Eric W. Biederman" wrote: > > Oleg Nesterov <oleg@tv-sign.ru> writes: > > > 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.
Yes, you are right. And do_fork() does free_pidmap() lockless in the error path. This path is not performance critical, so may be it is ok to add wrie_lock(tasklist) here.
> > 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.
Agree.
Oleg. - 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/
| |