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

From: Ming Lei <ming.lei@redhat.com>
To: John Garry <john.g.garry@oracle.com>
Cc: Jens Axboe <axboe@kernel.dk>, Christoph Hellwig <hch@lst.de>,
	linux-nvme@lists.infradead.org,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	linux-scsi@vger.kernel.org, linux-block@vger.kernel.org,
	Wen Xiong <wenxiong@linux.ibm.com>,
	Keith Busch <kbusch@kernel.org>,
	Xiang Chen <chenxiang66@hisilicon.com>,
	ming.lei@redhat.com
Subject: Re: [PATCH V2 5/9] scsi: hisi: take blk_mq_max_nr_hw_queues() into account for calculating io vectors
Date: Thu, 27 Jul 2023 18:56:30 +0800	[thread overview]
Message-ID: <ZMJNXk5MiViklCBX@ovpn-8-16.pek2.redhat.com> (raw)
In-Reply-To: <123dc4ca-88da-b2b1-44b2-791ea841572f@oracle.com>

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