Messages in this thread Patch in this message | | | Date | Fri, 10 Feb 2006 23:30:59 +0300 | From | Kirill Korotaev <> | Subject | Re: [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/
| |