lkml.org 
[lkml]   [2006]   [Feb]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH 04/20] pspace: Allow multiple instaces of the process id namespace
Eric,

All my commments are inline below.

> This patch modifies the fork/exit, signal handling, and pid and
> process group manipulating syscalls to support multiple process
> spaces, and implements the data for allow multiple instaces of the pid
> namespace.

[ ... skipped .... ]
> +extern struct pspace init_pspace;
> +
> +#define INVALID_PID 0x7fffffff
<<<< what is it for?

> +
> +static inline int pspace_task_visible(struct pspace *pspace, struct task_struct *tsk)
> +{
> + return (tsk->pspace == pspace) ||
> + ((tsk->pspace->child_reaper.pspace == pspace) &&
> + (tsk->pspace->child_reaper.task == tsk));
<<< the logic with child_reaper which can be somehow partly inside
pspace... and this check is not that abvious.

Actually I can't say your patch is cleaner somehow.
It is very big and most of the changes are trivial, which creates an
illusion that it is straightforward and clean.

[ ... skipped .... ]
> @@ -788,6 +801,16 @@ fastcall NORET_TYPE void do_exit(long co
> panic("Attempted to kill the idle task!");
> if (unlikely(is_init(tsk)))
> panic("Attempted to kill init!");
> +
> + /*
> + * If we are the pspace leader it is nonsense for the pspace
> + * to continue so kill everyone else in the pspace.
> + */
> + if (pspace_leader(tsk)) {
> + tsk->pspace->flags = PSPACE_EXIT;
> + kill_pspace_info(SIGKILL, (void *)1, tsk->pspace);
> + }
> +
<<<<

1.
flags are neither atomic nor protected with any lock.
So if it will be used later for something else in future, there will be
100% race. You also assigns them here, while everywhere in other places
a bit is checked.

2. due to 1) you code is buggy. in this respect do_exit() is not
serialized with copy_process().

3. due to the same 1) reason
> + kill_pspace_info(SIGKILL, (void *)1, tsk->pspace);
can miss a task being forked. Bang!!!

4.
So you are effectively inserting a code for cleaning up pspace here and
the same is actually required for other subsystems like networking/IPC etc.
I think you suppose that other resources are freed when the last
reference is dropped, but to tell the truth this is a way to deadlocks.
Because refs are put in too many places, where you can't make a real
cleanup due to locking etc. You can't for example call syncronize_net()
from bh, which is required for network cleanup.

Just an another argument why containers are easier/better and why you
will eventually end up with it.

> if (tsk->io_context)
> exit_io_context();
>

[ ... skipped ... ]

> @@ -1147,11 +1150,55 @@ retry:
> }
>
> /*
> + * kill_pspace_info() sends a signal to all processes in a process space.
> + * This is what kill(-1, sig) does.
> + */
> +
> +int __kill_pspace_info(int sig, struct siginfo *info, struct pspace *pspace)
> +{
> + struct task_struct *p = NULL;
> + int retval = 0, count = 0;
> +
> + for_each_process(p) {
> + int err;
> + /* Skip the current pspace leader */
> + if (current_pspace_leader(p))
> + continue;
> +
> + /* Skip the sender of the signal */
> + if (p->signal == current->signal)
> + continue;
> +
> + /* Skip processes outside the target process space */
> + if (!in_pspace(pspace, p))
> + continue;
> +
> + /* Finally it is a good process send the signal. */
> + err = group_send_sig_info(sig, info, p);
> + ++count;
> + if (err != -EPERM)
> + retval = err;
<<<<
why EPERM is ok?
do you want to miss some tasks?
> + }
> + return count ? retval : -ESRCH;
> +}
> +

Thanks,
Kirill

-
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/

\
 
 \ /
  Last update: 2006-02-10 21:32    [W:0.320 / U:0.092 seconds]
©2003-2011 Jasper Spaans. Advertise on this site