[RFC][PATCH] Use task_pgrp()/task_session() in copy_process
I am trying to replace process_group() and process_session() calls in copy_process() with task_pgrp() and task_session(). Since task_pid() task_pgrp(), task_session() for the swapper are NULL, I had to treat swapper as special in this patch and would like some comments. Should we instead do some magic in start_kernel() so we don't have to treat swapper special or maybe move the check for pid_t == 0 into task_pgrp()/ task_session() ? P.S: For find_attach_pid()/attach_pid(), pls see my recent patches to Containers@ and LKML. --- From: Sukadev Bhattiprolu <sukadev@us.ibm.com> Replace process_group() and process_session() with container-friendly task_pgrp() and task_session(). Signed-off-by: Sukadev Bhattiprolu <sukadev@us.ibm.com> Cc: Cedric Le Goater <clg@fr.ibm.com> Cc: Dave Hansen <haveblue@us.ibm.com> Cc: Serge Hallyn <serue@us.ibm.com> Cc: containers@lists.osdl.org --- kernel/fork.c | 9 +++++++-- 1 files changed, 7 insertions(+), 2 deletions(-) Index: lx26-20-rc2-mm1/kernel/fork.c =================================================================== --- lx26-20-rc2-mm1.orig/kernel/fork.c 2007-01-11 07:18:03.383853328 -0800 +++ lx26-20-rc2-mm1/kernel/fork.c 2007-01-11 07:19:55.550801360 -0800 @@ -1248,8 +1248,13 @@ static struct task_struct *copy_process( p->signal->tty = current->signal->tty; p->signal->pgrp = process_group(current); set_signal_session(p->signal, process_session(current)); - find_attach_pid(p, PIDTYPE_PGID, process_group(p)); - find_attach_pid(p, PIDTYPE_SID, process_session(p)); + if (current->pid) { + attach_pid(p, PIDTYPE_PGID, task_pgrp(current)); + attach_pid(p, PIDTYPE_SID, task_session(current)); + } else { + find_attach_pid(p, PIDTYPE_PGID, process_group(current)); + find_attach_pid(p, PIDTYPE_SID, process_session(current)); + } list_add_tail_rcu(&p->tasks, &init_task.tasks); __get_cpu_var(process_counts)++; _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
Re: [RFC][PATCH] Use task_pgrp()/task_session() in copy_process
On Thu, 2007-01-11 at 07:58 -0800, Sukadev Bhattiprolu wrote: > =================================================================== > --- lx26-20-rc2-mm1.orig/kernel/fork.c 2007-01-11 07:18:03.383853328 -0800 > +++ lx26-20-rc2-mm1/kernel/fork.c 2007-01-11 07:19:55.550801360 -0800 > @@ -1248,8 +1248,13 @@ static struct task_struct *copy_process( > p->signal->tty = current->signal->tty; > p->signal->pgrp = process_group(current); > set_signal_session(p->signal, process_session(current)); > - find_attach_pid(p, PIDTYPE_PGID, process_group(p)); > - find_attach_pid(p, PIDTYPE_SID, process_session(p)); > + if (current->pid) { > + attach_pid(p, PIDTYPE_PGID, task_pgrp(current)); > + attach_pid(p, PIDTYPE_SID, task_session(current)); > + } else { > + find_attach_pid(p, PIDTYPE_PGID, process_group(current)); > + find_attach_pid(p, PIDTYPE_SID, process_session(current)); > + } > > list_add_tail_rcu(&p->tasks, &init_task.tasks); > __get_cpu_var(process_counts)++; I know I've asked this before (and I know I'm going to ask it again), but why do we need both task_pgrp() and process_group() to both have similar-sounding names and both take the same kind of argument? :) This stuff _really_ needs to get cleaned up. It makes reviewing these patches much harder. In general, you should keep the hacks (which this is) to boot and init-time stuff. If you can initialize a structure so that it plays nicely for the rest of its life, do that. Don't put special cases in common code that everybody will have to look at. > Since task_pid() task_pgrp(), task_session() for the swapper are NULL, I > had to treat swapper as special in this patch and would like some comments. Can you do some research and find out _why_ these are NULL, and why they need to be kept NULL? -- Dave _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
Dave Hansen [haveblue@us.ibm.com] wrote: | On Thu, 2007-01-11 at 07:58 -0800, Sukadev Bhattiprolu wrote: | > =================================================================== | > --- lx26-20-rc2-mm1.orig/kernel/fork.c 2007-01-11 07:18:03.383853328 -0800 | > +++ lx26-20-rc2-mm1/kernel/fork.c 2007-01-11 07:19:55.550801360 -0800 | > @@ -1248,8 +1248,13 @@ static struct task_struct *copy_process( | > p->signal->tty = current->signal->tty; | > p->signal->pgrp = process_group(current); | > set_signal_session(p->signal, process_session(current)); | > - find_attach_pid(p, PIDTYPE_PGID, process_group(p)); | > - find_attach_pid(p, PIDTYPE_SID, process_session(p)); | > + if (current->pid) { | > + attach_pid(p, PIDTYPE_PGID, task_pgrp(current)); | > + attach_pid(p, PIDTYPE_SID, task_session(current)); | > + } else { | > + find_attach_pid(p, PIDTYPE_PGID, process_group(current)); | > + find_attach_pid(p, PIDTYPE_SID, process_session(current)); | > + } | > | > list_add_tail_rcu(&p->tasks, &init_task.tasks); | > __get_cpu_var(process_counts)++; | | I know I've asked this before (and I know I'm going to ask it again), | but why do we need both task_pgrp() and process_group() to both have | similar-sounding names and both take the same kind of argument? :) This | stuff _really_ needs to get cleaned up. It makes reviewing these | patches much harder. We are phasing out process_group(), process_session() which return a pid_t. I guess it also points to not having a special case for swapper. | | In general, you should keep the hacks (which this is) to boot and | init-time stuff. If you can initialize a structure so that it plays | nicely for the rest of its life, do that. Don't put special cases in | common code that everybody will have to look at. | | > Since task_pid() task_pgrp(), task_session() for the swapper are NULL, I | > had to treat swapper as special in this patch and would like some comments. | | Can you do some research and find out _why_ these are NULL, and why they | need to be kept NULL? task_struct for swapper is initialized by hand (INIT_TASK, INIT_SIGNALS etc) but no struct pid is ever allocated and attached to the swapper. This is normally done in copy_process() and so is done for all other processes starting with pid_t = 1 (/sbin/init). I am trying to understand if there is a history to it and if they need to be kept NULL. | | -- Dave _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
Sukadev Bhattiprolu <sukadev@us.ibm.com> writes: > Dave Hansen [haveblue@us.ibm.com] wrote: > | > | I know I've asked this before (and I know I'm going to ask it again), > | but why do we need both task_pgrp() and process_group() to both have > | similar-sounding names and both take the same kind of argument? :) This > | stuff _really_ needs to get cleaned up. It makes reviewing these > | patches much harder. > > We are phasing out process_group(), process_session() which return a > pid_t. I guess it also points to not having a special case for > swapper. Definitely. Removing the special cases is good. > | In general, you should keep the hacks (which this is) to boot and > | init-time stuff. If you can initialize a structure so that it plays > | nicely for the rest of its life, do that. Don't put special cases in > | common code that everybody will have to look at. > | > | > Since task_pid() task_pgrp(), task_session() for the swapper are NULL, I > | > had to treat swapper as special in this patch and would like some comments. > | > | Can you do some research and find out _why_ these are NULL, and why they > | need to be kept NULL? > > > task_struct for swapper is initialized by hand (INIT_TASK, INIT_SIGNALS > etc) but no struct pid is ever allocated and attached to the swapper. > This is normally done in copy_process() and so is done for all other > processes starting with pid_t = 1 (/sbin/init). > > I am trying to understand if there is a history to it and if they need to > be kept NULL. When attach_pid has completed successfully as well as having a struct pid pointer in your task_struct you are also on the appropriate list of that struct pid. So you can be found for signal delivery. Preserving that property for the init_task would be nice but we don't have that property for any other kernel thread so it should not be a big deal to place it in session and process group 1 before the first fork. There are enough corner cases I don't think we can set it all up with static initializers though. Largely I would suggest that we have enough information that if we are going to do this conversion we don't go through an intermediate step of find_attach_pid. There are few enough users we should just be able to do a handful of preparatory patches and just convert all of the uses of attach_pid. As for the rest of the history struct pid happened since things started being placed in git so you can find out a lot of the history and context with a simple git-log. Generally I take a fairly pragmatic approach. If I can't see a use for a change I don't send it. Which simply means attach_pid not taking a struct pid hasn't been a blocker for anything I have done lately. I think it makes sense to convert attach_pid. I think leaving an attach_find_pid behind is a horrible idea. There are not enough callers of attach_pid to make that worthwhile. set_special_pids can get it's pid from the init_task. Although we need to kill daemonize in the kernel (or at the very least upgraded it to support all of the namespaces we have merged). sys_setsid already has a struct pid for it's session so it can call __set_special_pids with that. In de_thread we already have a struct pid. In sys_setpgid we check to ensure the struct pid already exists. And in fork we already have a struct pid everywhere except that special init_task case. So it probably makes sense for pidmap_init to initialize the pid for the session and group of the idle task. And then there are no special cases left. Eric _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers
ebiederm@xmission.com (Eric W. Biederman) writes: > > So it probably makes sense for pidmap_init to initialize the > pid for the session and group of the idle task. And then there > are no special cases left. Well that almost works except if we did that alloc_pid could not successfully allocate pid 1. Grumble getting those special cases out of the boot path is a pain. If we had a non-hashed struct pid (init_pid?) that we filled in early (statically?), that would keep copy_process happy. Then we would need to call setsid() in the kernel right after the fork to assign a legitimate session and process group to pid == 1. Since the idle thread is not doing anything it shouldn't matter, although we can attach the idle thread after the fork to session and process group == 1 or set them to NULL if there is a corner case is anything that cares. Eric _______________________________________________ Containers mailing list Containers@lists.osdl.org https://lists.osdl.org/mailman/listinfo/containers