Re: [PATCH 6/9] scsi: hpsa: convert private reply queue to blk-mq hw queue

From: Ming Lei <tom.leiming@gmail.com>
To: Hannes Reinecke <hare@suse.de>
Cc: Ming Lei <ming.lei@redhat.com>, Jens Axboe <axboe@kernel.dk>,
	linux-block <linux-block@vger.kernel.org>,
	Linux SCSI List <linux-scsi@vger.kernel.org>,
	"Martin K . Petersen" <martin.petersen@oracle.com>,
	James Bottomley <James.Bottomley@hansenpartnership.com>,
	Bart Van Assche <bvanassche@acm.org>,
	Hannes Reinecke <hare@suse.com>,
	John Garry <john.garry@huawei.com>,
	Don Brace <don.brace@microsemi.com>,
	Kashyap Desai <kashyap.desai@broadcom.com>,
	Sathya Prakash <sathya.prakash@broadcom.com>,
	Christoph Hellwig <hch@lst.de>
Subject: Re: [PATCH 6/9] scsi: hpsa: convert private reply queue to blk-mq hw queue
Date: Fri, 31 May 2019 14:30:44 +0800	[thread overview]
Message-ID: <CACVXFVMFVqD3797qfe8K5bZnwv7fSqP7oz3AMyQLKWKH8Z89FA@mail.gmail.com> (raw)
In-Reply-To: <d489e4e8-e625-4c68-4ab8-9b70e5989cc8@suse.de>

On Fri, May 31, 2019 at 2:15 PM Hannes Reinecke <hare@suse.de> wrote:
>
> On 5/31/19 4:27 AM, Ming Lei wrote:
> > SCSI's reply qeueue is very similar with blk-mq's hw queue, both
> > assigned by IRQ vector, so map te private reply queue into blk-mq's hw
> > queue via .host_tagset.
> >
> > Then the private reply mapping can be removed.
> >
> > Another benefit is that the request/irq lost issue may be solved in
> > generic approach because managed IRQ may be shutdown during CPU
> > hotplug.
> >
> > Signed-off-by: Ming Lei <ming.lei@redhat.com>
> > ---
> >  drivers/scsi/hpsa.c | 49 ++++++++++++++++++---------------------------
> >  1 file changed, 19 insertions(+), 30 deletions(-)
> >
> There had been requests to make the internal interrupt mapping optional;
> but I guess we first should

For HPSA, either managed IRQ is used or single MSI-X vector is allocated,
I am pretty sure that both cases are covered in this patch, so not sure what the
'optional' means.

> > diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
> > index 1bef1da273c2..c7136f9f0ce1 100644
> > --- a/drivers/scsi/hpsa.c
> > +++ b/drivers/scsi/hpsa.c
> > @@ -51,6 +51,7 @@
> >  #include <linux/jiffies.h>
> >  #include <linux/percpu-defs.h>
> >  #include <linux/percpu.h>
> > +#include <linux/blk-mq-pci.h>
> >  #include <asm/unaligned.h>
> >  #include <asm/div64.h>
> >  #include "hpsa_cmd.h"
> > @@ -902,6 +903,18 @@ static ssize_t host_show_legacy_board(struct device *dev,
> >       return snprintf(buf, 20, "%d\n", h->legacy_board ? 1 : 0);
> >  }
> >
> > +static int hpsa_map_queues(struct Scsi_Host *shost)
> > +{
> > +     struct ctlr_info *h = shost_to_hba(shost);
> > +     struct blk_mq_queue_map *qmap = &shost->tag_set.map[HCTX_TYPE_DEFAULT];
> > +
> > +     /* Switch to cpu mapping in case that managed IRQ isn't used */
> > +     if (shost->nr_hw_queues > 1)
> > +             return blk_mq_pci_map_queues(qmap, h->pdev, 0);
> > +     else
> > +             return blk_mq_map_queues(qmap);
> > +}
> > +
> >  static DEVICE_ATTR_RO(raid_level);
> >  static DEVICE_ATTR_RO(lunid);
> >  static DEVICE_ATTR_RO(unique_id);
> This helper is pretty much shared between all converted drivers.
> Shouldn't we have a common function here?
> Something like
>
> scsi_mq_host_tag_map(struct Scsi_Host *shost, int offset)?

I am not sure if the common helper is helpful much, since the
condition for using
cpu map or pci map still depends on driver private state, we still
have to define
each driver's .map_queues too.

Also PCI device pointer has to be provided.

thanks,
Ming Lei