| Message ID | 20230726094027.535126-6-ming.lei@redhat.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | blk-mq: fix wrong queue mapping for kdump kernel | expand |
On 26/07/2023 10:40, Ming Lei wrote: > Take blk-mq's knowledge into account for calculating io queues. > > Fix wrong queue mapping in case of kdump kernel. > > On arm and ppc64, 'maxcpus=1' is passed to kdump kernel command line, > see `Documentation/admin-guide/kdump/kdump.rst`, so num_possible_cpus() > still returns all CPUs because 'maxcpus=1' just bring up one single > cpu core during booting. > > blk-mq sees single queue in kdump kernel, and in driver's viewpoint > there are still multiple queues, this inconsistency causes driver to apply > wrong queue mapping for handling IO, and IO timeout is triggered. > > Meantime, single queue makes much less resource utilization, and reduce > risk of kernel failure. > > Cc: Xiang Chen <chenxiang66@hisilicon.com> > Signed-off-by: Ming Lei <ming.lei@redhat.com> > --- > drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 3 +++ > 1 file changed, 3 insertions(+) > > diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c > index 20e1607c6282..60d2301e7f9d 100644 > --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c > +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c > @@ -2550,6 +2550,9 @@ static int interrupt_preinit_v3_hw(struct hisi_hba *hisi_hba) > > > hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW - hisi_hba->iopoll_q_cnt; > + if (hisi_hba->cq_nvecs > scsi_max_nr_hw_queues()) > + hisi_hba->cq_nvecs = scsi_max_nr_hw_queues(); > + > shost->nr_hw_queues = hisi_hba->cq_nvecs + hisi_hba->iopoll_q_cnt; For other drivers you limit the max MSI vectors which we try to allocate according to scsi_max_nr_hw_queues(), but here you continue to alloc the same max vectors but then limit the driver's completion queue count. Why not limit the max MSI vectors also here? Thanks, John > > return devm_add_action(&pdev->dev, hisi_sas_v3_free_vectors, pdev);
On Wed, Jul 26, 2023 at 04:42:42PM +0100, John Garry wrote: > On 26/07/2023 10:40, Ming Lei wrote: > > Take blk-mq's knowledge into account for calculating io queues. > > > > Fix wrong queue mapping in case of kdump kernel. > > > > On arm and ppc64, 'maxcpus=1' is passed to kdump kernel command line, > > see `Documentation/admin-guide/kdump/kdump.rst`, so num_possible_cpus() > > still returns all CPUs because 'maxcpus=1' just bring up one single > > cpu core during booting. > > > > blk-mq sees single queue in kdump kernel, and in driver's viewpoint > > there are still multiple queues, this inconsistency causes driver to apply > > wrong queue mapping for handling IO, and IO timeout is triggered. > > > > Meantime, single queue makes much less resource utilization, and reduce > > risk of kernel failure. > > > > Cc: Xiang Chen <chenxiang66@hisilicon.com> > > Signed-off-by: Ming Lei <ming.lei@redhat.com> > > --- > > drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 3 +++ > > 1 file changed, 3 insertions(+) > > > > diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c > > index 20e1607c6282..60d2301e7f9d 100644 > > --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c > > +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c > > @@ -2550,6 +2550,9 @@ static int interrupt_preinit_v3_hw(struct hisi_hba *hisi_hba) > > hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW - hisi_hba->iopoll_q_cnt; > > + if (hisi_hba->cq_nvecs > scsi_max_nr_hw_queues()) > > + hisi_hba->cq_nvecs = scsi_max_nr_hw_queues(); > > + > > shost->nr_hw_queues = hisi_hba->cq_nvecs + hisi_hba->iopoll_q_cnt; > > For other drivers you limit the max MSI vectors which we try to allocate > according to scsi_max_nr_hw_queues(), but here you continue to alloc the > same max vectors but then limit the driver's completion queue count. Why not > limit the max MSI vectors also here? Good catch! I guess it is because of the following line: hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW - hisi_hba->iopoll_q_cnt; I am a bit confused why hisi_sas_v3 takes ->iopoll_q_cnt into account allocated msi vectors? ->iopoll_q_cnt supposes to not consume msi vectors, so I think we need the following fix first: diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c index 20e1607c6282..032c13ce8373 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c @@ -2549,7 +2549,7 @@ static int interrupt_preinit_v3_hw(struct hisi_hba *hisi_hba) return -ENOENT; - hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW - hisi_hba->iopoll_q_cnt; + hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW; shost->nr_hw_queues = hisi_hba->cq_nvecs + hisi_hba->iopoll_q_cnt; return devm_add_action(&pdev->dev, hisi_sas_v3_free_vectors, pdev); Thanks, Ming
On 27/07/2023 02:15, Ming Lei wrote: >>> hisi_sas_v3_hw.c >>> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c >>> @@ -2550,6 +2550,9 @@ static int interrupt_preinit_v3_hw(struct hisi_hba *hisi_hba) >>> hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW - hisi_hba->iopoll_q_cnt; >>> + if (hisi_hba->cq_nvecs > scsi_max_nr_hw_queues()) >>> + hisi_hba->cq_nvecs = scsi_max_nr_hw_queues(); >>> + >>> shost->nr_hw_queues = hisi_hba->cq_nvecs + hisi_hba->iopoll_q_cnt; >> For other drivers you limit the max MSI vectors which we try to allocate >> according to scsi_max_nr_hw_queues(), but here you continue to alloc the >> same max vectors but then limit the driver's completion queue count. Why not >> limit the max MSI vectors also here? Ah, checking again, I think that this driver always allocates maximum possible MSI due to arm interrupt controller driver bug - see comment at top of function interrupt_preinit_v3_hw(). IIRC, there was a problem if we remove and re-insert the device driver that the GIC ITS fails to allocate MSI unless all MSI were previously allocated. Xiang Chen can confirm. > Good catch! > > I guess it is because of the following line: > > hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW - hisi_hba->iopoll_q_cnt; > > I am a bit confused why hisi_sas_v3 takes ->iopoll_q_cnt into account > allocated msi vectors? ->iopoll_q_cnt supposes to not consume msi > vectors, so I think we need the following fix first: > > diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c > index 20e1607c6282..032c13ce8373 100644 > --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c > +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c > @@ -2549,7 +2549,7 @@ static int interrupt_preinit_v3_hw(struct hisi_hba *hisi_hba) > return -ENOENT; > > > - hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW - hisi_hba->iopoll_q_cnt; > + hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW; > shost->nr_hw_queues = hisi_hba->cq_nvecs + hisi_hba->iopoll_q_cnt; Thanks, John
On Thu, Jul 27, 2023 at 08:35:06AM +0100, John Garry wrote: > On 27/07/2023 02:15, Ming Lei wrote: > > > > hisi_sas_v3_hw.c > > > > +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c > > > > @@ -2550,6 +2550,9 @@ static int interrupt_preinit_v3_hw(struct hisi_hba *hisi_hba) > > > > hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW - hisi_hba->iopoll_q_cnt; > > > > + if (hisi_hba->cq_nvecs > scsi_max_nr_hw_queues()) > > > > + hisi_hba->cq_nvecs = scsi_max_nr_hw_queues(); > > > > + > > > > shost->nr_hw_queues = hisi_hba->cq_nvecs + hisi_hba->iopoll_q_cnt; > > > For other drivers you limit the max MSI vectors which we try to allocate > > > according to scsi_max_nr_hw_queues(), but here you continue to alloc the > > > same max vectors but then limit the driver's completion queue count. Why not > > > limit the max MSI vectors also here? > > Ah, checking again, I think that this driver always allocates maximum > possible MSI due to arm interrupt controller driver bug - see comment at top > of function interrupt_preinit_v3_hw(). IIRC, there was a problem if we > remove and re-insert the device driver that the GIC ITS fails to allocate > MSI unless all MSI were previously allocated. My question is actually why hisi_hba->iopoll_q_cnt is subtracted from allocated vectors since io poll queues does _not_ use msi vector. So it isn't related with driver's msi vector allocation bug, is it? Thanks, Ming
On 27/07/2023 10:42, Ming Lei wrote: >>>>> hisi_sas_v3_hw.c >>>>> +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c >>>>> @@ -2550,6 +2550,9 @@ static int interrupt_preinit_v3_hw(struct hisi_hba *hisi_hba) >>>>> hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW - hisi_hba->iopoll_q_cnt; >>>>> + if (hisi_hba->cq_nvecs > scsi_max_nr_hw_queues()) >>>>> + hisi_hba->cq_nvecs = scsi_max_nr_hw_queues(); >>>>> + >>>>> shost->nr_hw_queues = hisi_hba->cq_nvecs + hisi_hba->iopoll_q_cnt; >>>> For other drivers you limit the max MSI vectors which we try to allocate >>>> according to scsi_max_nr_hw_queues(), but here you continue to alloc the >>>> same max vectors but then limit the driver's completion queue count. Why not >>>> limit the max MSI vectors also here? >> Ah, checking again, I think that this driver always allocates maximum >> possible MSI due to arm interrupt controller driver bug - see comment at top >> of function interrupt_preinit_v3_hw(). IIRC, there was a problem if we >> remove and re-insert the device driver that the GIC ITS fails to allocate >> MSI unless all MSI were previously allocated. > My question is actually why hisi_hba->iopoll_q_cnt is subtracted from > allocated vectors since io poll queues does_not_ use msi vector. > It is subtracted as superfluous vectors were allocated. As I mentioned, I think that the driver is forced to allocate all 32 MSI vectors, even though it really needs max of 32 - iopoll_q_cnt, i.e. we don't need an MSI vector for a HW queue assigned to polling. Then, since it gets 16x MSI for cq vectors, it subtracts iopoll_q_cnt to give the desired count in cq_nvecs. > So it isn't related with driver's msi vector allocation bug, is it? My deduction is this is how this currently "works" for non-zero iopoll queues: - allocate max MSI of 32, which gives 32 vectors including 16 cq vectors. That then gives: - cq_nvecs = 16 - iopoll_q_cnt - shost->nr_hw_queues = 16 - 16x MSI cq vectors were spread over all CPUs - in hisi_sas_map_queues() - HCTX_TYPE_DEFAULT qmap->nr_queues = 16 - iopoll_q_cnt, and for blk_mq_pci_map_queues() we setup affinity for 16 - iopoll_q_cnt hw queues. This looks broken, as we originally spread 16x vectors over all CPUs, but now only setup mappings for (16 - iopoll_q_cnt) vectors, whose affinity would spread a subset of CPUs. And then qmap->mq_map[] for other CPUs is not set at all. Thanks, John
On Thu, Jul 27, 2023 at 11:28:31AM +0100, John Garry wrote: > On 27/07/2023 10:42, Ming Lei wrote: > > > > > > hisi_sas_v3_hw.c > > > > > > +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c > > > > > > @@ -2550,6 +2550,9 @@ static int interrupt_preinit_v3_hw(struct hisi_hba *hisi_hba) > > > > > > hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW - hisi_hba->iopoll_q_cnt; > > > > > > + if (hisi_hba->cq_nvecs > scsi_max_nr_hw_queues()) > > > > > > + hisi_hba->cq_nvecs = scsi_max_nr_hw_queues(); > > > > > > + > > > > > > shost->nr_hw_queues = hisi_hba->cq_nvecs + hisi_hba->iopoll_q_cnt; > > > > > For other drivers you limit the max MSI vectors which we try to allocate > > > > > according to scsi_max_nr_hw_queues(), but here you continue to alloc the > > > > > same max vectors but then limit the driver's completion queue count. Why not > > > > > limit the max MSI vectors also here? > > > Ah, checking again, I think that this driver always allocates maximum > > > possible MSI due to arm interrupt controller driver bug - see comment at top > > > of function interrupt_preinit_v3_hw(). IIRC, there was a problem if we > > > remove and re-insert the device driver that the GIC ITS fails to allocate > > > MSI unless all MSI were previously allocated. > > My question is actually why hisi_hba->iopoll_q_cnt is subtracted from > > allocated vectors since io poll queues does_not_ use msi vector. > > > > It is subtracted as superfluous vectors were allocated. > > As I mentioned, I think that the driver is forced to allocate all 32 MSI > vectors, even though it really needs max of 32 - iopoll_q_cnt, i.e. we don't > need an MSI vector for a HW queue assigned to polling. Then, since it gets > 16x MSI for cq vectors, it subtracts iopoll_q_cnt to give the desired count > in cq_nvecs. Looks the current driver wires nr_irq_queues with nr_poll_queues, which is wrong logically. Here: 1) hisi_hba->queue_count means the max supported queue count(irq queues + poll queues) 2) max vectors(32) means the max supported irq queues, but actual nr_irq_queues can be less than allocated vectors because of the irq allocation bug 3) so the max supported poll queues should be (hisi_hba->queue_count - nr_irq_queues). What I meant is that nr_poll_queues shouldn't be related with allocated msi vectors. > > So it isn't related with driver's msi vector allocation bug, is it? > > My deduction is this is how this currently "works" for non-zero iopoll > queues: > - allocate max MSI of 32, which gives 32 vectors including 16 cq vectors. > That then gives: > - cq_nvecs = 16 - iopoll_q_cnt > - shost->nr_hw_queues = 16 > - 16x MSI cq vectors were spread over all CPUs It should be that cq_nvecs vectors spread over all CPUs, and iopoll_q_cnt are spread over all CPUs too. For each queue type, nr_queues of this type are spread over all CPUs. > > - in hisi_sas_map_queues() > - HCTX_TYPE_DEFAULT qmap->nr_queues = 16 - iopoll_q_cnt, and for > blk_mq_pci_map_queues() we setup affinity for 16 - iopoll_q_cnt hw queues. > This looks broken, as we originally spread 16x vectors over all CPUs, but > now only setup mappings for (16 - iopoll_q_cnt) vectors, whose affinity > would spread a subset of CPUs. And then qmap->mq_map[] for other CPUs is not > set at all. That isn't true, please see my above comment. Thanks, Ming
On 27/07/2023 11:56, Ming Lei wrote: >> As I mentioned, I think that the driver is forced to allocate all 32 MSI >> vectors, even though it really needs max of 32 - iopoll_q_cnt, i.e. we don't >> need an MSI vector for a HW queue assigned to polling. Then, since it gets >> 16x MSI for cq vectors, it subtracts iopoll_q_cnt to give the desired count >> in cq_nvecs. > Looks the current driver wires nr_irq_queues with nr_poll_queues, which is > wrong logically. > > Here: > > 1) hisi_hba->queue_count means the max supported queue count(irq queues > + poll queues) > JFYI, from https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hisilicon/Hi1620/Hi1620AcpiTables/Dsdt/Hi1620Pci.asl#L587C25-L587C41, hisi_hba->queue_count is fixed at 16. > 2) max vectors(32) means the max supported irq queues, I'll just mention some details of this HW, in case missed. For this HW, max vectors is 32, but not all are for completion queue interrupts. interrupts 0-15 are for housekeeping, like phy up notification interrupts 16-31 are for completion queue interrupts That is a fixed assignment. irq #16 is for HW queue #0 interrupt, irq #17 is for HW queue #1 interrupt, and so on. > but actual > nr_irq_queues can be less than allocated vectors because of the irq > allocation bug > > 3) so the max supported poll queues should be (hisi_hba->queue_count - > nr_irq_queues). > > What I meant is that nr_poll_queues shouldn't be related with allocated > msi vectors. Sure, I agree with that. nr_poll_queues is set in that driver as a module param. I am just saying that we have a fixed number of HW queues (16), each of which may be used for interrupt or polling mode. And since we always allocate max number of MSI, then number of interrupt queues available will be 16 - nr_poll_queues. > > >>> So it isn't related with driver's msi vector allocation bug, is it? >> My deduction is this is how this currently "works" for non-zero iopoll >> queues: >> - allocate max MSI of 32, which gives 32 vectors including 16 cq vectors. >> That then gives: >> - cq_nvecs = 16 - iopoll_q_cnt >> - shost->nr_hw_queues = 16 >> - 16x MSI cq vectors were spread over all CPUs > It should be that cq_nvecs vectors spread over all CPUs, and > iopoll_q_cnt are spread over all CPUs too. I agree, it should be, but I don't think that it is for HCTX_TYPE_DEFAULT, as below. > > For each queue type, nr_queues of this type are spread over all > CPUs. >> - in hisi_sas_map_queues() >> - HCTX_TYPE_DEFAULT qmap->nr_queues = 16 - iopoll_q_cnt, and for >> blk_mq_pci_map_queues() we setup affinity for 16 - iopoll_q_cnt hw queues. >> This looks broken, as we originally spread 16x vectors over all CPUs, but >> now only setup mappings for (16 - iopoll_q_cnt) vectors, whose affinity >> would spread a subset of CPUs. And then qmap->mq_map[] for other CPUs is not >> set at all. > That isn't true, please see my above comment. I am just basing that on what I mention above, so please let me know my inaccuracy there. Thanks, John
On Thu, Jul 27, 2023 at 12:30:34PM +0100, John Garry wrote: > On 27/07/2023 11:56, Ming Lei wrote: > > > As I mentioned, I think that the driver is forced to allocate all 32 MSI > > > vectors, even though it really needs max of 32 - iopoll_q_cnt, i.e. we don't > > > need an MSI vector for a HW queue assigned to polling. Then, since it gets > > > 16x MSI for cq vectors, it subtracts iopoll_q_cnt to give the desired count > > > in cq_nvecs. > > Looks the current driver wires nr_irq_queues with nr_poll_queues, which is > > wrong logically. > > > > Here: > > > > 1) hisi_hba->queue_count means the max supported queue count(irq queues > > + poll queues) > > > > JFYI, from https://github.com/tianocore/edk2-platforms/blob/master/Silicon/Hisilicon/Hi1620/Hi1620AcpiTables/Dsdt/Hi1620Pci.asl#L587C25-L587C41, > hisi_hba->queue_count is fixed at 16. > > > 2) max vectors(32) means the max supported irq queues, > I'll just mention some details of this HW, in case missed. > > For this HW, max vectors is 32, but not all are for completion queue > interrupts. > > interrupts 0-15 are for housekeeping, like phy up notification > interrupts 16-31 are for completion queue interrupts > > That is a fixed assignment. irq #16 is for HW queue #0 interrupt, irq #17 is > for HW queue #1 interrupt, and so on. > > > but actual > > nr_irq_queues can be less than allocated vectors because of the irq > > allocation bug > > > > 3) so the max supported poll queues should be (hisi_hba->queue_count - > > nr_irq_queues). > > > > What I meant is that nr_poll_queues shouldn't be related with allocated > > msi vectors. > > Sure, I agree with that. nr_poll_queues is set in that driver as a module > param. > > I am just saying that we have a fixed number of HW queues (16), each of > which may be used for interrupt or polling mode. And since we always > allocate max number of MSI, then number of interrupt queues available will > be 16 - nr_poll_queues. No. queue_count is fixed at 16, but pci_alloc_irq_vectors_affinity() still may return less vectors, which is one system-wide resource, and queue count is device resource. So when less vectors are allocated, you should have been capable of using more poll queues, unfortunately the current code can't support that. Even worse, hisi_hba->cq_nvecs can become negative if less vectors are returned. > > > > > > > > > So it isn't related with driver's msi vector allocation bug, is it? > > > My deduction is this is how this currently "works" for non-zero iopoll > > > queues: > > > - allocate max MSI of 32, which gives 32 vectors including 16 cq vectors. > > > That then gives: > > > - cq_nvecs = 16 - iopoll_q_cnt > > > - shost->nr_hw_queues = 16 > > > - 16x MSI cq vectors were spread over all CPUs > > It should be that cq_nvecs vectors spread over all CPUs, and > > iopoll_q_cnt are spread over all CPUs too. > > I agree, it should be, but I don't think that it is for HCTX_TYPE_DEFAULT, > as below. > > > > > For each queue type, nr_queues of this type are spread over all > > CPUs. >> - in hisi_sas_map_queues() > > > - HCTX_TYPE_DEFAULT qmap->nr_queues = 16 - iopoll_q_cnt, and for > > > blk_mq_pci_map_queues() we setup affinity for 16 - iopoll_q_cnt hw queues. > > > This looks broken, as we originally spread 16x vectors over all CPUs, but > > > now only setup mappings for (16 - iopoll_q_cnt) vectors, whose affinity > > > would spread a subset of CPUs. And then qmap->mq_map[] for other CPUs is not > > > set at all. > > That isn't true, please see my above comment. > > I am just basing that on what I mention above, so please let me know my > inaccuracy there. You said queue mapping for HCTX_TYPE_DEFAULT is broken, but it isn't. You said 'we originally spread 16x vectors over all CPUs', which isn't true. Again, '16 - iopoll_q_cnt' vectors are spread on all CPUs, and same with iopoll_q_cnt vectors. Since both blk_mq_map_queues() and blk_mq_pci_map_queues() does spread map->nr_queues over all CPUs, so there isn't spread a subset of CPUs. Thanks, Ming
>> >> I am just saying that we have a fixed number of HW queues (16), each of >> which may be used for interrupt or polling mode. And since we always >> allocate max number of MSI, then number of interrupt queues available will >> be 16 - nr_poll_queues. > > No. > > queue_count is fixed at 16, but pci_alloc_irq_vectors_affinity() still > may return less vectors, which is one system-wide resource, and queue > count is device resource. > > So when less vectors are allocated, you should have been capable of using > more poll queues, unfortunately the current code can't support that. > > Even worse, hisi_hba->cq_nvecs can become negative if less vectors are returned. OK, I see what you mean here. I thought that we were only considering case of vectors allocated was max vectors requested. Yes, I see how allocating less than max can cause an issue. I am not sure if increasing iopoll_q_cnt over driver module param value is proper then, but obviously we don't want cq_nvecs to become negative. > >> >>> >>> >>>>> So it isn't related with driver's msi vector allocation bug, is it? >>>> My deduction is this is how this currently "works" for non-zero iopoll >>>> queues: >>>> - allocate max MSI of 32, which gives 32 vectors including 16 cq vectors. >>>> That then gives: >>>> - cq_nvecs = 16 - iopoll_q_cnt >>>> - shost->nr_hw_queues = 16 >>>> - 16x MSI cq vectors were spread over all CPUs >>> It should be that cq_nvecs vectors spread over all CPUs, and >>> iopoll_q_cnt are spread over all CPUs too. >> >> I agree, it should be, but I don't think that it is for HCTX_TYPE_DEFAULT, >> as below. >> >>> >>> For each queue type, nr_queues of this type are spread over all >>> CPUs. >> - in hisi_sas_map_queues() >>>> - HCTX_TYPE_DEFAULT qmap->nr_queues = 16 - iopoll_q_cnt, and for >>>> blk_mq_pci_map_queues() we setup affinity for 16 - iopoll_q_cnt hw queues. >>>> This looks broken, as we originally spread 16x vectors over all CPUs, but >>>> now only setup mappings for (16 - iopoll_q_cnt) vectors, whose affinity >>>> would spread a subset of CPUs. And then qmap->mq_map[] for other CPUs is not >>>> set at all. >>> That isn't true, please see my above comment. >> >> I am just basing that on what I mention above, so please let me know my >> inaccuracy there. > > You said queue mapping for HCTX_TYPE_DEFAULT is broken, but it isn't. > > You said 'we originally spread 16x vectors over all CPUs', which isn't > true. Are you talking about the case of allocating less then max requested vectors, as above? If we have min_msi = 17, max_msi = 32, affinity_desc = {16, 0}, and we allocate 32 vectors from pci_alloc_irq_vectors_affinity(), then I would have thought that the affinity for the 16x cq vectors is spread over all CPUs. Is that wrong? > Again, '16 - iopoll_q_cnt' vectors are spread on all CPUs, and > same with iopoll_q_cnt vectors. > > Since both blk_mq_map_queues() and blk_mq_pci_map_queues() does spread > map->nr_queues over all CPUs, so there isn't spread a subset of CPUs. > Thanks, John
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c index 20e1607c6282..60d2301e7f9d 100644 --- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c +++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c @@ -2550,6 +2550,9 @@ static int interrupt_preinit_v3_hw(struct hisi_hba *hisi_hba) hisi_hba->cq_nvecs = vectors - BASE_VECTORS_V3_HW - hisi_hba->iopoll_q_cnt; + if (hisi_hba->cq_nvecs > scsi_max_nr_hw_queues()) + hisi_hba->cq_nvecs = scsi_max_nr_hw_queues(); + shost->nr_hw_queues = hisi_hba->cq_nvecs + hisi_hba->iopoll_q_cnt; return devm_add_action(&pdev->dev, hisi_sas_v3_free_vectors, pdev);
Take blk-mq's knowledge into account for calculating io queues. Fix wrong queue mapping in case of kdump kernel. On arm and ppc64, 'maxcpus=1' is passed to kdump kernel command line, see `Documentation/admin-guide/kdump/kdump.rst`, so num_possible_cpus() still returns all CPUs because 'maxcpus=1' just bring up one single cpu core during booting. blk-mq sees single queue in kdump kernel, and in driver's viewpoint there are still multiple queues, this inconsistency causes driver to apply wrong queue mapping for handling IO, and IO timeout is triggered. Meantime, single queue makes much less resource utilization, and reduce risk of kernel failure. Cc: Xiang Chen <chenxiang66@hisilicon.com> Signed-off-by: Ming Lei <ming.lei@redhat.com> --- drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 3 +++ 1 file changed, 3 insertions(+)