[Devel] Re: [RFC][PATCH] Use task_pgrp()/task_session() in copy_process
Eric W. Biederman
ebiederm at xmission.com
Thu Jan 11 12:54:19 PST 2007
Sukadev Bhattiprolu <sukadev at us.ibm.com> writes:
> Dave Hansen [haveblue at 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 at lists.osdl.org
https://lists.osdl.org/mailman/listinfo/containers
More information about the Devel
mailing list