> -----Original Message-----
> From: Ming Lei [mailto:ming....@redhat.com]
> Sent: Friday, March 9, 2018 5:33 PM
> To: Kashyap Desai
> Cc: James Bottomley; Jens Axboe; Martin K . Petersen; Christoph Hellwig;
> linux-scsi@vger.kernel.org; linux-bl...@vger.kernel.org; Meelis Roos;
Don
> Brace; Laurence Oberman; Mike Snitzer; Hannes Reinecke; Artem Bityutskiy
> Subject: Re: [PATCH V4 2/4] scsi: megaraid_sas: fix selection of reply
queue
>
> On Fri, Mar 09, 2018 at 04:37:56PM +0530, Kashyap Desai wrote:
> > > -----Original Message-----
> > > From: Ming Lei [mailto:ming....@redhat.com]
> > > Sent: Friday, March 9, 2018 9:02 AM
> > > To: James Bottomley; Jens Axboe; Martin K . Petersen
> > > Cc: Christoph Hellwig; linux-scsi@vger.kernel.org; linux-
> > > bl...@vger.kernel.org; Meelis Roos; Don Brace; Kashyap Desai;
> > > Laurence Oberman; Mike Snitzer; Ming Lei; Hannes Reinecke; James
> > > Bottomley; Artem Bityutskiy
> > > Subject: [PATCH V4 2/4] scsi: megaraid_sas: fix selection of reply
> > > queue
> > >
> > > From 84676c1f21 (genirq/affinity: assign vectors to all possible
> > > CPUs),
> > one
> > > msix vector can be created without any online CPU mapped, then
> > > command may be queued, and won't be notified after its completion.
> > >
> > > This patch setups mapping between cpu and reply queue according to
> > > irq affinity info retrived by pci_irq_get_affinity(), and uses this
> > > info to
> > choose
> > > reply queue for queuing one command.
> > >
> > > Then the chosen reply queue has to be active, and fixes IO hang
> > > caused
> > by
> > > using inactive reply queue which doesn't have any online CPU mapped.
> >
> > Also megaraid FW will use reply queue 0 for any async notification.
> > We want to set pre_vectors = 1 and make sure reply queue 0 is not part
> > of affinity hint.
> > To meet that requirement, I have to make some more changes like add
> > extra queue.
> > Example if reply queue supported by FW is 96 and online CPU is 16,
> > current driver will allocate 16 msix vector. We may have to allocate
> > 17 msix vector and reserve reply queue 0 for async reply from FW.
> >
> > I will be sending follow up patch soon.
>
> OK, but the above extra change shouldn't belong to this patch, which
focuses
> on fixing IO hang because of reply queue selection.
Fine. That will be a separate patch to handle reply queue 0 affinity
case.
>
> >
> > >
> > > Cc: Hannes Reinecke <h...@suse.de>
> > > Cc: "Martin K. Petersen" <martin.peter...@oracle.com>,
> > > Cc: James Bottomley <james.bottom...@hansenpartnership.com>,
> > > Cc: Christoph Hellwig <h...@lst.de>,
> > > Cc: Don Brace <don.br...@microsemi.com>
> > > Cc: Kashyap Desai <kashyap.de...@broadcom.com>
> > > Cc: Laurence Oberman <lober...@redhat.com>
> > > Cc: Mike Snitzer <snit...@redhat.com>
> > > Cc: Meelis Roos <mr...@linux.ee>
> > > Cc: Artem Bityutskiy <artem.bityuts...@intel.com>
> > > Fixes: 84676c1f21e8 ("genirq/affinity: assign vectors to all
> > > possible
> > CPUs")
> > > Signed-off-by: Ming Lei <ming....@redhat.com>
> > > ---
> > > drivers/scsi/megaraid/megaraid_sas.h | 2 +-
> > > drivers/scsi/megaraid/megaraid_sas_base.c | 34
> > > ++++++++++++++++++++++++++++-
> > > drivers/scsi/megaraid/megaraid_sas_fusion.c | 12 ++++------
> > > 3 files changed, 38 insertions(+), 10 deletions(-)
> > >
> > > diff --git a/drivers/scsi/megaraid/megaraid_sas.h
> > > b/drivers/scsi/megaraid/megaraid_sas.h
> > > index ba6503f37756..a644d2be55b6 100644
> > > --- a/drivers/scsi/megaraid/megaraid_sas.h
> > > +++ b/drivers/scsi/megaraid/megaraid_sas.h
> > > @@ -2127,7 +2127,7 @@ enum MR_PD_TYPE {
> > > #define MR_NVME_PAGE_SIZE_MASK 0x000000FF
> > >
> > > struct megasas_instance {
> > > -
> > > + unsigned int *reply_map;
> > > __le32 *producer;
> > > dma_addr_t producer_h;
> > > __le32 *consumer;
> > > diff --git a/drivers/scsi/megaraid/megaraid_sas_base.c
> > > b/drivers/scsi/megaraid/megaraid_sas_base.c
> > > index a71ee67df084..065956cb2aeb 100644
> > > --- a/drivers/scsi/megaraid/megaraid_sas_base.c
> > > +++ b/drivers/scsi/megaraid/megaraid_sas_base.c
> > > @@ -5165,6 +5165,26 @@ megasas_setup_jbod_map(struct
> > > megasas_instance *instance)
> > > instance->use_seqnum_jbod_fp = false; }
> > >
> > > +static void megasas_setup_reply_map(struct megasas_instance
> > > +*instance) {
> > > + const struct cpumask *mask;
> > > + unsigned int queue, cpu;
> > > +
> > > + for (queue = 0; queue < instance->msix_vectors; queue++) {
> > > + mask = pci_irq_get_affinity(instance->pdev, queue);
> > > + if (!mask)
> > > + goto fallback;
> > > +
> > > + for_each_cpu(cpu, mask)
> > > + instance->reply_map[cpu] = queue;
> > > + }
> > > + return;
> > > +
> > > +fallback:
> > > + for_each_possible_cpu(cpu)
> > > + instance->reply_map[cpu] = 0;
> >
> > Fallback should be better instead of just assigning to single reply
queue.
> > May be something like below.
> >
> > for_each_possible_cpu(cpu)
> > instance->reply_map[cpu] = cpu % instance->msix_vectors;;
> >
> > If smp_affinity_enable module parameter is set to 0, I see performance
> > drop because IO is going to single reply queue.
>
> OK, that looks one issue. If smp_affinity_enable is set as 0, we should
follow
> the original way.
Sent you the patch.
>
> >
> > > +}
> > > +
> > > /**
> > > * megasas_init_fw - Initializes the FW
> > > * @instance: Adapter soft state
> > > @@ -5343,6 +5363,8 @@ static int megasas_init_fw(struct
> > > megasas_instance
> > > *instance)
> > > goto fail_setup_irqs;
> > > }
> > >
> > > + megasas_setup_reply_map(instance);
> > > +
> > > dev_info(&instance->pdev->dev,
> > > "firmware supports msix\t: (%d)", fw_msix_count);
> > > dev_info(&instance->pdev->dev,
> > > @@ -6448,6 +6470,11 @@ static int megasas_probe_one(struct pci_dev
> > > *pdev,
> > > memset(instance, 0, sizeof(*instance));
> > > atomic_set(&instance->fw_reset_no_pci_access, 0);
> > >
> > > + instance->reply_map = kzalloc(sizeof(unsigned int) * nr_cpu_ids,
> > > + GFP_KERNEL);
> > > + if (!instance->reply_map)
> > > + goto fail_alloc_reply_map;
> > > +
> > We have common function megasas_alloc_ctrl_mem() to manage
> allocation.
> > Good candidate to move this allocation to megasas_alloc_ctrl_mem.
>
> Good point, will do that in V5.
Sent you the patch.
>
>
> --
> Ming
