> > On 02/07/2020 11:23, Kashyap Desai wrote: > >> > >> From: Hannes Reinecke <hare@xxxxxxxx> > >> > >> 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@xxxxxxx/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@xxxxxxxxxxxx"); > > @@ -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
