RE: [PATCH RFC v7 10/12] megaraid_sas: switch fusion adapters to MQ

On 02/07/2020 11:23, Kashyap Desai wrote:

quoted

quoted

From: Hannes Reinecke <hare@suse.com>

Fusion adapters can steer completions to individual queues, and we
now

have

quoted

support for shared host-wide tags.
So we can enable multiqueue support for fusion adapters and drop the

hand-

quoted

crafted interrupt affinity settings.

Shared host tag is primarily introduced for completeness of CPU
hotplug as discussed earlier - https://lwn.net/Articles/819419/

How shall I test CPU hotplug on megaraid_sas driver ?

I have scripts like this:

----8<-----

# hotplug.sh
# enable all cpus in the system
./enable_all.sh

for((i = 0; i < 50 ; i++))
do
echo "Looping ... number $i"
# run fio on all cpus with 40 second runtime ./create_fio_task_cpu.sh 4k
read
2048 1& echo "short sleep, then disable"
sleep 5
# disable some set of cpus which means managed interrupts get shutdown #
like cpu1-50 from 0-63 ./disable_all.sh echo "long sleep $i"
sleep 50
echo "long sleep over number $i"
./enable_all.sh
sleep 3
done

----->8-----

# enable_all.sh
for((i=0; i<63; i++))
do
echo 1 > /sys/devices/system/cpu/cpu$i/online
done

--->8----

I hope to add such a test to blktests when I get a chance.

quoted

My understanding is
- This RFC + patch set from above link is required for it. I could not
see above series is committed.

It is committed and part of 5.8-rc1

The latest rc should have some scheduler fixes also.

I also note that there has been much churn on blk-mq tag code lately, and
something may be broken, so I plan to verify latest rc myself soon.

Thanks. I will try merging 5.8-rc1 and RFC and see how CPU hot plug works.

quoted

Am I missing anything. ?

You could also add this from Hannes (and add megaraid sas support):

https://lore.kernel.org/linux-scsi/20200629072021.9864-1-
hare@suse.de/T/#t

That is, if it is required. I am not sure if megaraid sas uses "internal"
commands which needs to be guarded against cpu hotplug. Nor would any of
these commands be used during a test. For hisi_sas testing, I did not
bother
adding support, and I guess that you don't need to either.

Megaraid driver use internal command but it is excluded from can_queue.
All internal command are mapped to msix index 0, which is non-managed. So we
are good w.r.t internal command.

quoted

We do not want to completely move to shared host tag. It will be shared
host tag support by default, but user should have choice to go back to
legacy path.
We will completely move to shared host tag path once it is stable and no
more field issue observed over a period of time. -

Updated <megaraid_sas> patch will looks like this -

diff --git a/megaraid_sas_base.c b/megaraid_sas_base.c
index 0066833..3b503cb 100644
--- a/megaraid_sas_base.c
+++ b/megaraid_sas_base.c

@@ -37,6 +37,7 @@
  #include <linux/poll.h>
  #include <linux/vmalloc.h>
  #include <linux/irq_poll.h>
+#include <linux/blk-mq-pci.h>

  #include <scsi/scsi.h>
  #include <scsi/scsi_cmnd.h>

@@ -113,6 +114,10 @@ unsigned int enable_sdev_max_qd;
  module_param(enable_sdev_max_qd, int, 0444);
  MODULE_PARM_DESC(enable_sdev_max_qd, "Enable sdev max qd as

can_queue.

quoted

Default: 0");

+int host_tagset_disabled = 0;
+module_param(host_tagset_disabled, int, 0444);
+MODULE_PARM_DESC(host_tagset_disabled, "Shared host tagset

enable/disable

quoted

Default: enable(1)");

The logic seems inverted here: for passing 1, I would expect Shared host
tagset enabled, while it actually means to disable, right?

No. passing 1 means shared_hosttag support will be turned off.

quoted

+
  MODULE_LICENSE("GPL");
  MODULE_VERSION(MEGASAS_VERSION);
  MODULE_AUTHOR("megaraidlinux.pdl@broadcom.com");

@@ -3115,6 +3120,18 @@ megasas_bios_param(struct scsi_device *sdev,

struct

quoted

block_device *bdev,
         return 0;
  }

+static int megasas_map_queues(struct Scsi_Host *shost)
+{
+       struct megasas_instance *instance;
+       instance = (struct megasas_instance *)shost->hostdata;
+
+       if (instance->host->nr_hw_queues == 1)
+               return 0;
+
+       return
blk_mq_pci_map_queues(&shost->tag_set.map[HCTX_TYPE_DEFAULT],
+                       instance->pdev,
instance->low_latency_index_start);
+}
+
  static void megasas_aen_polling(struct work_struct *work);

  /**

@@ -3423,8 +3440,10 @@ static struct scsi_host_template

megasas_template =

quoted

{
         .eh_timed_out = megasas_reset_timer,
         .shost_attrs = megaraid_host_attrs,
         .bios_param = megasas_bios_param,
+       .map_queues = megasas_map_queues,
         .change_queue_depth = scsi_change_queue_depth,
         .max_segment_size = 0xffffffff,
+       .host_tagset = 1,

Is your intention to always have this set for Scsi_Host, and just change
nr_hw_queues?

Actually I wanted to turn off  this feature using host_tagset and not
through nr_hw_queue. I will address this.

Additional request -
In MR we have old controllers (called MFI_SERIES). We prefer not to change
behavior for those controller.
Having host_tagset in template does not allow to cherry pick different
values for different type of controller.
If host_tagset is part of Scsi_Host OR we add check in scsi_lib.c that
host_tagset = 1 only make sense if nr_hw_queues > 1, we can cherry pick in
driver.

quoted

  };

  /**

@@ -6793,7 +6812,21 @@ static int megasas_io_attach(struct

megasas_instance *instance)
         host->max_id = MEGASAS_MAX_DEV_PER_CHANNEL;
         host->max_lun = MEGASAS_MAX_LUN;
         host->max_cmd_len = 16;
+       host->nr_hw_queues = 1;

+       /* Use shared host tagset only for fusion adaptors
+        * if there are more than one managed interrupts.
+        */
+       if ((instance->adapter_type != MFI_SERIES) &&
+               (instance->msix_vectors > 0) &&
+               !host_tagset_disabled &&
+               instance->smp_affinity_enable)
+               host->nr_hw_queues = instance->msix_vectors -
+                       instance->low_latency_index_start;
+
+       dev_info(&instance->pdev->dev, "Max firmware commands: %d"
+               " for nr_hw_queues = %d\n", instance->max_fw_cmds,
+               host->nr_hw_queues);

note: it may be good for us to add a nr_hw_queues file to scsi host
sysfs folder

I will accommodate this.

quoted

         /*
          * Notify the mid-layer about the new controller
          */

@@ -8842,6 +8875,7 @@ static int __init megasas_init(void)
                 msix_vectors = 1;
                 rdpq_enable = 0;
                 dual_qdepth_disable = 1;
+               host_tagset_disabled = 1;
         }

Thanks,
John