Skip to contentSkip to conversation

Reduce bkl usage in do_coredump

PATCH In review Josh Aas 3 participants 4 messages Aug 9, 2004

Subsystems (2)
VFS exec and binfmt
lore
Share & tools
lore

Josh Aas

author
#0 lore raw

PATCH 1/1 Reduce bkl usage in do_coredump

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 <[email protected]>
Signature · 4 lines
Josh Aas
Silicon Graphics, Inc. (SGI)
Linux System Software
651-683-3068
fs/exec.c +8 -4
@@ -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;
 }
#1 lore raw

Review

Quoted 12 lines · Josh Aas

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

(quoted diff) +7 -1
-       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

fs/exec.c renamed from fs/exec.c.orig +7 -3
@@ -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;
 }
author
#2 lore raw
Quoted 5 lines

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.

Quoted 2 lines

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

We have actually seen contention in do_coredump.

Signature · 4 lines
Josh Aas
Silicon Graphics, Inc. (SGI)
Linux System Software
651-683-3068
#3 lore raw
In reply to Dave Hansen
Quoted 10 lines · Dave Hansen

Review

Quoted 9 lines
(quoted diff) +7 -1
-       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();
Quoted 2 lines

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