Re: [PATCH] Reduce bkl usage in do_coredump

[Dave Hansen <haveblue@us.ibm.com>]

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

On Mon, 2004-08-09 at 07:38, Josh Aas wrote:
> A patch that reduces bkl usage in do_coredump is attached. I don't see 
> anywhere that it is necessary except for the call to format_corename, 
> which is controlled via sysctl (sys_sysctl holds the bkl).

> -       format_corename(corename, core_pattern, signr);
> +       /* 
> +        * lock_kernel() because format_corename() is controlled by sysctl, which
> +        * uses lock_kernel()
> +        */
> +       lock_kernel();
> +       format_corename(corename, core_pattern, signr);
> +       unlock_kernel();

Might be nicer to just put the locking inside of format_corename() if it
is the function itself that really needs the locking.  If another use of
it popped up, that user would get the locking for free and couldn't
possibly forget it.  Also, it's nicer to put the lock closer to the code
that actually needs it.  Untested patch to do that attached.

BTW, were you actually seeing a BKL contention problem, or was this just
for cleanliness?

-- Dave

[-- Attachment #2: coredump_bkl-1.patch --]
[-- Type: text/x-patch, Size: 1325 bytes --]

--- clean-fixup.1/fs/exec.c.orig	2004-08-09 10:25:27.000000000 -0700
+++ clean-fixup.1/fs/exec.c	2004-08-09 10:27:25.000000000 -0700
@@ -1209,7 +1209,7 @@ EXPORT_SYMBOL(set_binfmt);
  * name into corename, which must have space for at least
  * CORENAME_MAX_SIZE bytes plus one byte for the zero terminator.
  */
-void format_corename(char *corename, const char *pattern, long signr)
+static void format_corename(char *corename, const char *pattern, long signr)
 {
 	const char *pat_ptr = pattern;
 	char *out_ptr = corename;
@@ -1217,6 +1217,10 @@ void format_corename(char *corename, con
 	int rc;
 	int pid_in_pattern = 0;
 
+	/* 
+	 * the format is controlled by sysctl, which uses lock_kernel()
+	 */
+	lock_kernel();
 	/* Repeat as long as we have more pattern to process and more output
 	   space */
 	while (*pat_ptr) {
@@ -1317,6 +1321,8 @@ void format_corename(char *corename, con
 	}
       out:
 	*out_ptr = 0;
+
+	unlock_kernel();
 }
 
 static void zap_threads (struct mm_struct *mm)
@@ -1373,7 +1379,6 @@ int do_coredump(long signr, int exit_cod
 	struct file * file;
 	int retval = 0;
 
-	lock_kernel();
 	binfmt = current->binfmt;
 	if (!binfmt || !binfmt->core_dump)
 		goto fail;
@@ -1418,6 +1423,5 @@ close_fail:
 fail_unlock:
 	complete_all(&mm->core_done);
 fail:
-	unlock_kernel();
 	return retval;
 }