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

[Anirban Chakraborty <anirban.chakraborty@xxxxxxxxxx>]

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.

> > Reported-and-checked-by: Douglas W. Styner <douglas.w.styner@xxxxxxxxx 
> > >
> > Signed-off-by: Anirban Chakraborty <anirban.chakraborty@xxxxxxxxxx>
> > ---
> > drivers/scsi/qla2xxx/qla_isr.c |   30 ++++++++++++++++++------------
> > 1 files changed, 18 insertions(+), 12 deletions(-)
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_isr.c b/drivers/scsi/qla2xxx/ 
> > qla_isr.c
> > index c8d0a17..bc92feb 100644
> > --- a/drivers/scsi/qla2xxx/qla_isr.c
> > +++ b/drivers/scsi/qla2xxx/qla_isr.c
> > @@ -37,6 +37,7 @@ qla2100_intr_handler(int irq, void *dev_id)
> > 	uint16_t	hccr;
> > 	uint16_t	mb[4];
> > 	struct rsp_que *rsp;
> > +	unsigned long	flags;
> >
> > 	rsp = (struct rsp_que *) dev_id;
> > 	if (!rsp) {
> > @@ -49,7 +50,7 @@ qla2100_intr_handler(int irq, void *dev_id)
> > 	reg = &ha->iobase->isp;
> > 	status = 0;
> >
> > -	spin_lock(&ha->hardware_lock);
> > +	spin_lock_irqsave(&ha->hardware_lock, flags);
> > 	vha = pci_get_drvdata(ha->pdev);
> > 	for (iter = 50; iter--; ) {
> > 		hccr = RD_REG_WORD(&reg->hccr);
> > @@ -101,7 +102,7 @@ qla2100_intr_handler(int irq, void *dev_id)
> > 			RD_REG_WORD(&reg->hccr);
> > 		}
> > 	}
> > -	spin_unlock(&ha->hardware_lock);
> > +	spin_unlock_irqrestore(&ha->hardware_lock, flags);
> >
> > 	if (test_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags) &&
> > 	    (status & MBX_INTERRUPT) && ha->flags.mbox_int) {
> > @@ -133,6 +134,7 @@ qla2300_intr_handler(int irq, void *dev_id)
> > 	uint16_t	mb[4];
> > 	struct rsp_que *rsp;
> > 	struct qla_hw_data *ha;
> > +	unsigned long	flags;
> >
> > 	rsp = (struct rsp_que *) dev_id;
> > 	if (!rsp) {
> > @@ -145,7 +147,7 @@ qla2300_intr_handler(int irq, void *dev_id)
> > 	reg = &ha->iobase->isp;
> > 	status = 0;
> >
> > -	spin_lock(&ha->hardware_lock);
> > +	spin_lock_irqsave(&ha->hardware_lock, flags);
> > 	vha = pci_get_drvdata(ha->pdev);
> > 	for (iter = 50; iter--; ) {
> > 		stat = RD_REG_DWORD(&reg->u.isp2300.host_status);
> > @@ -216,7 +218,7 @@ qla2300_intr_handler(int irq, void *dev_id)
> > 		WRT_REG_WORD(&reg->hccr, HCCR_CLR_RISC_INT);
> > 		RD_REG_WORD_RELAXED(&reg->hccr);
> > 	}
> > -	spin_unlock(&ha->hardware_lock);
> > +	spin_unlock_irqrestore(&ha->hardware_lock, flags);
> >
> > 	if (test_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags) &&
> > 	    (status & MBX_INTERRUPT) && ha->flags.mbox_int) {
> > @@ -1626,6 +1628,7 @@ qla24xx_intr_handler(int irq, void *dev_id)
> > 	uint32_t	hccr;
> > 	uint16_t	mb[4];
> > 	struct rsp_que *rsp;
> > +	unsigned long	flags;
> >
> > 	rsp = (struct rsp_que *) dev_id;
> > 	if (!rsp) {
> > @@ -1638,7 +1641,7 @@ qla24xx_intr_handler(int irq, void *dev_id)
> > 	reg = &ha->iobase->isp24;
> > 	status = 0;
> >
> > -	spin_lock(&ha->hardware_lock);
> > +	spin_lock_irqsave(&ha->hardware_lock, flags);
> > 	vha = pci_get_drvdata(ha->pdev);
> > 	for (iter = 50; iter--; ) {
> > 		stat = RD_REG_DWORD(&reg->host_status);
> > @@ -1688,7 +1691,7 @@ qla24xx_intr_handler(int irq, void *dev_id)
> > 		WRT_REG_DWORD(&reg->hccr, HCCRX_CLR_RISC_INT);
> > 		RD_REG_DWORD_RELAXED(&reg->hccr);
> > 	}
> > -	spin_unlock(&ha->hardware_lock);
> > +	spin_unlock_irqrestore(&ha->hardware_lock, flags);
> >
> > 	if (test_bit(MBX_INTR_WAIT, &ha->mbx_cmd_flags) &&
> > 	    (status & MBX_INTERRUPT) && ha->flags.mbox_int) {
> > @@ -1706,6 +1709,7 @@ qla24xx_msix_rsp_q(int irq, void *dev_id)
> > 	struct rsp_que *rsp;
> > 	struct device_reg_24xx __iomem *reg;
> > 	struct scsi_qla_host *vha;
> > +	unsigned long	flags;
> >
> > 	rsp = (struct rsp_que *) dev_id;
> > 	if (!rsp) {
> > @@ -1716,13 +1720,13 @@ qla24xx_msix_rsp_q(int irq, void *dev_id)
> > 	ha = rsp->hw;
> > 	reg = &ha->iobase->isp24;
> >
> > -	spin_lock_irq(&ha->hardware_lock);
> > +	spin_lock_irqsave(&ha->hardware_lock, flags);
>
> This doesn't make a lot of sense.  I can see in the ordinary isr  
> above,
> you were using spin_lock not spin_lock_irq, so you were going into the
> spin with other interrupts enabled.  However, in the MSIX case, the  
> net
> effect of the code (since we enter with interrupts enabled) is nil  
> plus
> the small latency saving and restoring the flags.
>
> I'm assuming Intel did the tests on a MSIX system, so I have a hard  
> time
> understanding how this could in any way fix their problem.
>
> James
They did their tests on a system with MSI enabled, for which the  
qla24xx_intr_handler gets invoked. So, the qla24xx_msix_rsp_q never  
got tested in that case. However, I do agree that spin_lock_irqsave is  
not needed in the MSI-X case as you mentioned that the interrupts are  
already disabled here.
Thanks,
-Anirban

--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html