diff mbox series

scsi: qla2xxx: Disable ATIO interrupt coalesce for quad port ISP27XX

Message ID 97dcf365-89ff-014d-a3e5-1404c6af511c@cybernetics.com (mailing list archive)
State Accepted
Headers show
Series scsi: qla2xxx: Disable ATIO interrupt coalesce for quad port ISP27XX | expand

Commit Message

Tony Battersby July 20, 2022, 1:43 p.m. UTC
This message was previously sent to scst-devel:
https://sourceforge.net/p/scst/mailman/scst-devel/thread/d381eb86-d446-4f9e-03c3-aa93d93dd074%40cybernetics.com/#msg37682919

I am using a quad-port 16 Gbps QLE2694L (ISP2071) with SCST 3.6 +
qla2x00t-32gbit and kernel 5.15.  The following old commit that went
into upstream kernel v4.16 is causing a problem for me:

commit d2b292c3f6fdef5819a276acd64915bae6384a7f
Author: Quinn Tran <quinn.tran@cavium.com>
Date:   Thu Dec 28 12:33:17 2017 -0800

    scsi: qla2xxx: Enable ATIO interrupt handshake for ISP27XX
    
    Enable ATIO Q interrupt handshake for ISP27XX. This patch
    coalesce ATIO's interrupts for Quad port ISP27XX adapter.
    Interrupt coalesce allows performance to scale for this
    specific case.

This commit was present in qla2xxx when qla2x00t-32gbit was first
imported into SCST.

With the "ATIO interrupt coalesce" enabled by the above patch, ATIO
interrupts are handled by qla24xx_msix_default() rather than by
qla83xx_msix_atio_q() for the ISP2071.  I guess this is supposed to
generate fewer interrupts so that ATIO entries can be handled in larger
batches.  But this causes a problem where sometimes
qlt_24xx_process_atio_queue() returns while the hardware is still adding
ATIO entries to the queue, but then the hardware doesn't generate
another interrupt until a separate event sometimes minutes later.  This
leaves incoming ATIO entries (i.e. incoming target-mode SCSI commands)
ignored in the adapter's hardware queue for a long time until the host
sends another command at a different time to generate an interrupt. 
There does not seem to be any timeout function that generates an
interrupt after a short period of inactivity to process the remainder of
the ATIO queue, which is what would be necessary for interrupt coalesce
to function properly.

In my case I am using virtual tape drives, which generally process only
one command at a time, so there will often never be a "next" command to
generate an interrupt until the previous command completes.  If the
previous command is stuck in the adapter's ATIO queue, then the host
will timeout and abort the command, and the task management function to
abort the command generates the interrupt that causes the command to
finally be received by SCST, so the command appears to have been aborted
immediately.  With disk drives the situation might be a bit different
since a busy disk might get a steady stream of commands to generate
additional interrupts to process the ATIO queue, but even that is not
guaranteed, so there might still be problems.

I tried to fix the problem by modifying qla24xx_msix_default() to call
qlt_24xx_process_atio_queue() before wrt_reg_dword(&reg->hccr,
HCCRX_CLR_RISC_INT) instead of after, but that didn't help.  The problem
was solved by disabling the ATIO interrupt coalesce feature and
re-enabling the dedicated ATIO MSI-X interrupt (qla83xx_msix_atio_q())
for calling qlt_24xx_process_atio_queue().  Another workaround is to use
ql2xenablemsix=0, but of course keeping MSI-X enabled is better.

The patch below is against the upstream Linux kernel.  It can be applied
to SCST with "patch -p4" from the qla2x00t-32gbit directory.

If someone really wants to keep the interrupt coalesce feature, it could
be turned into a module parameter instead.

From e5888ce5dbffff2d38d9dafd4841d6d3bcda4365 Mon Sep 17 00:00:00 2001
From: Tony Battersby <tonyb@cybernetics.com>
Date: Thu, 7 Jul 2022 15:08:01 -0400
Subject: [PATCH] scsi: qla2xxx: Disable ATIO interrupt coalesce for quad port ISP27XX

This partially reverts commit d2b292c3f6fdef5819a276acd64915bae6384a7f.

For some workloads where the host sends a batch of commands and then
pauses, ATIO interrupt coalesce can cause some incoming ATIO entries to
be ignored for extended periods of time, resulting in slow performance,
timeouts, and aborted commands.  So disable interrupt coalesce and
re-enable the dedicated ATIO MSI-X interrupt.

Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
---
 drivers/scsi/qla2xxx/qla_target.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

Comments

Martin K. Petersen Aug. 1, 2022, 11:57 p.m. UTC | #1
Quinn/Nilesh: Please review, thanks!

> I am using a quad-port 16 Gbps QLE2694L (ISP2071) with SCST 3.6 +
> qla2x00t-32gbit and kernel 5.15.  The following old commit that went
> into upstream kernel v4.16 is causing a problem for me:
>
> commit d2b292c3f6fdef5819a276acd64915bae6384a7f
> Author: Quinn Tran <quinn.tran@cavium.com>
> Date:   Thu Dec 28 12:33:17 2017 -0800
>
>     scsi: qla2xxx: Enable ATIO interrupt handshake for ISP27XX
>     
>     Enable ATIO Q interrupt handshake for ISP27XX. This patch
>     coalesce ATIO's interrupts for Quad port ISP27XX adapter.
>     Interrupt coalesce allows performance to scale for this
>     specific case.
>
> This commit was present in qla2xxx when qla2x00t-32gbit was first
> imported into SCST.
Himanshu Madhani Aug. 3, 2022, 2:34 p.m. UTC | #2
> On Jul 20, 2022, at 6:43 AM, Tony Battersby <tonyb@cybernetics.com> wrote:
> 
> This message was previously sent to scst-devel:
> https://sourceforge.net/p/scst/mailman/scst-devel/thread/d381eb86-d446-4f9e-03c3-aa93d93dd074%40cybernetics.com/#msg37682919
> 
> I am using a quad-port 16 Gbps QLE2694L (ISP2071) with SCST 3.6 +
> qla2x00t-32gbit and kernel 5.15.  The following old commit that went
> into upstream kernel v4.16 is causing a problem for me:
> 
> commit d2b292c3f6fdef5819a276acd64915bae6384a7f
> Author: Quinn Tran <quinn.tran@cavium.com>
> Date:   Thu Dec 28 12:33:17 2017 -0800
> 
>    scsi: qla2xxx: Enable ATIO interrupt handshake for ISP27XX
> 
>    Enable ATIO Q interrupt handshake for ISP27XX. This patch
>    coalesce ATIO's interrupts for Quad port ISP27XX adapter.
>    Interrupt coalesce allows performance to scale for this
>    specific case.
> 
> This commit was present in qla2xxx when qla2x00t-32gbit was first
> imported into SCST.
> 
> With the "ATIO interrupt coalesce" enabled by the above patch, ATIO
> interrupts are handled by qla24xx_msix_default() rather than by
> qla83xx_msix_atio_q() for the ISP2071.  I guess this is supposed to
> generate fewer interrupts so that ATIO entries can be handled in larger
> batches.  But this causes a problem where sometimes
> qlt_24xx_process_atio_queue() returns while the hardware is still adding
> ATIO entries to the queue, but then the hardware doesn't generate
> another interrupt until a separate event sometimes minutes later.  This
> leaves incoming ATIO entries (i.e. incoming target-mode SCSI commands)
> ignored in the adapter's hardware queue for a long time until the host
> sends another command at a different time to generate an interrupt. 
> There does not seem to be any timeout function that generates an
> interrupt after a short period of inactivity to process the remainder of
> the ATIO queue, which is what would be necessary for interrupt coalesce
> to function properly.
> 
> In my case I am using virtual tape drives, which generally process only
> one command at a time, so there will often never be a "next" command to
> generate an interrupt until the previous command completes.  If the
> previous command is stuck in the adapter's ATIO queue, then the host
> will timeout and abort the command, and the task management function to
> abort the command generates the interrupt that causes the command to
> finally be received by SCST, so the command appears to have been aborted
> immediately.  With disk drives the situation might be a bit different
> since a busy disk might get a steady stream of commands to generate
> additional interrupts to process the ATIO queue, but even that is not
> guaranteed, so there might still be problems.
> 
> I tried to fix the problem by modifying qla24xx_msix_default() to call
> qlt_24xx_process_atio_queue() before wrt_reg_dword(&reg->hccr,
> HCCRX_CLR_RISC_INT) instead of after, but that didn't help.  The problem
> was solved by disabling the ATIO interrupt coalesce feature and
> re-enabling the dedicated ATIO MSI-X interrupt (qla83xx_msix_atio_q())
> for calling qlt_24xx_process_atio_queue().  Another workaround is to use
> ql2xenablemsix=0, but of course keeping MSI-X enabled is better.
> 
> The patch below is against the upstream Linux kernel.  It can be applied
> to SCST with "patch -p4" from the qla2x00t-32gbit directory.
> 
> If someone really wants to keep the interrupt coalesce feature, it could
> be turned into a module parameter instead.
> 
> From e5888ce5dbffff2d38d9dafd4841d6d3bcda4365 Mon Sep 17 00:00:00 2001
> From: Tony Battersby <tonyb@cybernetics.com>
> Date: Thu, 7 Jul 2022 15:08:01 -0400
> Subject: [PATCH] scsi: qla2xxx: Disable ATIO interrupt coalesce for quad port ISP27XX
> 
> This partially reverts commit d2b292c3f6fdef5819a276acd64915bae6384a7f.
> 
> For some workloads where the host sends a batch of commands and then
> pauses, ATIO interrupt coalesce can cause some incoming ATIO entries to
> be ignored for extended periods of time, resulting in slow performance,
> timeouts, and aborted commands.  So disable interrupt coalesce and
> re-enable the dedicated ATIO MSI-X interrupt.
> 
> Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
> ---
> drivers/scsi/qla2xxx/qla_target.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
> index cb97f625970d..80fd9980dfc9 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -6932,14 +6932,8 @@ qlt_24xx_config_rings(struct scsi_qla_host *vha)
> 
> 	if (ha->flags.msix_enabled) {
> 		if (IS_QLA83XX(ha) || IS_QLA27XX(ha) || IS_QLA28XX(ha)) {
> -			if (IS_QLA2071(ha)) {
> -				/* 4 ports Baker: Enable Interrupt Handshake */
> -				icb->msix_atio = 0;
> -				icb->firmware_options_2 |= cpu_to_le32(BIT_26);
> -			} else {
> -				icb->msix_atio = cpu_to_le16(msix->entry);
> -				icb->firmware_options_2 &= cpu_to_le32(~BIT_26);
> -			}
> +			icb->msix_atio = cpu_to_le16(msix->entry);
> +			icb->firmware_options_2 &= cpu_to_le32(~BIT_26);
> 			ql_dbg(ql_dbg_init, vha, 0xf072,
> 			    "Registering ICB vector 0x%x for atio que.\n",
> 			    msix->entry);
> 

Looks Good. 

Reviewed-by: Himanshu Madhani <himanshu.madhani@oracle.com>

--
Himanshu Madhani	Oracle Linux Engineering
Nilesh Javali Aug. 5, 2022, 10:09 a.m. UTC | #3
> -----Original Message-----
> From: Tony Battersby <tonyb@cybernetics.com>
> Sent: Wednesday, July 20, 2022 7:14 PM
> To: Quinn Tran <qutran@marvell.com>; Nilesh Javali <njavali@marvell.com>;
> GR-QLogic-Storage-Upstream <GR-QLogic-Storage-
> Upstream@marvell.com>; James E.J. Bottomley <jejb@linux.ibm.com>;
> Martin K. Petersen <martin.petersen@oracle.com>
> Cc: linux-scsi@vger.kernel.org
> Subject: [EXT] [PATCH] scsi: qla2xxx: Disable ATIO interrupt coalesce for quad
> port ISP27XX
> 
> External Email
> 
> ----------------------------------------------------------------------
> This message was previously sent to scst-devel:
> https://urldefense.proofpoint.com/v2/url?u=https-
> 3A__sourceforge.net_p_scst_mailman_scst-2Ddevel_thread_d381eb86-
> 2Dd446-2D4f9e-2D03c3-2Daa93d93dd074-2540cybernetics.com_-
> 23msg37682919&d=DwICaQ&c=nKjWec2b6R0mOyPaz7xtfQ&r=FAW9wuzbtH
> IZL7SV63sr8rG59Hctu-eGu0G9pxwOXgQ&m=5GSS_PnOk-k6a-
> MRuYO0WiUj5AYxgz6x5377udNiDMopJa4pfZuAGdw9D6_7hGm0&s=Zr-
> 65TXp2V-zJGp7xf3uPA_nJIDQdeTeJ11VpYsiBKc&e=
> 
> I am using a quad-port 16 Gbps QLE2694L (ISP2071) with SCST 3.6 +
> qla2x00t-32gbit and kernel 5.15.  The following old commit that went
> into upstream kernel v4.16 is causing a problem for me:
> 
> commit d2b292c3f6fdef5819a276acd64915bae6384a7f
> Author: Quinn Tran <quinn.tran@cavium.com>
> Date:   Thu Dec 28 12:33:17 2017 -0800
> 
>     scsi: qla2xxx: Enable ATIO interrupt handshake for ISP27XX
> 
>     Enable ATIO Q interrupt handshake for ISP27XX. This patch
>     coalesce ATIO's interrupts for Quad port ISP27XX adapter.
>     Interrupt coalesce allows performance to scale for this
>     specific case.
> 
> This commit was present in qla2xxx when qla2x00t-32gbit was first
> imported into SCST.
> 
> With the "ATIO interrupt coalesce" enabled by the above patch, ATIO
> interrupts are handled by qla24xx_msix_default() rather than by
> qla83xx_msix_atio_q() for the ISP2071.  I guess this is supposed to
> generate fewer interrupts so that ATIO entries can be handled in larger
> batches.  But this causes a problem where sometimes
> qlt_24xx_process_atio_queue() returns while the hardware is still adding
> ATIO entries to the queue, but then the hardware doesn't generate
> another interrupt until a separate event sometimes minutes later.  This
> leaves incoming ATIO entries (i.e. incoming target-mode SCSI commands)
> ignored in the adapter's hardware queue for a long time until the host
> sends another command at a different time to generate an interrupt.
> There does not seem to be any timeout function that generates an
> interrupt after a short period of inactivity to process the remainder of
> the ATIO queue, which is what would be necessary for interrupt coalesce
> to function properly.
> 
> In my case I am using virtual tape drives, which generally process only
> one command at a time, so there will often never be a "next" command to
> generate an interrupt until the previous command completes.  If the
> previous command is stuck in the adapter's ATIO queue, then the host
> will timeout and abort the command, and the task management function to
> abort the command generates the interrupt that causes the command to
> finally be received by SCST, so the command appears to have been aborted
> immediately.  With disk drives the situation might be a bit different
> since a busy disk might get a steady stream of commands to generate
> additional interrupts to process the ATIO queue, but even that is not
> guaranteed, so there might still be problems.
> 
> I tried to fix the problem by modifying qla24xx_msix_default() to call
> qlt_24xx_process_atio_queue() before wrt_reg_dword(&reg->hccr,
> HCCRX_CLR_RISC_INT) instead of after, but that didn't help.  The problem
> was solved by disabling the ATIO interrupt coalesce feature and
> re-enabling the dedicated ATIO MSI-X interrupt (qla83xx_msix_atio_q())
> for calling qlt_24xx_process_atio_queue().  Another workaround is to use
> ql2xenablemsix=0, but of course keeping MSI-X enabled is better.
> 
> The patch below is against the upstream Linux kernel.  It can be applied
> to SCST with "patch -p4" from the qla2x00t-32gbit directory.
> 
> If someone really wants to keep the interrupt coalesce feature, it could
> be turned into a module parameter instead.
> 
> From e5888ce5dbffff2d38d9dafd4841d6d3bcda4365 Mon Sep 17 00:00:00
> 2001
> From: Tony Battersby <tonyb@cybernetics.com>
> Date: Thu, 7 Jul 2022 15:08:01 -0400
> Subject: [PATCH] scsi: qla2xxx: Disable ATIO interrupt coalesce for quad port
> ISP27XX
> 
> This partially reverts commit d2b292c3f6fdef5819a276acd64915bae6384a7f.
> 
> For some workloads where the host sends a batch of commands and then
> pauses, ATIO interrupt coalesce can cause some incoming ATIO entries to
> be ignored for extended periods of time, resulting in slow performance,
> timeouts, and aborted commands.  So disable interrupt coalesce and
> re-enable the dedicated ATIO MSI-X interrupt.
> 
> Signed-off-by: Tony Battersby <tonyb@cybernetics.com>
> ---
>  drivers/scsi/qla2xxx/qla_target.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_target.c
> b/drivers/scsi/qla2xxx/qla_target.c
> index cb97f625970d..80fd9980dfc9 100644
> --- a/drivers/scsi/qla2xxx/qla_target.c
> +++ b/drivers/scsi/qla2xxx/qla_target.c
> @@ -6932,14 +6932,8 @@ qlt_24xx_config_rings(struct scsi_qla_host *vha)
> 
>  	if (ha->flags.msix_enabled) {
>  		if (IS_QLA83XX(ha) || IS_QLA27XX(ha) || IS_QLA28XX(ha)) {
> -			if (IS_QLA2071(ha)) {
> -				/* 4 ports Baker: Enable Interrupt Handshake
> */
> -				icb->msix_atio = 0;
> -				icb->firmware_options_2 |=
> cpu_to_le32(BIT_26);
> -			} else {
> -				icb->msix_atio = cpu_to_le16(msix->entry);
> -				icb->firmware_options_2 &=
> cpu_to_le32(~BIT_26);
> -			}
> +			icb->msix_atio = cpu_to_le16(msix->entry);
> +			icb->firmware_options_2 &= cpu_to_le32(~BIT_26);
>  			ql_dbg(ql_dbg_init, vha, 0xf072,
>  			    "Registering ICB vector 0x%x for atio que.\n",
>  			    msix->entry);

Thanks for the patch.
Reviewed-by: Nilesh Javali <njavali@marvell.com>
Martin K. Petersen Aug. 12, 2022, 1:58 a.m. UTC | #4
Tony,

> This partially reverts commit d2b292c3f6fdef5819a276acd64915bae6384a7f.
>
> For some workloads where the host sends a batch of commands and then
> pauses, ATIO interrupt coalesce can cause some incoming ATIO entries
> to be ignored for extended periods of time, resulting in slow
> performance, timeouts, and aborted commands.  So disable interrupt
> coalesce and re-enable the dedicated ATIO MSI-X interrupt.

Applied to 5.20/scsi-staging, thanks!
diff mbox series

Patch

diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c
index cb97f625970d..80fd9980dfc9 100644
--- a/drivers/scsi/qla2xxx/qla_target.c
+++ b/drivers/scsi/qla2xxx/qla_target.c
@@ -6932,14 +6932,8 @@  qlt_24xx_config_rings(struct scsi_qla_host *vha)
 
 	if (ha->flags.msix_enabled) {
 		if (IS_QLA83XX(ha) || IS_QLA27XX(ha) || IS_QLA28XX(ha)) {
-			if (IS_QLA2071(ha)) {
-				/* 4 ports Baker: Enable Interrupt Handshake */
-				icb->msix_atio = 0;
-				icb->firmware_options_2 |= cpu_to_le32(BIT_26);
-			} else {
-				icb->msix_atio = cpu_to_le16(msix->entry);
-				icb->firmware_options_2 &= cpu_to_le32(~BIT_26);
-			}
+			icb->msix_atio = cpu_to_le16(msix->entry);
+			icb->firmware_options_2 &= cpu_to_le32(~BIT_26);
 			ql_dbg(ql_dbg_init, vha, 0xf072,
 			    "Registering ICB vector 0x%x for atio que.\n",
 			    msix->entry);