| Message ID | 20250130173314.608836-1-sagar.biradar@microchip.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] aacraid: Reply queue mapping to CPUs based on IRQ affinity | expand |
This patch has already been Reviewed-by and Tested-by Red Hat. However, after applying this patch to scsi/6.14/staging I've asked our QA engineer, Marco Patalono, to please re-test this patch and confirm this it fixes the problem. What were you trying to do that didn't work? While running the offline CPU automation on my system with an ASR405 storage controller (AACRAID), I hit the following hung_task_timeouts: [ 85.132499] aacraid: Host adapter abort request. [ 85.132499] aacraid: Outstanding commands on (0,1,3,0): [ 103.569532] aacraid: Host adapter abort request. [ 103.569532] aacraid: Outstanding commands on (0,1,3,0): [ 106.897528] aacraid: Host adapter abort request. [ 106.897528] aacraid: Outstanding commands on (0,1,3,0): [ 106.912542] aacraid: Host bus reset request. SCSI hang ? [ 106.919612] aacraid 0000:84:00.0: Controller reset type is 3 [-- MARK -- Fri Jun 14 16:10:00 2024] [ 188.472054] aacraid: Comm Interface type2 enabled [ 270.986453] aacraid: Host adapter abort request. [ 270.986453] aacraid: Outstanding commands on (0,1,3,0): [ 271.006495] aacraid: Host bus reset request. SCSI hang ? [ 271.020473] aacraid 0000:84:00.0: Controller reset type is 3 [ 352.307955] aacraid: Comm Interface type2 enabled [ 369.271587] INFO: task kworker/u65:1:130 blocked for more than 122 seconds. [ 369.271599] Not tainted 6.9.0-7.el10.x86_64 #1 [ 369.271604] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 369.271977] INFO: task kworker/u65:2:132 blocked for more than 122 seconds. [ 369.271980] Not tainted 6.9.0-7.el10.x86_64 #1 [ 369.271981] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 369.272344] INFO: task xfsaild/dm-0:778 blocked for more than 122 seconds. [ 369.272346] Not tainted 6.9.0-7.el10.x86_64 #1 [ 369.272348] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 369.273538] INFO: task restraintd:1399 blocked for more than 122 seconds. [ 369.273541] Not tainted 6.9.0-7.el10.x86_64 #1 [ 369.273542] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 369.273895] INFO: task main.sh:1900 blocked for more than 122 seconds. [ 369.273897] Not tainted 6.9.0-7.el10.x86_64 #1 [ 369.273898] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. [ 373.404946] sd 0:1:3:0: [sdb] tag#454 timing out command, waited 180s [ 424.076623] aacraid: Host adapter abort request. [ 424.076623] aacraid: Outstanding commands on (0,1,3,0): Please provide the package NVR for which bug is seen: RHEL-10.0-20240603.86 kernel-6.9.0-7.el10 How reproducible: Often Steps to reproduce: Generate I/O to disks controller by the ASR8405 Offline all CPUs except 2 Online all CPUs With the patched/upstream kernel provided, I am not seeing any issues when running the offline CPU automation after loading the module parameter as shown below: # echo 'options aacraid aac_cpu_offline_feature=1' | sudo tee /etc/modprobe.d/aacraid.conf # dracut -f # reboot Tested-by: Marco Patalano <mpatalan@redhat.com> On 1/30/25 12:33 PM, Sagar Biradar wrote: > patch "(9dc704dcc09e scsi: aacraid: Reply queue mapping to > CPUs based on IRQ affinity)" caused issues starting V6.4.0, > which was later reverted by > patch "(c5becf57dd56 Revert "scsi: aacraid: Reply queue mapping to CPUs > based on IRQ affinity)" > > Add a new modparam "aac_cpu_offline_feature" to control CPU offlining. > By default, it's disabled (0), but can be enabled during driver load > with: > insmod ./aacraid.ko aac_cpu_offline_feature=1 > Enabling this feature allows CPU offlining but may cause some IO > performance drop. It is recommended to enable it during driver load as > the relevant changes are part of the initialization routine. > > Fixes an I/O hang that arises because of an MSIx vector not having a > mapped online CPU upon receiving completion. > > SCSI cmds use the mq_map to get the vector_no via blk_mq_unique_tag() > and blk_mq_unique_tag_to_hwq() - which are setup during the blk_mq init. > > For reserved cmds, or the ones before the blk_mq init, use the vector_no > 0, which is the norm since don't yet have a proper mapping to the queues. > > Reviewed-by: Gilbert Wu <gilbert.wu@microchip.com> > Reviewed-by:John Meneghini <jmeneghi@redhat.com> > Reviewed-by:Tomas Henzl <thenzl@redhat.com> > Tested-by: Marco Patalano <mpatalan@redhat.com> > Signed-off-by: Sagar Biradar <Sagar.Biradar@microchip.com> > --- > drivers/scsi/aacraid/aachba.c | 6 ++++++ > drivers/scsi/aacraid/aacraid.h | 2 ++ > drivers/scsi/aacraid/commsup.c | 9 ++++++++- > drivers/scsi/aacraid/linit.c | 24 ++++++++++++++++++++++++ > drivers/scsi/aacraid/src.c | 28 ++++++++++++++++++++++++++-- > 5 files changed, 66 insertions(+), 3 deletions(-) > > diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c > index abf6a82b74af..f325e79a1a01 100644 > --- a/drivers/scsi/aacraid/aachba.c > +++ b/drivers/scsi/aacraid/aachba.c > @@ -328,6 +328,12 @@ MODULE_PARM_DESC(wwn, "Select a WWN type for the arrays:\n" > "\t1 - Array Meta Data Signature (default)\n" > "\t2 - Adapter Serial Number"); > > +int aac_cpu_offline_feature; > +module_param_named(aac_cpu_offline_feature, aac_cpu_offline_feature, int, 0644); > +MODULE_PARM_DESC(aac_cpu_offline_feature, > + "This enables CPU offline feature and may result in IO performance drop in some cases:\n" > + "\t0 - Disable (default)\n" > + "\t1 - Enable"); > > static inline int aac_valid_context(struct scsi_cmnd *scsicmd, > struct fib *fibptr) { > diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h > index 8c384c25dca1..dba7ffc6d543 100644 > --- a/drivers/scsi/aacraid/aacraid.h > +++ b/drivers/scsi/aacraid/aacraid.h > @@ -1673,6 +1673,7 @@ struct aac_dev > u32 handle_pci_error; > bool init_reset; > u8 soft_reset_support; > + u8 use_map_queue; > }; > > #define aac_adapter_interrupt(dev) \ > @@ -2777,4 +2778,5 @@ extern int update_interval; > extern int check_interval; > extern int aac_check_reset; > extern int aac_fib_dump; > +extern int aac_cpu_offline_feature; > #endif > diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c > index ffef61c4aa01..1d62730e5c1b 100644 > --- a/drivers/scsi/aacraid/commsup.c > +++ b/drivers/scsi/aacraid/commsup.c > @@ -223,8 +223,15 @@ int aac_fib_setup(struct aac_dev * dev) > struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd *scmd) > { > struct fib *fibptr; > + u32 blk_tag; > + int i; > > - fibptr = &dev->fibs[scsi_cmd_to_rq(scmd)->tag]; > + if (aac_cpu_offline_feature == 1) { > + blk_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd)); > + i = blk_mq_unique_tag_to_tag(blk_tag); > + fibptr = &dev->fibs[i]; > + } else > + fibptr = &dev->fibs[scsi_cmd_to_rq(scmd)->tag]; > /* > * Null out fields that depend on being zero at the start of > * each I/O > diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c > index 91170a67cc91..fc4d35950ccb 100644 > --- a/drivers/scsi/aacraid/linit.c > +++ b/drivers/scsi/aacraid/linit.c > @@ -506,6 +506,23 @@ static int aac_sdev_configure(struct scsi_device *sdev, > return 0; > } > > +/** > + * aac_map_queues - Map hardware queues for the SCSI host > + * @shost: SCSI host structure > + * > + * Maps the default hardware queue for the given SCSI host to the > + * corresponding PCI device and enables mapped queue usage. > + */ > + > +static void aac_map_queues(struct Scsi_Host *shost) > +{ > + struct aac_dev *aac = (struct aac_dev *)shost->hostdata; > + > + blk_mq_map_hw_queues(&shost->tag_set.map[HCTX_TYPE_DEFAULT], > + &aac->pdev->dev, 0); > + aac->use_map_queue = true; > +} > + > /** > * aac_change_queue_depth - alter queue depths > * @sdev: SCSI device we are considering > @@ -1490,6 +1507,7 @@ static const struct scsi_host_template aac_driver_template = { > .bios_param = aac_biosparm, > .shost_groups = aac_host_groups, > .sdev_configure = aac_sdev_configure, > + .map_queues = aac_map_queues, > .change_queue_depth = aac_change_queue_depth, > .sdev_groups = aac_dev_groups, > .eh_abort_handler = aac_eh_abort, > @@ -1777,6 +1795,11 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id) > shost->max_lun = AAC_MAX_LUN; > > pci_set_drvdata(pdev, shost); > + if (aac_cpu_offline_feature == 1) { > + shost->nr_hw_queues = aac->max_msix; > + shost->can_queue = aac->vector_cap; > + shost->host_tagset = 1; > + } > > error = scsi_add_host(shost, &pdev->dev); > if (error) > @@ -1908,6 +1931,7 @@ static void aac_remove_one(struct pci_dev *pdev) > struct aac_dev *aac = (struct aac_dev *)shost->hostdata; > > aac_cancel_rescan_worker(aac); > + aac->use_map_queue = false; > scsi_remove_host(shost); > > __aac_shutdown(aac); > diff --git a/drivers/scsi/aacraid/src.c b/drivers/scsi/aacraid/src.c > index 28115ed637e8..b9bed3e255c4 100644 > --- a/drivers/scsi/aacraid/src.c > +++ b/drivers/scsi/aacraid/src.c > @@ -493,6 +493,10 @@ static int aac_src_deliver_message(struct fib *fib) > #endif > > u16 vector_no; > + struct scsi_cmnd *scmd; > + u32 blk_tag; > + struct Scsi_Host *shost = dev->scsi_host_ptr; > + struct blk_mq_queue_map *qmap; > > atomic_inc(&q->numpending); > > @@ -505,8 +509,28 @@ static int aac_src_deliver_message(struct fib *fib) > if ((dev->comm_interface == AAC_COMM_MESSAGE_TYPE3) > && dev->sa_firmware) > vector_no = aac_get_vector(dev); > - else > - vector_no = fib->vector_no; > + else { > + if (aac_cpu_offline_feature == 1) { > + if (!fib->vector_no || !fib->callback_data) { > + if (shost && dev->use_map_queue) { > + qmap = &shost->tag_set.map[HCTX_TYPE_DEFAULT]; > + vector_no = qmap->mq_map[raw_smp_processor_id()]; > + } > + /* > + * We hardcode the vector_no for > + * reserved commands as a valid shost is > + * absent during the init > + */ > + else > + vector_no = 0; > + } else { > + scmd = (struct scsi_cmnd *)fib->callback_data; > + blk_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd)); > + vector_no = blk_mq_unique_tag_to_hwq(blk_tag); > + } > + } else > + vector_no = fib->vector_no; > + } > > if (native_hba) { > if (fib->flags & FIB_CONTEXT_FLAG_NATIVE_HBA_TMF) {
Someone told me this email was confusing. To be clear: Red Hat has reviewed and tested multiple versions of this patch internally and we are shipping an out of tree version of this patch in RHEL-8. These changes provide a critical fix for one of our customers who is using aacraid in a production environment with offline cpu support. We've asked Sagar to submit this private version of his patch upstream but he found that it needed some rework because of the recent changes made by patch (bd326a5ad6397c scsi: replace blk_mq_pci_map_queues with blk_mq_map_hw_queues) So after refactoring his patch I asked our Red Hat QA department to retest this latest version the patch with an upstream kernel. What's included below is a description of the origin problem that was seen with a RHEL-10 kernel, based on v6.9, with a description of the test that was run. All tests are passing with this patch based on scsi/6.14/staging. Martin, can we please merge this into 6.14, or perhaps 6.15? /John On 2/10/25 12:20 PM, John Meneghini wrote: > This patch has already been Reviewed-by and Tested-by Red Hat. > > However, after applying this patch to scsi/6.14/staging I've asked our QA engineer, Marco Patalono, > to please re-test this patch and confirm this it fixes the problem. > > What were you trying to do that didn't work? > > While running the offline CPU automation on my system with an ASR405 storage controller > (AACRAID), I hit the following hung_task_timeouts: > > [ 85.132499] aacraid: Host adapter abort request. > [ 85.132499] aacraid: Outstanding commands on (0,1,3,0): > [ 103.569532] aacraid: Host adapter abort request. > [ 103.569532] aacraid: Outstanding commands on (0,1,3,0): > [ 106.897528] aacraid: Host adapter abort request. > [ 106.897528] aacraid: Outstanding commands on (0,1,3,0): > [ 106.912542] aacraid: Host bus reset request. SCSI hang ? > [ 106.919612] aacraid 0000:84:00.0: Controller reset type is 3 > [-- MARK -- Fri Jun 14 16:10:00 2024] > [ 188.472054] aacraid: Comm Interface type2 enabled > [ 270.986453] aacraid: Host adapter abort request. > [ 270.986453] aacraid: Outstanding commands on (0,1,3,0): > [ 271.006495] aacraid: Host bus reset request. SCSI hang ? > [ 271.020473] aacraid 0000:84:00.0: Controller reset type is 3 > [ 352.307955] aacraid: Comm Interface type2 enabled > [ 369.271587] INFO: task kworker/u65:1:130 blocked for more than 122 seconds. > [ 369.271599] Not tainted 6.9.0-7.el10.x86_64 #1 > [ 369.271604] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > [ 369.271977] INFO: task kworker/u65:2:132 blocked for more than 122 seconds. > [ 369.271980] Not tainted 6.9.0-7.el10.x86_64 #1 > [ 369.271981] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > [ 369.272344] INFO: task xfsaild/dm-0:778 blocked for more than 122 seconds. > [ 369.272346] Not tainted 6.9.0-7.el10.x86_64 #1 > [ 369.272348] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > [ 369.273538] INFO: task restraintd:1399 blocked for more than 122 seconds. > [ 369.273541] Not tainted 6.9.0-7.el10.x86_64 #1 > [ 369.273542] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > [ 369.273895] INFO: task main.sh:1900 blocked for more than 122 seconds. > [ 369.273897] Not tainted 6.9.0-7.el10.x86_64 #1 > [ 369.273898] "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message. > [ 373.404946] sd 0:1:3:0: [sdb] tag#454 timing out command, waited 180s > [ 424.076623] aacraid: Host adapter abort request. > [ 424.076623] aacraid: Outstanding commands on (0,1,3,0): > > Please provide the package NVR for which bug is seen: > RHEL-10.0-20240603.86 > > kernel-6.9.0-7.el10 > > How reproducible: Often > > Steps to reproduce: > Generate I/O to disks controller by the ASR8405 > Offline all CPUs except 2 > Online all CPUs > > With the patched/upstream kernel provided, I am not seeing any issues when running > the offline CPU automation after loading the module parameter as shown below: > > # echo 'options aacraid aac_cpu_offline_feature=1' | sudo tee /etc/modprobe.d/aacraid.conf > # dracut -f > # reboot > > Tested-by: Marco Patalano <mpatalan@redhat.com> > > On 1/30/25 12:33 PM, Sagar Biradar wrote: >> patch "(9dc704dcc09e scsi: aacraid: Reply queue mapping to >> CPUs based on IRQ affinity)" caused issues starting V6.4.0, >> which was later reverted by >> patch "(c5becf57dd56 Revert "scsi: aacraid: Reply queue mapping to CPUs >> based on IRQ affinity)" >> >> Add a new modparam "aac_cpu_offline_feature" to control CPU offlining. >> By default, it's disabled (0), but can be enabled during driver load >> with: >> insmod ./aacraid.ko aac_cpu_offline_feature=1 >> Enabling this feature allows CPU offlining but may cause some IO >> performance drop. It is recommended to enable it during driver load as >> the relevant changes are part of the initialization routine. >> >> Fixes an I/O hang that arises because of an MSIx vector not having a >> mapped online CPU upon receiving completion. >> >> SCSI cmds use the mq_map to get the vector_no via blk_mq_unique_tag() >> and blk_mq_unique_tag_to_hwq() - which are setup during the blk_mq init. >> >> For reserved cmds, or the ones before the blk_mq init, use the vector_no >> 0, which is the norm since don't yet have a proper mapping to the queues. >> >> Reviewed-by: Gilbert Wu <gilbert.wu@microchip.com> >> Reviewed-by:John Meneghini <jmeneghi@redhat.com> >> Reviewed-by:Tomas Henzl <thenzl@redhat.com> >> Tested-by: Marco Patalano <mpatalan@redhat.com> >> Signed-off-by: Sagar Biradar <Sagar.Biradar@microchip.com> >> --- >> drivers/scsi/aacraid/aachba.c | 6 ++++++ >> drivers/scsi/aacraid/aacraid.h | 2 ++ >> drivers/scsi/aacraid/commsup.c | 9 ++++++++- >> drivers/scsi/aacraid/linit.c | 24 ++++++++++++++++++++++++ >> drivers/scsi/aacraid/src.c | 28 ++++++++++++++++++++++++++-- >> 5 files changed, 66 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c >> index abf6a82b74af..f325e79a1a01 100644 >> --- a/drivers/scsi/aacraid/aachba.c >> +++ b/drivers/scsi/aacraid/aachba.c >> @@ -328,6 +328,12 @@ MODULE_PARM_DESC(wwn, "Select a WWN type for the arrays:\n" >> "\t1 - Array Meta Data Signature (default)\n" >> "\t2 - Adapter Serial Number"); >> +int aac_cpu_offline_feature; >> +module_param_named(aac_cpu_offline_feature, aac_cpu_offline_feature, int, 0644); >> +MODULE_PARM_DESC(aac_cpu_offline_feature, >> + "This enables CPU offline feature and may result in IO performance drop in some cases:\n" >> + "\t0 - Disable (default)\n" >> + "\t1 - Enable"); >> static inline int aac_valid_context(struct scsi_cmnd *scsicmd, >> struct fib *fibptr) { >> diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h >> index 8c384c25dca1..dba7ffc6d543 100644 >> --- a/drivers/scsi/aacraid/aacraid.h >> +++ b/drivers/scsi/aacraid/aacraid.h >> @@ -1673,6 +1673,7 @@ struct aac_dev >> u32 handle_pci_error; >> bool init_reset; >> u8 soft_reset_support; >> + u8 use_map_queue; >> }; >> #define aac_adapter_interrupt(dev) \ >> @@ -2777,4 +2778,5 @@ extern int update_interval; >> extern int check_interval; >> extern int aac_check_reset; >> extern int aac_fib_dump; >> +extern int aac_cpu_offline_feature; >> #endif >> diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c >> index ffef61c4aa01..1d62730e5c1b 100644 >> --- a/drivers/scsi/aacraid/commsup.c >> +++ b/drivers/scsi/aacraid/commsup.c >> @@ -223,8 +223,15 @@ int aac_fib_setup(struct aac_dev * dev) >> struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd *scmd) >> { >> struct fib *fibptr; >> + u32 blk_tag; >> + int i; >> - fibptr = &dev->fibs[scsi_cmd_to_rq(scmd)->tag]; >> + if (aac_cpu_offline_feature == 1) { >> + blk_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd)); >> + i = blk_mq_unique_tag_to_tag(blk_tag); >> + fibptr = &dev->fibs[i]; >> + } else >> + fibptr = &dev->fibs[scsi_cmd_to_rq(scmd)->tag]; >> /* >> * Null out fields that depend on being zero at the start of >> * each I/O >> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c >> index 91170a67cc91..fc4d35950ccb 100644 >> --- a/drivers/scsi/aacraid/linit.c >> +++ b/drivers/scsi/aacraid/linit.c >> @@ -506,6 +506,23 @@ static int aac_sdev_configure(struct scsi_device *sdev, >> return 0; >> } >> +/** >> + * aac_map_queues - Map hardware queues for the SCSI host >> + * @shost: SCSI host structure >> + * >> + * Maps the default hardware queue for the given SCSI host to the >> + * corresponding PCI device and enables mapped queue usage. >> + */ >> + >> +static void aac_map_queues(struct Scsi_Host *shost) >> +{ >> + struct aac_dev *aac = (struct aac_dev *)shost->hostdata; >> + >> + blk_mq_map_hw_queues(&shost->tag_set.map[HCTX_TYPE_DEFAULT], >> + &aac->pdev->dev, 0); >> + aac->use_map_queue = true; >> +} >> + >> /** >> * aac_change_queue_depth - alter queue depths >> * @sdev: SCSI device we are considering >> @@ -1490,6 +1507,7 @@ static const struct scsi_host_template aac_driver_template = { >> .bios_param = aac_biosparm, >> .shost_groups = aac_host_groups, >> .sdev_configure = aac_sdev_configure, >> + .map_queues = aac_map_queues, >> .change_queue_depth = aac_change_queue_depth, >> .sdev_groups = aac_dev_groups, >> .eh_abort_handler = aac_eh_abort, >> @@ -1777,6 +1795,11 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id) >> shost->max_lun = AAC_MAX_LUN; >> pci_set_drvdata(pdev, shost); >> + if (aac_cpu_offline_feature == 1) { >> + shost->nr_hw_queues = aac->max_msix; >> + shost->can_queue = aac->vector_cap; >> + shost->host_tagset = 1; >> + } >> error = scsi_add_host(shost, &pdev->dev); >> if (error) >> @@ -1908,6 +1931,7 @@ static void aac_remove_one(struct pci_dev *pdev) >> struct aac_dev *aac = (struct aac_dev *)shost->hostdata; >> aac_cancel_rescan_worker(aac); >> + aac->use_map_queue = false; >> scsi_remove_host(shost); >> __aac_shutdown(aac); >> diff --git a/drivers/scsi/aacraid/src.c b/drivers/scsi/aacraid/src.c >> index 28115ed637e8..b9bed3e255c4 100644 >> --- a/drivers/scsi/aacraid/src.c >> +++ b/drivers/scsi/aacraid/src.c >> @@ -493,6 +493,10 @@ static int aac_src_deliver_message(struct fib *fib) >> #endif >> u16 vector_no; >> + struct scsi_cmnd *scmd; >> + u32 blk_tag; >> + struct Scsi_Host *shost = dev->scsi_host_ptr; >> + struct blk_mq_queue_map *qmap; >> atomic_inc(&q->numpending); >> @@ -505,8 +509,28 @@ static int aac_src_deliver_message(struct fib *fib) >> if ((dev->comm_interface == AAC_COMM_MESSAGE_TYPE3) >> && dev->sa_firmware) >> vector_no = aac_get_vector(dev); >> - else >> - vector_no = fib->vector_no; >> + else { >> + if (aac_cpu_offline_feature == 1) { >> + if (!fib->vector_no || !fib->callback_data) { >> + if (shost && dev->use_map_queue) { >> + qmap = &shost->tag_set.map[HCTX_TYPE_DEFAULT]; >> + vector_no = qmap->mq_map[raw_smp_processor_id()]; >> + } >> + /* >> + * We hardcode the vector_no for >> + * reserved commands as a valid shost is >> + * absent during the init >> + */ >> + else >> + vector_no = 0; >> + } else { >> + scmd = (struct scsi_cmnd *)fib->callback_data; >> + blk_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd)); >> + vector_no = blk_mq_unique_tag_to_hwq(blk_tag); >> + } >> + } else >> + vector_no = fib->vector_no; >> + } >> if (native_hba) { >> if (fib->flags & FIB_CONTEXT_FLAG_NATIVE_HBA_TMF) { >
Hi Sagar! > Add a new modparam "aac_cpu_offline_feature" to control CPU offlining. > By default, it's disabled (0), but can be enabled during driver load > with: > insmod ./aacraid.ko aac_cpu_offline_feature=1 We are very hesitant when it comes to adding new module parameters. And why wouldn't you want offlining to just work? Is the performance penalty really substantial enough that we have to introduce an explicit "don't be broken" option?
Sending it again as outlook threw some delivery errors. Response inline :
> From: Martin K. Petersen > Sent: Wednesday, February 12, 2025 6:56 PM > [You don't often get email from "martin.petersen@oracle.comjames.bottomley@hansenpartnership.comjmeneghi"@redhat.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] Appears to still be a problem. I'll work with Sagar and see if we can clean this up. >> Add a new modparam "aac_cpu_offline_feature" to control CPU offlining. >> By default, it's disabled (0), but can be enabled during driver load > >> with: >> insmod ./aacraid.ko aac_cpu_offline_feature=1 > > We are very hesitant when it comes to adding new module parameters. And > why wouldn't you want offlining to just work? Is the performance penalty > really substantial enough that we have to introduce an explicit "don't > be broken" option? Yes, this is something that we debated about internally, before asking Sagar to send this patch. I agree that it would be much better if we simply fix the driver and make offline_cpu suport work. The modparam was added as a compromise, to allow current users and customers who do NOT care about cpu_offline support to keep the increased performance they want. People generally complain any time there is a performance regression. The current upstream driver is more or less unchanged when the mod param is of off, which is the default. So upstream users will see no performance regression... but don't try to offline a cpu or you will see a panic. This is the state of the current upstream driver. > Thank you for your time to review and giving your valuable opinion. > There are two reasons why I chose the modparam way > 1) As you rightly guessed - the performance penalty is high when it comes to few RAID level configurations - which is not desired > 2) Not a lot of people would use CPU offlining feature as part of their regular usage. This is mostly for admin purposes. > > These two reasons made me opt for the modparam. > We and our folks at RedHat did venture into trying few other options - but this seemed like a nice fit. Another option we thought about was making this a kconfig option. We have a patch that replaces the modparam with a Kconfig option. However, I agree it would be better to just fix the driver, performance impact notwithstanding, and ship it. For my part I'd rather have a correctly functioning driver, that's slower, but doesn't panic. It's really up to the upstream community. We need to understand what they want. /John
Sorry, we didn't see a panic with the offline cpu test, we saw a Call Trace.
He's a little more information from our testing.
These are notes from our QA group who tested this patch.
---
With "aac_cpu_offline_feature=0", the system continued to have issues with the offline_cpu test:
Sep 03 14:49:45 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: aacraid: Host adapter abort request.
aacraid: Outstanding commands on (0,1,3,0):
Sep 03 14:50:12 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: aacraid: Host adapter abort request.
aacraid: Outstanding commands on (0,1,3,0):
Sep 03 14:50:15 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: aacraid: Host adapter abort request.
aacraid: Outstanding commands on (0,1,3,0):
Sep 03 14:50:22 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: aacraid: Host adapter abort request.
aacraid: Outstanding commands on (0,1,3,0):
Sep 03 14:50:22 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: aacraid: Host bus reset request. SCSI hang ?
Sep 03 14:50:22 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: aacraid 0000:84:00.0: outstanding cmd: midlevel-0
Sep 03 14:50:22 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: aacraid 0000:84:00.0: outstanding cmd: lowlevel-0
Sep 03 14:50:22 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: aacraid 0000:84:00.0: outstanding cmd: error handler-2
Sep 03 14:50:22 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: aacraid 0000:84:00.0: outstanding cmd: firmware-0
Sep 03 14:50:22 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: aacraid 0000:84:00.0: outstanding cmd: kernel-0
Sep 03 14:50:23 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: aacraid 0000:84:00.0: Controller reset type is 3
Sep 03 14:50:23 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: aacraid 0000:84:00.0: Issuing IOP reset
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: INFO: task kworker/u513:2:478 blocked for more than 122 seconds.
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: Not tainted 5.14.0-503.5118_1431178045.el9.x86_64 #1
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: "echo 0 > /proc/sys/kernel/hung_task_timeout_secs" disables this message.
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: task:kworker/u513:2 state:D stack:0 pid:478 tgid:478 ppid:2 flags:0x00004000
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: Workqueue: xfs-cil/dm-0 xlog_cil_push_work [xfs]
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: Call Trace:
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: <TASK>
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: __schedule+0x229/0x550
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: schedule+0x2e/0xd0
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: xlog_wait_on_iclog+0x16b/0x180 [xfs]
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: ? __pfx_default_wake_function+0x10/0x10
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: xlog_cil_push_work+0x6c6/0x700 [xfs]
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: ? srso_return_thunk+0x5/0x5f
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: process_one_work+0x197/0x380
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: worker_thread+0x2fe/0x410
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: ? __pfx_worker_thread+0x10/0x10
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: kthread+0xe0/0x100
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: ? __pfx_kthread+0x10/0x10
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: ret_from_fork+0x2c/0x50
Sep 03 14:52:03 storageqe-34.fast.eng.rdu2.dc.redhat.com kernel: </TASK>
Using John's test kernel, I enabled the aac_cpu_offline_feature:
I then rebooted and ran the offline cpu test - no crashes or hangs were observed.
I generated I/O with FIO and observed the following stats:
# fio -filename=/home/test1G.img -iodepth=64 -thread -rw=randwrite -ioengine=libaio -bs=4K -direct=1 -runtime=300 -time_based -size=1G -group_reporting -name=mytest -numjobs=4
WRITE: bw=495MiB/s (519MB/s), 495MiB/s-495MiB/s (519MB/s-519MB/s), io=145GiB (156GB), run=300001-300001msec
I then ran FIO again with aacraid aac_cpu_offline_feature=0 - statistics below:
# fio -filename=/home/test1G.img -iodepth=64 -thread -rw=randwrite -ioengine=libaio -bs=4K -direct=1 -runtime=300 -time_based -size=1G -group_reporting -name=mytest -numjobs=4
WRITE: bw=505MiB/s (529MB/s), 505MiB/s-505MiB/s (529MB/s-529MB/s), io=148GiB (159GB), run=300001-300001msec
/John
On 2/13/25 5:03 PM, John Meneghini wrote:
>> From: Martin K. Petersen
>> Sent: Wednesday, February 12, 2025 6:56 PM
>
>> [You don't often get email from "martin.petersen@oracle.comjames.bottomley@hansenpartnership.comjmeneghi"@redhat.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> Appears to still be a problem. I'll work with Sagar and see if we can clean this up.
>
>>> Add a new modparam "aac_cpu_offline_feature" to control CPU offlining. By default, it's disabled (0), but can be enabled during driver load
>>
>>> with:
>>> insmod ./aacraid.ko aac_cpu_offline_feature=1
>>
>> We are very hesitant when it comes to adding new module parameters. And
>> why wouldn't you want offlining to just work? Is the performance penalty
>> really substantial enough that we have to introduce an explicit "don't
>> be broken" option?
>
> Yes, this is something that we debated about internally, before asking Sagar to send this patch.
>
> I agree that it would be much better if we simply fix the driver and make offline_cpu suport work.
>
> The modparam was added as a compromise, to allow current users and customers who do NOT care about
> cpu_offline support to keep the increased performance they want. People generally complain any
> time there is a performance regression.
>
> The current upstream driver is more or less unchanged when the mod param is of off, which is the default.
> So upstream users will see no performance regression... but don't try to offline a cpu or you will see
> a panic. This is the state of the current upstream driver.
>
>> Thank you for your time to review and giving your valuable opinion.
>> There are two reasons why I chose the modparam way
>> 1) As you rightly guessed - the performance penalty is high when it comes to few RAID level configurations - which is not desired
>> 2) Not a lot of people would use CPU offlining feature as part of their regular usage. This is mostly for admin purposes.
>>
>> These two reasons made me opt for the modparam.
>> We and our folks at RedHat did venture into trying few other options - but this seemed like a nice fit.
>
> Another option we thought about was making this a kconfig option. We have a patch that replaces the modparam with
> a Kconfig option.
>
> However, I agree it would be better to just fix the driver, performance impact notwithstanding, and ship it. For
> my part I'd rather have a correctly functioning driver, that's slower, but doesn't panic.
>
> It's really up to the upstream community. We need to understand what they want.
>
> /John
>
>
John, > However, I agree it would be better to just fix the driver, > performance impact notwithstanding, and ship it. For my part I'd > rather have a correctly functioning driver, that's slower, but doesn't > panic. I prefer to have a driver that doesn't panic when the user performs a reasonably normal administrative action. If go-faster stripes are desired in specific configurations, then make the performance mode an opt-in. Based on your benchmarks, however, I'm not entirely convinced it's worth it...
On 2/24/25 22:15, John Meneghini wrote: > On 2/20/25 9:38 PM, Martin K. Petersen wrote: >> >> John, >> >>> However, I agree it would be better to just fix the driver, >>> performance impact notwithstanding, and ship it. For my part I'd >>> rather have a correctly functioning driver, that's slower, but doesn't >>> panic. >> >> I prefer to have a driver that doesn't panic when the user performs a >> reasonably normal administrative action. > > Agreed. The only clarification I want to make is that users will > not see a panic, they will see IO timeouts and Host bus resets. > It was my mistake to report earlier that the host would panic. > > When aac_cpu_offline_feature is disabled users will see higher performance > but if they start off-lining CPUS they may see IO timeouts. This is the > state of the current driver and this is the problem which the original > patch: > commit 9dc704dcc09e ("scsi: aacraid: Reply queue mapping to CPUs based > on IRQ affinity") > was supposed to have fixed. The problem was the original patch didn't > fix the > problem correctly and it resulted in the regression reported in Bugzilla > 217599[1]. > > This patch circles back and fixes the original problem correctly. The extra > 'aac_cpu_offline_feature' modparam was added to disable the new code path > because of concerns raised during our testing at Red Hat about reduced > performance with this patch. > >> If go-faster stripes are desired in specific configurations, then make >> the performance mode an opt-in. Based on your benchmarks, however, I'm >> not entirely convinced it's worth it... > > I agree. So how about if we can just take out the > aac_cpu_offline_feature modparam...? > > Alternatively we can replace the modparam with a kConfig option. The > default setting for the new Kconfig option will be offline_cpu_support_on and > performance_mode_off. That way we can ship a default kernel configuration that > provides a working aacraid driver which safely supports off-lining > CPUS. If people are really unhappy with the performance, and they> don't care about offline cpu support, they can re-config their kernel. > > Personally I prefer option 1, but we the thoughts of the upstream users. > > I've added the original authors of Bugzilla 217599[1] to the cc list to > get their attention and review. > Do we have an idea what these 'specific use-cases' are? And how much performance impact we have? I could imagine a single-threaded workload driving just one blk-mq queue would benefit from spreading out onto several interrupts. But then, this would be true for most of the multiqueue drivers; and indeed quite some drivers (eg megaraid_sas & mpt3sas 'smp_affinity_enable') have the very same module option. Wouldn't it be an idea to check if we can make this a generic / blk-mq queue option instead of having each driver to implement the same functionality on it's own? Topic for LSF? Cheers, Hannes
Hannes, > Wouldn't it be an idea to check if we can make this a generic / blk-mq > queue option instead of having each driver to implement the same > functionality on it's own? > > Topic for LSF? I would love to have a generic fix.
diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c index abf6a82b74af..f325e79a1a01 100644 --- a/drivers/scsi/aacraid/aachba.c +++ b/drivers/scsi/aacraid/aachba.c @@ -328,6 +328,12 @@ MODULE_PARM_DESC(wwn, "Select a WWN type for the arrays:\n" "\t1 - Array Meta Data Signature (default)\n" "\t2 - Adapter Serial Number"); +int aac_cpu_offline_feature; +module_param_named(aac_cpu_offline_feature, aac_cpu_offline_feature, int, 0644); +MODULE_PARM_DESC(aac_cpu_offline_feature, + "This enables CPU offline feature and may result in IO performance drop in some cases:\n" + "\t0 - Disable (default)\n" + "\t1 - Enable"); static inline int aac_valid_context(struct scsi_cmnd *scsicmd, struct fib *fibptr) { diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h index 8c384c25dca1..dba7ffc6d543 100644 --- a/drivers/scsi/aacraid/aacraid.h +++ b/drivers/scsi/aacraid/aacraid.h @@ -1673,6 +1673,7 @@ struct aac_dev u32 handle_pci_error; bool init_reset; u8 soft_reset_support; + u8 use_map_queue; }; #define aac_adapter_interrupt(dev) \ @@ -2777,4 +2778,5 @@ extern int update_interval; extern int check_interval; extern int aac_check_reset; extern int aac_fib_dump; +extern int aac_cpu_offline_feature; #endif diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c index ffef61c4aa01..1d62730e5c1b 100644 --- a/drivers/scsi/aacraid/commsup.c +++ b/drivers/scsi/aacraid/commsup.c @@ -223,8 +223,15 @@ int aac_fib_setup(struct aac_dev * dev) struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd *scmd) { struct fib *fibptr; + u32 blk_tag; + int i; - fibptr = &dev->fibs[scsi_cmd_to_rq(scmd)->tag]; + if (aac_cpu_offline_feature == 1) { + blk_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd)); + i = blk_mq_unique_tag_to_tag(blk_tag); + fibptr = &dev->fibs[i]; + } else + fibptr = &dev->fibs[scsi_cmd_to_rq(scmd)->tag]; /* * Null out fields that depend on being zero at the start of * each I/O diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c index 91170a67cc91..fc4d35950ccb 100644 --- a/drivers/scsi/aacraid/linit.c +++ b/drivers/scsi/aacraid/linit.c @@ -506,6 +506,23 @@ static int aac_sdev_configure(struct scsi_device *sdev, return 0; } +/** + * aac_map_queues - Map hardware queues for the SCSI host + * @shost: SCSI host structure + * + * Maps the default hardware queue for the given SCSI host to the + * corresponding PCI device and enables mapped queue usage. + */ + +static void aac_map_queues(struct Scsi_Host *shost) +{ + struct aac_dev *aac = (struct aac_dev *)shost->hostdata; + + blk_mq_map_hw_queues(&shost->tag_set.map[HCTX_TYPE_DEFAULT], + &aac->pdev->dev, 0); + aac->use_map_queue = true; +} + /** * aac_change_queue_depth - alter queue depths * @sdev: SCSI device we are considering @@ -1490,6 +1507,7 @@ static const struct scsi_host_template aac_driver_template = { .bios_param = aac_biosparm, .shost_groups = aac_host_groups, .sdev_configure = aac_sdev_configure, + .map_queues = aac_map_queues, .change_queue_depth = aac_change_queue_depth, .sdev_groups = aac_dev_groups, .eh_abort_handler = aac_eh_abort, @@ -1777,6 +1795,11 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id) shost->max_lun = AAC_MAX_LUN; pci_set_drvdata(pdev, shost); + if (aac_cpu_offline_feature == 1) { + shost->nr_hw_queues = aac->max_msix; + shost->can_queue = aac->vector_cap; + shost->host_tagset = 1; + } error = scsi_add_host(shost, &pdev->dev); if (error) @@ -1908,6 +1931,7 @@ static void aac_remove_one(struct pci_dev *pdev) struct aac_dev *aac = (struct aac_dev *)shost->hostdata; aac_cancel_rescan_worker(aac); + aac->use_map_queue = false; scsi_remove_host(shost); __aac_shutdown(aac); diff --git a/drivers/scsi/aacraid/src.c b/drivers/scsi/aacraid/src.c index 28115ed637e8..b9bed3e255c4 100644 --- a/drivers/scsi/aacraid/src.c +++ b/drivers/scsi/aacraid/src.c @@ -493,6 +493,10 @@ static int aac_src_deliver_message(struct fib *fib) #endif u16 vector_no; + struct scsi_cmnd *scmd; + u32 blk_tag; + struct Scsi_Host *shost = dev->scsi_host_ptr; + struct blk_mq_queue_map *qmap; atomic_inc(&q->numpending); @@ -505,8 +509,28 @@ static int aac_src_deliver_message(struct fib *fib) if ((dev->comm_interface == AAC_COMM_MESSAGE_TYPE3) && dev->sa_firmware) vector_no = aac_get_vector(dev); - else - vector_no = fib->vector_no; + else { + if (aac_cpu_offline_feature == 1) { + if (!fib->vector_no || !fib->callback_data) { + if (shost && dev->use_map_queue) { + qmap = &shost->tag_set.map[HCTX_TYPE_DEFAULT]; + vector_no = qmap->mq_map[raw_smp_processor_id()]; + } + /* + * We hardcode the vector_no for + * reserved commands as a valid shost is + * absent during the init + */ + else + vector_no = 0; + } else { + scmd = (struct scsi_cmnd *)fib->callback_data; + blk_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd)); + vector_no = blk_mq_unique_tag_to_hwq(blk_tag); + } + } else + vector_no = fib->vector_no; + } if (native_hba) { if (fib->flags & FIB_CONTEXT_FLAG_NATIVE_HBA_TMF) {