[RFC][PATCH] BKL reduction in do_exit

[RFC][PATCH] BKL reduction in do_exit

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 1091 bytes --]

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
haveblue@us.ibm.com



[-- Attachment #2: do_exit-bkl_reduction-2.5.7.patch --]
[-- Type: text/plain, Size: 525 bytes --]

--- linux-2.5.7-clean/kernel/exit.c	Tue Apr  2 10:43:28 2002
+++ linux//kernel/exit.c	Wed Apr  3 08:56:00 2002
@@ -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);

Re: [RFC][PATCH] BKL reduction in do_exit

[Linus Torvalds]

On Wed, 3 Apr 2002, Dave Hansen wrote:
>
> 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?

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

Re: [RFC][PATCH] BKL reduction in do_exit

[Dave Hansen]

[-- Attachment #1: Type: text/plain, Size: 236 bytes --]

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

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

-- 
Dave Hansen
haveblue@us.ibm.com

[-- Attachment #2: do_exit-bkl_reduction-2.5.7.patch.2 --]
[-- Type: text/plain, Size: 1305 bytes --]

--- linux-2.5.7-clean/drivers/char/tty_io.c	Thu Mar  7 18:18:54 2002
+++ linux/drivers/char/tty_io.c	Wed Apr  3 10:03:27 2002
@@ -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)
--- linux-2.5.7-clean/ipc/sem.c	Thu Mar  7 18:18:25 2002
+++ linux/ipc/sem.c	Wed Apr  3 10:07:28 2002
@@ -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
--- linux-2.5.7-clean/kernel/exit.c	Tue Apr  2 10:43:28 2002
+++ linux/kernel/exit.c	Wed Apr  3 10:06:19 2002
@@ -498,7 +498,6 @@
 #endif
 	__exit_mm(tsk);
 
-	lock_kernel();
 	sem_exit();
 	__exit_files(tsk);
 	__exit_fs(tsk);