RE: [PATCH v3 10/13] scsi: fnic: Add support for multiqueue (MQ) in fnic_main.c

[Karan Tilak Kumar (kartilak)]

On Wednesday, November 15, 2023 7:04 AM, John Garry <john.g.garry@xxxxxxxxxx> wrote:
>
> On 14/11/2023 22:38, Karan Tilak Kumar wrote:
> > Set map_queues in the fnic_host_template to fnic_mq_map_queues_cpus.
> > Define fnic_mq_map_queues_cpus to set cpu assignment to fnic queues.
> > Refactor code in fnic_probe to enable vnic queues before scsi_add_host.
> > Modify notify set to the correct index.
> >
> > Changes between v2 and v3:
> >      Incorporate review comment from Hannes:
> >          Replace cpy_wq_base with copy_wq_base.
> >      Incorporate review comment from John Garry:
> >     Replace code in fnic_mq_map_queues_cpus
> >     with blk_mq_pci_map_queues.
> >      Replace shost_printk logs with FNIC_MAIN_DBG.
>
> JFYI, This comment does not belong here ...
>
> >
> > Reviewed-by: Sesidhar Baddela <sebaddel@xxxxxxxxx>
> > Reviewed-by: Arulprabhu Ponnusamy <arulponn@xxxxxxxxx>
> > Reviewed-by: Hannes Reinecke <hare@xxxxxxx>
> > Signed-off-by: Karan Tilak Kumar <kartilak@xxxxxxxxx>
> > ---
>
> ... should be placed here.

Thanks John. I'll keep this in mind for the next time.


> Regardless of a couple of comments, below, feel free to pick up:
>
> Reviewed-by: John Garry <john.g.garry@xxxxxxxxxx>
>

Thanks for your review.

> > +void fnic_mq_map_queues_cpus(struct Scsi_Host *host) {
> > +     struct fc_lport *lp = shost_priv(host);
> > +     struct fnic *fnic = lport_priv(lp);
> > +     struct pci_dev *l_pdev = fnic->pdev;
> > +     int intr_mode = fnic->config.intr_mode;
> > +     struct blk_mq_queue_map *qmap =
> > +&host->tag_set.map[HCTX_TYPE_DEFAULT];
> > +
> > +     if (intr_mode == VNIC_DEV_INTR_MODE_MSI || intr_mode == VNIC_DEV_INTR_MODE_INTX) {
> > +             FNIC_MAIN_DBG(KERN_ERR, fnic->lport->host, fnic->fnic_num,
> > +                     "intr_mode is not msix\n");
>
>                       Are these checks just paranoia? I mean that it is strange to have
>                       fnic_mq_map_queues_cpus() called but not be required to do anything.
>

Unified Computing Servers Management (UCSM) is a GUI tool to configure Cisco Servers. 
There are interrupt options that can be modified to INTX or MSI or MSI-x.
All these options are still supported. 

However, we do not support multiqueue (MQ) on MSI or INTX.
These checks are present to only prevent an MQ "misconfiguration".

> > +     for (hwq = 0; hwq < fnic->wq_copy_count; hwq++)
> > +             kfree(fnic->sw_copy_wq[hwq].io_req_table);
>
>  you might be able to use device-managed methods for allocating this memory, like devm_kzalloc() (so that the manual memory free'ing is not required).
>

Thanks for this information. We can consider this in a future patchset.

Regards,
Karan