[PATCH V2 5/9] scsi: hisi: take blk_mq_max_nr_hw_queues() into account for calculating io vectors

[John Garry]

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