Re: [patch] BSD process accounting: new locking

["Nick Piggin" <s3293115@student.anu.edu.au>]

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

> I have attached a very small patch (test9) to remove the kernel lock from
> kernel/acct.c. If I am missing something major (a brain?), I apologise in
> advance. I have tested this on my UP x86 with spinlock debugging. I would
> appreciate comments or an explanation of why this can't be done if you
have
> the time. Thanks.
>
> Nick
>

Maybe there was a possibility of a race when acct_auto_close calls sys_acct
in the last patch. If so, this should fix it.

Nick.

[-- Attachment #2: bsdacct2.patch --]
[-- Type: application/octet-stream, Size: 3365 bytes --]

--- linux/kernel/acct.c	Mon Oct 30 01:02:56 2000
+++ v2.4.0-test9/linux/kernel/acct.c	Mon Oct 30 01:05:40 2000
@@ -41,6 +41,10 @@
  *	Oh, fsck... Oopsable SMP race in do_process_acct() - we must hold
  * ->mmap_sem to walk the vma list of current->mm. Nasty, since it leaks
  * a struct file opened for write. Fixed. 2/6/2000, AV.
+ *
+ *  2000-10-27 Modified by Nick Piggin to remove usage of the big kernel
+ *             lock in favour of local spinlocks
+ *
  */
 
 #include <linux/config.h>
@@ -77,6 +81,7 @@
 static struct file *acct_file;
 static struct timer_list acct_timer;
 static void do_acct_process(long, struct file *);
+static spinlock_t acct_lock = SPIN_LOCK_UNLOCKED;
 
 /*
  * Called whenever the timer says to check the free space.
@@ -95,11 +100,11 @@
 	int res;
 	int act;
 
-	lock_kernel();
+        spin_lock(&acct_lock);
 	res = acct_active;
 	if (!file || !acct_needcheck)
-		goto out;
-	unlock_kernel();
+		goto out_unlock;
+        spin_unlock(&acct_lock);
 
 	/* May block */
 	if (vfs_statfs(file->f_dentry->d_inode->i_sb, &sbuf))
@@ -113,14 +118,14 @@
 		act = 0;
 
 	/*
-	 * If some joker switched acct_file under us we'ld better be
+	 * If some joker switched acct_file under us we'd better be
 	 * silent and _not_ touch anything.
 	 */
-	lock_kernel();
+        spin_lock(&acct_lock);
 	if (file != acct_file) {
 		if (act)
 			res = act>0;
-		goto out;
+		goto out_unlock;
 	}
 
 	if (acct_active) {
@@ -140,8 +145,8 @@
 	acct_timer.expires = jiffies + ACCT_TIMEOUT*HZ;
 	add_timer(&acct_timer);
 	res = acct_active;
-out:
-	unlock_kernel();
+out_unlock:
+        spin_unlock(&acct_lock); 
 	return res;
 }
 
@@ -182,7 +187,7 @@
 	}
 
 	error = 0;
-	lock_kernel();
+        spin_lock(&acct_lock);
 	if (acct_file) {
 		old_acct = acct_file;
 		del_timer(&acct_timer);
@@ -200,7 +205,7 @@
 		acct_timer.expires = jiffies + ACCT_TIMEOUT*HZ;
 		add_timer(&acct_timer);
 	}
-	unlock_kernel();
+        spin_unlock(&acct_lock);
 	if (old_acct) {
 		do_acct_process(0,old_acct);
 		filp_close(old_acct, NULL);
@@ -214,10 +219,24 @@
 
 void acct_auto_close(kdev_t dev)
 {
-	lock_kernel();
-	if (acct_file && acct_file->f_dentry->d_inode->i_dev == dev)
-		sys_acct(NULL);
-	unlock_kernel();
+        struct file *old_acct;
+
+        spin_lock(&acct_lock);
+	if (acct_file && acct_file->f_dentry->d_inode->i_dev == dev) {
+
+                /* Run the same code as sys_acct(NULL) here. This simplifies locking */
+                old_acct = acct_file;
+                del_timer(&acct_timer);
+                acct_active = 0;
+                acct_needcheck = 0;
+                acct_file = NULL;
+
+                spin_unlock(&acct_lock);
+
+                do_acct_process(0, old_acct);
+                filp_close(old_acct, NULL);
+        } else 
+                spin_unlock(&acct_lock);
 }
 
 /*
@@ -348,15 +367,15 @@
 int acct_process(long exitcode)
 {
 	struct file *file = NULL;
-	lock_kernel();
+        spin_lock(&acct_lock);
 	if (acct_file) {
 		file = acct_file;
 		get_file(file);
-		unlock_kernel();
+                spin_unlock(&acct_lock);
 		do_acct_process(exitcode, acct_file);
 		fput(file);
 	} else
-		unlock_kernel();
+                spin_unlock(&acct_lock);
 	return 0;
 }