introduce SYS_CLONE_MASK

For review only.

To implement for-in-kerenl-use-only CLONE_ flags, we need to filter out them in sys_clone().

These below

	arch/sparc/
	arch/sparc64/
	arch/ia64/
	arch/v850/
	arch/xtensa/

are not changed, they use assembly to implement sys_clone().

 include/linux/sched.h               |    3 +++
 arch/alpha/kernel/process.c         |    3 ++-
 arch/arm/kernel/sys_arm.c           |    3 ++-
 arch/arm26/kernel/sys_arm.c         |    2 +-
 arch/avr32/kernel/process.c         |    2 +-
 arch/blackfin/kernel/process.c      |    2 +-
 arch/cris/arch-v10/kernel/process.c |    3 ++-
 arch/cris/arch-v32/kernel/process.c |    3 ++-
 arch/frv/kernel/process.c           |    3 ++-
 arch/h8300/kernel/process.c         |    2 +-
 arch/i386/kernel/process.c          |    2 +-
 arch/m32r/kernel/process.c          |    2 +-
 arch/m68k/kernel/process.c          |    2 +-
 arch/m68knommu/kernel/process.c     |    2 +-
 arch/mips/kernel/linux32.c          |    2 +-
 arch/mips/kernel/syscall.c          |    2 +-
 arch/parisc/kernel/process.c        |    3 ++-
 arch/powerpc/kernel/process.c       |    3 ++-
 arch/s390/kernel/compat_linux.c     |    2 +-
 arch/s390/kernel/process.c          |    2 +-
 arch/sh/kernel/process.c            |    2 +-
 arch/sh64/kernel/process.c          |    2 +-
 arch/um/sys-i386/syscalls.c         |    4 ++--
 arch/um/sys-x86_64/syscalls.c       |    4 ++--
 arch/x86_64/ia32/sys_ia32.c         |    3 ++-
 arch/x86_64/kernel/process.c        |    3 ++-
 arch/xtensa/kernel/process.c        |    3 ++-
 27 files changed, 41 insertions(+), 28 deletions(-)

@@ -27,6 +27,9 @@
 #define CLONE_NEWUTS		0x04000000	/* New utsname group? */
 #define CLONE_NEWIPC		0x08000000	/* New ipcs */
 
+/* user-space visible */
+#define SYS_CLONE_MASK		(~0x0)
+
 /*
  * Scheduling policies
  */

@@ -249,7 +249,8 @@ alpha_clone(unsigned long clone_flags, u
 	if (!usp)
 		usp = rdusp();
 
-	return do_fork(clone_flags, usp, regs, 0, parent_tid, child_tid);
+	return do_fork(clone_flags & SYS_CLONE_MASK, usp, regs, 0,
+			parent_tid, child_tid);
 }
 
 int

@@ -252,7 +252,8 @@ asmlinkage int sys_clone(unsigned long c
 	if (!newsp)
 		newsp = regs->ARM_sp;
 
-	return do_fork(clone_flags, newsp, regs, 0, parent_tidptr, child_tidptr);
+	return do_fork(clone_flags & SYS_CLONE_MASK, newsp, regs, 0,
+			parent_tidptr, child_tidptr);
 }
 
 asmlinkage int sys_vfork(struct pt_regs *regs)

@@ -256,7 +256,7 @@ asmlinkage int sys_clone(unsigned long c
 	if (!newsp)
 		newsp = regs->ARM_sp;
 
-	return do_fork(clone_flags, newsp, regs, 0, NULL, NULL);
+	return do_fork(clone_flags & SYS_CLONE_MASK, newsp, regs, 0, NULL, NULL);
 }
 
 asmlinkage int sys_vfork(struct pt_regs *regs)

@@ -359,7 +359,7 @@ asmlinkage int sys_clone(unsigned long c
 {
 	if (!newsp)
 		newsp = regs->sp;
-	return do_fork(clone_flags, newsp, regs, 0,
+	return do_fork(clone_flags & SYS_CLONE_MASK, newsp, regs, 0,
 		       (int __user *)parent_tidptr,
 		       (int __user *)child_tidptr);
 }

@@ -239,7 +239,7 @@ asmlinkage int bfin_clone(struct pt_regs
 		newsp = rdusp();
 	else
 		newsp -= 12;
-	return do_fork(clone_flags, newsp, regs, 0, NULL, NULL);
+	return do_fork(clone_flags & SYS_CLONE_MASK, newsp, regs, 0, NULL, NULL);
 }
 
 int

@@ -189,7 +189,8 @@ asmlinkage int sys_clone(unsigned long n
 {
 	if (!newusp)
 		newusp = rdusp();
-	return do_fork(flags, newusp, regs, 0, parent_tid, child_tid);
+	return do_fork(flags & SYS_CLONE_MASK, newusp, regs, 0,
+			parent_tid, child_tid);
 }
 
 /* vfork is a system call in i386 because of register-pressure - maybe

@@ -206,7 +206,8 @@ sys_clone(unsigned long newusp, unsigned
 	if (!newusp)
 		newusp = rdusp();
 
-	return do_fork(flags, newusp, regs, 0, parent_tid, child_tid);
+	return do_fork(flags & SYS_CLONE_MASK, newusp, regs, 0,
+			parent_tid, child_tid);
 }
 
 /*

@@ -183,7 +183,8 @@ asmlinkage int sys_clone(unsigned long c
 {
 	if (!newsp)
 		newsp = user_stack(__frame);
-	return do_fork(clone_flags, newsp, __frame, 0, parent_tidptr, child_tidptr);
+	return do_fork(clone_flags & SYS_CLONE_MASK, newsp, __frame, 0,
+			parent_tidptr, child_tidptr);
 } /* end sys_clone() */
 
 /*****************************************************************************/

@@ -187,7 +187,7 @@ asmlinkage int h8300_clone(struct pt_reg
 	newsp = regs->er2;
 	if (!newsp)
 		newsp  = rdusp();
-	return do_fork(clone_flags, newsp, regs, 0, NULL, NULL);
+	return do_fork(clone_flags & SYS_CLONE_MASK, newsp, regs, 0, NULL, NULL);
 
 }
 

@@ -727,7 +727,7 @@ asmlinkage int sys_clone(struct pt_regs 
 	unsigned long newsp;
 	int __user *parent_tidptr, *child_tidptr;
 
-	clone_flags = regs.ebx;
+	clone_flags = regs.ebx & SYS_CLONE_MASK;
 	newsp = regs.ecx;
 	parent_tidptr = (int __user *)regs.edx;
 	child_tidptr = (int __user *)regs.edi;

@@ -289,7 +289,7 @@ asmlinkage int sys_clone(unsigned long c
 	if (!newsp)
 		newsp = regs.spu;
 
-	return do_fork(clone_flags, newsp, &regs, 0,
+	return do_fork(clone_flags & SYS_CLONE_MASK, newsp, &regs, 0,
 		       (int __user *)parent_tidptr, (int __user *)child_tidptr);
 }
 

@@ -225,7 +225,7 @@ asmlinkage int m68k_clone(struct pt_regs
 	int __user *parent_tidptr, *child_tidptr;
 
 	/* syscall2 puts clone_flags in d1 and usp in d2 */
-	clone_flags = regs->d1;
+	clone_flags = regs->d1 & SYS_CLONE_MASK;
 	newsp = regs->d2;
 	parent_tidptr = (int __user *)regs->d3;
 	child_tidptr = (int __user *)regs->d4;

@@ -192,7 +192,7 @@ asmlinkage int m68k_clone(struct pt_regs
 	unsigned long newsp;
 
 	/* syscall2 puts clone_flags in d1 and usp in d2 */
-	clone_flags = regs->d1;
+	clone_flags = regs->d1 & SYS_CLONE_MASK;
 	newsp = regs->d2;
 	if (!newsp)
 		newsp = rdusp();

@@ -564,7 +564,7 @@ _sys32_clone(nabi_no_regargs struct pt_r
 	unsigned long newsp;
 	int __user *parent_tidptr, *child_tidptr;
 
-	clone_flags = regs.regs[4];
+	clone_flags = regs.regs[4] & SYS_CLONE_MASK;
 	newsp = regs.regs[5];
 	if (!newsp)
 		newsp = regs.regs[29];

@@ -181,7 +181,7 @@ _sys_clone(nabi_no_regargs struct pt_reg
 	unsigned long newsp;
 	int __user *parent_tidptr, *child_tidptr;
 
-	clone_flags = regs.regs[4];
+	clone_flags = regs.regs[4] & SYS_CLONE_MASK;
 	newsp = regs.regs[5];
 	if (!newsp)
 		newsp = regs.regs[29];

@@ -252,7 +252,8 @@ sys_clone(unsigned long clone_flags, uns
 	else
 	  child_tidptr = NULL;
 
-	return do_fork(clone_flags, usp, regs, 0, parent_tidptr, child_tidptr);
+	return do_fork(clone_flags & SYS_CLONE_MASK, usp, regs, 0,
+			parent_tidptr, child_tidptr);
 }
 
 int

@@ -775,7 +775,8 @@ int sys_clone(unsigned long clone_flags,
 		child_tidp = TRUNC_PTR(child_tidp);
 	}
 #endif
- 	return do_fork(clone_flags, usp, regs, 0, parent_tidp, child_tidp);
+ 	return do_fork(clone_flags & SYS_CLONE_MASK, usp, regs, 0,
+ 			parent_tidp, child_tidp);
 }
 
 int sys_fork(unsigned long p1, unsigned long p2, unsigned long p3,

@@ -927,7 +927,7 @@ asmlinkage long sys32_clone(void)
 	unsigned long newsp;
 	int __user *parent_tidptr, *child_tidptr;
 
-	clone_flags = regs->gprs[3] & 0xffffffffUL;
+	clone_flags = regs->gprs[3] & SYS_CLONE_MASK;
 	newsp = regs->orig_gpr2 & 0x7fffffffUL;
 	parent_tidptr = compat_ptr(regs->gprs[4]);
 	child_tidptr = compat_ptr(regs->gprs[5]);

@@ -292,7 +292,7 @@ asmlinkage long sys_clone(void)
 	unsigned long newsp;
 	int __user *parent_tidptr, *child_tidptr;
 
-	clone_flags = regs->gprs[3];
+	clone_flags = regs->gprs[3] & SYS_CLONE_MASK;
 	newsp = regs->orig_gpr2;
 	parent_tidptr = (int __user *) regs->gprs[4];
 	child_tidptr = (int __user *) regs->gprs[5];

@@ -393,7 +393,7 @@ asmlinkage int sys_clone(unsigned long c
 	struct pt_regs *regs = RELOC_HIDE(&__regs, 0);
 	if (!newsp)
 		newsp = regs->regs[15];
-	return do_fork(clone_flags, newsp, regs, 0,
+	return do_fork(clone_flags & SYS_CLONE_MASK, newsp, regs, 0,
 			(int __user *)parent_tidptr,
 			(int __user *)child_tidptr);
 }

@@ -548,7 +548,7 @@ asmlinkage int sys_clone(unsigned long c
 {
 	if (!newsp)
 		newsp = pregs->regs[15];
-	return do_fork(clone_flags, newsp, pregs, 0, 0, 0);
+	return do_fork(clone_flags & SYS_CLONE_MASK, newsp, pregs, 0, 0, 0);
 }
 
 /*

@@ -78,8 +78,8 @@ long sys_clone(unsigned long clone_flags
 		newsp = UPT_SP(&current->thread.regs.regs);
 
 	current->thread.forking = 1;
-	ret = do_fork(clone_flags, newsp, &current->thread.regs, 0, parent_tid,
-		      child_tid);
+	ret = do_fork(clone_flags & SYS_CLONE_MASK, newsp,
+			&current->thread.regs, 0, parent_tid, child_tid);
 	current->thread.forking = 0;
 	return ret;
 }

@@ -135,8 +135,8 @@ long sys_clone(unsigned long clone_flags
 	if (!newsp)
 		newsp = UPT_SP(&current->thread.regs.regs);
 	current->thread.forking = 1;
-	ret = do_fork(clone_flags, newsp, &current->thread.regs, 0, parent_tid,
-		      child_tid);
+	ret = do_fork(clone_flags & SYS_CLONE_MASK, newsp,
+			&current->thread.regs, 0, parent_tid, child_tid);
 	current->thread.forking = 0;
 	return ret;
 }

@@ -811,7 +811,8 @@ asmlinkage long sys32_clone(unsigned int
 	void __user *child_tid = (void __user *)regs->rdi;
 	if (!newsp)
 		newsp = regs->rsp;
-        return do_fork(clone_flags, newsp, regs, 0, parent_tid, child_tid);
+        return do_fork(clone_flags & SYS_CLONE_MASK, newsp, regs, 0,
+        		parent_tid, child_tid);
 }
 
 /*

@@ -715,7 +715,8 @@ sys_clone(unsigned long clone_flags, uns
 {
 	if (!newsp)
 		newsp = regs->rsp;
-	return do_fork(clone_flags, newsp, regs, 0, parent_tid, child_tid);
+	return do_fork(clone_flags & SYS_CLONE_MASK, newsp, regs, 0,
+			parent_tid, child_tid);
 }
 
 /*

@@ -430,7 +430,8 @@ long xtensa_clone(unsigned long clone_fl
 {
         if (!newsp)
                 newsp = regs->areg[1];
-        return do_fork(clone_flags, newsp, regs, 0, parent_tid, child_tid);
+        return do_fork(clone_flags & SYS_CLONE_MASK, newsp, regs, 0,
+        		parent_tid, child_tid);
 }
 
 /*

Nack

The current clone_flags field is for user space consumption and we have proposed users for all or almost all of the remaining bits.

If we are going to have kernel only flags please use an additional argument to do_fork and copy_process.

Your current scheme also has the bad side that if user space supplied a kernel flag it is hard to detect it and return -EINVAL. Which limits future expansion. Silently dropping clone flags is a real pain, if you are trying to detect if a new flag has been implemented.

Eric

OK.

Yes, we can do this. But we have a number of architectures which use sys_clone() to implement kernel_thread(). It would be nice to have an architecture neutral kernel_thread() implementation as you proposed. We should change all of them if we want to add a new parameter to do_fork().

Perhaps it is better to add reparent_kthread() (next patch) to kthread() and forget about CLONE_KERNEL_THREAD.

Anyway, re-parenting to swapper breaks pstree, it doesn't show kernel threads. And if ->parent == /sbin/init, we can't remove us from ->children (unless we forbid sub-thread-of-init exec). So the only safe change is set ->exit_state = -1.

Yes. But that is what we are doing now. copy_process() just ignores unknown flags.

Oleg.

Please.

Yes. We certainly need ->exit_state = -1.
Earlier I had forgotten about second the use of ->children to update the parent pointer of processes when their parent exits.

There is a practical question how much we care about pstree being confused (I assume it doesn't crash). If this is just a confusion issue then I say go for it. PPID == 0 is a very legitimate way to say the kernel is the parent process.

There are a few more cases where we are likely to get PPID == 0 in the future and /sbin/init already has that now. Plus there is a lot of historic precedent. The odd part is PPID = 0 having multiple children.

If we decide maintaining a tree is important I would much rather put init_task on the task_list so we can see it in /proc then go the other way around.

I would like a confirmation that it PPID == 0 is what is confusing pstree just to make certain we haven't half filled in some field in init_task and are thus giving in correct /proc output. But that is all the double checking I would do.

Agreed. I fixed that in sys_unshare but I should really submit a patch to do the same for sys_clone at some point.

When know flags aren't implemented we certainly return -EINVAL.

Given that this line of work looks to fix the race that messes allows a threaded init to generate unkillable zombies I can probably find some time in the next while to work on it.

Eric

I concur with Eric's assessment. Adding new magic bits to the generic clone path seems like a poor way to cope with kernel threads. I think it's better if kernel thread setup gets less like normal user process setup. I also agree with Eric that PPID of 0 is a very natural way for kernel threads to be displayed. We need to know more about the nature of the compatibility issue in procps to judge whether there is good reason to avoid changing it.

Thanks,
Roland

I just investigated the procps issue. Using init_task as the parent nothing sticks out as being wrong in /proc.

Further when I modified pstree to accept 0 as it's starting pid (from which all else would be rooted). All of the kernel threads showed up.

So if anything I it is a feature that kernel threads don't show up by default in pstree (when PPID == 0). It isn't a subtle kernel bug.

Eric

OK. I just got the OK from management. The system we were booting was for research only. We had NR_CPUS=num_online_cpus()=4096 which were non-hyperthreaded. With no attached I/O and the tweak I originally posted plus one change Jack has already gotten accepted, the machine booted in approx 12 minutes.

Thanks,
Robin

How much of that time was between the time the kernel was loaded and before user space was started?

Twelve minutes sounds like a long time for a boot, if you aren't fsck'ing filesystems.

Eric

I am scrambling for my timed boot logs. All I am finding right now are boots from 512p machines. For that machine, it took 178 seconds to get to mounting the root filesystem and 75 seconds to get to the login prompt. Of that 75 seconds, 24 was because an NFS server was down and 14 was due to PBS taking time to start.

68.994913| Memory: 2074872032k/2082172528k available (9785k code, 7319136k reserved, 5228k data, 672k init)
36.566117| migration_cost=3869,61218,76048
24.074113| mount: RPC: Remote system error - No route to host
13.972892| PBS mom

Those are the long standouts. There are quite a few lines in the boot output which take 2 seconds or less, but none of those are too surprising. Plain and simply, booting that large of a machine has always taken considerable time. The first time we booted a 512p, it took nearly an hour, now that is down to 8 or 9 minutes.

Not fsck'ing filesystems (xfs). One of the NFS servers that was specified in /etc/fstab was missing, but that is unimportant.

Thanks,
Robin

OK, will do tomorrow.

No, it doesn't crash. It just doesn't show kernel threads (ps ax is OK). I didn't look into the sources, but I guess the reason is that pstree assumes that the "root" of the tree is "pid == 1" process.

I personally think this is acceptable (and Roland seems to think the same). Still, to be safe, I'll break this into 2 patches, the first one sets ->exit_state, the second re-parents to swapper.

In fact, we can do some odd things to make pstree happy. We need ->parent only because /proc needs some ->parent fields. But I'd prefer to avoid these hacks.

Still, it is sad that we can't have additional flags for kernel_thread(). However, I agree with your and Roland's objections.

Oleg.

I would say this is more a benefit than a problem. With a couple of these systems we are testing, the number of kernel threads is far greater than the number of user processes and having pstree not normally show them, but maybe have an option we add later to show them again would be beneficial.

Thanks,
Robin

Sure.

Robin how many kernel thread per cpu are you seeing?

I don't know that we have a problem in this regard but it feels like you are seeing a lot more kernel threads per cpu than I would expect from looking at small systems.

I am wondering if somewhere we are using per cpu kernel threads when we should not be...

Eric

10.

FYI, pid 1539 is kthread.

a01:~ # ps -ef | egrep "\[.*\/255\]"

root       512     1  0 Apr08 ?        00:00:00 [migration/255]
root       513     1  0 Apr08 ?        00:00:00 [ksoftirqd/255]
root      1281     1  0 Apr08 ?        00:00:02 [events/255]
root      2435  1539  0 Apr08 ?        00:00:00 [kblockd/255]
root      3159  1539  0 Apr08 ?        00:00:00 [aio/255]
root      4007  1539  0 Apr08 ?        00:00:00 [cqueue/255]
root      8653  1539  0 Apr08 ?        00:00:00 [ata/255]
root     17438  1539  0 Apr08 ?        00:00:00 [xfslogd/255]
root     17950  1539  0 Apr08 ?        00:00:00 [xfsdatad/255]
root     18426  1539  0 Apr08 ?        00:00:00 [rpciod/255]

Thanks,
Robin

Then we have to fix pstree and all that. (In fact, I'm secretly trying to patch ps f to DTRT ;p)

Jan

Done that and the result is that ps afwx now looks like:

  PID TTY      STAT   TIME COMMAND
 2722 ?        S      0:00 [lockd]
 2721 ?        S<     0:00 [rpciod/0]
  749 ?        S<     0:00 [ata_aux]
  748 ?        S<     0:00 [ata/0]
  496 ?        S<     0:00 [xfssyncd]
  495 ?        S<     0:00 [xfsbufd]
  484 ?        S<     0:00 [xfsdatad/0]
  482 ?        S<     0:00 [xfslogd/0]
  427 ?        S<     0:00 [scsi_eh_0]
  204 ?        S<     0:00 [kpsmoused]
  110 ?        S<     0:00 [aio/0]
  109 ?        S<     0:00 [kswapd0]
  108 ?        S      0:00 [pdflush]
  107 ?        S      0:00 [pdflush]
   85 ?        S<     0:00 [kseriod]
   21 ?        S<     0:00 [kacpid]
   20 ?        S<     0:00 [kblockd/0]
    5 ?        S<     0:00 [kthread]
    4 ?        S<     0:00 [khelper]
    3 ?        S<     0:00 [events/0]
    2 ?        SN     0:00 [ksoftirqd/0]
    1 ?        Ss     0:02 init [3]  
  537 ?        S<s    0:02  \_ /sbin/udevd --daemon
 1600 ?        Ss     0:00  \_ /usr/bin/dbus-daemon --system
 1692 ?        Ss     0:00  \_ /sbin/acpid
 1923 ?        Ss     0:00  \_ /sbin/resmgrd
 1985 ?        Ss     0:00  \_ /usr/sbin/polkitd
 2014 ?        Ss     0:02  \_ /usr/sbin/hald --daemon=yes
 2022 ?        S      0:00  |   \_ hald-runner
 2051 ?        S      0:00  |       \_ hald-addon-keyboard: listening on /d
 2061 ?        S      0:00  |       \_ hald-addon-keyboard: listening on /d
 2085 ?        S      0:00  |       \_ hald-addon-acpi: listening on acpid 
 2086 ?        S      0:00  |       \_ hald-addon-storage: polling /dev/hdc
 2601 ?        Ss     0:00  \_ /sbin/syslog-ng
 2602 ?        Ss     0:00  \_ /usr/sbin/sshd -o PidFile=/var/run/sshd.init
 2607 ?        Ss     0:00  \_ /sbin/klogd -c 1 -x -x
 2617 ?        Ss     0:00  \_ /usr/sbin/gpm -m /dev/input/mice -t ps2
 2623 ?        Ss     0:00  \_ /sbin/portmap
 2634 ?        Ss     0:00  \_ login -- root     
 2688 tty1     Ss+    0:00  |   \_ -bash
 2635 ?        Ss     0:00  \_ login -- jengelh     
 2733 tty2     Ss     0:00  |   \_ -bash
 2850 tty2     R+     0:00  |       \_ ./ps afwx
 2636 tty3     Ss+    0:00  \_ /sbin/mingetty tty3
 2637 tty4     Ss+    0:00  \_ /sbin/mingetty tty4
 2638 tty5     Ss+    0:00  \_ /sbin/mingetty tty5
 2651 tty6     Ss+    0:00  \_ /sbin/mingetty tty6

Except for the unsorted bunch before process 1, this looks good, and of course it only took a single line change.

Index: procps-3.2.7/ps/display.c
===================================================================
@@ -482,7 +482,7 @@ static void show_tree(const int self, co
       more_children = 0;
     else
       if(processes[i+1]->ppid != self_pid) more_children = 0;
-    if(self_pid==1 && ADOPTED(processes[i]) && forest_type!='u')
+    if(ADOPTED(processes[i]) && forest_type!='u')
       show_tree(i++, n, level,   more_children);
     else
       show_tree(i++, n, level+1, more_children);

Jan