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

[Kashyap Desai <kashyap.desai@broadcom.com>]

>
> On 02/07/2020 11:23, Kashyap Desai wrote:
> >>
> >> From: Hannes Reinecke <hare@suse.com>
> >>
> >> Fusion adapters can steer completions to individual queues, and we
> >> now
> > have
> >> support for shared host-wide tags.
> >> So we can enable multiqueue support for fusion adapters and drop the
> > hand-
> >> 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.
>
> > 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.

>
> > 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.

>
> >
> > 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.
> > 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
> > 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.
>
> > +
> >   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
> > 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 =
> > {
> >          .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.


>
> >   };
> >
> >   /**
> > @@ -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.

>
> >          /*
> >           * 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