[PATCH] Reduce bkl usage in do_coredump

[PATCH] Reduce bkl usage in do_coredump

[Josh Aas]

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

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).

Also make format_corename() static.

Signed-off-by: Josh Aas <josha@sgi.com>

-- 
Josh Aas
Silicon Graphics, Inc. (SGI)
Linux System Software
651-683-3068

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

--- a/fs/exec.c	2004-08-09 09:14:53.000000000 -0500
+++ b/fs/exec.c	2004-08-09 09:18:50.000000000 -0500
@@ -1193,7 +1193,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;
@@ -1357,7 +1357,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;
@@ -1375,7 +1374,13 @@ int do_coredump(long signr, int exit_cod
 	if (current->rlim[RLIMIT_CORE].rlim_cur < binfmt->min_coredump)
 		goto fail_unlock;
 
- 	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();
 	file = filp_open(corename, O_CREAT | 2 | O_NOFOLLOW | O_LARGEFILE, 0600);
 	if (IS_ERR(file))
 		goto fail_unlock;
@@ -1402,6 +1407,5 @@ close_fail:
 fail_unlock:
 	complete_all(&mm->core_done);
 fail:
-	unlock_kernel();
 	return retval;
 }

Re: [PATCH] Reduce bkl usage in do_coredump

[Dave Hansen]

[-- 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;
 }

Re: [PATCH] Reduce bkl usage in do_coredump

[Josh Aas]

> 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.

Probably a good idea.

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

We have actually seen contention in do_coredump.

-- 
Josh Aas
Silicon Graphics, Inc. (SGI)
Linux System Software
651-683-3068

Re: [PATCH] Reduce bkl usage in do_coredump

[Andrew Morton]

Dave Hansen <haveblue@us.ibm.com> wrote:
>
>  > -       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.

Nope.  format_corename() just takes a char *.  It has no knowledge about
what that char*'s locking rules are.  It's the caller who chose to use a
char* which is protected by lock_kernel().