[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 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