Re: [PATCH] qla2xxx: Resolved a performance issue in interrupt handler

[Anirban Chakraborty <anirban.chakraborty@qlogic.com>]

On Tue, 9 Jun 2009, James Bottomley wrote:

> On Tue, 2009-06-09 at 13:34 -0700, Anirban Chakraborty wrote:
> > On Jun 9, 2009, at 12:32 PM, James Bottomley wrote:
> > 
> > > On Tue, 2009-06-09 at 11:42 -0700, Anirban Chakraborty wrote:
> > >> Reverted back a change from spin_lock to spin_lock_irq* that was  
> > >> causing
> > >> the cycle times to go up.
> > >> The patch is based on scsi-misc-2.6.
> > >
> > > Some more explanation of this would be greatly appreciated.  The cause
> > > looks to be that holding off interrupts in the isr could potentially
> > > reduce latency (caused by taking a different interrupt while holding  
> > > the
> > > hardware lock) and increase the chance of coalescing (by holding off
> > > interrupts).  However, if that's the case, possibly using an
> > > IRQF_DISABLED isr rather than going back to spin_lock_irqsave()  
> > > would be
> > > a better fix?
> > The primary cause is, as you mentioned, the contention for the  
> > hardware lock in the isr.
> > I'll check it with IRQF_DISABLED too. However, I was wondering if  
> > there is any additional savings if we use IRQF_DISABLED vs. using the  
> > spin_lock_irqsave. In the former case, probably we'd enter the isr  
> > with interrupts disabled and in the latter case it would be done upon  
> > entering the isr.
> 
> It depends what the root cause is ... if it's really latency introduced
> by other interrupts, then IRQF_DISABLED might be the better course.  If
> it's purely interrupt problems in the spin locked critical sections,
> then spin_lock_irq might be the better solution ... what would be useful
> is to have the test rig at intel which turned up the problem see what
> happens to the results for each case.
> 
> James

I have attached the patch with IRQF_DISABLED. In my setup, I didn't find any 
significant difference in performance numbers between IRQF_DISABLED and 
spin_lock_irqsave. It would be interesting to see the numbers from Douglas's 
rig.
Doug, could you please run it once more with this patch and let us know the 
numbers vs. the one you already tested with spin_lock_irqsave earlier.
Thanks much,
Anirban

diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/qla_isr.c
index c8d0a17..d07bd97 100644
--- a/drivers/scsi/qla2xxx/qla_isr.c
+++ b/drivers/scsi/qla2xxx/qla_isr.c
@@ -1991,7 +1991,7 @@ skip_msix:
 skip_msi:
 
 	ret = request_irq(ha->pdev->irq, ha->isp_ops->intr_handler,
-	    IRQF_SHARED, QLA2XXX_DRIVER_NAME, rsp);
+	    IRQF_DISABLED, QLA2XXX_DRIVER_NAME, rsp);
 	if (ret) {
 		qla_printk(KERN_WARNING, ha,
 		    "Failed to reserve interrupt %d already in use.\n",