From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-it0-f66.google.com ([209.85.214.66]:54359 "EHLO mail-it0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932189AbeCIODM (ORCPT ); Fri, 9 Mar 2018 09:03:12 -0500 Received: by mail-it0-f66.google.com with SMTP id c11so2850196ith.4 for ; Fri, 09 Mar 2018 06:03:11 -0800 (PST) From: Kashyap Desai References: <20180309033218.23042-1-ming.lei@redhat.com> <20180309033218.23042-3-ming.lei@redhat.com> <20180309120309.GA30257@ming.t460p> In-Reply-To: <20180309120309.GA30257@ming.t460p> MIME-Version: 1.0 Date: Fri, 9 Mar 2018 19:33:09 +0530 Message-ID: <9606999c22c5ff59f39b43ef10270b96@mail.gmail.com> Subject: RE: [PATCH V4 2/4] scsi: megaraid_sas: fix selection of reply queue To: Ming Lei Cc: James Bottomley , Jens Axboe , "Martin K . Petersen" , Christoph Hellwig , linux-scsi@vger.kernel.org, linux-block@vger.kernel.org, Meelis Roos , Don Brace , Laurence Oberman , Mike Snitzer , Hannes Reinecke , Artem Bityutskiy Content-Type: text/plain; charset="UTF-8" Sender: linux-block-owner@vger.kernel.org List-Id: linux-block@vger.kernel.org > -----Original Message----- > From: Ming Lei [mailto:ming.lei@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-block@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.lei@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- > > > block@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 > > > Cc: "Martin K. Petersen" , > > > Cc: James Bottomley , > > > Cc: Christoph Hellwig , > > > Cc: Don Brace > > > Cc: Kashyap Desai > > > Cc: Laurence Oberman > > > Cc: Mike Snitzer > > > Cc: Meelis Roos > > > Cc: Artem Bityutskiy > > > Fixes: 84676c1f21e8 ("genirq/affinity: assign vectors to all > > > possible > > CPUs") > > > Signed-off-by: Ming Lei > > > --- > > > 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