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 09:15:46 +0800	[thread overview]
Message-ID: <ZMHFQm2BpuGjjlTm@ovpn-8-16.pek2.redhat.com> (raw)
In-Reply-To: <07f84570-5307-df3c-99ab-a996bfcbf83a@oracle.com>

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

