pid_max hang again...

pid_max hang again...

[Paul Larson]

In the nightly bk pull testing I do, I saw that this got commited
yesterday:

-ChangeSet@1.619, 2002-09-05 08:45:49-07:00, mingo@elte.hu
-  [PATCH] pid-max-2.5.33-A0
-  
-  This is the pid-max patch, the one i sent for 2.5.31 was botched.  I
-  have removed the 'once' debugging stupidity - now PIDs start at 0
-  again.
-  Also, for an unknown reason the previous patch missed the hunk that 
-  had the declaration of 'DEFAULT_PID_MAX' which made it not compile

It looks like this change dropped us back to the same error all this was
originally supposed to fix.  When you hit PID_MAX, get_pid() starts
looping forever looking for a free pid and hangs.  I could probably make
my original fix work on this very easily if you'd like.

I wonder though, would it be possible to do this in a more simple way by
just throttling max_threads back to something more sane if it gets
defaulted too high?  Since it gets checked before we even get to the
get_pid call in copy_process().  That would keep the number of processes
down to a sane level without the risk.

Thanks,
Paul Larson
http://www.linuxtestproject.org

Re: pid_max hang again...

[Ingo Molnar]

On 6 Sep 2002, Paul Larson wrote:

> It looks like this change dropped us back to the same error all this was
> originally supposed to fix.  When you hit PID_MAX, get_pid() starts
> looping forever looking for a free pid and hangs.  I could probably make
> my original fix work on this very easily if you'd like.

yes please send a patch for this. Reintroduction of the looping bug was
unintended.

> I wonder though, would it be possible to do this in a more simple way by
> just throttling max_threads back to something more sane if it gets
> defaulted too high?  Since it gets checked before we even get to the
> get_pid call in copy_process().  That would keep the number of processes
> down to a sane level without the risk.

this is a good approach as well, but now pid_max can be adjusted runtime
so truncating max_threads as a side-effect looks a bit problematic. We
should rather fail the fork() cleanly.

	Ingo

Re: pid_max hang again...

[Paul Larson]

On Fri, 2002-09-06 at 10:39, Ingo Molnar wrote:
> 
> On 6 Sep 2002, Paul Larson wrote:
> 
> > It looks like this change dropped us back to the same error all this was
> > originally supposed to fix.  When you hit PID_MAX, get_pid() starts
> > looping forever looking for a free pid and hangs.  I could probably make
> > my original fix work on this very easily if you'd like.
> 
> yes please send a patch for this. Reintroduction of the looping bug was
> unintended.
> 
> > I wonder though, would it be possible to do this in a more simple way by
> > just throttling max_threads back to something more sane if it gets
> > defaulted too high?  Since it gets checked before we even get to the
> > get_pid call in copy_process().  That would keep the number of processes
> > down to a sane level without the risk.
> 
> this is a good approach as well, but now pid_max can be adjusted runtime
> so truncating max_threads as a side-effect looks a bit problematic. We
> should rather fail the fork() cleanly.
I agree, unless this was just going to be temporary.  I'll pull the
get_pid() fix up to the current version and send it in a bit.

Thanks,
Paul Larson

[PATCH] pid_max hang again...

[Paul Larson]

On Fri, 2002-09-06 at 10:39, Ingo Molnar wrote:
> On 6 Sep 2002, Paul Larson wrote:
> > It looks like this change dropped us back to the same error all this was
> > originally supposed to fix.  When you hit PID_MAX, get_pid() starts
> > looping forever looking for a free pid and hangs.  I could probably make
> > my original fix work on this very easily if you'd like.
> 
> yes please send a patch for this. Reintroduction of the looping bug was
> unintended.

Here's the original one without all the bitmap stuff, JUST the fix for
the hang problem plain and simple.  It should apply cleanly against the
current bk.  The only other small bit in here moves the test for flags &
CLONE_IDLETASK outside of get_pid since we can skip the unnecessary call
to get_pid if it's true.  Except for the last part, this is the same
thing I put into 2.4 some time ago.

Please comment or apply.

Thanks,
Paul Larson

diff -Nru a/kernel/fork.c b/kernel/fork.c
--- a/kernel/fork.c	Fri Sep  6 20:19:21 2002
+++ b/kernel/fork.c	Fri Sep  6 20:19:21 2002
@@ -28,6 +28,7 @@
 #include <linux/security.h>
 #include <linux/futex.h>
 #include <linux/ptrace.h>
+#include <linux/compiler.h>
 
 #include <asm/pgtable.h>
 #include <asm/pgalloc.h>
@@ -162,12 +163,10 @@
 static int get_pid(unsigned long flags)
 {
 	struct task_struct *p;
-	int pid;
-
-	if (flags & CLONE_IDLETASK)
-		return 0;
+	int pid, beginpid;
 
 	spin_lock(&lastpid_lock);
+	beginpid = last_pid;
 	if (++last_pid > pid_max) {
 		last_pid = 300;		/* Skip daemons etc. */
 		goto inside;
@@ -188,6 +187,8 @@
 						last_pid = 300;
 					next_safe = pid_max;
 				}
+				if (unlikely(last_pid == beginpid))
+					goto nomorepids;
 				goto repeat;
 			}
 			if (p->pid > last_pid && next_safe > p->pid)
@@ -205,6 +206,11 @@
 	spin_unlock(&lastpid_lock);
 
 	return pid;
+
+nomorepids:
+	read_unlock(&tasklist_lock);
+	spin_unlock(&lastpid_lock);
+	return 0;
 }
 
 static inline int dup_mmap(struct mm_struct * mm)
@@ -707,7 +713,13 @@
 	p->state = TASK_UNINTERRUPTIBLE;
 
 	copy_flags(clone_flags, p);
-	p->pid = get_pid(clone_flags);
+	if (clone_flags & CLONE_IDLETASK)
+		p->pid = 0;
+	else {
+		p->pid = get_pid(clone_flags);
+		if (p->pid == 0)
+			goto bad_fork_cleanup;
+	}
 	p->proc_dentry = NULL;
 
 	INIT_LIST_HEAD(&p->run_list);