Re: Implement fork1_to_pid(). It's fork1(), but with pid as argument
> I've now gone back and forth on this diff a few times. I like the > simplfication of allocpid(), but then it seems like deck-chair shuffling > as the test is just moved to another function. To me, it also feels like deckchair rearrangement. A special startup case condition (pid 1, and pid 0 to be honest) is handled at a particular point in the callgraph, and I don't see the point of pushing it upwards. The refactor does not seem more clear to me. > But eliminating the magic-at-a-distance randompid frobbing is nice, as is > making it clear that there's really only one PID that will ever be > special. Indeed, it would even allow kthreads to be created before init > (though what _else_ they depend on is unclear...). Maybe setting the USB > tasks loose earlier would be useful, for example. Hmm. If the usage cases were proven first...
Re: Implement fork1_to_pid(). It's fork1(), but with pid as argument
>>> Philip Guenther 27-Feb-17 06:09 >>> : > I've now gone back and forth on this diff a few times. I like the > simplfication of allocpid(), but then it seems like deck-chair shuffling > as the test is just moved to another function. > > But eliminating the magic-at-a-distance randompid frobbing is nice, as is > making it clear that there's really only one PID that will ever be > special. Indeed, it would even allow kthreads to be created before init > (though what _else_ they depend on is unclear...). Maybe setting the USB > tasks loose earlier would be useful, for example. Hmm. > > Since I'm so mixed, do any other devs have an opinion? I like the removal of the global randompid, and using a flag to ask for PID 1 is much better than the original proposal of specifying the PID. Since the bikeshed could always do with a new coat of paint, how about renaming allocpid() to randompid()? :) But you have an ok on the FORK_PID1 implementation and the man page change you suggest. Tom > > > --- a/share/man/man9/fork1.9 > > > +++ b/share/man/man9/fork1.9 > > > @@ -109,6 +109,9 @@ must also be set. > > > .It Dv FORK_PTRACE > > > The child will start with tracing enabled, as if > > > ptrace(PT_TRACE_ME, 0, 0, 0) had been invoked in the child. > > > +.It Dv FORK_PID1 > > > +Special flag to assign PID 1 for > > > +.Xr init 8 process. > > I think this would be a nice place to explicitly mention the default is a > random pid, perhaps: > .It Dv FORK_PID1 > The child is > .Xr init 8 >and is assigned PID 1 instead of a random PID. > > Philip
Re: Implement fork1_to_pid(). It's fork1(), but with pid as argument
On Fri, 17 Feb 2017, Ossi Herrala wrote: > ping? Sorry about that; life has been busy. > On Sun, Feb 12, 2017 at 07:58:27PM +0200, Ossi Herrala wrote: > > On Sun, Feb 12, 2017 at 01:34:14AM +0200, Ossi Herrala wrote: > > > > > After couple of private mails with guenther@ we came up with the > > > following patch. > > > > > > Introduce new flag FORK_PID1 for fork1(9). > > > > > > This flag is special and only for forking init(8) which wants to be > > > PID 1. > > > > > > With this flag in place, it's possible to remove global randompid > > > variable which was used to control if allocpid() returns random PIDs > > > or PID 1 for init(8). Now allocpid() can also be simplified. > > > > New patch which works with latest changes to fork1(9). > > > > This version also documents FORK_PID1 flag in fork1(9) man page. I've now gone back and forth on this diff a few times. I like the simplfication of allocpid(), but then it seems like deck-chair shuffling as the test is just moved to another function. But eliminating the magic-at-a-distance randompid frobbing is nice, as is making it clear that there's really only one PID that will ever be special. Indeed, it would even allow kthreads to be created before init (though what _else_ they depend on is unclear...). Maybe setting the USB tasks loose earlier would be useful, for example. Hmm. Since I'm so mixed, do any other devs have an opinion? > > --- a/share/man/man9/fork1.9 > > +++ b/share/man/man9/fork1.9 > > @@ -109,6 +109,9 @@ must also be set. > > .It Dv FORK_PTRACE > > The child will start with tracing enabled, as if > > ptrace(PT_TRACE_ME, 0, 0, 0) had been invoked in the child. > > +.It Dv FORK_PID1 > > +Special flag to assign PID 1 for > > +.Xr init 8 process. I think this would be a nice place to explicitly mention the default is a random pid, perhaps: .It Dv FORK_PID1 The child is .Xr init 8 and is assigned PID 1 instead of a random PID. Philip
Re: Implement fork1_to_pid(). It's fork1(), but with pid as argument
ping?
On Sun, Feb 12, 2017 at 07:58:27PM +0200, Ossi Herrala wrote:
> On Sun, Feb 12, 2017 at 01:34:14AM +0200, Ossi Herrala wrote:
>
> > After couple of private mails with guenther@ we came up with the
> > following patch.
> >
> > Introduce new flag FORK_PID1 for fork1(9).
> >
> > This flag is special and only for forking init(8) which wants to be
> > PID 1.
> >
> > With this flag in place, it's possible to remove global randompid
> > variable which was used to control if allocpid() returns random PIDs
> > or PID 1 for init(8). Now allocpid() can also be simplified.
> >
>
> New patch which works with latest changes to fork1(9).
>
> This version also documents FORK_PID1 flag in fork1(9) man page.
>
>
> ---
> share/man/man9/fork1.9 | 3 +++
> sys/kern/init_main.c | 5 ++---
> sys/kern/kern_fork.c | 24 ++--
> sys/sys/proc.h | 2 +-
> 4 files changed, 16 insertions(+), 18 deletions(-)
>
> diff --git a/share/man/man9/fork1.9 b/share/man/man9/fork1.9
> index c22c9800aa4..21175416ed4 100644
> --- a/share/man/man9/fork1.9
> +++ b/share/man/man9/fork1.9
> @@ -109,6 +109,9 @@ must also be set.
> .It Dv FORK_PTRACE
> The child will start with tracing enabled, as if
> ptrace(PT_TRACE_ME, 0, 0, 0) had been invoked in the child.
> +.It Dv FORK_PID1
> +Special flag to assign PID 1 for
> +.Xr init 8 process.
> .El
> .Pp
> If
> diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c
> index faa74aa4244..f1975a6aa89 100644
> --- a/sys/kern/init_main.c
> +++ b/sys/kern/init_main.c
> @@ -437,13 +437,12 @@ main(void *framep)
> {
> struct proc *initproc;
>
> - if (fork1(p, FORK_FORK, start_init, NULL, NULL, &initproc))
> + if (fork1(p, FORK_FORK|FORK_PID1, start_init, NULL, NULL,
> + &initproc))
> panic("fork init");
> initprocess = initproc->p_p;
> }
>
> - randompid = 1;
> -
> /*
>* Create any kernel threads whose creation was deferred because
>* initprocess had not yet been created.
> diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
> index 38c1be4a981..b5f99d3c4dc 100644
> --- a/sys/kern/kern_fork.c
> +++ b/sys/kern/kern_fork.c
> @@ -71,7 +71,6 @@
>
> int nprocesses = 1; /* process 0 */
> int nthreads = 1; /* proc 0 */
> -int randompid; /* when set to 1, pid's go random */
> struct forkstat forkstat;
>
> void fork_return(void *);
> @@ -220,7 +219,11 @@ process_new(struct proc *p, struct process *parent, int
> flags)
> (caddr_t)&pr->ps_endcopy - (caddr_t)&pr->ps_startcopy);
>
> process_initialize(pr, p);
> - pr->ps_pid = allocpid();
> +
> + if (flags & FORK_PID1)
> + pr->ps_pid = 1;
> + else
> + pr->ps_pid = allocpid();
>
> /* post-copy fixups */
> pr->ps_pptr = parent;
> @@ -332,7 +335,7 @@ fork1(struct proc *curp, int flags, void (*func)(void *),
> void *arg,
>
> KASSERT((flags & ~(FORK_FORK | FORK_VFORK | FORK_PPWAIT | FORK_PTRACE
> | FORK_IDLE | FORK_SHAREVM | FORK_SHAREFILES | FORK_NOZOMBIE
> - | FORK_SYSTEM | FORK_SIGHAND)) == 0);
> + | FORK_SYSTEM | FORK_SIGHAND | FORK_PID1)) == 0);
> KASSERT((flags & FORK_SIGHAND) == 0 || (flags & FORK_SHAREVM));
> KASSERT(func != NULL);
>
> @@ -631,19 +634,12 @@ ispidtaken(pid_t pid)
> pid_t
> allocpid(void)
> {
> - static pid_t lastpid;
> pid_t pid;
>
> - if (!randompid) {
> - /* only used early on for system processes */
> - pid = ++lastpid;
> - } else {
> - /* Find an unused pid satisfying lastpid < pid <= PID_MAX */
> - do {
> - pid = arc4random_uniform(PID_MAX - lastpid) + 1 +
> - lastpid;
> - } while (ispidtaken(pid));
> - }
> + /* Find an unused pid satisfying 1 < pid <= PID_MAX */
> + do {
> + pid = 2 + arc4random_uniform(PID_MAX - 1);
> + } while (ispidtaken(pid));
>
> return pid;
> }
> diff --git a/sys/sys/proc.h b/sys/sys/proc.h
> index 4f015f2eeb0..b3b3ac4bb0d 100644
> --- a/sys/sys/proc.h
> +++ b/sys/sys/proc.h
> @@ -449,6 +449,7 @@ struct uidinfo *uid_find(uid_t);
> #define FORK_SHAREVM 0x0080
> #define FORK_SIGHAND 0x0200
> #define FORK_PTRACE 0x0400
> +#define FORK_PID10x0800
>
> #define EXIT_NORMAL 0x0001
> #define EXIT_THREAD 0x0002
> @@ -470,7 +471,6 @@ extern struct proc proc0; /* Process slot for
> swapper. */
> extern struct process process0; /* Process slot for kernel
> threads. */
> extern int nprocesses, maxprocess; /* Cur and max number of processes. */
> extern int nthreads, maxthread; /* Cur and max number of
> threads. */
> -extern int randompid;/* fork() should create random
> pid's */
>
> LIST_HEAD(proclist, proc);
>
Re: Implement fork1_to_pid(). It's fork1(), but with pid as argument
On Sun, Feb 12, 2017 at 01:34:14AM +0200, Ossi Herrala wrote:
> After couple of private mails with guenther@ we came up with the
> following patch.
>
> Introduce new flag FORK_PID1 for fork1(9).
>
> This flag is special and only for forking init(8) which wants to be
> PID 1.
>
> With this flag in place, it's possible to remove global randompid
> variable which was used to control if allocpid() returns random PIDs
> or PID 1 for init(8). Now allocpid() can also be simplified.
>
New patch which works with latest changes to fork1(9).
This version also documents FORK_PID1 flag in fork1(9) man page.
---
share/man/man9/fork1.9 | 3 +++
sys/kern/init_main.c | 5 ++---
sys/kern/kern_fork.c | 24 ++--
sys/sys/proc.h | 2 +-
4 files changed, 16 insertions(+), 18 deletions(-)
diff --git a/share/man/man9/fork1.9 b/share/man/man9/fork1.9
index c22c9800aa4..21175416ed4 100644
--- a/share/man/man9/fork1.9
+++ b/share/man/man9/fork1.9
@@ -109,6 +109,9 @@ must also be set.
.It Dv FORK_PTRACE
The child will start with tracing enabled, as if
ptrace(PT_TRACE_ME, 0, 0, 0) had been invoked in the child.
+.It Dv FORK_PID1
+Special flag to assign PID 1 for
+.Xr init 8 process.
.El
.Pp
If
diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c
index faa74aa4244..f1975a6aa89 100644
--- a/sys/kern/init_main.c
+++ b/sys/kern/init_main.c
@@ -437,13 +437,12 @@ main(void *framep)
{
struct proc *initproc;
- if (fork1(p, FORK_FORK, start_init, NULL, NULL, &initproc))
+ if (fork1(p, FORK_FORK|FORK_PID1, start_init, NULL, NULL,
+ &initproc))
panic("fork init");
initprocess = initproc->p_p;
}
- randompid = 1;
-
/*
* Create any kernel threads whose creation was deferred because
* initprocess had not yet been created.
diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index 38c1be4a981..b5f99d3c4dc 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -71,7 +71,6 @@
intnprocesses = 1; /* process 0 */
intnthreads = 1; /* proc 0 */
-intrandompid; /* when set to 1, pid's go random */
struct forkstat forkstat;
void fork_return(void *);
@@ -220,7 +219,11 @@ process_new(struct proc *p, struct process *parent, int
flags)
(caddr_t)&pr->ps_endcopy - (caddr_t)&pr->ps_startcopy);
process_initialize(pr, p);
- pr->ps_pid = allocpid();
+
+ if (flags & FORK_PID1)
+ pr->ps_pid = 1;
+ else
+ pr->ps_pid = allocpid();
/* post-copy fixups */
pr->ps_pptr = parent;
@@ -332,7 +335,7 @@ fork1(struct proc *curp, int flags, void (*func)(void *),
void *arg,
KASSERT((flags & ~(FORK_FORK | FORK_VFORK | FORK_PPWAIT | FORK_PTRACE
| FORK_IDLE | FORK_SHAREVM | FORK_SHAREFILES | FORK_NOZOMBIE
- | FORK_SYSTEM | FORK_SIGHAND)) == 0);
+ | FORK_SYSTEM | FORK_SIGHAND | FORK_PID1)) == 0);
KASSERT((flags & FORK_SIGHAND) == 0 || (flags & FORK_SHAREVM));
KASSERT(func != NULL);
@@ -631,19 +634,12 @@ ispidtaken(pid_t pid)
pid_t
allocpid(void)
{
- static pid_t lastpid;
pid_t pid;
- if (!randompid) {
- /* only used early on for system processes */
- pid = ++lastpid;
- } else {
- /* Find an unused pid satisfying lastpid < pid <= PID_MAX */
- do {
- pid = arc4random_uniform(PID_MAX - lastpid) + 1 +
- lastpid;
- } while (ispidtaken(pid));
- }
+ /* Find an unused pid satisfying 1 < pid <= PID_MAX */
+ do {
+ pid = 2 + arc4random_uniform(PID_MAX - 1);
+ } while (ispidtaken(pid));
return pid;
}
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 4f015f2eeb0..b3b3ac4bb0d 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -449,6 +449,7 @@ struct uidinfo *uid_find(uid_t);
#define FORK_SHAREVM 0x0080
#define FORK_SIGHAND 0x0200
#define FORK_PTRACE0x0400
+#define FORK_PID1 0x0800
#define EXIT_NORMAL0x0001
#define EXIT_THREAD0x0002
@@ -470,7 +471,6 @@ extern struct proc proc0; /* Process slot for
swapper. */
extern struct process process0;/* Process slot for kernel
threads. */
extern int nprocesses, maxprocess; /* Cur and max number of processes. */
extern int nthreads, maxthread;/* Cur and max number of
threads. */
-extern int randompid; /* fork() should create random pid's */
LIST_HEAD(proclist, proc);
LIST_HEAD(processlist, process);
--
2.11.1
--
Ossi Herrala
Re: Implement fork1_to_pid(). It's fork1(), but with pid as argument
On Sun, Feb 05, 2017 at 04:51:50PM -0800, Philip Guenther wrote:
> On Sun, 5 Feb 2017, Ossi Herrala wrote:
> > init(8) is wanted to have process ID 1. It's also the only process which
> > is assigned non-random PID (well, there's also swapper as PID 0).
> >
> > This patch renames fork1() to fork1_to_pid() and introduces new argument
> > "pid" which can be used to select PID for new process. When pid is 0,
> > random PID is assigned. fork1() is then wrapper to fork1_to_pid() with
> > pid argument being 0. No functional change in fork1().
>
> I wouldn't have a problem with this diff...except it adds a *ninth*
> argument to a function. fork1() is already really bad with eight
> arguments: we need to break up its uses and refactor it to be less
> confusing and overloaded and not add Yet Another Argument With Magic
> Value.
>
After couple of private mails with guenther@ we came up with the
following patch.
Introduce new flag FORK_PID1 for fork1(9).
This flag is special and only for forking init(8) which wants to be
PID 1.
With this flag in place, it's possible to remove global randompid
variable which was used to control if allocpid() returns random PIDs
or PID 1 for init(8). Now allocpid() can also be simplified.
---
sys/kern/init_main.c | 6 ++
sys/kern/kern_fork.c | 22 +-
sys/sys/proc.h | 2 +-
3 files changed, 12 insertions(+), 18 deletions(-)
diff --git a/sys/kern/init_main.c b/sys/kern/init_main.c
index e21a8306854..7324727b415 100644
--- a/sys/kern/init_main.c
+++ b/sys/kern/init_main.c
@@ -437,14 +437,12 @@ main(void *framep)
{
struct proc *initproc;
- if (fork1(p, FORK_FORK, NULL, 0, start_init, NULL, NULL,
- &initproc))
+ if (fork1(p, FORK_FORK|FORK_PID1, NULL, 0, start_init, NULL,
+ NULL, &initproc))
panic("fork init");
initprocess = initproc->p_p;
}
- randompid = 1;
-
/*
* Create any kernel threads whose creation was deferred because
* initprocess had not yet been created.
diff --git a/sys/kern/kern_fork.c b/sys/kern/kern_fork.c
index 3ff2085f732..5ed6251d390 100644
--- a/sys/kern/kern_fork.c
+++ b/sys/kern/kern_fork.c
@@ -71,7 +71,6 @@
intnprocesses = 1; /* process 0 */
intnthreads = 1; /* proc 0 */
-intrandompid; /* when set to 1, pid's go random */
struct forkstat forkstat;
void fork_return(void *);
@@ -193,7 +192,11 @@ process_new(struct proc *p, struct process *parent, int
flags)
(caddr_t)&pr->ps_endcopy - (caddr_t)&pr->ps_startcopy);
process_initialize(pr, p);
- pr->ps_pid = allocpid();
+
+ if (flags & FORK_PID1)
+ pr->ps_pid = 1;
+ else
+ pr->ps_pid = allocpid();
/* post-copy fixups */
pr->ps_pptr = parent;
@@ -590,19 +593,12 @@ ispidtaken(pid_t pid)
pid_t
allocpid(void)
{
- static pid_t lastpid;
pid_t pid;
- if (!randompid) {
- /* only used early on for system processes */
- pid = ++lastpid;
- } else {
- /* Find an unused pid satisfying lastpid < pid <= PID_MAX */
- do {
- pid = arc4random_uniform(PID_MAX - lastpid) + 1 +
- lastpid;
- } while (ispidtaken(pid));
- }
+ /* Find an unused pid satisfying 1 < pid <= PID_MAX */
+ do {
+ pid = 2 + arc4random_uniform(PID_MAX - 1);
+ } while (ispidtaken(pid));
return pid;
}
diff --git a/sys/sys/proc.h b/sys/sys/proc.h
index 1a1f0966518..5f0b310ff23 100644
--- a/sys/sys/proc.h
+++ b/sys/sys/proc.h
@@ -457,6 +457,7 @@ struct uidinfo *uid_find(uid_t);
#define FORK_SIGHAND 0x0200
#define FORK_PTRACE0x0400
#define FORK_THREAD0x0800
+#define FORK_PID1 0x1000
#define EXIT_NORMAL0x0001
#define EXIT_THREAD0x0002
@@ -478,7 +479,6 @@ extern struct proc proc0; /* Process slot for
swapper. */
extern struct process process0;/* Process slot for kernel
threads. */
extern int nprocesses, maxprocess; /* Cur and max number of processes. */
extern int nthreads, maxthread;/* Cur and max number of
threads. */
-extern int randompid; /* fork() should create random pid's */
LIST_HEAD(proclist, proc);
LIST_HEAD(processlist, process);
--
2.11.0
--
Ossi Herrala
--
Ossi Herrala
Re: Implement fork1_to_pid(). It's fork1(), but with pid as argument
On Sun, 5 Feb 2017, Ossi Herrala wrote: > init(8) is wanted to have process ID 1. It's also the only process which > is assigned non-random PID (well, there's also swapper as PID 0). > > This patch renames fork1() to fork1_to_pid() and introduces new argument > "pid" which can be used to select PID for new process. When pid is 0, > random PID is assigned. fork1() is then wrapper to fork1_to_pid() with > pid argument being 0. No functional change in fork1(). I wouldn't have a problem with this diff...except it adds a *ninth* argument to a function. fork1() is already really bad with eight arguments: we need to break up its uses and refactor it to be less confusing and overloaded and not add Yet Another Argument With Magic Value. Philip Guenther
