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

  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.

Reviewed-by: Sesidhar Baddela <sebaddel@cisco.com>
Reviewed-by: Arulprabhu Ponnusamy <arulponn@cisco.com>
Reviewed-by: Hannes Reinecke <hare@suse.de>
Signed-off-by: Karan Tilak Kumar <kartilak@cisco.com>
---
 drivers/scsi/fnic/fnic.h      |   4 +-
 drivers/scsi/fnic/fnic_main.c | 109 ++++++++++++++++++++++++----------
 2 files changed, 78 insertions(+), 35 deletions(-)
  

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@cisco.com>
> Reviewed-by: Arulprabhu Ponnusamy <arulponn@cisco.com>
> Reviewed-by: Hannes Reinecke <hare@suse.de>
> Signed-off-by: Karan Tilak Kumar <kartilak@cisco.com>
> ---

... should be placed here.



Regardless of a couple of comments, below, feel free to pick up:

Reviewed-by: John Garry <john.g.garry@oracle.com>

>   drivers/scsi/fnic/fnic.h      |   4 +-
>   drivers/scsi/fnic/fnic_main.c | 109 ++++++++++++++++++++++++----------
>   2 files changed, 78 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/scsi/fnic/fnic.h b/drivers/scsi/fnic/fnic.h
> index 97a2e547584a..5777a54c99c3 100644
> --- a/drivers/scsi/fnic/fnic.h
> +++ b/drivers/scsi/fnic/fnic.h
> @@ -109,7 +109,7 @@ static inline u64 fnic_flags_and_state(struct scsi_cmnd *cmd)
>   #define FNIC_ABT_TERM_DELAY_TIMEOUT  500        /* mSec */
>   
>   #define FNIC_MAX_FCP_TARGET     256
> -
> +#define FNIC_PCI_OFFSET		2
>   /**
>    * state_flags to identify host state along along with fnic's state
>    **/
> @@ -384,7 +384,7 @@ void fnic_wq_copy_cleanup_handler(struct vnic_wq_copy *wq,
>   int fnic_fw_reset_handler(struct fnic *fnic);
>   void fnic_terminate_rport_io(struct fc_rport *);
>   const char *fnic_state_to_str(unsigned int state);
> -
> +void fnic_mq_map_queues_cpus(struct Scsi_Host *host);
>   void fnic_log_q_error(struct fnic *fnic);
>   void fnic_handle_link_event(struct fnic *fnic);
>   
> diff --git a/drivers/scsi/fnic/fnic_main.c b/drivers/scsi/fnic/fnic_main.c
> index 8bb0165490f4..93c457d88fc2 100644
> --- a/drivers/scsi/fnic/fnic_main.c
> +++ b/drivers/scsi/fnic/fnic_main.c
> @@ -12,9 +12,11 @@
>   #include <linux/pci.h>
>   #include <linux/skbuff.h>
>   #include <linux/interrupt.h>
> +#include <linux/irq.h>
>   #include <linux/spinlock.h>
>   #include <linux/workqueue.h>
>   #include <linux/if_ether.h>
> +#include <linux/blk-mq-pci.h>
>   #include <scsi/fc/fc_fip.h>
>   #include <scsi/scsi_host.h>
>   #include <scsi/scsi_transport.h>
> @@ -116,6 +118,7 @@ static const struct scsi_host_template fnic_host_template = {
>   	.shost_groups = fnic_host_groups,
>   	.track_queue_depth = 1,
>   	.cmd_size = sizeof(struct fnic_cmd_priv),
> +	.map_queues = fnic_mq_map_queues_cpus,
>   };
>   
>   static void
> @@ -392,7 +395,7 @@ static int fnic_notify_set(struct fnic *fnic)
>   		err = vnic_dev_notify_set(fnic->vdev, -1);
>   		break;
>   	case VNIC_DEV_INTR_MODE_MSIX:
> -		err = vnic_dev_notify_set(fnic->vdev, FNIC_MSIX_ERR_NOTIFY);
> +		err = vnic_dev_notify_set(fnic->vdev, fnic->wq_copy_count + fnic->copy_wq_base);
>   		break;
>   	default:
>   		shost_printk(KERN_ERR, fnic->lport->host,
> @@ -565,11 +568,6 @@ static int fnic_scsi_drv_init(struct fnic *fnic)
>   	host->max_cmd_len = FCOE_MAX_CMD_LEN;
>   
>   	host->nr_hw_queues = fnic->wq_copy_count;
> -	if (host->nr_hw_queues > 1)
> -		shost_printk(KERN_ERR, host,
> -				"fnic: blk-mq is not supported");
> -
> -	host->nr_hw_queues = fnic->wq_copy_count = 1;
>   
>   	shost_printk(KERN_INFO, host,
>   			"fnic: can_queue: %d max_lun: %llu",
> @@ -582,15 +580,42 @@ static int fnic_scsi_drv_init(struct fnic *fnic)
>   	return 0;
>   }
>   
> +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.

> +		return;
> +	}
> +
> +	FNIC_MAIN_DBG(KERN_INFO, fnic->lport->host, fnic->fnic_num,
> +			"qmap->nr_queues: %d\n", qmap->nr_queues);
> +
> +	if (l_pdev == NULL) {
> +		FNIC_MAIN_DBG(KERN_ERR, fnic->lport->host, fnic->fnic_num,
> +						"l_pdev is null\n");
> +		return;
> +	}
> +
> +	blk_mq_pci_map_queues(qmap, l_pdev, FNIC_PCI_OFFSET);
> +}
> +
>   static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   {
>   	struct Scsi_Host *host;
>   	struct fc_lport *lp;
>   	struct fnic *fnic;
>   	mempool_t *pool;
> -	int err;
> +	int err = 0;
>   	int i;
>   	unsigned long flags;
> +	int hwq;
>   
>   	atomic_inc(&fnic_num);
>   	/*
> @@ -607,8 +632,8 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	fnic = lport_priv(lp);
>   	fnic->lport = lp;
>   	fnic->ctlr.lp = lp;
> -
>   	fnic->link_events = 0;
> +	fnic->pdev = pdev;
>   
>   	snprintf(fnic->name, sizeof(fnic->name) - 1, "%s%d", DRV_NAME,
>   		 host->host_no);
> @@ -617,11 +642,6 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	fnic->fnic_num = atomic_read(&fnic_num);
>   	fnic_stats_debugfs_init(fnic);
>   
> -	/* Setup PCI resources */
> -	pci_set_drvdata(pdev, fnic);
> -
> -	fnic->pdev = pdev;
> -
>   	err = pci_enable_device(pdev);
>   	if (err) {
>   		shost_printk(KERN_ERR, fnic->lport->host,
> @@ -723,7 +743,8 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   		goto err_out_dev_close;
>   	}
>   
> -	fnic_scsi_drv_init(fnic);
> +	/* Setup PCI resources */
> +	pci_set_drvdata(pdev, fnic);
>   
>   	fnic_get_res_counts(fnic);
>   
> @@ -743,6 +764,16 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   		goto err_out_clear_intr;
>   	}
>   
> +	fnic_scsi_drv_init(fnic);
> +
> +	for (hwq = 0; hwq < fnic->wq_copy_count; hwq++) {
> +		fnic->sw_copy_wq[hwq].ioreq_table_size = fnic->fnic_max_tag_id;
> +		fnic->sw_copy_wq[hwq].io_req_table =
> +					kzalloc((fnic->sw_copy_wq[hwq].ioreq_table_size + 1) *
> +					sizeof(struct fnic_io_req *), GFP_KERNEL);
> +	}
> +	shost_printk(KERN_INFO, fnic->lport->host, "fnic copy wqs: %d, Q0 ioreq table size: %d\n",
> +			fnic->wq_copy_count, fnic->sw_copy_wq[0].ioreq_table_size);
>   
>   	/* initialize all fnic locks */
>   	spin_lock_init(&fnic->fnic_lock);
> @@ -827,16 +858,32 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   
>   	/* allocate RQ buffers and post them to RQ*/
>   	for (i = 0; i < fnic->rq_count; i++) {
> -		vnic_rq_enable(&fnic->rq[i]);
>   		err = vnic_rq_fill(&fnic->rq[i], fnic_alloc_rq_frame);
>   		if (err) {
>   			shost_printk(KERN_ERR, fnic->lport->host,
>   				     "fnic_alloc_rq_frame can't alloc "
>   				     "frame\n");
> -			goto err_out_free_rq_buf;
> +			goto err_out_rq_buf;
>   		}
>   	}
>   
> +	/* Enable all queues */
> +	for (i = 0; i < fnic->raw_wq_count; i++)
> +		vnic_wq_enable(&fnic->wq[i]);
> +	for (i = 0; i < fnic->rq_count; i++) {
> +		if (!ioread32(&fnic->rq[i].ctrl->enable))
> +			vnic_rq_enable(&fnic->rq[i]);
> +	}
> +	for (i = 0; i < fnic->wq_copy_count; i++)
> +		vnic_wq_copy_enable(&fnic->hw_copy_wq[i]);
> +
> +	err = fnic_request_intr(fnic);
> +	if (err) {
> +		shost_printk(KERN_ERR, fnic->lport->host,
> +			     "Unable to request irq.\n");
> +		goto err_out_request_intr;
> +	}
> +
>   	/*
>   	 * Initialization done with PCI system, hardware, firmware.
>   	 * Add host to SCSI
> @@ -845,9 +892,10 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	if (err) {
>   		shost_printk(KERN_ERR, fnic->lport->host,
>   			     "fnic: scsi_add_host failed...exiting\n");
> -		goto err_out_free_rq_buf;
> +		goto err_out_scsi_add_host;
>   	}
>   
> +
>   	/* Start local port initiatialization */
>   
>   	lp->link_up = 0;
> @@ -871,7 +919,7 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	if (!fc_exch_mgr_alloc(lp, FC_CLASS_3, FCPIO_HOST_EXCH_RANGE_START,
>   			       FCPIO_HOST_EXCH_RANGE_END, NULL)) {
>   		err = -ENOMEM;
> -		goto err_out_remove_scsi_host;
> +		goto err_out_fc_exch_mgr_alloc;
>   	}
>   
>   	fc_lport_init_stats(lp);
> @@ -899,21 +947,8 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   	skb_queue_head_init(&fnic->frame_queue);
>   	skb_queue_head_init(&fnic->tx_queue);
>   
> -	/* Enable all queues */
> -	for (i = 0; i < fnic->raw_wq_count; i++)
> -		vnic_wq_enable(&fnic->wq[i]);
> -	for (i = 0; i < fnic->wq_copy_count; i++)
> -		vnic_wq_copy_enable(&fnic->hw_copy_wq[i]);
> -
>   	fc_fabric_login(lp);
>   
> -	err = fnic_request_intr(fnic);
> -	if (err) {
> -		shost_printk(KERN_ERR, fnic->lport->host,
> -			     "Unable to request irq.\n");
> -		goto err_out_free_exch_mgr;
> -	}
> -
>   	vnic_dev_enable(fnic->vdev);
>   
>   	for (i = 0; i < fnic->intr_count; i++)
> @@ -925,12 +960,15 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   
>   err_out_free_exch_mgr:
>   	fc_exch_mgr_free(lp);
> -err_out_remove_scsi_host:
> +err_out_fc_exch_mgr_alloc:
>   	fc_remove_host(lp->host);
>   	scsi_remove_host(lp->host);
> -err_out_free_rq_buf:
> +err_out_scsi_add_host:
> +	fnic_free_intr(fnic);
> +err_out_request_intr:
>   	for (i = 0; i < fnic->rq_count; i++)
>   		vnic_rq_clean(&fnic->rq[i], fnic_free_rq_buf);
> +err_out_rq_buf:
>   	vnic_dev_notify_unset(fnic->vdev);
>   err_out_free_max_pool:
>   	mempool_destroy(fnic->io_sgl_pool[FNIC_SGL_CACHE_MAX]);
> @@ -939,6 +977,8 @@ static int fnic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>   err_out_free_ioreq_pool:
>   	mempool_destroy(fnic->io_req_pool);
>   err_out_free_resources:
> +	for (hwq = 0; hwq < fnic->wq_copy_count; hwq++)
> +		kfree(fnic->sw_copy_wq[hwq].io_req_table);
>   	fnic_free_vnic_resources(fnic);
>   err_out_clear_intr:
>   	fnic_clear_intr_mode(fnic);
> @@ -965,6 +1005,7 @@ static void fnic_remove(struct pci_dev *pdev)
>   	struct fnic *fnic = pci_get_drvdata(pdev);
>   	struct fc_lport *lp = fnic->lport;
>   	unsigned long flags;
> +	int hwq;
>   
>   	/*
>   	 * Mark state so that the workqueue thread stops forwarding
> @@ -1025,6 +1066,8 @@ static void fnic_remove(struct pci_dev *pdev)
>   
>   	fc_remove_host(fnic->lport->host);
>   	scsi_remove_host(fnic->lport->host);
> +	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).

>   	fc_exch_mgr_free(fnic->lport);
>   	vnic_dev_notify_unset(fnic->vdev);
>   	fnic_free_intr(fnic);
  

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

On 15/11/2023 23:20, Karan Tilak Kumar (kartilak) wrote:
>>> +
>>> +     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.

By this I assume it means nr_hw_queues = 1 for MSI or INTX

> These checks are present to only prevent an MQ "misconfiguration".

If you check blk_mq_update_queue_map(), for set->ops->map_queues unset 
we call blk_mq_map_queues() - I am just wondering what does the 
equivalent to blk_mq_map_queues() for you in these other modes.

I am concerned that it is not proper that we have a set->ops->map_queues 
method, but it does nothing in some scenarios.

Thanks,
John
  

On 16/11/2023 16:32, John Garry wrote:
> 
> If you check blk_mq_update_queue_map(), for set->ops->map_queues unset 
> we call blk_mq_map_queues() - I am just wondering what does the 
> equivalent to blk_mq_map_queues() for you in these other modes.
> 
> I am concerned that it is not proper that we have a set->ops->map_queues 
> method, but it does nothing in some scenarios.

I think we get away with this as mq_map[for all CPUs] is 0, which points 
to the only HW queue.

Thanks,
John
  

On Thursday, November 16, 2023 8:33 AM, John Garry <john.g.garry@oracle.com> wrote:
>
> On 15/11/2023 23:20, Karan Tilak Kumar (kartilak) wrote:
> >>> +
> >>> +     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.
>
> By this I assume it means nr_hw_queues = 1 for MSI or INTX

That's correct, John.

> > These checks are present to only prevent an MQ "misconfiguration".
>
> > If you check blk_mq_update_queue_map(), for set->ops->map_queues unset we call blk_mq_map_queues() - I am just wondering what does the equivalent to blk_mq_map_queues() for you in these other modes.
>
> > I am concerned that it is not proper that we have a set->ops->map_queues method, but it does nothing in some scenarios.
>
> I think we get away with this as mq_map[for all CPUs] is 0, which points to the only HW queue.

Yes, that's correct. This is what I found during testing as well.

Regards,
Karan