BKL reduction in do_exit

A week ago, I posted this:
http://groups.google.com/groups?selm=linux.kernel.3CA20C9B.20309%40us.ibm.com

Nobody had anything to sayabout it, so here's a patch. It moves the disassociate_ctty(1) up, and releases the BKl after it gets done. Is this a sane thing to do, or do some of those exit_*() functions still need the tty?

The patch reduces hold times of the BKL in do_exit() by a factor of 100.
They were on the order of 200us, now they're about 1.5us. However,
those numbers were on Martin Bligh's NUMA-Q box, so they represent a serious worst-case scenario.

The patch is stable, but I don't properly understand the consequences of moving the disassociate_ctty(1) up. I guess we could do this instead:

lock_kernel();

 	sem_exit();
	unlock_kernel();

__exit_files(tsk);
__exit_fs(tsk);
exit_namespace(tsk);
exit_sighand(tsk);

 	exit_thread();
	
	lock_kernel();
	if (current->leader)
		disassociate_ctty(1);
	unlock_kernel();

But, I hesitated to do that because of the lock bouncing consequences on the NUMA-Q.

Dave Hansen
[email protected]

@@ -500,15 +500,16 @@
 
 	lock_kernel();
 	sem_exit();
+	if (current->leader)
+		disassociate_ctty(1);
+	unlock_kernel();
+	
 	__exit_files(tsk);
 	__exit_fs(tsk);
 	exit_namespace(tsk);
 	exit_sighand(tsk);
 	exit_thread();
 
-	if (current->leader)
-		disassociate_ctty(1);
-
 	put_exec_domain(tsk->thread_info->exec_domain);
 	if (tsk->binfmt && tsk->binfmt->module)
 		__MOD_DEC_USE_COUNT(tsk->binfmt->module);

I'd prefer to have the BKL just moved into the functions that need it, and removed altogether from do_exit().

That's especially true as I don't know if sem_exit() actually needs the BKL any more at all - so that if it doesn't, we can just remove it from there (at which point it is a local implementation issue, rather than a cross-module thing).

The disassociate_tty thing falls under a similar heading - we're going to have to fix up the tty layer some day anyway, let's make the BKL detail a tty layer internal thing.

Linus

I like the push-it-down approach better too. Patch attached.

Dave Hansen
[email protected]

@@ -569,6 +569,8 @@
 	struct task_struct *p;
 	int tty_pgrp = -1;
 
+	lock_kernel();
+
 	if (tty) {
 		tty_pgrp = tty->pgrp;
 		if (on_exit && tty->driver.type != TTY_DRIVER_TYPE_PTY)

@@ -578,6 +580,7 @@
 			kill_pg(current->tty_old_pgrp, SIGHUP, on_exit);
 			kill_pg(current->tty_old_pgrp, SIGCONT, on_exit);
 		}
+		unlock_kernel();	
 		return;
 	}
 	if (tty_pgrp > 0) {

@@ -595,6 +598,7 @@
 	  	if (p->session == current->session)
 			p->tty = NULL;
 	read_unlock(&tasklist_lock);
+	unlock_kernel();
 }
 
 void stop_tty(struct tty_struct *tty)

@@ -995,6 +995,8 @@
 	struct sem_array *sma;
 	int nsems, i;
 
+	lock_kernel();
+
 	/* If the current process was sleeping for a semaphore,
 	 * remove it from the queue.
 	 */

@@ -1051,6 +1053,8 @@
 		sem_unlock(semid);
 	}
 	current->semundo = NULL;
+
+	unlock_kernel();
 }
 
 #ifdef CONFIG_PROC_FS

@@ -498,7 +498,6 @@
 #endif
 	__exit_mm(tsk);
 
-	lock_kernel();
 	sem_exit();
 	__exit_files(tsk);
 	__exit_fs(tsk);