On Mon, Jun 03, 2019 at 02:00:19PM +0100, John Garry wrote:

On 03/06/2019 12:00, Ming Lei wrote:

quoted

On Fri, May 31, 2019 at 12:38:10PM +0100, John Garry wrote:

quoted

quoted

quoted

quoted

-fallback:
-     for_each_possible_cpu(cpu)
-             hisi_hba->reply_map[cpu] = cpu % hisi_hba->queue_count;
-     /* Don't clean all CQ masks */
-}
-
 static int interrupt_init_v3_hw(struct hisi_hba *hisi_hba)
 {
      struct device *dev = hisi_hba->dev;

@@ -2383,11 +2359,6 @@ static int interrupt_init_v3_hw(struct hisi_hba *hisi_hba)

              min_msi = MIN_AFFINE_VECTORS_V3_HW;

-             hisi_hba->reply_map = devm_kcalloc(dev, nr_cpu_ids,
-                                                sizeof(unsigned int),
-                                                GFP_KERNEL);
-             if (!hisi_hba->reply_map)
-                     return -ENOMEM;
              vectors = pci_alloc_irq_vectors_affinity(hisi_hba->pci_dev,
                                                       min_msi, max_msi,
                                                       PCI_IRQ_MSI |

@@ -2395,7 +2366,6 @@ static int interrupt_init_v3_hw(struct hisi_hba *hisi_hba)
                                                       &desc);
              if (vectors < 0)
                      return -ENOENT;
-             setup_reply_map_v3_hw(hisi_hba, vectors - BASE_VECTORS_V3_HW);
      } else {
              min_msi = max_msi;
              vectors = pci_alloc_irq_vectors(hisi_hba->pci_dev, min_msi,

@@ -2896,6 +2866,18 @@ static void debugfs_snapshot_restore_v3_hw(struct hisi_hba *hisi_hba)
      clear_bit(HISI_SAS_REJECT_CMD_BIT, &hisi_hba->flags);
 }

+static int hisi_sas_map_queues(struct Scsi_Host *shost)
+{
+     struct hisi_hba *hisi_hba = shost_priv(shost);
+     struct blk_mq_queue_map *qmap = &shost->tag_set.map[HCTX_TYPE_DEFAULT];
+
+     if (auto_affine_msi_experimental)
+             return blk_mq_pci_map_queues(qmap, hisi_hba->pci_dev,
+                             BASE_VECTORS_V3_HW);
+     else
+             return blk_mq_map_queues(qmap);

I don't think that the mapping which blk_mq_map_queues() creates are not
want we want. I'm guessing that we still would like a mapping similar to
what blk_mq_pci_map_queues() produces, which is an even spread, putting
adjacent CPUs on the same queue.

For my system with 96 cpus and 16 queues, blk_mq_map_queues() would map
queue 0 to cpu 0, 16, 32, 48 ..., queue 1 to cpu 1, 17, 33 and so on.

Hi Ming,

quoted

blk_mq_map_queues() is the default or fallback mapping in case that managed
irq isn't used. If the mapping isn't good enough, we still can improve it
in future, then any driver applying it can got improved.

That's the right attitude. However, as I see, we can only know the mapping
when we know the interrupt affinity or some other mapping restriction or
rule etc, which we don't know in this case.

For now, personally I would rather if we only expose multiple queues for
when auto_affine_msi_experimental is set. I fear that we may make a
performance regression for !auto_affine_msi_experimental with this patch. We
would need to test.

I suggest to use the blk-mq generic helper.

The default queue mapping of blk_mq_map_queues() has been used for a
while, so far so good, such as, very similar way is applied on
megaraid_sas and mpt3sas, see _base_assign_reply_queues() and
megasas_setup_reply_map().

If performance drop is caused, just report it out, we could fix it.
Or even you can write a new .map_queues method just for hisi_sas v3.


Thanks,
Ming