Re: [PATCH 01/23] tref: Implement task references.

From: Eric W. Biederman
Date: Mon Mar 06 2006 - 20:38:23 EST


Oleg Nesterov <oleg@xxxxxxxxxx> writes:

> I think I have a really good idea.
>
> Forget about task ref for a moment. I thinks we can greatly
> simplify the pids management. We don't PIDTYPE_MAX hash tables,
> we need only one.
>
> The plan:
>
> kill PIDTYPE_TGID
> (copy_process/unhash_process need a simple fix)

Worth doing. But I think it is an independent problem.
I almost wonder if it is possible to let the thread group leader
exit, at which point we really would need PIDTYPE_TGID.

Or do you need this to have the thread list embedded in the task_struct?

> kill 'struct pid'
>
> Now,
>
> struct task_struct
> {
> ...
> struct list_head pids[PIDTYPE_MAX];
> struct list_head tgrp;
> ...
> };
>
> static inline struct task_struct *next_thread(struct task_struct *p)
> {
> return list_entry(p->tgrp.next, struct task_struct, tgrp);
> }
>
> struct pid_head
> {
> pid_t nr;
> struct hlist_node chain;
> struct list_head tasks[PIDTYPE_MAX];
> };

pid_head is decent but I am very tempted to call this struct pid.
Especially if we start getting a lot of pointers to them a simple
name that makes sense is useful.

> kernel/pid.c:
>
> static kmem_cache_t *pid_cachep;
>
> static struct hlist_head *pid_hash;
> #define pid_bucket(nr) (pid_hash + pid_hashfn(nr))
>
> // alloc_pidmap() becomes static,
> // do_fork() calls this instead
> struct pid_head *alloc_pid(void)
> {
> struct pid_head *pid;
>
> pid = kmem_cache_alloc(pid_cachep, GFP_KERNEL);
>
> if (likely(pid)) {
> enum pid_type type;
>
> for (type = 0; type < PIDTYPE_MAX; ++type)
> INIT_LIST_HEAD(pid->tasks + type);
>
> pid->nr = alloc_pidmap();
> hlist_add_head_rcu(&pid->chain, pid_bucket(pid->nr));
> }
>
> return pid;
> }

Hmm. I guess that works. I'm tempted to still return just a pid_t.
I guess I can't see how the struct pid_head, will be used.

There may be another problem here as well. I don't think we have a lock
at this point that makes us safe to update the hash table.

> static struct pid_head *find_pid(pid_t nr)
> {
> struct hlist_node *node;
> struct pid_head *pid;
>
> hlist_for_each_entry_rcu(pid, node, pid_bucket(nr), chain)
> if (pid->nr == nr)
> return pid;
>
> return NULL;
> }

I'm pretty certain there are uses for find_pid, outside of pid.c

> And noe we can inplement pid_ref almost for free, just add ->count
> to 'struct pid_head'.

If we want to kill the tasklist_lock we also want to add a lock
to struct pid_head. Otherwise I don't see how we can safely bump
the count, above zero. But using the tasklist_lock for the first
version shouldn't be a problem.


Eric

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