[PATCH 6/7] aacraid: unlock around kfree

[PATCH 6/7] aacraid: unlock around kfree

[Mark Haverkamp]

Received from Mark Salyzyn from Adaptec:

kfree has the possibility of sleeping. It is generally considered poor
manners to hold on to a lock with the possibility of a system call
induced sleep.

Applies to scsi-misc-2.6 git tree

Signed-off-by: Mark Haverkamp <markh@osdl.org>

Index: scsi-misc-aac-1/drivers/scsi/aacraid/commctrl.c
===================================================================
--- scsi-misc-aac-1.orig/drivers/scsi/aacraid/commctrl.c	2005-07-05 14:18:07.000000000 -0700
+++ scsi-misc-aac-1/drivers/scsi/aacraid/commctrl.c	2005-07-05 14:59:24.000000000 -0700
@@ -324,8 +324,10 @@
 		/*
 		 *	Free the space occupied by this copy of the fib.
 		 */
+		spin_unlock_irq(&dev->fib_lock);
 		kfree(fib->hw_fib);
 		kfree(fib);
+		spin_lock_irq(&dev->fib_lock);
 	}
 	/*
 	 *	Remove the Context from the AdapterFibContext List
@@ -338,7 +340,9 @@
 	/*
 	 *	Free the space occupied by the Context
 	 */
+	spin_unlock_irq(&dev->fib_lock);
 	kfree(fibctx);
+	spin_lock_irq(&dev->fib_lock);
 	return 0;
 }
 

-- 
Mark Haverkamp <markh@osdl.org>

Re: [PATCH 6/7] aacraid: unlock around kfree

[James Bottomley]

On Wed, 2005-08-03 at 15:39 -0700, Mark Haverkamp wrote:
> kfree has the possibility of sleeping. It is generally considered poor
> manners to hold on to a lock with the possibility of a system call
> induced sleep.

Erm, this isn't true ... we'd have no end of driver issues if kfree
could block.

James

RE: [PATCH 6/7] aacraid: unlock around kfree

[Salyzyn, Mark]

Good to know. I went through one too many code reviews and had no
defense against the request.

There is a follow-up to this one not yet submitted to MarkH where I
endeavor to pre-allocate with kmalloc(,GFP_ATOMIC|GFP_KERNEL) a pool
before entering a locked list traversal rather than risking making the
calls with the locks held. The code is in an independent kernel thread
handling deferred actions triggered by the interrupt. Am I barking up
the wrong tree?

Sincerely -- Mark Salyzyn

-----Original Message-----
From: James Bottomley [mailto:James.Bottomley@SteelEye.com] 
Sent: Wednesday, August 03, 2005 7:07 PM
To: Mark Haverkamp
Cc: linux-scsi; Salyzyn, Mark
Subject: Re: [PATCH 6/7] aacraid: unlock around kfree

On Wed, 2005-08-03 at 15:39 -0700, Mark Haverkamp wrote:
> kfree has the possibility of sleeping. It is generally considered poor
> manners to hold on to a lock with the possibility of a system call
> induced sleep.

Erm, this isn't true ... we'd have no end of driver issues if kfree
could block.

James

RE: [PATCH 6/7] aacraid: unlock around kfree

[Arjan van de Ven]

On Thu, 2005-08-04 at 07:33 -0400, Salyzyn, Mark wrote:
> Good to know. I went through one too many code reviews and had no
> defense against the request.
> 
> There is a follow-up to this one not yet submitted to MarkH where I
> endeavor to pre-allocate with kmalloc(,GFP_ATOMIC|GFP_KERNEL) a pool
> before entering a locked list traversal rather than risking making the
> calls with the locks held. The code is in an independent kernel thread
> handling deferred actions triggered by the interrupt. Am I barking up
> the wrong tree?

why do you need the GFP_ATOMIC if you allocate outside all locks?
Yes the idea is the right one; do a sleeping alloc outside all locks,
instead of a far more fragile atomic alloc inside locks.

Note: if you don't want the allocation to cause IO (which could recurse
into your driver) you need to use GFP_NOIO not GFP_KERNEL; the
difference is important for avoiding recursion-deadlocks

RE: [PATCH 6/7] aacraid: unlock around kfree

[Salyzyn, Mark]

Cool! Thanks for the information! Sounds like the patch will be
necessary. However, I have never seen the issue bite us, but who knows,
it may be the contributor to a system lockup after 8000 hours of uptime
... I'd prefer the paranoid delusion and a stable system!

As for the GFP_ATOMIC, the intent is to move the kmalloc w/GFP_ATOMIC
from inside the loop to outside the loop and perform a guestimate+ on
the possible pool requirements of the loop. Since this is an independent
thread, there will be no need to use GFP_NOIO. The code has been tested
for almost a year now in the Adaptec branch, and the guestimate was
tweaked in that time to be 100% reliable.

Sincerely -- Mark Salyzyn

-----Original Message-----
From: Arjan van de Ven [mailto:arjan@infradead.org] 
Sent: Thursday, August 04, 2005 7:48 AM
To: Salyzyn, Mark
Cc: linux-scsi; Mark Haverkamp; James Bottomley
Subject: RE: [PATCH 6/7] aacraid: unlock around kfree

On Thu, 2005-08-04 at 07:33 -0400, Salyzyn, Mark wrote:
> Good to know. I went through one too many code reviews and had no
> defense against the request.
> 
> There is a follow-up to this one not yet submitted to MarkH where I
> endeavor to pre-allocate with kmalloc(,GFP_ATOMIC|GFP_KERNEL) a pool
> before entering a locked list traversal rather than risking making the
> calls with the locks held. The code is in an independent kernel thread
> handling deferred actions triggered by the interrupt. Am I barking up
> the wrong tree?

why do you need the GFP_ATOMIC if you allocate outside all locks?
Yes the idea is the right one; do a sleeping alloc outside all locks,
instead of a far more fragile atomic alloc inside locks.

Note: if you don't want the allocation to cause IO (which could recurse
into your driver) you need to use GFP_NOIO not GFP_KERNEL; the
difference is important for avoiding recursion-deadlocks

RE: [PATCH 6/7] aacraid: unlock around kfree

[Arjan van de Ven]

On Thu, 2005-08-04 at 08:04 -0400, Salyzyn, Mark wrote:
> Cool! Thanks for the information! Sounds like the patch will be
> necessary. However, I have never seen the issue bite us, but who knows,
> it may be the contributor to a system lockup after 8000 hours of uptime
> ... I'd prefer the paranoid delusion and a stable system!
> 
> As for the GFP_ATOMIC, the intent is to move the kmalloc w/GFP_ATOMIC
> from inside the loop to outside the loop and perform a guestimate+ on
> the possible pool requirements of the loop. 

the big gain is if you can get rid of the atomic. Anything else is
pointless really... atomic allocations are *really* bad for the vm and
should be avoided if at all possible.