Messages in this thread | | | Date | Fri, 03 Mar 2006 22:23:04 +0300 | From | Oleg Nesterov <> | Subject | Re: [PATCH 01/23] tref: Implement task references. |
| |
Eric W. Biederman wrote: > > +++ devel-akpm/kernel/task_ref.c 2006-02-27 20:28:59.000000000 -0800 > @@ -0,0 +1,131 @@ > +#include <linux/sched.h> > +#include <linux/task_ref.h> > + > +struct task_ref init_tref = { > + .count = ATOMIC_INIT(1), > + .type = PIDTYPE_PID, > + .pid = 0, > + .task = NULL, > +};
Make it static? Actually, I don't understand why init_tref is better than NULL. Yes, NULL will add some checks into task_ref.c, but we can avoid some costly atoimic ops.
> +void tref_put(struct task_ref *ref) > +{ > + might_sleep(); > + if (atomic_dec_and_test(&ref->count)) { > + struct task_struct *task; > + BUG_ON(ref == &init_tref); > + /* Carefully serialize against __detach_pid and tref_get_by_pid */ > + write_lock_irq(&tasklist_lock); > + task = ref->task; > + if (task) > + task->pids[ref->type].ref = NULL; > + write_unlock_irq(&tasklist_lock); > + kfree(ref); > + }
I think this is racy. Suppose ref->count == 1. What if another cpu does tref_get_by_task() between atomic_dec_and_test() and write_lock_irq() ? It takes tasklist_lock, increments ->count again, and returns the pointer to the memory which will be freed soon.
> +struct task_ref *tref_get_by_pid(int pid, enum pid_type type) > +{ > + struct task_struct *task; > + struct task_ref *tref; > + > + /* Lookup the and pin the task */ > + read_lock(&tasklist_lock); > + task = find_task_by_pid_type(type, pid); > + if (task) > + get_task_struct(task); > + read_unlock(&tasklist_lock); > + > + /* Now get the tref */ > + if (task) { > + tref = tref_get_by_task(task, type); > + put_task_struct(task); > + } > + else > + tref = tref_get(&init_tref); > + return tref; > +}
I beleive this could be simplified, we don't need to get/put task_struct,
rcu_read_lock();
task = find_task_by_pid_type(type, pid); if (task) tref = tref_get_by_task(task, type); else tref = tref_get(&init_tref);
rcu_read_unlock();
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/
| |