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
