[SRU C/B][PATCH 0/1] mpt3sas: fix periodic resets under high-load activity

classic Classic list List threaded Threaded
5 messages Options
Reply | Threaded
Open this post in threaded view
|

[SRU C/B][PATCH 0/1] mpt3sas: fix periodic resets under high-load activity

Mauricio Faria de Oliveira-3
BugLink: https://bugs.launchpad.net/bugs/1810781

[Impact]

* Adapter resets periodically during high-load activity.

* I/O stalls until reset/reinit is complete (latency) and I/O performance
degrades across cluster (e.g., low throughput from data spread over nodes).

* The mpt3sas driver relies in a FW queue (Reply Post Descriptor Queue) in the I/O completion path; there's a MMIO register that driver uses to flag an empty entry in such queue, called Reply Post Host Index. This value is updated during the driver interrupt routine [in _base_interrupt() function].

* Happens that there are 2 registers representing the Reply Post Host Index according to the type of the adapter. They are differentiated in the driver through the "ioc->combined_reply_queue" check. By the MPI specification (vendor spec), driver should use this combined reply queue according to the number of maximum MSI-X vectors that the adapter exposes and the spec version (SAS 3.0 vs SAS 3.5).

* Currently, this is wrong checked for a class of adapters, which was fixed in the upstream
kernel commit 2b48be65685a [0]. Without this commit, we can observe spontaneous resets in the
driver due to queue overflow (FW is not aware that there are free entries in the Reply Post Descriptor Queue). The dmesg log will show the following output in case of this error:

  mpt3sas_cm0: fault_state(0x2100)!
  mpt3sas_cm0: sending diag reset !!
  mpt3sas_cm0: diag reset: SUCCESS
[followed by a lot of driver messages as result of the reset procedure]

* During these resets, I/O is stalled so it may affect performance.

[Test Case]

* It's not trivial to test the problem, but given a machine with an affected device, an I/O benchmark like FIO could be used to exercise the I/O path in a heavy way and trigger the issue.

* We have reports that the adapter "LSI Logic / Symbios Logic Device [1000:00ac]" is affected by the issue. And this commit resolved the problem.

[Regression Potential]

* This is a long-term issue from the mpt3sas driver, affecting only a class of adapters of this vendor. Since it's a clearly bug, the fix is necessary. The potential of regressions is unknown, but likely low - it changes the register used for the index updates given some set of characteristics of the adapter (according to the spec.), which restricts even more the scope of this patch.

[Additional info]

* Disco is already fixed.
* Xenial and earlier do not need this fix (no support for affected adapters).

P.S.: thanks to Guilherme Piccoli for the great explanation and most of
the SRU template.

Chaitra P B (1):
  scsi: mpt3sas: As per MPI-spec, use combined reply queue for SAS3.5
    controllers when HBA supports more than 16 MSI-x vectors.

 drivers/scsi/mpt3sas/mpt3sas_base.c | 15 ++++++++-------
 drivers/scsi/mpt3sas/mpt3sas_base.h |  1 +
 2 files changed, 9 insertions(+), 7 deletions(-)

--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

[SRU C/B][PATCH 1/1] scsi: mpt3sas: As per MPI-spec, use combined reply queue for SAS3.5 controllers when HBA supports more than 16 MSI-x vectors.

Mauricio Faria de Oliveira-3
From: Chaitra P B <[hidden email]>

BugLink: https://bugs.launchpad.net/bugs/1810781

Presently driver is using combined reply queue feature when MSI-x vectors >
8 for both SAS3 and SAS3.5 controllers.  But as per MPI-spec,

1. For SAS3 controllers, driver should use combined reply queue when HBA
supports more than 8 MSI-x vectors.

2. For SAS3.5 controllers, driver should use combined reply queue when HBA
supports more than 16 MSI-x vectors.

Modified driver code to use combined reply queue for SAS3 controllers when
HBA supports > 8 MSI-x vectors and for SAS3.5 controllers when HBA supports
> 16 MSI-x vectors.

Signed-off-by: Chaitra P B <[hidden email]>
Signed-off-by: Martin K. Petersen <[hidden email]>
(cherry picked from commit 2b48be65685a23f4ffc7a06858992bc31e98e198)
Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 15 ++++++++-------
 drivers/scsi/mpt3sas/mpt3sas_base.h |  1 +
 2 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 3c8c17c0b547..7efc7f00c907 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -2924,10 +2924,9 @@ mpt3sas_base_unmap_resources(struct MPT3SAS_ADAPTER *ioc)
  _base_free_irq(ioc);
  _base_disable_msix(ioc);
 
- if (ioc->combined_reply_queue) {
- kfree(ioc->replyPostRegisterIndex);
- ioc->replyPostRegisterIndex = NULL;
- }
+ kfree(ioc->replyPostRegisterIndex);
+ ioc->replyPostRegisterIndex = NULL;
+
 
  if (ioc->chip_phys) {
  iounmap(ioc->chip);
@@ -3034,7 +3033,7 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
  /* Use the Combined reply queue feature only for SAS3 C0 & higher
  * revision HBAs and also only when reply queue count is greater than 8
  */
- if (ioc->combined_reply_queue && ioc->reply_queue_count > 8) {
+ if (ioc->combined_reply_queue) {
  /* Determine the Supplemental Reply Post Host Index Registers
  * Addresse. Supplemental Reply Post Host Index Registers
  * starts at offset MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET and
@@ -3058,8 +3057,7 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
      MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET +
      (i * MPT3_SUP_REPLY_POST_HOST_INDEX_REG_OFFSET));
  }
- } else
- ioc->combined_reply_queue = 0;
+ }
 
  if (ioc->is_warpdrive) {
  ioc->reply_post_host_index[0] = (resource_size_t __iomem *)
@@ -5682,6 +5680,9 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc)
  facts->WhoInit = mpi_reply.WhoInit;
  facts->NumberOfPorts = mpi_reply.NumberOfPorts;
  facts->MaxMSIxVectors = mpi_reply.MaxMSIxVectors;
+ if (ioc->msix_enable && (facts->MaxMSIxVectors <=
+    MAX_COMBINED_MSIX_VECTORS(ioc->is_gen35_ioc)))
+ ioc->combined_reply_queue = 0;
  facts->RequestCredit = le16_to_cpu(mpi_reply.RequestCredit);
  facts->MaxReplyDescriptorPostQueueDepth =
     le16_to_cpu(mpi_reply.MaxReplyDescriptorPostQueueDepth);
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
index f02974c0be4a..e3095fb0f77a 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.h
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
@@ -323,6 +323,7 @@
  * There are twelve Supplemental Reply Post Host Index Registers
  * and each register is at offset 0x10 bytes from the previous one.
  */
+#define MAX_COMBINED_MSIX_VECTORS(gen35) ((gen35 == 1) ? 16 : 8)
 #define MPT3_SUP_REPLY_POST_HOST_INDEX_REG_COUNT_G3 12
 #define MPT3_SUP_REPLY_POST_HOST_INDEX_REG_COUNT_G35 16
 #define MPT3_SUP_REPLY_POST_HOST_INDEX_REG_OFFSET (0x10)
--
2.17.1


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU C/B][PATCH 1/1] scsi: mpt3sas: As per MPI-spec, use combined reply queue for SAS3.5 controllers when HBA supports more than 16 MSI-x vectors.

Stefan Bader-2
On 07.01.19 16:21, Mauricio Faria de Oliveira wrote:

> From: Chaitra P B <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1810781
>
> Presently driver is using combined reply queue feature when MSI-x vectors >
> 8 for both SAS3 and SAS3.5 controllers.  But as per MPI-spec,
>
> 1. For SAS3 controllers, driver should use combined reply queue when HBA
> supports more than 8 MSI-x vectors.
>
> 2. For SAS3.5 controllers, driver should use combined reply queue when HBA
> supports more than 16 MSI-x vectors.
>
> Modified driver code to use combined reply queue for SAS3 controllers when
> HBA supports > 8 MSI-x vectors and for SAS3.5 controllers when HBA supports
>> 16 MSI-x vectors.
>
> Signed-off-by: Chaitra P B <[hidden email]>
> Signed-off-by: Martin K. Petersen <[hidden email]>
> (cherry picked from commit 2b48be65685a23f4ffc7a06858992bc31e98e198)
> Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
Acked-by: Stefan Bader <[hidden email]>
> ---

Driver specific clean cherry pick and so far seems to have no fixup and testable.

>  drivers/scsi/mpt3sas/mpt3sas_base.c | 15 ++++++++-------
>  drivers/scsi/mpt3sas/mpt3sas_base.h |  1 +
>  2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 3c8c17c0b547..7efc7f00c907 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -2924,10 +2924,9 @@ mpt3sas_base_unmap_resources(struct MPT3SAS_ADAPTER *ioc)
>   _base_free_irq(ioc);
>   _base_disable_msix(ioc);
>  
> - if (ioc->combined_reply_queue) {
> - kfree(ioc->replyPostRegisterIndex);
> - ioc->replyPostRegisterIndex = NULL;
> - }
> + kfree(ioc->replyPostRegisterIndex);
> + ioc->replyPostRegisterIndex = NULL;
> +
>  
>   if (ioc->chip_phys) {
>   iounmap(ioc->chip);
> @@ -3034,7 +3033,7 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
>   /* Use the Combined reply queue feature only for SAS3 C0 & higher
>   * revision HBAs and also only when reply queue count is greater than 8
>   */
> - if (ioc->combined_reply_queue && ioc->reply_queue_count > 8) {
> + if (ioc->combined_reply_queue) {
>   /* Determine the Supplemental Reply Post Host Index Registers
>   * Addresse. Supplemental Reply Post Host Index Registers
>   * starts at offset MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET and
> @@ -3058,8 +3057,7 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
>       MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET +
>       (i * MPT3_SUP_REPLY_POST_HOST_INDEX_REG_OFFSET));
>   }
> - } else
> - ioc->combined_reply_queue = 0;
> + }
>  
>   if (ioc->is_warpdrive) {
>   ioc->reply_post_host_index[0] = (resource_size_t __iomem *)
> @@ -5682,6 +5680,9 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc)
>   facts->WhoInit = mpi_reply.WhoInit;
>   facts->NumberOfPorts = mpi_reply.NumberOfPorts;
>   facts->MaxMSIxVectors = mpi_reply.MaxMSIxVectors;
> + if (ioc->msix_enable && (facts->MaxMSIxVectors <=
> +    MAX_COMBINED_MSIX_VECTORS(ioc->is_gen35_ioc)))
> + ioc->combined_reply_queue = 0;
>   facts->RequestCredit = le16_to_cpu(mpi_reply.RequestCredit);
>   facts->MaxReplyDescriptorPostQueueDepth =
>      le16_to_cpu(mpi_reply.MaxReplyDescriptorPostQueueDepth);
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
> index f02974c0be4a..e3095fb0f77a 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.h
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
> @@ -323,6 +323,7 @@
>   * There are twelve Supplemental Reply Post Host Index Registers
>   * and each register is at offset 0x10 bytes from the previous one.
>   */
> +#define MAX_COMBINED_MSIX_VECTORS(gen35) ((gen35 == 1) ? 16 : 8)
>  #define MPT3_SUP_REPLY_POST_HOST_INDEX_REG_COUNT_G3 12
>  #define MPT3_SUP_REPLY_POST_HOST_INDEX_REG_COUNT_G35 16
>  #define MPT3_SUP_REPLY_POST_HOST_INDEX_REG_OFFSET (0x10)
>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

ACK: [SRU C/B][PATCH 1/1] scsi: mpt3sas: As per MPI-spec, use combined reply queue for SAS3.5 controllers when HBA supports more than 16 MSI-x vectors.

Khaled Elmously
In reply to this post by Mauricio Faria de Oliveira-3
On 2019-01-07 13:21:27 , Mauricio Faria de Oliveira wrote:

> From: Chaitra P B <[hidden email]>
>
> BugLink: https://bugs.launchpad.net/bugs/1810781
>
> Presently driver is using combined reply queue feature when MSI-x vectors >
> 8 for both SAS3 and SAS3.5 controllers.  But as per MPI-spec,
>
> 1. For SAS3 controllers, driver should use combined reply queue when HBA
> supports more than 8 MSI-x vectors.
>
> 2. For SAS3.5 controllers, driver should use combined reply queue when HBA
> supports more than 16 MSI-x vectors.
>
> Modified driver code to use combined reply queue for SAS3 controllers when
> HBA supports > 8 MSI-x vectors and for SAS3.5 controllers when HBA supports
> > 16 MSI-x vectors.
>
> Signed-off-by: Chaitra P B <[hidden email]>
> Signed-off-by: Martin K. Petersen <[hidden email]>
> (cherry picked from commit 2b48be65685a23f4ffc7a06858992bc31e98e198)
> Signed-off-by: Mauricio Faria de Oliveira <[hidden email]>
> ---
>  drivers/scsi/mpt3sas/mpt3sas_base.c | 15 ++++++++-------
>  drivers/scsi/mpt3sas/mpt3sas_base.h |  1 +
>  2 files changed, 9 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c b/drivers/scsi/mpt3sas/mpt3sas_base.c
> index 3c8c17c0b547..7efc7f00c907 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.c
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
> @@ -2924,10 +2924,9 @@ mpt3sas_base_unmap_resources(struct MPT3SAS_ADAPTER *ioc)
>   _base_free_irq(ioc);
>   _base_disable_msix(ioc);
>  
> - if (ioc->combined_reply_queue) {
> - kfree(ioc->replyPostRegisterIndex);
> - ioc->replyPostRegisterIndex = NULL;
> - }
> + kfree(ioc->replyPostRegisterIndex);
> + ioc->replyPostRegisterIndex = NULL;
> +
>  
>   if (ioc->chip_phys) {
>   iounmap(ioc->chip);
> @@ -3034,7 +3033,7 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
>   /* Use the Combined reply queue feature only for SAS3 C0 & higher
>   * revision HBAs and also only when reply queue count is greater than 8
>   */
> - if (ioc->combined_reply_queue && ioc->reply_queue_count > 8) {
> + if (ioc->combined_reply_queue) {
>   /* Determine the Supplemental Reply Post Host Index Registers
>   * Addresse. Supplemental Reply Post Host Index Registers
>   * starts at offset MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET and
> @@ -3058,8 +3057,7 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc)
>       MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET +
>       (i * MPT3_SUP_REPLY_POST_HOST_INDEX_REG_OFFSET));
>   }
> - } else
> - ioc->combined_reply_queue = 0;
> + }
>  
>   if (ioc->is_warpdrive) {
>   ioc->reply_post_host_index[0] = (resource_size_t __iomem *)
> @@ -5682,6 +5680,9 @@ _base_get_ioc_facts(struct MPT3SAS_ADAPTER *ioc)
>   facts->WhoInit = mpi_reply.WhoInit;
>   facts->NumberOfPorts = mpi_reply.NumberOfPorts;
>   facts->MaxMSIxVectors = mpi_reply.MaxMSIxVectors;
> + if (ioc->msix_enable && (facts->MaxMSIxVectors <=
> +    MAX_COMBINED_MSIX_VECTORS(ioc->is_gen35_ioc)))
> + ioc->combined_reply_queue = 0;
>   facts->RequestCredit = le16_to_cpu(mpi_reply.RequestCredit);
>   facts->MaxReplyDescriptorPostQueueDepth =
>      le16_to_cpu(mpi_reply.MaxReplyDescriptorPostQueueDepth);
> diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.h b/drivers/scsi/mpt3sas/mpt3sas_base.h
> index f02974c0be4a..e3095fb0f77a 100644
> --- a/drivers/scsi/mpt3sas/mpt3sas_base.h
> +++ b/drivers/scsi/mpt3sas/mpt3sas_base.h
> @@ -323,6 +323,7 @@
>   * There are twelve Supplemental Reply Post Host Index Registers
>   * and each register is at offset 0x10 bytes from the previous one.
>   */
> +#define MAX_COMBINED_MSIX_VECTORS(gen35) ((gen35 == 1) ? 16 : 8)
>  #define MPT3_SUP_REPLY_POST_HOST_INDEX_REG_COUNT_G3 12
>  #define MPT3_SUP_REPLY_POST_HOST_INDEX_REG_COUNT_G35 16
>  #define MPT3_SUP_REPLY_POST_HOST_INDEX_REG_OFFSET (0x10)

Acked-by: Khalid Elmously <[hidden email]>


--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team
Reply | Threaded
Open this post in threaded view
|

APPLIED: [SRU C/B][PATCH 0/1] mpt3sas: fix periodic resets under high-load activity

Khaled Elmously
In reply to this post by Mauricio Faria de Oliveira-3
On 2019-01-07 13:21:26 , Mauricio Faria de Oliveira wrote:

> BugLink: https://bugs.launchpad.net/bugs/1810781
>
> [Impact]
>
> * Adapter resets periodically during high-load activity.
>
> * I/O stalls until reset/reinit is complete (latency) and I/O performance
> degrades across cluster (e.g., low throughput from data spread over nodes).
>
> * The mpt3sas driver relies in a FW queue (Reply Post Descriptor Queue) in the I/O completion path; there's a MMIO register that driver uses to flag an empty entry in such queue, called Reply Post Host Index. This value is updated during the driver interrupt routine [in _base_interrupt() function].
>
> * Happens that there are 2 registers representing the Reply Post Host Index according to the type of the adapter. They are differentiated in the driver through the "ioc->combined_reply_queue" check. By the MPI specification (vendor spec), driver should use this combined reply queue according to the number of maximum MSI-X vectors that the adapter exposes and the spec version (SAS 3.0 vs SAS 3.5).
>
> * Currently, this is wrong checked for a class of adapters, which was fixed in the upstream
> kernel commit 2b48be65685a [0]. Without this commit, we can observe spontaneous resets in the
> driver due to queue overflow (FW is not aware that there are free entries in the Reply Post Descriptor Queue). The dmesg log will show the following output in case of this error:
>
>   mpt3sas_cm0: fault_state(0x2100)!
>   mpt3sas_cm0: sending diag reset !!
>   mpt3sas_cm0: diag reset: SUCCESS
> [followed by a lot of driver messages as result of the reset procedure]
>
> * During these resets, I/O is stalled so it may affect performance.
>
> [Test Case]
>
> * It's not trivial to test the problem, but given a machine with an affected device, an I/O benchmark like FIO could be used to exercise the I/O path in a heavy way and trigger the issue.
>
> * We have reports that the adapter "LSI Logic / Symbios Logic Device [1000:00ac]" is affected by the issue. And this commit resolved the problem.
>
> [Regression Potential]
>
> * This is a long-term issue from the mpt3sas driver, affecting only a class of adapters of this vendor. Since it's a clearly bug, the fix is necessary. The potential of regressions is unknown, but likely low - it changes the register used for the index updates given some set of characteristics of the adapter (according to the spec.), which restricts even more the scope of this patch.
>
> [Additional info]
>
> * Disco is already fixed.
> * Xenial and earlier do not need this fix (no support for affected adapters).
>
> P.S.: thanks to Guilherme Piccoli for the great explanation and most of
> the SRU template.
>
> Chaitra P B (1):
>   scsi: mpt3sas: As per MPI-spec, use combined reply queue for SAS3.5
>     controllers when HBA supports more than 16 MSI-x vectors.
>
>  drivers/scsi/mpt3sas/mpt3sas_base.c | 15 ++++++++-------
>  drivers/scsi/mpt3sas/mpt3sas_base.h |  1 +
>  2 files changed, 9 insertions(+), 7 deletions(-)
>
> --
> 2.17.1
>
>
> --
> kernel-team mailing list
> [hidden email]
> https://lists.ubuntu.com/mailman/listinfo/kernel-team

--
kernel-team mailing list
[hidden email]
https://lists.ubuntu.com/mailman/listinfo/kernel-team