Re: [PATCH 05/17] lpfc: NVME Initiator: Base modifications Part D

[James Smart]

On 1/18/2017 5:39 AM, Johannes Thumshirn wrote:
I know this might be annoying by now, but it's really hard to follow 
code that

is so much indented and lpfc really needs some love in the readability case.
Please don't misunderstand me, it has nothing to do with you and lpfc is not
the only driver that could need some refactoring. It's just I've spend a lot
of time looking at lpfc since I've started working in the storage area and
the code style is somewhat inconsistent with the reset of the kernel (as are
other SCSI drivers, I know). If you and Martin/James B. don't mind I'd even
volunteer to help you with cleaning it up a bit.


I agree with the idea but want to hold off a little while longer. first, 
I want the driver to stabilize after adding a second protocol. second, I 
want it to harden a bit.


-- james


--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 05/17] lpfc: NVME Initiator: Base modifications Part D

[Johannes Thumshirn]

On Tue, Jan 17, 2017 at 05:20:48PM -0800, James Smart wrote:
> 
> NVME Initiator: Base modifications
> 
> This is part D of parts A..F.
> 
> Part D is the 2nd half of the mods to lpfc_init.c. This is the location
> of most of changes for the following:
> - sli3 ring vs sli4 wq splits
> - buffer pools are allocated/freed
> - sgl pools allocated/freed
> - adapter resources split up
> - queue config decided and enacted
> - receive buffer management
> 
> *
> 
> Refer to Part A for a description of base modifications
> 
> Signed-off-by: Dick Kennedy 
> Signed-off-by: James Smart 
> ---
>  drivers/scsi/lpfc/lpfc_init.c | 1088 
> +++--
>  1 file changed, 616 insertions(+), 472 deletions(-)
> 
> diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
> index ca54beb..ea12eca 100644
> --- a/drivers/scsi/lpfc/lpfc_init.c
> +++ b/drivers/scsi/lpfc/lpfc_init.c
> @@ -7876,14 +7876,14 @@ lpfc_sli4_queue_create(struct lpfc_hba *phba)
>  void
>  lpfc_sli4_queue_destroy(struct lpfc_hba *phba)
>  {
> - int idx;
> + int idx, numwq;
>  
>   if (phba->cfg_fof)
>   lpfc_fof_queue_destroy(phba);
>  
>   if (phba->sli4_hba.hba_eq != NULL) {
>   /* Release HBA event queue */
> - for (idx = 0; idx < phba->cfg_fcp_io_channel; idx++) {
> + for (idx = 0; idx < phba->io_channel; idx++) {
>   if (phba->sli4_hba.hba_eq[idx] != NULL) {
>   lpfc_sli4_queue_free(
>   phba->sli4_hba.hba_eq[idx]);
> @@ -7907,9 +7907,22 @@ lpfc_sli4_queue_destroy(struct lpfc_hba *phba)
>   phba->sli4_hba.fcp_cq = NULL;
>   }
>  
> + if (phba->sli4_hba.nvme_cq != NULL) {
> + /* Release NVME completion queue */
> + for (idx = 0; idx < phba->cfg_nvme_io_channel; idx++) {
> + if (phba->sli4_hba.nvme_cq[idx] != NULL) {
> + lpfc_sli4_queue_free(
> + phba->sli4_hba.nvme_cq[idx]);
> + phba->sli4_hba.nvme_cq[idx] = NULL;
> + }
> + }
> + kfree(phba->sli4_hba.nvme_cq);
> + phba->sli4_hba.nvme_cq = NULL;
> + }
> +


static inline void lpfc_sli4_release_nvme_cqs(struct lpfc_hba *phba)
{
int i;

for (i = 0; i  < phba->cfg_nvme_io_channel; i++) {
if (phba->sli4_hba.nvme_cq[idx] != NULL) {
lpfc_sli4_queue_free(phba->sli4_hba.nvme_cq[idx]);
phba->sli4_hba.nvme_cq[idx] = NULL;
}
kfree(phba->sli4_hba.nvme_cq);
phba->sli4_hba.nvme_cq = NULL;
}
}

And then:

if (phba->sli4_hba.nvme_cq)
lpfc_sli4_release_nvme_cqs(phba);

I know this might be annoying by now, but it's really hard to follow code that
is so much indented and lpfc really needs some love in the readability case.
Please don't misunderstand me, it has nothing to do with you and lpfc is not
the only driver that could need some refactoring. It's just I've spend a lot
of time looking at lpfc since I've started working in the storage area and
the code style is somewhat inconsistent with the reset of the kernel (as are
other SCSI drivers, I know). If you and Martin/James B. don't mind I'd even
volunteer to help you with cleaning it up a bit.

[...]

> @@ -7920,12 +7933,32 @@ lpfc_sli4_queue_destroy(struct lpfc_hba *phba)
>   phba->sli4_hba.fcp_wq = NULL;
>   }
>  
> + if (phba->sli4_hba.nvme_wq != NULL) {
> + /* Release NVME work queue */
> + numwq = phba->cfg_nvme_max_hw_queue;
> + for (idx = 0; idx < numwq; idx++) {
> + if (phba->sli4_hba.nvme_wq[idx] != NULL) {
> + lpfc_sli4_queue_free(
> + phba->sli4_hba.nvme_wq[idx]);
> + phba->sli4_hba.nvme_wq[idx] = NULL;
> + }
> + }
> + kfree(phba->sli4_hba.nvme_wq);
> + phba->sli4_hba.nvme_wq = NULL;
> + }
> +

Same goes for the WQs here.

[...]

> + nvme_cqidx = 0;
> + nvme_wqidx = 0;
> + if (phba->cfg_nvme_io_channel) {
> + /* Set up fast-path NVME Response Complete Queue */
> + if (!phba->sli4_hba.nvme_cq) {

Hint, if you write a comment what the specific code block is doing, you might
actually write a function. Here sth. like 'lpfc_nvme_setup_fast_path_cq()'
comes to my mind. Especially if you have to use more than 3 levels of
indentation and the block is more then 75-100 LoC. It is most likely getting
inlined by the compiler anyways.

[...]

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 

Re: [PATCH 05/17] lpfc: NVME Initiator: Base modifications Part D

[Hannes Reinecke]

On 01/18/2017 02:20 AM, James Smart wrote:
> 
> NVME Initiator: Base modifications
> 
> This is part D of parts A..F.
> 
> Part D is the 2nd half of the mods to lpfc_init.c. This is the location
> of most of changes for the following:
> - sli3 ring vs sli4 wq splits
> - buffer pools are allocated/freed
> - sgl pools allocated/freed
> - adapter resources split up
> - queue config decided and enacted
> - receive buffer management
> 
> *
> 
> Refer to Part A for a description of base modifications
> 
> Signed-off-by: Dick Kennedy 
> Signed-off-by: James Smart 
> ---
>  drivers/scsi/lpfc/lpfc_init.c | 1088 
> +++--
>  1 file changed, 616 insertions(+), 472 deletions(-)
> 
Reviewed-by: Hannes Reinecke 

Cheers,

Hannes
-- 
Dr. Hannes ReineckeTeamlead Storage & Networking
h...@suse.de   +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)
--
To unsubscribe from this list: send the line "unsubscribe linux-scsi" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 05/17] lpfc: NVME Initiator: Base modifications Part D

[James Smart]

NVME Initiator: Base modifications

This is part D of parts A..F.

Part D is the 2nd half of the mods to lpfc_init.c. This is the location
of most of changes for the following:
- sli3 ring vs sli4 wq splits
- buffer pools are allocated/freed
- sgl pools allocated/freed
- adapter resources split up
- queue config decided and enacted
- receive buffer management

*

Refer to Part A for a description of base modifications

Signed-off-by: Dick Kennedy 
Signed-off-by: James Smart 
---
 drivers/scsi/lpfc/lpfc_init.c | 1088 +++--
 1 file changed, 616 insertions(+), 472 deletions(-)

diff --git a/drivers/scsi/lpfc/lpfc_init.c b/drivers/scsi/lpfc/lpfc_init.c
index ca54beb..ea12eca 100644
--- a/drivers/scsi/lpfc/lpfc_init.c
+++ b/drivers/scsi/lpfc/lpfc_init.c
@@ -7876,14 +7876,14 @@ lpfc_sli4_queue_create(struct lpfc_hba *phba)
 void
 lpfc_sli4_queue_destroy(struct lpfc_hba *phba)
 {
-   int idx;
+   int idx, numwq;
 
if (phba->cfg_fof)
lpfc_fof_queue_destroy(phba);
 
if (phba->sli4_hba.hba_eq != NULL) {
/* Release HBA event queue */
-   for (idx = 0; idx < phba->cfg_fcp_io_channel; idx++) {
+   for (idx = 0; idx < phba->io_channel; idx++) {
if (phba->sli4_hba.hba_eq[idx] != NULL) {
lpfc_sli4_queue_free(
phba->sli4_hba.hba_eq[idx]);
@@ -7907,9 +7907,22 @@ lpfc_sli4_queue_destroy(struct lpfc_hba *phba)
phba->sli4_hba.fcp_cq = NULL;
}
 
+   if (phba->sli4_hba.nvme_cq != NULL) {
+   /* Release NVME completion queue */
+   for (idx = 0; idx < phba->cfg_nvme_io_channel; idx++) {
+   if (phba->sli4_hba.nvme_cq[idx] != NULL) {
+   lpfc_sli4_queue_free(
+   phba->sli4_hba.nvme_cq[idx]);
+   phba->sli4_hba.nvme_cq[idx] = NULL;
+   }
+   }
+   kfree(phba->sli4_hba.nvme_cq);
+   phba->sli4_hba.nvme_cq = NULL;
+   }
+
if (phba->sli4_hba.fcp_wq != NULL) {
/* Release FCP work queue */
-   for (idx = 0; idx < phba->cfg_fcp_io_channel; idx++) {
+   for (idx = 0; idx < phba->cfg_fcp_max_hw_queue; idx++) {
if (phba->sli4_hba.fcp_wq[idx] != NULL) {
lpfc_sli4_queue_free(
phba->sli4_hba.fcp_wq[idx]);
@@ -7920,12 +7933,32 @@ lpfc_sli4_queue_destroy(struct lpfc_hba *phba)
phba->sli4_hba.fcp_wq = NULL;
}
 
+   if (phba->sli4_hba.nvme_wq != NULL) {
+   /* Release NVME work queue */
+   numwq = phba->cfg_nvme_max_hw_queue;
+   for (idx = 0; idx < numwq; idx++) {
+   if (phba->sli4_hba.nvme_wq[idx] != NULL) {
+   lpfc_sli4_queue_free(
+   phba->sli4_hba.nvme_wq[idx]);
+   phba->sli4_hba.nvme_wq[idx] = NULL;
+   }
+   }
+   kfree(phba->sli4_hba.nvme_wq);
+   phba->sli4_hba.nvme_wq = NULL;
+   }
+
/* Release FCP CQ mapping array */
if (phba->sli4_hba.fcp_cq_map != NULL) {
kfree(phba->sli4_hba.fcp_cq_map);
phba->sli4_hba.fcp_cq_map = NULL;
}
 
+   /* Release NVME CQ mapping array */
+   if (phba->sli4_hba.nvme_cq_map != NULL) {
+   kfree(phba->sli4_hba.nvme_cq_map);
+   phba->sli4_hba.nvme_cq_map = NULL;
+   }
+
/* Release mailbox command work queue */
if (phba->sli4_hba.mbx_wq != NULL) {
lpfc_sli4_queue_free(phba->sli4_hba.mbx_wq);
@@ -7938,6 +7971,12 @@ lpfc_sli4_queue_destroy(struct lpfc_hba *phba)
phba->sli4_hba.els_wq = NULL;
}
 
+   /* Release ELS work queue */
+   if (phba->sli4_hba.nvmels_wq != NULL) {
+   lpfc_sli4_queue_free(phba->sli4_hba.nvmels_wq);
+   phba->sli4_hba.nvmels_wq = NULL;
+   }
+
/* Release unsolicited receive queue */
if (phba->sli4_hba.hdr_rq != NULL) {
lpfc_sli4_queue_free(phba->sli4_hba.hdr_rq);
@@ -7954,15 +7993,83 @@ lpfc_sli4_queue_destroy(struct lpfc_hba *phba)
phba->sli4_hba.els_cq = NULL;
}
 
+   /* Release NVME LS complete queue */
+   if (phba->sli4_hba.nvmels_cq != NULL) {
+   lpfc_sli4_queue_free(phba->sli4_hba.nvmels_cq);
+   phba->sli4_hba.nvmels_cq = NULL;
+   }
+
/* Release mailbox command complete queue */
if (phba->sli4_hba.mbx_cq != NULL) {
lpfc_sli4_queue_free(phba->sli4_hba.mbx_cq);
phba->sli4_hba.mbx_cq = NULL;