mpt3sas: full mq support

Cleanup the MSI-X handling allowing us to use
the PCI-layer provided vector allocation.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 100 +++++++++++++++++-------------------
 drivers/scsi/mpt3sas/mpt3sas_base.h |   2 -
 2 files changed, 46 insertions(+), 56 deletions(-)

drivers/scsi/mpt3sas/mpt3sas_base.c +31 -46

index f00ef88..0185a8d 100644
@@ -1129,7 +1129,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
                /* TMs are on msix_index == 0 */
                if (reply_q->msix_index == 0)
                        continue;
-               synchronize_irq(reply_q->vector);
+               synchronize_irq(pci_irq_vector(ioc->pdev, reply_q->msix_index));
        }
 }
 

@@ -1818,11 +1818,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
 
        list_for_each_entry_safe(reply_q, next, &ioc->reply_queue_list, list) {
                list_del(&reply_q->list);
-               if (smp_affinity_enable) {
-                       irq_set_affinity_hint(reply_q->vector, NULL);
-                       free_cpumask_var(reply_q->affinity_hint);
-               }
-               free_irq(reply_q->vector, reply_q);
+               free_irq(pci_irq_vector(ioc->pdev, reply_q->msix_index),
+                        reply_q);
                kfree(reply_q);
        }
 }

@@ -1831,13 +1828,13 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
  * _base_request_irq - request irq
  * @ioc: per adapter object
  * @index: msix index into vector table
- * @vector: irq vector
  *
  * Inserting respective reply_queue into the list.
  */
 static int
-_base_request_irq(struct MPT3SAS_ADAPTER *ioc, u8 index, u32 vector)
+_base_request_irq(struct MPT3SAS_ADAPTER *ioc, u8 index)
 {
+       struct pci_dev *pdev = ioc->pdev;
        struct adapter_reply_queue *reply_q;
        int r;
 

@@ -1849,14 +1846,6 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
        }
        reply_q->ioc = ioc;
        reply_q->msix_index = index;
-       reply_q->vector = vector;
-
-       if (smp_affinity_enable) {
-               if (!zalloc_cpumask_var(&reply_q->affinity_hint, GFP_KERNEL)) {
-                       kfree(reply_q);
-                       return -ENOMEM;
-               }
-       }
 
        atomic_set(&reply_q->busy, 0);
        if (ioc->msix_enable)

@@ -1865,12 +1854,11 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
        else
                snprintf(reply_q->name, MPT_NAME_LENGTH, "%s%d",
                    ioc->driver_name, ioc->id);
-       r = request_irq(vector, _base_interrupt, IRQF_SHARED, reply_q->name,
-           reply_q);
+       r = request_irq(pci_irq_vector(pdev, index), _base_interrupt,
+                       IRQF_SHARED, reply_q->name, reply_q);
        if (r) {
                pr_err(MPT3SAS_FMT "unable to allocate interrupt %d!\n",
-                   reply_q->name, vector);
-               free_cpumask_var(reply_q->affinity_hint);
+                      reply_q->name, pci_irq_vector(pdev, index));
                kfree(reply_q);
                return -EBUSY;
        }

@@ -1906,6 +1894,21 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
        if (!nr_msix)
                return;
 
+       if (smp_affinity_enable) {
+               list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {
+                       const cpumask_t *mask = pci_irq_get_affinity(ioc->pdev,
+                                                       reply_q->msix_index);
+                       if (!mask) {
+                               pr_warn(MPT3SAS_FMT "no affinity for msi %x\n",
+                                       ioc->name, reply_q->msix_index);
+                               continue;
+                       }
+
+                       for_each_cpu(cpu, mask)
+                               ioc->cpu_msix_table[cpu] = reply_q->msix_index;
+               }
+               return;
+       }
        cpu = cpumask_first(cpu_online_mask);
 
        list_for_each_entry(reply_q, &ioc->reply_queue_list, list) {

@@ -1919,18 +1922,9 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
                        group++;
 
                for (i = 0 ; i < group ; i++) {
-                       ioc->cpu_msix_table[cpu] = index;
-                       if (smp_affinity_enable)
-                               cpumask_or(reply_q->affinity_hint,
-                                  reply_q->affinity_hint, get_cpu_mask(cpu));
+                       ioc->cpu_msix_table[cpu] = reply_q->msix_index;
                        cpu = cpumask_next(cpu, cpu_online_mask);
                }
-               if (smp_affinity_enable)
-                       if (irq_set_affinity_hint(reply_q->vector,
-                                          reply_q->affinity_hint))
-                               dinitprintk(ioc, pr_info(MPT3SAS_FMT
-                                "Err setting affinity hint to irq vector %d\n",
-                                ioc->name, reply_q->vector));
                index++;
        }
 }

@@ -1957,10 +1951,10 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
 static int
 _base_enable_msix(struct MPT3SAS_ADAPTER *ioc)
 {
-       struct msix_entry *entries, *a;
        int r;
        int i, local_max_msix_vectors;
        u8 try_msix = 0;
+       unsigned int irq_flags = PCI_IRQ_MSIX;
 
        if (msix_disable == -1 || msix_disable == 0)
                try_msix = 1;

@@ -1972,7 +1966,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
                goto try_ioapic;
 
        ioc->reply_queue_count = min_t(int, ioc->cpu_count,
-           ioc->msix_vector_count);
+               ioc->msix_vector_count);
 
        printk(MPT3SAS_FMT "MSI-X vectors supported: %d, no of cores"
          ": %d, max_msix_vectors: %d\n", ioc->name, ioc->msix_vector_count,

@@ -1993,46 +1987,42 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
        if (ioc->msix_vector_count < ioc->cpu_count)
                smp_affinity_enable = 0;
 
-       entries = kcalloc(ioc->reply_queue_count, sizeof(struct msix_entry),
-           GFP_KERNEL);
-       if (!entries) {
-               dfailprintk(ioc, pr_info(MPT3SAS_FMT
-                       "kcalloc failed @ at %s:%d/%s() !!!\n",
-                       ioc->name, __FILE__, __LINE__, __func__));
-               goto try_ioapic;
-       }
+       if (smp_affinity_enable)
+               irq_flags |= PCI_IRQ_AFFINITY;
 
-       for (i = 0, a = entries; i < ioc->reply_queue_count; i++, a++)
-               a->entry = i;
-
-       r = pci_enable_msix_exact(ioc->pdev, entries, ioc->reply_queue_count);
-       if (r) {
+       r = pci_alloc_irq_vectors(ioc->pdev, 1, ioc->reply_queue_count,
+                                 irq_flags);
+       if (r < 0) {
                dfailprintk(ioc, pr_info(MPT3SAS_FMT
-                       "pci_enable_msix_exact failed (r=%d) !!!\n",

drivers/scsi/mpt3sas/mpt3sas_base.c +13 -5

+       for (i = 0; i < ioc->msix_vector_count; i++) {
+               r = _base_request_irq(ioc, i);
                if (r) {
                        _base_free_irq(ioc);
                        _base_disable_msix(ioc);
-                       kfree(entries);
                        goto try_ioapic;
                }
        }
 
-       kfree(entries);
        return 0;
 
 /* failback to io_apic interrupt routing */
  try_ioapic:
 
        ioc->reply_queue_count = 1;
-       r = _base_request_irq(ioc, 0, ioc->pdev->irq);
+       r = pci_alloc_irq_vectors(ioc->pdev, 1, 1, PCI_IRQ_LEGACY);
+       if (r < 0) {
+               dfailprintk(ioc, pr_info(MPT3SAS_FMT
+                       "pci_alloc_irq_vector(legacy) failed (r=%d) !!!\n",
+                       ioc->name, r));
+       } else
+               r = _base_request_irq(ioc, 0);
 
        return r;
 }

@@ -2203,7 +2193,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
        list_for_each_entry(reply_q, &ioc->reply_queue_list, list)
                pr_info(MPT3SAS_FMT "%s: IRQ %d\n",
                    reply_q->name,  ((ioc->msix_enable) ? "PCI-MSI-X enabled" :
-                   "IO-APIC enabled"), reply_q->vector);
+                   "IO-APIC enabled"),
+                   pci_irq_vector(ioc->pdev, reply_q->msix_index));
 
        pr_info(MPT3SAS_FMT "iomem(0x%016llx), mapped(0x%p), size(%d)\n",
            ioc->name, (unsigned long long)chip_phys, ioc->chip, memap_sz);

@@ -5338,7 +5329,8 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
                    sizeof(resource_size_t *), GFP_KERNEL);
                if (!ioc->reply_post_host_index) {
                        dfailprintk(ioc, pr_info(MPT3SAS_FMT "allocation "
-                               "for cpu_msix_table failed!!!\n", ioc->name));
+                               "for reply_post_host_index failed!!!\n",
+                               ioc->name));
                        r = -ENOMEM;
                        goto out_free_resources;
                }

drivers/scsi/mpt3sas/mpt3sas_base.h +0 -3

index 394fe13..505574e 100644
@@ -719,12 +719,10 @@ struct _event_ack_list {
 struct adapter_reply_queue {
        struct MPT3SAS_ADAPTER  *ioc;
        u8                      msix_index;
-       unsigned int            vector;
        u32                     reply_post_host_index;
        Mpi2ReplyDescriptorsUnion_t *reply_post_free;
        char                    name[MPT_NAME_LENGTH];
        atomic_t                busy;
-       cpumask_var_t           affinity_hint;
        struct list_head        list;
 };
 
--

1.8.5.6

Quoted 9 lines · Hannes Reinecke

Cleanup the MSI-X handling allowing us to use
the PCI-layer provided vector allocation.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 100 +++++++++++++++++-------------------
 drivers/scsi/mpt3sas/mpt3sas_base.h |   2 -
 2 files changed, 46 insertions(+), 56 deletions(-)

drivers/scsi/mpt3sas/mpt3sas_base.c static int mpt3sas_remove_dead_ioc_func(void *arg) · line 2003

Quoted 5 lines

        ioc->msix_enable = 1;

drivers/scsi/mpt3sas/mpt3sas_base.c +1 -2

-       for (i = 0, a = entries; i < ioc->reply_queue_count; i++, a++) {
-               r = _base_request_irq(ioc, i, a->vector);
+       ioc->msix_vector_count = r;

Here we should also reinitialize ioc->reply_queue_count if pci_alloc_irq_vectors returns with value less than requested MSIx vector count (i.e. ioc->reply_queue_count).

Split off _dechain_st() as separate function.
No functional change.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

drivers/scsi/mpt3sas/mpt3sas_base.c +1 -0

index 09d0008..120b317 100644
@@ -2365,6 +2365,19 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
 	return smid;
 }
 
+static void

Quoted 16 lines · Hannes Reinecke

Split off _dechain_st() as separate function.
No functional change.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

drivers/scsi/mpt3sas/mpt3sas_base.c +0 -0

index 09d0008..120b317 100644
@@ -2365,6 +2365,19 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
 	return smid;
 }
 

drivers/scsi/mpt3sas/mpt3sas_base.c static int mpt3sas_remove_dead_ioc_func(void *arg) · line 2378

Quoted 11 lines

drivers/scsi/mpt3sas/mpt3sas_base.c +11 -0

+static void
+_dechain_st(struct MPT3SAS_ADAPTER *ioc, struct scsiio_tracker *st)
+{
+	struct chain_tracker *chain_req;
+
+	while (!list_empty(&st->chain_list)) {
+		chain_req = list_first_entry(&st->chain_list,
+					     struct chain_tracker,
+					     tracker_list);
+		list_move(&chain_req->tracker_list, &ioc->free_chain_list);
+	}

Why not just use list_splice_init?

No functional change.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

drivers/scsi/mpt3sas/mpt3sas_base.c +0 -0

index 120b317..dd14596 100644
@@ -2366,6 +2366,19 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
 }
 

drivers/scsi/mpt3sas/mpt3sas_base.c +5 -10

+       }
+}
+
+static void
 _dechain_st(struct MPT3SAS_ADAPTER *ioc, struct scsiio_tracker *st)
 {
        struct chain_tracker *chain_req;

@@ -2402,15 +2415,7 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
                list_add(&ioc->scsi_lookup[i].tracker_list, &ioc->free_list);
                spin_unlock_irqrestore(&ioc->scsi_lookup_lock, flags);
 
-               /*
-                * See _wait_for_commands_to_complete() call with regards
-                * to this code.
-                */
-               if (ioc->shost_recovery && ioc->pending_io_count) {
-                       if (ioc->pending_io_count == 1)
-                               wake_up(&ioc->reset_wq);
-                       ioc->pending_io_count--;
-               }
+               _base_recovery_check(ioc);
                return;
        } else if (smid < ioc->internal_smid) {
                /* hi-priority */
--

1.8.5.6

Quoted 14 lines · Hannes Reinecke

No functional change.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt3sas/mpt3sas_base.c | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

drivers/scsi/mpt3sas/mpt3sas_base.c +0 -0

index 120b317..dd14596 100644
@@ -2366,6 +2366,19 @@ static int mpt3sas_remove_dead_ioc_func(void *arg)
 }
 

drivers/scsi/mpt3sas/mpt3sas_base.c static int mpt3sas_remove_dead_ioc_func(void *arg) · line 2377

Quoted 10 lines

drivers/scsi/mpt3sas/mpt3sas_base.c +9 -0

 static void
+_base_recovery_check(struct MPT3SAS_ADAPTER *ioc)
+{
+       /*
+        * See _wait_for_commands_to_complete() call with regards to this code.
+        */
+       if (ioc->shost_recovery && ioc->pending_io_count) {
+               if (ioc->pending_io_count == 1)
+                       wake_up(&ioc->reset_wq);
+               ioc->pending_io_count = 0;

I think here we should reduce the pending_io_count by one instead of assigning it to zero in-order to have same behavior as before.

Just a wrapper around the scsi lookup array and only used in one place, so open-code it.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt3sas/mpt3sas_scsih.c | 15 +--------------
 1 file changed, 1 insertion(+), 14 deletions(-)

drivers/scsi/mpt3sas/mpt3sas_scsih.c +1 -15

index b5c966e..979f95a 100644
@@ -1061,19 +1061,6 @@ struct _sas_node *
 }
 
 /**
- * _scsih_scsi_lookup_get - returns scmd entry
- * @ioc: per adapter object
- * @smid: system request message index
- *
- * Returns the smid stored scmd pointer.
- */
-static struct scsi_cmnd *
-_scsih_scsi_lookup_get(struct MPT3SAS_ADAPTER *ioc, u16 smid)
-{
-       return ioc->scsi_lookup[smid - 1].scmd;
-}
-
-/**
  * _scsih_scsi_lookup_get_clear - returns scmd entry
  * @ioc: per adapter object
  * @smid: system request message index

@@ -6101,7 +6088,7 @@ static inline bool ata_12_16_cmd(struct scsi_cmnd *scmd)
        for (smid = 1; smid <= ioc->scsiio_depth; smid++) {
                if (ioc->shost_recovery)
                        goto out;
-               scmd = _scsih_scsi_lookup_get(ioc, smid);
+               scmd = ioc->scsi_lookup[smid - 1].scmd;
                if (!scmd)
                        continue;
                sdev = scmd->device;
--

1.8.5.6

Quoted 2 lines · Hannes Reinecke

Just a wrapper around the scsi lookup array and only used in one place, so open-code it.

I think this whole series of patches created over 14.101.00.00 version driver not on the latest 15.100.00.00 driver. So this patch will conflict with 15.100.00.00 version driver i.e. with commit 459325c466d278d3c9f51ddc9bb544c014136fd1(mpt3sas: Fix for Crusader to achieve product targets with SAS devices).

When sending a TMF via the ioctl interface we should be using the hi-priority queue instead of the scsi queue to be consistent with overall TMF usage.

When sending a TMF via the ioctl interface we should be using the hi-priority queue instead of the scsi queue to be consistent with overall TMF usage.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/scsi/mpt3sas/mpt3sas_ctl.c +0 -0

index 95f0f24..e952175 100644

drivers/scsi/mpt3sas/mpt3sas_ctl.c +0 -1

                if (!smid) {
                        pr_err(MPT3SAS_FMT "%s: failed obtaining a smid\n",
                            ioc->name, __func__);
--

1.8.5.6

Quoted 14 lines · Hannes Reinecke

When sending a TMF via the ioctl interface we should be using the hi-priority queue instead of the scsi queue to be consistent with overall TMF usage.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/scsi/mpt3sas/mpt3sas_ctl.c +0 -0

index 95f0f24..e952175 100644

drivers/scsi/mpt3sas/mpt3sas_ctl.c enum block_state { · line 723

Quoted 5 lines

drivers/scsi/mpt3sas/mpt3sas_ctl.c +1 -1

@@ -720,7 +720,7 @@ enum block_state {
                }
        } else {
 
-               smid = mpt3sas_base_get_smid_scsiio(ioc, ioc->ctl_cb_idx, NULL);
+               smid = mpt3sas_base_get_smid_hpr(ioc, ioc->ctl_cb_idx);

This else part is not for TM path, It is for IO path. For the TM request we are already using smid from hpr queue, as shown below

if (mpi_request->Function == MPI2_FUNCTION_SCSI_TASK_MGMT) {
                smid = mpt3sas_base_get_smid_hpr(ioc, ioc->ctl_cb_idx);
                if (!smid) {
                        pr_err(MPT3SAS_FMT "%s: failed obtaining a smid\n",
                            ioc->name, __func__);
                        ret = -EAGAIN;
                        goto out;
                }
        } else {

                smid = mpt3sas_base_get_smid_scsiio(ioc, ioc->ctl_cb_idx, NULL);

Quoted 24 lines · Sreekanth Reddy

Quoted 14 lines · Hannes Reinecke

When sending a TMF via the ioctl interface we should be using the hi-priority queue instead of the scsi queue to be consistent with overall TMF usage.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/scsi/mpt3sas/mpt3sas_ctl.c +0 -0

index 95f0f24..e952175 100644

drivers/scsi/mpt3sas/mpt3sas_ctl.c enum block_state { · line 723

Quoted 5 lines

drivers/scsi/mpt3sas/mpt3sas_ctl.c +1 -1

@@ -720,7 +720,7 @@ enum block_state {
                }
        } else {
 
-               smid = mpt3sas_base_get_smid_scsiio(ioc, ioc->ctl_cb_idx, NULL);
+               smid = mpt3sas_base_get_smid_hpr(ioc, ioc->ctl_cb_idx);

This else part is not for TM path, It is for IO path. For the TM request we are already using smid from hpr queue, as shown below

Quoted 12 lines

if (mpi_request->Function == MPI2_FUNCTION_SCSI_TASK_MGMT) {
                smid = mpt3sas_base_get_smid_hpr(ioc, ioc->ctl_cb_idx);
                if (!smid) {
                        pr_err(MPT3SAS_FMT "%s: failed obtaining a smid\n",
                            ioc->name, __func__);
                        ret = -EAGAIN;
                        goto out;
                }
        } else {

                smid = mpt3sas_base_get_smid_scsiio(ioc, ioc->ctl_cb_idx, NULL);

Yes, I know.
But the current method of inserting a SCSI command completely goes against blk-mq request usage; with blk-mq the tags are managed with the block layer, so you need to integrate with that to get a correct tag.

Is this _really_ a crucial functionality? Can't we just drop it and use sg/bsg for this?
It would make my life _so_ much easier; the alternative would be to either implement 'real' reserved command handling or rewriting the above code-path in terms of 'struct request', packing and unpacking as we go. Neither is very appealing.

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)

Quoted 42 lines · Hannes Reinecke

Quoted 24 lines · Sreekanth Reddy

Quoted 14 lines · Hannes Reinecke

When sending a TMF via the ioctl interface we should be using the hi-priority queue instead of the scsi queue to be consistent with overall TMF usage.

Signed-off-by Hannes Reinecke <[email protected]>

---
 drivers/scsi/mpt3sas/mpt3sas_ctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

drivers/scsi/mpt3sas/mpt3sas_ctl.c +0 -0

index 95f0f24..e952175 100644

drivers/scsi/mpt3sas/mpt3sas_ctl.c enum block_state { · line 723

Quoted 5 lines

drivers/scsi/mpt3sas/mpt3sas_ctl.c +1 -1

@@ -720,7 +720,7 @@ enum block_state {
                }
        } else {
 
-               smid = mpt3sas_base_get_smid_scsiio(ioc, ioc->ctl_cb_idx, NULL);
+               smid = mpt3sas_base_get_smid_hpr(ioc, ioc->ctl_cb_idx);

This else part is not for TM path, It is for IO path. For the TM request we are already using smid from hpr queue, as shown below

Quoted 12 lines

if (mpi_request->Function == MPI2_FUNCTION_SCSI_TASK_MGMT) {
                smid = mpt3sas_base_get_smid_hpr(ioc, ioc->ctl_cb_idx);
                if (!smid) {
                        pr_err(MPT3SAS_FMT "%s: failed obtaining a smid\n",
                            ioc->name, __func__);
                        ret = -EAGAIN;
                        goto out;
                }
        } else {

                smid = mpt3sas_base_get_smid_scsiio(ioc, ioc->ctl_cb_idx, NULL);

Yes, I know.
But the current method of inserting a SCSI command completely goes against blk-mq request usage; with blk-mq the tags are managed with the block layer, so you need to integrate with that to get a correct tag.

Quoted 6 lines

Is this _really_ a crucial functionality? Can't we just drop it and use sg/bsg for this?
It would make my life _so_ much easier; the alternative would be to either implement 'real' reserved command handling or rewriting the above code-path in terms of 'struct request', packing and unpacking as we go. Neither is very appealing.

I think it is better to take out one request frame from can_queue count and reserve it for this ioctl scsi io. Since we allow only one ioctl command at a time.

Enable lockless command submission for scsi-mq by moving the command structure into the payload for struct request.

 4 files changed, 427 insertions(+), 57 deletions(-)

drivers/scsi/mpt3sas/mpt3sas_base.c +4 -0

index 942fb7e..29e139f 100644
@@ -867,6 +867,20 @@ struct scsiio_tracker *
 {
 	WARN_ON(!smid);
 	WARN_ON(smid >= ioc->hi_priority_smid);
+	if (shost_use_blk_mq(ioc->shost)) {
+		u16 hwq = (smid - 1) % ioc->shost->nr_hw_queues;
+		u16 tag = (smid - 1) / ioc->shost->nr_hw_queues;
+		struct blk_mq_tag_set *tagset = &ioc->shost->tag_set;

Quoted 2 lines · Hannes Reinecke

Enable lockless command submission for scsi-mq by moving the command structure into the payload for struct request.

We support cmd_size for both the mq and !mq code path, so this could be simplified by always taking your new block-mq path.

Quoted 1 line

 4 files changed, 427 insertions(+), 57 deletions(-)

The amount of new code scare me. Especially compared to the very short changelog.

Quoted 16 lines

drivers/scsi/mpt3sas/mpt3sas_base.c +8 -0

index 942fb7e..29e139f 100644
@@ -867,6 +867,20 @@ struct scsiio_tracker *
 {
 	WARN_ON(!smid);
 	WARN_ON(smid >= ioc->hi_priority_smid);
+	if (shost_use_blk_mq(ioc->shost)) {
+		u16 hwq = (smid - 1) % ioc->shost->nr_hw_queues;
+		u16 tag = (smid - 1) / ioc->shost->nr_hw_queues;
+		struct blk_mq_tag_set *tagset = &ioc->shost->tag_set;
+		struct request *req;
+
+		req = blk_mq_tag_to_rq(tagset->tags[hwq], tag);
+		if (req) {

drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 883

Quoted 6 lines

drivers/scsi/mpt3sas/mpt3sas_base.c +6 -0

+			struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
+
+			return scsi_cmd_priv(cmd);
+		} else
+			return NULL;
+	}

This looks like it basically duplicates scsi_host_find_tag().

drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 2353

Quoted 8 lines

drivers/scsi/mpt3sas/mpt3sas_base.c +8 -0

+	if (shost_use_blk_mq(ioc->shost)) {
+		unsigned int unique_tag = blk_mq_unique_tag(scmd->request);
+		u16 hwq = blk_mq_unique_tag_to_hwq(unique_tag);
+		u16 tag = blk_mq_unique_tag_to_tag(unique_tag);
+		u16 smid = (tag * ioc->shost->nr_hw_queues) + hwq + 1;
+
+		request = scsi_cmd_priv(scmd);
+		request->scmd = scmd;

no need for a backpointer, blk_mq_rq_from_pdu gets your back to the request, and scsi_cmd_priv back to driver private data.

drivers/scsi/mpt3sas/mpt3sas_base.c struct scsiio_tracker * · line 2356

Quoted 3 lines

drivers/scsi/mpt3sas/mpt3sas_base.c +3 -0

+		request->cb_idx = cb_idx;
+		request->msix_io = hwq;
+		request->smid = smid;

why do we need to store the smid?

drivers/scsi/mpt3sas/mpt3sas_base.c enum block_state { · line 642

Quoted 2 lines

drivers/scsi/mpt3sas/mpt3sas_base.c +0 -0

@@ -562,12 +642,7 @@ enum block_state {
 _ctl_set_task_mid(struct MPT3SAS_ADAPTER *ioc, struct mpt3_ioctl_command *karg,

I think this function just needs to go away instantly. Trying to send task management requests from userspace for any active smid is plain dangerous.

Yes, they are with your latest patchset.

Yes, but scsi_host_find_tag() still relies on req->special, which it really shouldn't after your patches; scsi_cmd_priv() should be used there.

Quoted 4 lines

I think this function just needs to go away instantly. Trying to send task management requests from userspace for any active smid is plain dangerous.

Hey, I didn't to it. I just did the conversion.

But indeed, it's of questionable value.

I can remove it with the next iteration.

So, I'm happy to rebase it on top of your BLOCK_PC changes. Just tell me which tree to use.

Hi all,

this is a patchset to enable full multiqueue support for the mpt3sas driver. While the HBA only has a single mailbox register for submitting commands, it does have individual receive queues per MSI-X interrupt and as such does benefit from converting it to full multiqueue support.

Quoted 6 lines · Hannes Reinecke

Hi all,

this is a patchset to enable full multiqueue support for the mpt3sas driver. While the HBA only has a single mailbox register for submitting commands, it does have individual receive queues per MSI-X interrupt and as such does benefit from converting it to full multiqueue support.

Explanation and numbers on why this would be beneficial, please. We should not need multiple submissions queues for a single register to benefit from multiple completion queues.

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Tuesday, January 31, 2017 4:47 PM
To: Christoph Hellwig
Cc: Martin K. Petersen; James Bottomley; [email protected];

Prakash; Kashyap Desai; [email protected]
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 4 lines · Christoph Hellwig

Quoted 3 lines · Hannes Reinecke

Hi all,

this is a patchset to enable full multiqueue support for the mpt3sas

driver.

Quoted 3 lines

Quoted 3 lines

While the HBA only has a single mailbox register for submitting commands, it does have individual receive queues per MSI-X interrupt and as such does benefit from converting it to full multiqueue

Quoted 5 lines

Explanation and numbers on why this would be beneficial, please. We should not need multiple submissions queues for a single register to benefit from multiple completion queues.

Well, the actual throughput very strongly depends on the blk-mq-sched patches from Jens.
As this is barely finished I didn't post any numbers yet.

However:
With multiqueue support:
4k seq read : io=60573MB, bw=1009.2MB/s, iops=258353, runt= 60021msec With scsi-mq on 1 queue:
4k seq read : io=17369MB, bw=296291KB/s, iops=74072, runt= 60028msec So yes, there _is_ a benefit.

(Which is actually quite cool, as these tests were done on a SAS3 HBA,

we're getting close to the theoretical maximum of 1.2GB/s). (Unlike the single-queue case :-)

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB

(AG Nürnberg)

Quoted 5 lines

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Tuesday, January 31, 2017 4:47 PM
To: Christoph Hellwig
Cc: Martin K. Petersen; James Bottomley; [email protected];

Sathya

Quoted 12 lines

Prakash; Kashyap Desai; [email protected]
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 4 lines · Christoph Hellwig

Quoted 3 lines · Hannes Reinecke

Hi all,

this is a patchset to enable full multiqueue support for the mpt3sas

driver.

Quoted 3 lines

Quoted 3 lines

While the HBA only has a single mailbox register for submitting commands, it does have individual receive queues per MSI-X interrupt and as such does benefit from converting it to full multiqueue

support.

Quoted 9 lines

Quoted 5 lines

Explanation and numbers on why this would be beneficial, please. We should not need multiple submissions queues for a single register to benefit from multiple completion queues.

Well, the actual throughput very strongly depends on the blk-mq-sched patches from Jens.
As this is barely finished I didn't post any numbers yet.

Quoted 8 lines

However:
With multiqueue support:
4k seq read : io=60573MB, bw=1009.2MB/s, iops=258353, runt= 60021msec With scsi-mq on 1 queue:
4k seq read : io=17369MB, bw=296291KB/s, iops=74072, runt= 60028msec So yes, there _is_ a benefit.

(Which is actually quite cool, as these tests were done on a SAS3 HBA,

so

Quoted 2 lines

we're getting close to the theoretical maximum of 1.2GB/s). (Unlike the single-queue case :-)

Hannes -

Can you share detail about setup ? How many drives do you have and how is connection (enclosure -> drives. ??) ?
To me it looks like current mpt3sas driver might be taking more hit in spinlock operation (penalty on NUMA arch is more compare to single core server) unlike we have in megaraid_sas driver use of shared blk tag.

I mean " [PATCH 08/10] mpt3sas: lockless command submission for scsi-mq" patch is improving performance removing spinlock overhead and attempting to get request using blk_tags.
Are you seeing performance improvement if you hard code nr_hw_queues = 1 in below code changes part of "[PATCH 10/10] mpt3sas: scsi-mq interrupt steering"

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Wednesday, February 01, 2017 12:21 PM
To: Kashyap Desai; Christoph Hellwig
Cc: Martin K. Petersen; James Bottomley; [email protected]; Sathya
Prakash Veerichetty; PDL-MPT-FUSIONLINUX; Sreekanth Reddy
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 13 lines · Kashyap Desai

Quoted 5 lines

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Tuesday, January 31, 2017 4:47 PM
To: Christoph Hellwig
Cc: Martin K. Petersen; James Bottomley; [email protected];

Sathya

Quoted 7 lines

Prakash; Kashyap Desai; [email protected]
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 3 lines · Christoph Hellwig

Quoted 2 lines · Hannes Reinecke

Hi all,

Quoted 20 lines

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

this is a patchset to enable full multiqueue support for the mpt3sas

driver.

Quoted 4 lines

Quoted 4 lines

While the HBA only has a single mailbox register for submitting commands, it does have individual receive queues per MSI-X interrupt and as such does benefit from converting it to full multiqueue

support.

Quoted 12 lines

Quoted 5 lines

Explanation and numbers on why this would be beneficial, please. We should not need multiple submissions queues for a single register to benefit from multiple completion queues.

Well, the actual throughput very strongly depends on the blk-mq-sched patches from Jens.
As this is barely finished I didn't post any numbers yet.

However:
With multiqueue support:
4k seq read : io=60573MB, bw=1009.2MB/s, iops=258353, runt=

60021msec

Quoted 10 lines

Quoted 6 lines

With scsi-mq on 1 queue:
4k seq read : io=17369MB, bw=296291KB/s, iops=74072, runt= 60028msec So yes, there _is_ a benefit.

(Which is actually quite cool, as these tests were done on a SAS3 HBA,

so

Quoted 2 lines

we're getting close to the theoretical maximum of 1.2GB/s). (Unlike the single-queue case :-)

Quoted 9 lines

Hannes -

Can you share detail about setup ? How many drives do you have and how is connection (enclosure -> drives. ??) ?
To me it looks like current mpt3sas driver might be taking more hit in spinlock operation (penalty on NUMA arch is more compare to single core
server) unlike we have in megaraid_sas driver use of shared blk tag.

The tests were done with a single LSI SAS3008 connected to a NetApp E- series (2660), using 4 LUNs under MD-RAID0.

Megaraid_sas is even worse here; due to the odd nature of the 'fusion' implementation we're ending up having _two_ sets of tags, making it really hard to use scsi-mq here.

(Not that I didn't try; but lacking a proper backend it's really hard to evaluate
the benefit of those ... spinning HDDs simply don't cut it here)

Quoted 1 line

I mean " [PATCH 08/10] mpt3sas: lockless command submission for scsi-

mq"

Quoted 6 lines

patch is improving performance removing spinlock overhead and attempting to get request using blk_tags.
Are you seeing performance improvement if you hard code nr_hw_queues = 1 in below code changes part of "[PATCH 10/10] mpt3sas: scsi-mq interrupt steering"

No. The numbers posted above are generated with exactly that patch; the first line is running with nr_hw_queues=32 and the second line with nr_hw_queues=1.

Curiously, though, patch 8/10 also reduces the 'can_queue' value by dividing
it by the number of CPUs (required for blk tag space scaling). If I _increase_ can_queue after setting up the tagspace to the original value
performance _drops_ again.
Most unexpected; I'll be doing more experimenting there.

Full results will be presented at VAULT, btw :-)

Cheers,

Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Quoted 63 lines

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Wednesday, February 01, 2017 12:21 PM
To: Kashyap Desai; Christoph Hellwig
Cc: Martin K. Petersen; James Bottomley; [email protected]; Sathya
Prakash Veerichetty; PDL-MPT-FUSIONLINUX; Sreekanth Reddy
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 13 lines · Kashyap Desai

Quoted 5 lines

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Tuesday, January 31, 2017 4:47 PM
To: Christoph Hellwig
Cc: Martin K. Petersen; James Bottomley; [email protected];

Sathya

Quoted 7 lines

Prakash; Kashyap Desai; [email protected]
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 3 lines · Christoph Hellwig

Quoted 2 lines · Hannes Reinecke

Hi all,

Quoted 20 lines

Quoted 7 lines

Quoted 2 lines

Quoted 2 lines

this is a patchset to enable full multiqueue support for the mpt3sas

driver.

Quoted 4 lines

Quoted 4 lines

While the HBA only has a single mailbox register for submitting commands, it does have individual receive queues per MSI-X interrupt and as such does benefit from converting it to full multiqueue

support.

Quoted 12 lines

Quoted 5 lines

Explanation and numbers on why this would be beneficial, please. We should not need multiple submissions queues for a single register to benefit from multiple completion queues.

Well, the actual throughput very strongly depends on the blk-mq-sched patches from Jens.
As this is barely finished I didn't post any numbers yet.

However:
With multiqueue support:
4k seq read : io=60573MB, bw=1009.2MB/s, iops=258353, runt=

60021msec

Quoted 10 lines

Quoted 6 lines

With scsi-mq on 1 queue:
4k seq read : io=17369MB, bw=296291KB/s, iops=74072, runt= 60028msec So yes, there _is_ a benefit.

(Which is actually quite cool, as these tests were done on a SAS3 HBA,

so

Quoted 2 lines

we're getting close to the theoretical maximum of 1.2GB/s). (Unlike the single-queue case :-)

Quoted 9 lines

Hannes -

Can you share detail about setup ? How many drives do you have and how is connection (enclosure -> drives. ??) ?
To me it looks like current mpt3sas driver might be taking more hit in spinlock operation (penalty on NUMA arch is more compare to single core
server) unlike we have in megaraid_sas driver use of shared blk tag.

Quoted 6 lines

The tests were done with a single LSI SAS3008 connected to a NetApp E- series (2660), using 4 LUNs under MD-RAID0.

Megaraid_sas is even worse here; due to the odd nature of the 'fusion' implementation we're ending up having _two_ sets of tags, making it really hard to use scsi-mq here.

Current megaraid_sas as single submission queue exposed to the blk-mq will not encounter similar performance issue.
We may not see significant improvement of performance if we attempt the same for megaraid_sas driver.
We had similar discussion for megaraid_sas and hpsa.
http://www.spinics.net/lists/linux-scsi/msg101838.html

I am seeing this patch series is similar attempt for mpt3sas..Am I missing anything ?

Megaraid_sas driver just do indexing from blk_tag and fire IO quick enough unlike mpt3sas where we have lock contention @driver level as bottleneck.

Quoted 12 lines

(Not that I didn't try; but lacking a proper backend it's really hard to evaluate
the benefit of those ... spinning HDDs simply don't cut it here)

Quoted 1 line

I mean " [PATCH 08/10] mpt3sas: lockless command submission for scsi-

mq"

Quoted 6 lines

patch is improving performance removing spinlock overhead and attempting to get request using blk_tags.
Are you seeing performance improvement if you hard code nr_hw_queues = 1 in below code changes part of "[PATCH 10/10] mpt3sas: scsi-mq interrupt steering"

Quoted 3 lines

No. The numbers posted above are generated with exactly that patch; the first line is running with nr_hw_queues=32 and the second line with nr_hw_queues=1.

Thanks Hannes. That clarifies. Can you share <fio> script you have used ?

If my understanding correct, you will see theoretical maximum of 1.2GBp/s if you restrict your work load to single numa node. This is just for understanding if <mpt3sas> driver spinlocks are adding overhead. We have seen such overhead on multi-socket server and it is reasonable to reduce lock in mpt3sas driver, but only concern is exposing HBA for multiple submission queue to blk-mq is really not required and trying to figure out if we have any side effect of doing that.

Quoted 54 lines · Kashyap Desai

Quoted 54 lines

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Wednesday, February 01, 2017 12:21 PM
To: Kashyap Desai; Christoph Hellwig
Cc: Martin K. Petersen; James Bottomley; [email protected]; Sathya
Prakash Veerichetty; PDL-MPT-FUSIONLINUX; Sreekanth Reddy
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 42 lines · Kashyap Desai

Quoted 6 lines

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Tuesday, January 31, 2017 4:47 PM
To: Christoph Hellwig
Cc: Martin K. Petersen; James Bottomley; [email protected];

Sathya

Quoted 6 lines

Prakash; Kashyap Desai; [email protected]
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 1 line · Christoph Hellwig

Quoted 13 lines

Quoted 6 lines · Hannes Reinecke

Quoted 5 lines

Hi all,

this is a patchset to enable full multiqueue support for the mpt3sas

driver.

Quoted 5 lines

Quoted 5 lines

While the HBA only has a single mailbox register for submitting commands, it does have individual receive queues per MSI-X interrupt and as such does benefit from converting it to full multiqueue

support.

Quoted 13 lines

Quoted 6 lines

Explanation and numbers on why this would be beneficial, please. We should not need multiple submissions queues for a single register to benefit from multiple completion queues.

Well, the actual throughput very strongly depends on the blk-mq-sched patches from Jens.
As this is barely finished I didn't post any numbers yet.

However:
With multiqueue support:
4k seq read : io=60573MB, bw=1009.2MB/s, iops=258353, runt=

Quoted 5 lines

Quoted 5 lines

60021msec

Quoted 4 lines

Quoted 4 lines

With scsi-mq on 1 queue:
4k seq read : io=17369MB, bw=296291KB/s, iops=74072, runt= 60028msec So yes, there _is_ a benefit.

Quoted 35 lines

Quoted 20 lines

Quoted 11 lines

Quoted 4 lines

(Which is actually quite cool, as these tests were done on a SAS3 HBA,

so

Quoted 3 lines

we're getting close to the theoretical maximum of 1.2GB/s). (Unlike the single-queue case :-)

Quoted 9 lines

Hannes -

Can you share detail about setup ? How many drives do you have and how is connection (enclosure -> drives. ??) ?
To me it looks like current mpt3sas driver might be taking more hit in spinlock operation (penalty on NUMA arch is more compare to single core
server) unlike we have in megaraid_sas driver use of shared blk tag.

Quoted 6 lines

The tests were done with a single LSI SAS3008 connected to a NetApp E- series (2660), using 4 LUNs under MD-RAID0.

Megaraid_sas is even worse here; due to the odd nature of the 'fusion' implementation we're ending up having _two_ sets of tags, making it really hard to use scsi-mq here.

Current megaraid_sas as single submission queue exposed to the blk-mq will not encounter similar performance issue.
We may not see significant improvement of performance if we attempt the same for megaraid_sas driver.
We had similar discussion for megaraid_sas and hpsa.
http://www.spinics.net/lists/linux-scsi/msg101838.html

Quoted 3 lines

I am seeing this patch series is similar attempt for mpt3sas..Am I missing anything ?

No, you don't. That is precisely the case.

The different here is that mpt3sas is actually exposing hardware capabilities, whereas with megaraid_sas (and hpsa) we're limited by the hardware implementation to a single completion queue shared between HBA and OS. With mpt3sas we're having per-interrupt completion queues (well, for newer firmware :-) so we can take advantage of scsi-mq.

(And if someone had done a _proper_ design of the megaraid_sas_fusion thing by exposing several submission and completion queues for megaraid_sas itself instead of bolting the existing megaraid_sas single queue approach ontop of the mpt3sas multiqueue design we could have done the same thing there ... sigh)

Quoted 24 lines

Megaraid_sas driver just do indexing from blk_tag and fire IO quick enough unlike mpt3sas where we have lock contention @driver level as bottleneck.

Quoted 14 lines

(Not that I didn't try; but lacking a proper backend it's really hard to evaluate
the benefit of those ... spinning HDDs simply don't cut it here)

Quoted 1 line

I mean " [PATCH 08/10] mpt3sas: lockless command submission for scsi-

mq"

Quoted 7 lines

patch is improving performance removing spinlock overhead and attempting to get request using blk_tags.
Are you seeing performance improvement if you hard code nr_hw_queues = 1 in below code changes part of "[PATCH 10/10] mpt3sas: scsi-mq interrupt steering"

Quoted 3 lines

No. The numbers posted above are generated with exactly that patch; the first line is running with nr_hw_queues=32 and the second line with nr_hw_queues=1.

Thanks Hannes. That clarifies. Can you share <fio> script you have used ?

Quoted 8 lines

If my understanding correct, you will see theoretical maximum of 1.2GBp/s if you restrict your work load to single numa node. This is just for understanding if <mpt3sas> driver spinlocks are adding overhead. We have seen such overhead on multi-socket server and it is reasonable to reduce lock in mpt3sas driver, but only concern is exposing HBA for multiple submission queue to blk-mq is really not required and trying to figure out if we have any side effect of doing that.

Well, the HBA has per-MSIx completion queues, so I don't see any issues with exposing them.
blk-mq is designed to handle per-CPU queues, so exposing all hardware queues will be beneficial especially in a low-latency context; and as the experiments show, even when connected to an external storage there is a benefit to be had.

But exposing all queues might even reduce or even resolve your FW Fault status 0x2100 state; with that patch you now have each queue pulling request off the completion queue and updating the reply post host index in parallel, making the situation far more unlikely.

Cheers,

Hannes
--
Dr. Hannes Reinecke zSeries & Storage
[email protected] +49 911 74053 688
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg)

Quoted 56 lines · Hannes Reinecke

Quoted 54 lines · Kashyap Desai

Quoted 54 lines

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Wednesday, February 01, 2017 12:21 PM
To: Kashyap Desai; Christoph Hellwig
Cc: Martin K. Petersen; James Bottomley; [email protected]; Sathya
Prakash Veerichetty; PDL-MPT-FUSIONLINUX; Sreekanth Reddy
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 42 lines · Kashyap Desai

Quoted 6 lines

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Tuesday, January 31, 2017 4:47 PM
To: Christoph Hellwig
Cc: Martin K. Petersen; James Bottomley; [email protected];

Sathya

Quoted 6 lines

Prakash; Kashyap Desai; [email protected]
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 1 line · Christoph Hellwig

Quoted 13 lines

Quoted 6 lines · Hannes Reinecke

Quoted 5 lines

Hi all,

this is a patchset to enable full multiqueue support for the mpt3sas

driver.

Quoted 5 lines

Quoted 5 lines

While the HBA only has a single mailbox register for submitting commands, it does have individual receive queues per MSI-X interrupt and as such does benefit from converting it to full multiqueue

support.

Quoted 13 lines

Quoted 6 lines

Explanation and numbers on why this would be beneficial, please. We should not need multiple submissions queues for a single register to benefit from multiple completion queues.

Well, the actual throughput very strongly depends on the blk-mq-sched patches from Jens.
As this is barely finished I didn't post any numbers yet.

However:
With multiqueue support:
4k seq read : io=60573MB, bw=1009.2MB/s, iops=258353, runt=

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

60021msec

Quoted 4 lines

Quoted 4 lines

With scsi-mq on 1 queue:
4k seq read : io=17369MB, bw=296291KB/s, iops=74072, runt= 60028msec So yes, there _is_ a benefit.

Hannes,

I have created a md raid0 with 4 SAS SSD drives using below command, #mdadm --create /dev/md0 --level=0 --raid-devices=4 /dev/sdg /dev/sdh /dev/sdi /dev/sdj

And here is 'mdadm --detail /dev/md0' command output,
--------------------------------------------------------------------------------------------------------------------------
/dev/md0:

        Version : 1.2

Creation Time : Thu Feb 9 14:38:47 2017
Raid Level : raid0
Array Size : 780918784 (744.74 GiB 799.66 GB)
Raid Devices : 4
Total Devices : 4
Persistence : Superblock is persistent

Update Time : Thu Feb 9 14:38:47 2017
State : clean
Active Devices : 4
Working Devices : 4
Failed Devices : 0
Spare Devices : 0

Chunk Size : 512K

Name : host_name
UUID : b63f9da7:b7de9a25:6a46ca00:42214e22
Events : 0

Number Major Minor RaidDevice State
0 8 96 0 active sync /dev/sdg
1 8 112 1 active sync /dev/sdh
2 8 144 2 active sync /dev/sdj
3 8 128 3 active sync /dev/sdi
------------------------------------------------------------------------------------------------------------------------------

Then I have used below fio profile to run 4K sequence read operations with nr_hw_queues=1 driver and with nr_hw_queues=24 driver (as my system has two numa node and each with 12 cpus).
-----------------------------------------------------
global]
ioengine=libaio
group_reporting
direct=1
rw=read
bs=4k
allow_mounted_write=0
iodepth=128
runtime=150s

[job1]
filename=/dev/md0
-----------------------------------------------------

Here are the fio results when nr_hw_queues=1 (i.e. single request queue) with various number of job counts
1JOB 4k read : io=213268MB, bw=1421.8MB/s, iops=363975, runt=150001msec 2JOBs 4k read : io=309605MB, bw=2064.2MB/s, iops=528389, runt=150001msec 4JOBs 4k read : io=281001MB, bw=1873.4MB/s, iops=479569, runt=150002msec 8JOBs 4k read : io=236297MB, bw=1575.2MB/s, iops=403236, runt=150016msec

Here are the fio results when nr_hw_queues=24 (i.e. multiple request queue) with various number of job counts
1JOB 4k read : io=95194MB, bw=649852KB/s, iops=162463, runt=150001msec
2JOBs 4k read : io=189343MB, bw=1262.3MB/s, iops=323142, runt=150001msec 4JOBs 4k read : io=314832MB, bw=2098.9MB/s, iops=537309, runt=150001msec 8JOBs 4k read : io=277015MB, bw=1846.8MB/s, iops=472769, runt=150001msec

Here we can see that on less number of jobs count, single request queue (nr_hw_queues=1) is giving more IOPs than multi request queues(nr_hw_queues=24).

Can you please share your fio profile, so that I can try same thing on my system.

Quoted 137 lines · Sreekanth Reddy

Quoted 56 lines · Hannes Reinecke

Quoted 54 lines · Kashyap Desai

Quoted 54 lines

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Wednesday, February 01, 2017 12:21 PM
To: Kashyap Desai; Christoph Hellwig
Cc: Martin K. Petersen; James Bottomley; [email protected]; Sathya
Prakash Veerichetty; PDL-MPT-FUSIONLINUX; Sreekanth Reddy
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 42 lines · Kashyap Desai

Quoted 6 lines

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Tuesday, January 31, 2017 4:47 PM
To: Christoph Hellwig
Cc: Martin K. Petersen; James Bottomley; [email protected];

Sathya

Quoted 6 lines

Prakash; Kashyap Desai; [email protected]
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 1 line · Christoph Hellwig

Quoted 13 lines

Quoted 6 lines · Hannes Reinecke

Quoted 5 lines

Hi all,

this is a patchset to enable full multiqueue support for the mpt3sas

driver.

Quoted 5 lines

Quoted 5 lines

While the HBA only has a single mailbox register for submitting commands, it does have individual receive queues per MSI-X interrupt and as such does benefit from converting it to full multiqueue

support.

Quoted 13 lines

Quoted 6 lines

Explanation and numbers on why this would be beneficial, please. We should not need multiple submissions queues for a single register to benefit from multiple completion queues.

Well, the actual throughput very strongly depends on the blk-mq-sched patches from Jens.
As this is barely finished I didn't post any numbers yet.

However:
With multiqueue support:
4k seq read : io=60573MB, bw=1009.2MB/s, iops=258353, runt=

Quoted 5 lines

Quoted 5 lines

Quoted 5 lines

60021msec

Quoted 4 lines

Quoted 4 lines

With scsi-mq on 1 queue:
4k seq read : io=17369MB, bw=296291KB/s, iops=74072, runt= 60028msec So yes, there _is_ a benefit.

Hannes,

I have created a md raid0 with 4 SAS SSD drives using below command, #mdadm --create /dev/md0 --level=0 --raid-devices=4 /dev/sdg /dev/sdh /dev/sdi /dev/sdj

And here is 'mdadm --detail /dev/md0' command output,
--------------------------------------------------------------------------------------------------------------------------
/dev/md0:

        Version : 1.2

Creation Time : Thu Feb 9 14:38:47 2017
Raid Level : raid0
Array Size : 780918784 (744.74 GiB 799.66 GB)
Raid Devices : 4
Total Devices : 4
Persistence : Superblock is persistent

Update Time : Thu Feb 9 14:38:47 2017
State : clean
Active Devices : 4
Working Devices : 4
Failed Devices : 0
Spare Devices : 0

Chunk Size : 512K

Name : host_name
UUID : b63f9da7:b7de9a25:6a46ca00:42214e22
Events : 0

Number Major Minor RaidDevice State
0 8 96 0 active sync /dev/sdg
1 8 112 1 active sync /dev/sdh
2 8 144 2 active sync /dev/sdj
3 8 128 3 active sync /dev/sdi
------------------------------------------------------------------------------------------------------------------------------

Then I have used below fio profile to run 4K sequence read operations with nr_hw_queues=1 driver and with nr_hw_queues=24 driver (as my system has two numa node and each with 12 cpus).
-----------------------------------------------------
global]
ioengine=libaio
group_reporting
direct=1
rw=read
bs=4k
allow_mounted_write=0
iodepth=128
runtime=150s

[job1]
filename=/dev/md0
-----------------------------------------------------

Here are the fio results when nr_hw_queues=1 (i.e. single request queue) with various number of job counts
1JOB 4k read : io=213268MB, bw=1421.8MB/s, iops=363975, runt=150001msec 2JOBs 4k read : io=309605MB, bw=2064.2MB/s, iops=528389, runt=150001msec 4JOBs 4k read : io=281001MB, bw=1873.4MB/s, iops=479569, runt=150002msec 8JOBs 4k read : io=236297MB, bw=1575.2MB/s, iops=403236, runt=150016msec

Here are the fio results when nr_hw_queues=24 (i.e. multiple request queue) with various number of job counts
1JOB 4k read : io=95194MB, bw=649852KB/s, iops=162463, runt=150001msec
2JOBs 4k read : io=189343MB, bw=1262.3MB/s, iops=323142, runt=150001msec 4JOBs 4k read : io=314832MB, bw=2098.9MB/s, iops=537309, runt=150001msec 8JOBs 4k read : io=277015MB, bw=1846.8MB/s, iops=472769, runt=150001msec

Here we can see that on less number of jobs count, single request queue (nr_hw_queues=1) is giving more IOPs than multi request queues(nr_hw_queues=24).

Quoted 3 lines

Can you please share your fio profile, so that I can try same thing on my system.

Have you tried with the latest git update from Jens for-4.11/block (or for-4.11/next) branch?

I've found that using the mq-deadline scheduler has a noticeable performance boost.

The fio job I'm using is essentially the same; you just should make sure to specify a 'numjob=' statement in there.
Otherwise fio will just use a single CPU, which of course leads to averse effects in the multiqueue case.

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)

Quoted 1 line · Hannes Reinecke

On 02/09/2017 02:03 PM, Sreekanth Reddy wrote:

Quoted 78 lines

Quoted 78 lines

Hannes,

I have created a md raid0 with 4 SAS SSD drives using below command, #mdadm --create /dev/md0 --level=0 --raid-devices=4 /dev/sdg /dev/sdh /dev/sdi /dev/sdj

And here is 'mdadm --detail /dev/md0' command output,
--------------------------------------------------------------------------------------------------------------------------
/dev/md0:

        Version : 1.2

Creation Time : Thu Feb 9 14:38:47 2017
Raid Level : raid0
Array Size : 780918784 (744.74 GiB 799.66 GB)
Raid Devices : 4
Total Devices : 4
Persistence : Superblock is persistent

Update Time : Thu Feb 9 14:38:47 2017
State : clean
Active Devices : 4
Working Devices : 4
Failed Devices : 0
Spare Devices : 0

Chunk Size : 512K

Name : host_name
UUID : b63f9da7:b7de9a25:6a46ca00:42214e22
Events : 0

Number Major Minor RaidDevice State
0 8 96 0 active sync /dev/sdg
1 8 112 1 active sync /dev/sdh
2 8 144 2 active sync /dev/sdj
3 8 128 3 active sync /dev/sdi
------------------------------------------------------------------------------------------------------------------------------

Then I have used below fio profile to run 4K sequence read operations with nr_hw_queues=1 driver and with nr_hw_queues=24 driver (as my system has two numa node and each with 12 cpus).
-----------------------------------------------------
global]
ioengine=libaio
group_reporting
direct=1
rw=read
bs=4k
allow_mounted_write=0
iodepth=128
runtime=150s

[job1]
filename=/dev/md0
-----------------------------------------------------

Here are the fio results when nr_hw_queues=1 (i.e. single request queue) with various number of job counts
1JOB 4k read : io=213268MB, bw=1421.8MB/s, iops=363975, runt=150001msec 2JOBs 4k read : io=309605MB, bw=2064.2MB/s, iops=528389, runt=150001msec 4JOBs 4k read : io=281001MB, bw=1873.4MB/s, iops=479569, runt=150002msec 8JOBs 4k read : io=236297MB, bw=1575.2MB/s, iops=403236, runt=150016msec

Here are the fio results when nr_hw_queues=24 (i.e. multiple request queue) with various number of job counts
1JOB 4k read : io=95194MB, bw=649852KB/s, iops=162463, runt=150001msec
2JOBs 4k read : io=189343MB, bw=1262.3MB/s, iops=323142, runt=150001msec 4JOBs 4k read : io=314832MB, bw=2098.9MB/s, iops=537309, runt=150001msec 8JOBs 4k read : io=277015MB, bw=1846.8MB/s, iops=472769, runt=150001msec

Here we can see that on less number of jobs count, single request queue (nr_hw_queues=1) is giving more IOPs than multi request queues(nr_hw_queues=24).

Can you please share your fio profile, so that I can try same thing on my system.

Quoted 2 lines

Have you tried with the latest git update from Jens for-4.11/block (or for-4.11/next) branch?

I am using below git repo,

https://git.kernel.org/cgit/linux/kernel/git/mkp/scsi.git/log/?h=4.11/scsi-queue

Today I will try with Jens for-4.11/block.

Quoted 7 lines

I've found that using the mq-deadline scheduler has a noticeable performance boost.

The fio job I'm using is essentially the same; you just should make sure to specify a 'numjob=' statement in there.
Otherwise fio will just use a single CPU, which of course leads to averse effects in the multiqueue case.

Yes I am providing 'numjob=' on fio command line as shown below,

# fio md_fio_profile --numjobs=8 --output=fio_results.txt

Quoted 2 lines · Sreekanth Reddy

Quoted 1 line · Hannes Reinecke

On 02/09/2017 02:03 PM, Sreekanth Reddy wrote:

[ .. ]

Quoted 78 lines

Quoted 78 lines

Quoted 78 lines

Hannes,

I have created a md raid0 with 4 SAS SSD drives using below command, #mdadm --create /dev/md0 --level=0 --raid-devices=4 /dev/sdg /dev/sdh /dev/sdi /dev/sdj

And here is 'mdadm --detail /dev/md0' command output,
--------------------------------------------------------------------------------------------------------------------------
/dev/md0:

        Version : 1.2

Creation Time : Thu Feb 9 14:38:47 2017
Raid Level : raid0
Array Size : 780918784 (744.74 GiB 799.66 GB)
Raid Devices : 4
Total Devices : 4
Persistence : Superblock is persistent

Update Time : Thu Feb 9 14:38:47 2017
State : clean
Active Devices : 4
Working Devices : 4
Failed Devices : 0
Spare Devices : 0

Chunk Size : 512K

Name : host_name
UUID : b63f9da7:b7de9a25:6a46ca00:42214e22
Events : 0

Number Major Minor RaidDevice State
0 8 96 0 active sync /dev/sdg
1 8 112 1 active sync /dev/sdh
2 8 144 2 active sync /dev/sdj
3 8 128 3 active sync /dev/sdi
------------------------------------------------------------------------------------------------------------------------------

Then I have used below fio profile to run 4K sequence read operations with nr_hw_queues=1 driver and with nr_hw_queues=24 driver (as my system has two numa node and each with 12 cpus).
-----------------------------------------------------
global]
ioengine=libaio
group_reporting
direct=1
rw=read
bs=4k
allow_mounted_write=0
iodepth=128
runtime=150s

[job1]
filename=/dev/md0
-----------------------------------------------------

Here are the fio results when nr_hw_queues=1 (i.e. single request queue) with various number of job counts
1JOB 4k read : io=213268MB, bw=1421.8MB/s, iops=363975, runt=150001msec 2JOBs 4k read : io=309605MB, bw=2064.2MB/s, iops=528389, runt=150001msec 4JOBs 4k read : io=281001MB, bw=1873.4MB/s, iops=479569, runt=150002msec 8JOBs 4k read : io=236297MB, bw=1575.2MB/s, iops=403236, runt=150016msec

Here are the fio results when nr_hw_queues=24 (i.e. multiple request queue) with various number of job counts
1JOB 4k read : io=95194MB, bw=649852KB/s, iops=162463, runt=150001msec
2JOBs 4k read : io=189343MB, bw=1262.3MB/s, iops=323142, runt=150001msec 4JOBs 4k read : io=314832MB, bw=2098.9MB/s, iops=537309, runt=150001msec 8JOBs 4k read : io=277015MB, bw=1846.8MB/s, iops=472769, runt=150001msec

Here we can see that on less number of jobs count, single request queue (nr_hw_queues=1) is giving more IOPs than multi request queues(nr_hw_queues=24).

Can you please share your fio profile, so that I can try same thing on my system.

Quoted 9 lines

Quoted 2 lines

Have you tried with the latest git update from Jens for-4.11/block (or for-4.11/next) branch?

I am using below git repo,

https://git.kernel.org/cgit/linux/kernel/git/mkp/scsi.git/log/?h=4.11/scsi-queue

Today I will try with Jens for-4.11/block.

By all means, do.

Quoted 8 lines

Quoted 7 lines

I've found that using the mq-deadline scheduler has a noticeable performance boost.

The fio job I'm using is essentially the same; you just should make sure to specify a 'numjob=' statement in there.
Otherwise fio will just use a single CPU, which of course leads to averse effects in the multiqueue case.

Quoted 4 lines

Yes I am providing 'numjob=' on fio command line as shown below,

# fio md_fio_profile --numjobs=8 --output=fio_results.txt

Still, it looks as if you'd be using less jobs than you have CPUs. Which means you'll be running into a tag starvation scenario on those CPUs, especially for the small blocksizes.
What are the results if you set 'numjobs' to the number of CPUs?

Cheers,

Hannes
--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)

Quoted 101 lines · Hannes Reinecke

Quoted 2 lines · Sreekanth Reddy

Quoted 1 line · Hannes Reinecke

On 02/09/2017 02:03 PM, Sreekanth Reddy wrote:

[ .. ]

Quoted 78 lines

Quoted 78 lines

Quoted 78 lines

Hannes,

I have created a md raid0 with 4 SAS SSD drives using below command, #mdadm --create /dev/md0 --level=0 --raid-devices=4 /dev/sdg /dev/sdh /dev/sdi /dev/sdj

And here is 'mdadm --detail /dev/md0' command output,
--------------------------------------------------------------------------------------------------------------------------
/dev/md0:

        Version : 1.2

Creation Time : Thu Feb 9 14:38:47 2017
Raid Level : raid0
Array Size : 780918784 (744.74 GiB 799.66 GB)
Raid Devices : 4
Total Devices : 4
Persistence : Superblock is persistent

Update Time : Thu Feb 9 14:38:47 2017
State : clean
Active Devices : 4
Working Devices : 4
Failed Devices : 0
Spare Devices : 0

Chunk Size : 512K

Name : host_name
UUID : b63f9da7:b7de9a25:6a46ca00:42214e22
Events : 0

Number Major Minor RaidDevice State
0 8 96 0 active sync /dev/sdg
1 8 112 1 active sync /dev/sdh
2 8 144 2 active sync /dev/sdj
3 8 128 3 active sync /dev/sdi
------------------------------------------------------------------------------------------------------------------------------

Then I have used below fio profile to run 4K sequence read operations with nr_hw_queues=1 driver and with nr_hw_queues=24 driver (as my system has two numa node and each with 12 cpus).
-----------------------------------------------------
global]
ioengine=libaio
group_reporting
direct=1
rw=read
bs=4k
allow_mounted_write=0
iodepth=128
runtime=150s

[job1]
filename=/dev/md0
-----------------------------------------------------

Here are the fio results when nr_hw_queues=1 (i.e. single request queue) with various number of job counts
1JOB 4k read : io=213268MB, bw=1421.8MB/s, iops=363975, runt=150001msec 2JOBs 4k read : io=309605MB, bw=2064.2MB/s, iops=528389, runt=150001msec 4JOBs 4k read : io=281001MB, bw=1873.4MB/s, iops=479569, runt=150002msec 8JOBs 4k read : io=236297MB, bw=1575.2MB/s, iops=403236, runt=150016msec

Here are the fio results when nr_hw_queues=24 (i.e. multiple request queue) with various number of job counts
1JOB 4k read : io=95194MB, bw=649852KB/s, iops=162463, runt=150001msec
2JOBs 4k read : io=189343MB, bw=1262.3MB/s, iops=323142, runt=150001msec 4JOBs 4k read : io=314832MB, bw=2098.9MB/s, iops=537309, runt=150001msec 8JOBs 4k read : io=277015MB, bw=1846.8MB/s, iops=472769, runt=150001msec

Here we can see that on less number of jobs count, single request queue (nr_hw_queues=1) is giving more IOPs than multi request queues(nr_hw_queues=24).

Can you please share your fio profile, so that I can try same thing on my system.

Quoted 9 lines

Quoted 2 lines

Have you tried with the latest git update from Jens for-4.11/block (or for-4.11/next) branch?

I am using below git repo,

https://git.kernel.org/cgit/linux/kernel/git/mkp/scsi.git/log/?h=4.11/scsi-queue

Today I will try with Jens for-4.11/block.

By all means, do.

Quoted 8 lines

Quoted 7 lines

I've found that using the mq-deadline scheduler has a noticeable performance boost.

The fio job I'm using is essentially the same; you just should make sure to specify a 'numjob=' statement in there.
Otherwise fio will just use a single CPU, which of course leads to averse effects in the multiqueue case.

Quoted 9 lines

Quoted 4 lines

Yes I am providing 'numjob=' on fio command line as shown below,

# fio md_fio_profile --numjobs=8 --output=fio_results.txt

Still, it looks as if you'd be using less jobs than you have CPUs. Which means you'll be running into a tag starvation scenario on those CPUs, especially for the small blocksizes.
What are the results if you set 'numjobs' to the number of CPUs?

Hannes,

Tried on Jens for-4.11/block kernel repo and also set each block PD's scheduler as 'mq-deadline', and here is my results for 4K SR on md0 (raid0 with 4 drives). I have 24 CPUs and so tried even with setting numjobs=24.

fio results when nr_hw_queues=1 (i.e. single request queue) with various number of job counts

4k read when numjobs=1 : io=215553MB, bw=1437.9MB/s, iops=367874, runt=150001msec
4k read when numjobs=2 : io=307771MB, bw=2051.9MB/s, iops=525258, runt=150001msec
4k read when numjobs=4 : io=300382MB, bw=2002.6MB/s, iops=512644, runt=150002msec
4k read when numjobs=8 : io=320609MB, bw=2137.4MB/s, iops=547162, runt=150003msec
4k read when numjobs=24: io=275701MB, bw=1837.1MB/s, iops=470510, runt=150006msec

fio results when nr_hw_queues=24 (i.e. multiple request queue) with various number of job counts,

4k read when numjobs=1 : io=177600MB, bw=1183.2MB/s, iops=303102, runt=150001msec
4k read when numjobs=2 : io=182416MB, bw=1216.1MB/s, iops=311320, runt=150001msec
4k read when numjobs=4 : io=347553MB, bw=2316.2MB/s, iops=593149, runt=150002msec
4k read when numjobs=8 : io=349995MB, bw=2333.3MB/s, iops=597312, runt=150003msec
4k read when numjobs=24: io=350618MB, bw=2337.4MB/s, iops=598359, runt=150007msec

On less number of jobs single queue performing better. Where as on more number of jobs multi-queue is performing better.

Quoted 147 lines · Sreekanth Reddy

Quoted 101 lines · Hannes Reinecke

Quoted 2 lines · Sreekanth Reddy

Quoted 1 line · Hannes Reinecke

On 02/09/2017 02:03 PM, Sreekanth Reddy wrote:

[ .. ]

Quoted 78 lines

Quoted 78 lines

Quoted 78 lines

Hannes,

I have created a md raid0 with 4 SAS SSD drives using below command, #mdadm --create /dev/md0 --level=0 --raid-devices=4 /dev/sdg /dev/sdh /dev/sdi /dev/sdj

And here is 'mdadm --detail /dev/md0' command output,
--------------------------------------------------------------------------------------------------------------------------
/dev/md0:

        Version : 1.2

Creation Time : Thu Feb 9 14:38:47 2017
Raid Level : raid0
Array Size : 780918784 (744.74 GiB 799.66 GB)
Raid Devices : 4
Total Devices : 4
Persistence : Superblock is persistent

Update Time : Thu Feb 9 14:38:47 2017
State : clean
Active Devices : 4
Working Devices : 4
Failed Devices : 0
Spare Devices : 0

Chunk Size : 512K

Name : host_name
UUID : b63f9da7:b7de9a25:6a46ca00:42214e22
Events : 0

Number Major Minor RaidDevice State
0 8 96 0 active sync /dev/sdg
1 8 112 1 active sync /dev/sdh
2 8 144 2 active sync /dev/sdj
3 8 128 3 active sync /dev/sdi
------------------------------------------------------------------------------------------------------------------------------

Then I have used below fio profile to run 4K sequence read operations with nr_hw_queues=1 driver and with nr_hw_queues=24 driver (as my system has two numa node and each with 12 cpus).
-----------------------------------------------------
global]
ioengine=libaio
group_reporting
direct=1
rw=read
bs=4k
allow_mounted_write=0
iodepth=128
runtime=150s

[job1]
filename=/dev/md0
-----------------------------------------------------

Here are the fio results when nr_hw_queues=1 (i.e. single request queue) with various number of job counts
1JOB 4k read : io=213268MB, bw=1421.8MB/s, iops=363975, runt=150001msec 2JOBs 4k read : io=309605MB, bw=2064.2MB/s, iops=528389, runt=150001msec 4JOBs 4k read : io=281001MB, bw=1873.4MB/s, iops=479569, runt=150002msec 8JOBs 4k read : io=236297MB, bw=1575.2MB/s, iops=403236, runt=150016msec

Here are the fio results when nr_hw_queues=24 (i.e. multiple request queue) with various number of job counts
1JOB 4k read : io=95194MB, bw=649852KB/s, iops=162463, runt=150001msec
2JOBs 4k read : io=189343MB, bw=1262.3MB/s, iops=323142, runt=150001msec 4JOBs 4k read : io=314832MB, bw=2098.9MB/s, iops=537309, runt=150001msec 8JOBs 4k read : io=277015MB, bw=1846.8MB/s, iops=472769, runt=150001msec

Here we can see that on less number of jobs count, single request queue (nr_hw_queues=1) is giving more IOPs than multi request queues(nr_hw_queues=24).

Can you please share your fio profile, so that I can try same thing on my system.

Quoted 9 lines

Quoted 2 lines

Have you tried with the latest git update from Jens for-4.11/block (or for-4.11/next) branch?

I am using below git repo,

https://git.kernel.org/cgit/linux/kernel/git/mkp/scsi.git/log/?h=4.11/scsi-queue

Today I will try with Jens for-4.11/block.

By all means, do.

Quoted 8 lines

Quoted 7 lines

I've found that using the mq-deadline scheduler has a noticeable performance boost.

The fio job I'm using is essentially the same; you just should make sure to specify a 'numjob=' statement in there.
Otherwise fio will just use a single CPU, which of course leads to averse effects in the multiqueue case.

Quoted 9 lines

Quoted 4 lines

Yes I am providing 'numjob=' on fio command line as shown below,

# fio md_fio_profile --numjobs=8 --output=fio_results.txt

Still, it looks as if you'd be using less jobs than you have CPUs. Which means you'll be running into a tag starvation scenario on those CPUs, especially for the small blocksizes.
What are the results if you set 'numjobs' to the number of CPUs?

Hannes,

Tried on Jens for-4.11/block kernel repo and also set each block PD's scheduler as 'mq-deadline', and here is my results for 4K SR on md0 (raid0 with 4 drives). I have 24 CPUs and so tried even with setting numjobs=24.

fio results when nr_hw_queues=1 (i.e. single request queue) with various number of job counts

4k read when numjobs=1 : io=215553MB, bw=1437.9MB/s, iops=367874, runt=150001msec
4k read when numjobs=2 : io=307771MB, bw=2051.9MB/s, iops=525258, runt=150001msec
4k read when numjobs=4 : io=300382MB, bw=2002.6MB/s, iops=512644, runt=150002msec
4k read when numjobs=8 : io=320609MB, bw=2137.4MB/s, iops=547162, runt=150003msec
4k read when numjobs=24: io=275701MB, bw=1837.1MB/s, iops=470510, runt=150006msec

fio results when nr_hw_queues=24 (i.e. multiple request queue) with various number of job counts,

4k read when numjobs=1 : io=177600MB, bw=1183.2MB/s, iops=303102, runt=150001msec
4k read when numjobs=2 : io=182416MB, bw=1216.1MB/s, iops=311320, runt=150001msec
4k read when numjobs=4 : io=347553MB, bw=2316.2MB/s, iops=593149, runt=150002msec
4k read when numjobs=8 : io=349995MB, bw=2333.3MB/s, iops=597312, runt=150003msec
4k read when numjobs=24: io=350618MB, bw=2337.4MB/s, iops=598359, runt=150007msec

Quoted 3 lines

On less number of jobs single queue performing better. Where as on more number of jobs multi-queue is performing better.

Thank you for these numbers. They do very much fit with my results.

So it's as I suspected; with more parallelism we do gain from multiqueue. And with single-issue processes we do suffer a performance penalty.

However, I strongly suspect that this is an issue with block-mq itself, and not so much with mpt3sas.
Reason is that block-mq needs split the tag space into distinct ranges for each queue, and hence is hitting tag starvation far earlier the more queues are registered.
block-mq _can_ work around this by moving the issuing process onto another CPU (and thus use the tagspace from there), but this involved calling 'schedule' in the hot path. And might well account for the performance drop here.

I will be doing more tests with a high nr_hw_queue count and a low I/O issuer count; I really do guess that it's the block-layer which is performing suboptimal here.
In any case, we will be discussing blk-mq performance at LSF/MM this year; I will be bringing up the poor single-queue performance there.

At the end of the day, I strongly suspect that every self-respecting process doing heavy I/O already _is_ multithreaded, so I would not trying to optimize for the single-queue case.

Cheers,

Hannes

Hannes,

Result I have posted last time is with merge operation enabled in block layer. If I disable merge operation then I don't see much improvement with
multiple hw request queues. Here is the result,

fio results when nr_hw_queues=1,
4k read when numjobs=24: io=248387MB, bw=1655.1MB/s, iops=423905, runt=150003msec

fio results when nr_hw_queues=24,
4k read when numjobs=24: io=263904MB, bw=1759.4MB/s, iops=450393, runt=150001msec

Thanks,
Sreekanth

Quoted 9 lines

Hannes,

Result I have posted last time is with merge operation enabled in block layer. If I disable merge operation then I don't see much improvement with
multiple hw request queues. Here is the result,

Quoted 7 lines

fio results when nr_hw_queues=1,
4k read when numjobs=24: io=248387MB, bw=1655.1MB/s, iops=423905, runt=150003msec

fio results when nr_hw_queues=24,
4k read when numjobs=24: io=263904MB, bw=1759.4MB/s, iops=450393, runt=150001msec

Hannes -

I worked with Sreekanth and also understand pros/cons of Patch #10.
" [PATCH 10/10] mpt3sas: scsi-mq interrupt steering"

In above patch, can_queue of HBA is divided based on logic CPU, it means we want to mimic as if mpt3sas HBA support multi queue distributing actual resources which is single Submission H/W Queue. This approach badly impact many performance areas.

nr_hw_queues = 1 is what I observe as best performance approach since it never throttle IO if sdev->queue_depth is set to HBA queue depth. In case of nr_hw_queues = "CPUs" throttle IO at SCSI level since we never allow more than "updated can_queue" in LLD.

Below code bring actual HBA can_queue very low ( Ea on 96 logical core CPU new can_queue goes to 42, if HBA queue depth is 4K). It means we will see lots of IO throttling in scsi mid layer due to shost->can_queue reach the limit very soon if you have <fio> jobs with higher QD.

	if (ioc->shost->nr_hw_queues > 1) {
		ioc->shost->nr_hw_queues = ioc->msix_vector_count;
		ioc->shost->can_queue /= ioc->msix_vector_count;
	}

I observe negative performance if I have 8 SSD drives attached to Ventura (latest IT controller). 16 fio jobs at QD=128 gives ~1600K IOPs and the moment I switch to nr_hw_queues = "CPUs", it gave hardly ~850K IOPs. This is mainly because of host_busy stuck at very low ~169 on my setup.

May be as Sreekanth mentioned, performance improvement you have observed is due to nomerges=2 is not set and OS will attempt soft back/front merge.

I debug live machine and understood we never see parallel instance of "scsi_dispatch_cmd" as we expect due to can_queue is less. If we really has *very* large HBA QD, this patch #10 to expose multiple SQ may be useful.

For now, we are looking for updated version of patch which will only keep IT HBA in SQ mode (like we are doing in <megaraid_sas> driver) and add interface to use blk_tag in both scsi.mq and !scsi.mq mode. Sreekanth has already started working on it, but we may need to check full performance test run to post the actual patch.
May be we can cherry pick few patches from this series and get blk_tag support to improve performance of <mpt3sas> later which will not allow use to choose nr_hw_queue to be tunable.

Thanks, Kashyap

Quoted 3 lines

Thanks,
Sreekanth

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Wednesday, February 15, 2017 3:35 PM
To: Kashyap Desai; Sreekanth Reddy
Cc: Christoph Hellwig; Martin K. Petersen; James Bottomley; linux- [email protected]; Sathya Prakash Veerichetty; PDL-MPT-FUSIONLINUX
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 26 lines · Kashyap Desai

Quoted 8 lines

Hannes,

Result I have posted last time is with merge operation enabled in block layer. If I disable merge operation then I don't see much improvement with multiple hw request queues. Here is the result,

Quoted 7 lines

fio results when nr_hw_queues=1,
4k read when numjobs=24: io=248387MB, bw=1655.1MB/s, iops=423905, runt=150003msec

fio results when nr_hw_queues=24,
4k read when numjobs=24: io=263904MB, bw=1759.4MB/s, iops=450393, runt=150001msec

Hannes -

I worked with Sreekanth and also understand pros/cons of Patch #10.
" [PATCH 10/10] mpt3sas: scsi-mq interrupt steering"

In above patch, can_queue of HBA is divided based on logic CPU, it means we want to mimic as if mpt3sas HBA support multi queue distributing actual resources which is single Submission H/W Queue. This approach badly impact many performance areas.

Quoted 5 lines

nr_hw_queues = 1 is what I observe as best performance approach since it never throttle IO if sdev->queue_depth is set to HBA queue depth. In case of nr_hw_queues = "CPUs" throttle IO at SCSI level since we never allow more than "updated can_queue" in LLD.

True.
And this was actually one of the things I wanted to demonstrate with this patchset :-) ATM blk-mq really works best when having a distinct tag space per port/device. As soon as the hardware provides a _shared_ tag space you end up with tag starvation issues as blk-mq only allows you to do a static split of the available tagspace.
While this patchset demonstrates that the HBA itself _does_ benefit from using block-mq (especially on highly parallel loads), it also demonstrates that
_block-mq_ has issues with singlethreaded loads on this HBA (or, rather, type of HBA, as I doubt this issue is affecting mpt3sas only).

Quoted 4 lines

Below code bring actual HBA can_queue very low ( Ea on 96 logical core CPU new can_queue goes to 42, if HBA queue depth is 4K). It means we will see lots of IO throttling in scsi mid layer due to shost->can_queue reach the limit very soon if you have <fio> jobs with

higher QD.

Quoted 10 lines

	if (ioc->shost->nr_hw_queues > 1) {
		ioc->shost->nr_hw_queues = ioc->msix_vector_count;
		ioc->shost->can_queue /= ioc->msix_vector_count;
	}

I observe negative performance if I have 8 SSD drives attached to Ventura (latest IT controller). 16 fio jobs at QD=128 gives ~1600K IOPs and the moment I switch to nr_hw_queues = "CPUs", it gave hardly ~850K IOPs. This is mainly because of host_busy stuck at very low ~169 on

my setup.

Quoted 1 line

Which actually might be an issue with the way scsi is hooked into blk-mq. The SCSI stack is using 'can_queue' as a check for 'host_busy', ie if the host is
capable of accepting more commands.
As we're limiting can_queue (to get the per-queue command depth correctly) we should be using the _overall_ command depth for the can_queue value itself to make the host_busy check work correctly.

I've attached a patch for that; can you test if it makes a difference?

From current limited testing, I don't see major performance boost if we have

Quoted 3 lines

May be as Sreekanth mentioned, performance improvement you have observed is due to nomerges=2 is not set and OS will attempt soft

back/front merge.

Quoted 6 lines

I debug live machine and understood we never see parallel instance of "scsi_dispatch_cmd" as we expect due to can_queue is less. If we really has
*very* large HBA QD, this patch #10 to expose multiple SQ may be useful.

As mentioned, the above patch might help here.
The patch actually _reduced_ throughput on my end, as the requests never stayed long enough in the queue to be merged. Hence I've refrained from posting it.
But as you're able to test with SSDs this patch really should make a difference, and certainly should remove the arbitrary stalls due to host_busy.

Quoted 9 lines

For now, we are looking for updated version of patch which will only keep IT HBA in SQ mode (like we are doing in <megaraid_sas> driver) and add interface to use blk_tag in both scsi.mq and !scsi.mq mode. Sreekanth has already started working on it, but we may need to check full performance test run to post the actual patch.
May be we can cherry pick few patches from this series and get blk_tag support to improve performance of <mpt3sas> later which will not allow use to choose nr_hw_queue to be tunable.

Sure, no problem with that.
I'll be preparing another submission round, and we can discuss how we go from there.

Cheers,

Hannes

Quoted 6 lines

Thanks, Kashyap

Quoted 3 lines

Thanks,
Sreekanth

--
Dr. Hannes Reinecke Teamlead Storage & Networking
[email protected] +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton HRB 21284 (AG Nürnberg)

Quoted 77 lines

-----Original Message-----
From: Hannes Reinecke [mailto:[email protected]]
Sent: Wednesday, February 15, 2017 3:35 PM
To: Kashyap Desai; Sreekanth Reddy
Cc: Christoph Hellwig; Martin K. Petersen; James Bottomley; linux- [email protected]; Sathya Prakash Veerichetty; PDL-MPT-FUSIONLINUX
Subject: Re: [PATCH 00/10] mpt3sas: full mq support

Quoted 26 lines · Kashyap Desai

Quoted 8 lines

Hannes,

Result I have posted last time is with merge operation enabled in block layer. If I disable merge operation then I don't see much improvement with multiple hw request queues. Here is the result,

Quoted 7 lines

fio results when nr_hw_queues=1,
4k read when numjobs=24: io=248387MB, bw=1655.1MB/s, iops=423905, runt=150003msec

fio results when nr_hw_queues=24,
4k read when numjobs=24: io=263904MB, bw=1759.4MB/s, iops=450393, runt=150001msec

Hannes -

I worked with Sreekanth and also understand pros/cons of Patch #10.
" [PATCH 10/10] mpt3sas: scsi-mq interrupt steering"

In above patch, can_queue of HBA is divided based on logic CPU, it means we want to mimic as if mpt3sas HBA support multi queue distributing actual resources which is single Submission H/W Queue. This approach badly impact many performance areas.

Quoted 5 lines

nr_hw_queues = 1 is what I observe as best performance approach since it never throttle IO if sdev->queue_depth is set to HBA queue depth. In case of nr_hw_queues = "CPUs" throttle IO at SCSI level since we never allow more than "updated can_queue" in LLD.

True.
And this was actually one of the things I wanted to demonstrate with this patchset :-) ATM blk-mq really works best when having a distinct tag space per port/device. As soon as the hardware provides a _shared_ tag space you end up with tag starvation issues as blk-mq only allows you to do a static split of the available tagspace.
While this patchset demonstrates that the HBA itself _does_ benefit from using block-mq (especially on highly parallel loads), it also demonstrates that
_block-mq_ has issues with singlethreaded loads on this HBA (or, rather, type of HBA, as I doubt this issue is affecting mpt3sas only).

Quoted 4 lines

Below code bring actual HBA can_queue very low ( Ea on 96 logical core CPU new can_queue goes to 42, if HBA queue depth is 4K). It means we will see lots of IO throttling in scsi mid layer due to shost->can_queue reach the limit very soon if you have <fio> jobs with

higher QD.

Quoted 10 lines

	if (ioc->shost->nr_hw_queues > 1) {
		ioc->shost->nr_hw_queues = ioc->msix_vector_count;
		ioc->shost->can_queue /= ioc->msix_vector_count;
	}

I observe negative performance if I have 8 SSD drives attached to Ventura (latest IT controller). 16 fio jobs at QD=128 gives ~1600K IOPs and the moment I switch to nr_hw_queues = "CPUs", it gave hardly ~850K IOPs. This is mainly because of host_busy stuck at very low ~169 on

my setup.

Quoted 1 line

Which actually might be an issue with the way scsi is hooked into blk-mq. The SCSI stack is using 'can_queue' as a check for 'host_busy', ie if the host is
capable of accepting more commands.
As we're limiting can_queue (to get the per-queue command depth correctly) we should be using the _overall_ command depth for the can_queue value itself to make the host_busy check work correctly.

Quoted 1 line

I've attached a patch for that; can you test if it makes a difference?

Hannes -
Attached patch works fine for me. FYI - We need to set device queue depth to can_queue as we are currently not doing in mpt3sas driver.

With attached patch when I tried, I see ~2-3% improvement running multiple jobs. Single job profile no difference.

So looks like we are good to reach performance with single nr_hw_queues.

We have some patches to be send so want to know how to rebase this patch series as few patches coming from Broadcom. Can we consider below as plan ?

- Patches from 1-7 will be reposted. Also Sreekanth will complete review on existing patch 1-7.
- We need blk_tag support only for nr_hw_queue = 1.

With that say, we will have many code changes/function without " shost_use_blk_mq" check and assume it is single nr_hw_queue supported <mpt3sas> driver.

Ea - Below function can be simplify - just refer tag from scmd->request and don't need check of shost_use_blk_mq + nr_hw_queue etc..

u16
mpt3sas_base_get_smid_scsiio(struct MPT3SAS_ADAPTER *ioc, u8 cb_idx,
struct scsi_cmnd *scmd)

{
        struct scsiio_tracker *request;
        u16 smid = scmd->request->tag +  1;
         ...
         return request->smid;
}

- Later we can explore if nr_hw_queue more than one really add benefit. From current limited testing, I don't see major performance boost if we have nr_hw_queue more than one.

Quoted 4 lines

- Later we can explore if nr_hw_queue more than one really add benefit. From current limited testing, I don't see major performance boost if we have nr_hw_queue more than one.

Well, the _actual_ code to support mq is rather trivial, and really serves as a
good testbed for scsi-mq.
I would prefer to leave it in, and disable it via a module parameter.

But in either case, I can rebase the patches to leave any notions of 'nr_hw_queues' to patch 8 for implementing full mq support.

And we need to discuss how to handle MPI2_FUNCTION_SCSI_IO_REQUEST; the current method doesn't work with blk-mq.
I really would like to see that go, especially as sg/bsg supports the same functionality ...

Patch 1-7 look fine to me with minor fixups, and I'd love to see them go into 4.11. The last one looks really questionable, and 8 and 9 will need some work so that the MPT passthrough ioctls either go away or make use of struct request and the block layer and SCSI infrastructure.

The SCSI passthrough commands pass in pre-formatted SGLs, so the driver just has to map them.
If we were converting that we first have to re-format the (driver-specific) SGLs into linux sg lists, only to have them converted back into driver-specific ones once queuecommand is called. You sure it's worth the effort?

The driver already reserves some tags for precisely this use-case, so it won't conflict with normal I/O operation.
So where's the problem with that?

Quoted 7 lines · Hannes Reinecke

The SCSI passthrough commands pass in pre-formatted SGLs, so the driver just has to map them.
If we were converting that we first have to re-format the (driver-specific) SGLs into linux sg lists, only to have them converted back into driver-specific ones once queuecommand is called. You sure it's worth the effort?

Quoted 3 lines

The driver already reserves some tags for precisely this use-case, so it won't conflict with normal I/O operation.
So where's the problem with that?

If it was an entirely separate path that would be easy, but it's not - see all the poking into the tag maps that your patch 8 includes. If it was just a few tags on the side not interacting with the scsi or blk-mq it wouldn't be such a problem.

But we do; we're getting the index/tag/smid from the high-priority list, which is separated from the normal SCSI I/O tag space. (which reminds me; there's another cleanup patch to be had in _ctl_do_mpt_command(), but that's beside the point).

Quoted 4 lines · Hannes Reinecke

But we do; we're getting the index/tag/smid from the high-priority list, which is separated from the normal SCSI I/O tag space. (which reminds me; there's another cleanup patch to be had in _ctl_do_mpt_command(), but that's beside the point).

The calls to blk_mq_tagset_busy_iter added in patch 8 indicate the contrary.

Patch 1-7 look fine to me with minor fixups, and I'd love to see them go into 4.11.

Quoted 2 lines · Christoph Hellwig

Patch 1-7 look fine to me with minor fixups, and I'd love to see them go into 4.11.

Any chance to see a resend of these?