linux-kernel - RE: [PATCH v3 10/13] scsi: fnic: Add support for multiqueue (MQ) in fnic

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <SJ0PR11MB5896F9B7FA37D78B91BB1D42C3B1A@SJ0PR11MB5896.namprd11.prod.outlook.com>
Date:   Wed, 15 Nov 2023 23:20:26 +0000
From:   "Karan Tilak Kumar (kartilak)" <kartilak@...co.com>
To:     John Garry <john.g.garry@...cle.com>,
        "Sesidhar Baddela (sebaddel)" <sebaddel@...co.com>
CC:     "Arulprabhu Ponnusamy (arulponn)" <arulponn@...co.com>,
        "Dhanraj Jhawar (djhawar)" <djhawar@...co.com>,
        "Gian Carlo Boffa (gcboffa)" <gcboffa@...co.com>,
        "Masa Kai (mkai2)" <mkai2@...co.com>,
        "Satish Kharat (satishkh)" <satishkh@...co.com>,
        "jejb@...ux.ibm.com" <jejb@...ux.ibm.com>,
        "martin.petersen@...cle.com" <martin.petersen@...cle.com>,
        "linux-scsi@...r.kernel.org" <linux-scsi@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Hannes Reinecke <hare@...e.de>
Subject: RE: [PATCH v3 10/13] scsi: fnic: Add support for multiqueue (MQ) in
 fnic_main.c

On Wednesday, November 15, 2023 7:04 AM, John Garry <john.g.garry@...cle.com> 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@...co.com>
> > Reviewed-by: Arulprabhu Ponnusamy <arulponn@...co.com>
> > Reviewed-by: Hannes Reinecke <hare@...e.de>
> > Signed-off-by: Karan Tilak Kumar <kartilak@...co.com>
> > ---
>
> ... 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@...cle.com>
>

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