[PATCH 2/2] be2iscsi: Moving to pci_pools v3

[PATCH 2/2] be2iscsi: Moving to pci_pools v3

[Jayamohan Kallickal]

 This patch contains changes to use pci_pools for iscsi hdr
instead of pci_alloc_consistent. Here we alloc and free to pool
for every IO

v3:
- Remove cleanup loop in beiscsi_session_destroy
- Fixup for allocation failure handling in beiscsi_alloc_pdu
- Removed unused variable in beiscsi_session_destroy.
 
Signed-off-by: Jayamohan Kallickal <jayamohank@serverengines.com>
---
 drivers/scsi/be2iscsi/be_iscsi.c |   44 +++++++++---------------
 drivers/scsi/be2iscsi/be_main.c  |   67 +++++++++++++++++--------------------
 drivers/scsi/be2iscsi/be_main.h  |    1 -
 3 files changed, 48 insertions(+), 64 deletions(-)

diff --git a/drivers/scsi/be2iscsi/be_iscsi.c b/drivers/scsi/be2iscsi/be_iscsi.c
index f18e643..2fd2544 100644
--- a/drivers/scsi/be2iscsi/be_iscsi.c
+++ b/drivers/scsi/be2iscsi/be_iscsi.c
@@ -45,14 +45,9 @@ struct iscsi_cls_session *beiscsi_session_create(struct iscsi_endpoint *ep,
 	struct beiscsi_endpoint *beiscsi_ep;
 	struct iscsi_cls_session *cls_session;
 	struct beiscsi_hba *phba;
-	struct iscsi_task *task;
 	struct iscsi_session *sess;
 	struct beiscsi_session *beiscsi_sess;
 	struct beiscsi_io_task *io_task;
-	unsigned int max_size, num_cmd;
-	dma_addr_t bus_add;
-	u64 pa_addr;
-	void *vaddr;
 
 	SE_DEBUG(DBG_LVL_8, "In beiscsi_session_create\n");
 
@@ -80,20 +75,18 @@ struct iscsi_cls_session *beiscsi_session_create(struct iscsi_endpoint *ep,
 	if (!cls_session)
 		return NULL;
 	sess = cls_session->dd_data;
-	max_size = ALIGN(sizeof(struct be_cmd_bhs), 64) * sess->cmds_max;
-	vaddr = pci_alloc_consistent(phba->pcidev, max_size, &bus_add);
-	pa_addr = (__u64) bus_add;
-
-	for (num_cmd = 0; num_cmd < sess->cmds_max; num_cmd++) {
-		task = sess->cmds[num_cmd];
-		io_task = task->dd_data;
-		io_task->cmd_bhs = vaddr;
-		io_task->bhs_pa.u.a64.address = pa_addr;
-		io_task->alloc_size = max_size;
-		vaddr += ALIGN(sizeof(struct be_cmd_bhs), 64);
-		pa_addr += ALIGN(sizeof(struct be_cmd_bhs), 64);
-	}
+	beiscsi_sess = sess->dd_data;
+	beiscsi_sess->bhs_pool =  pci_pool_create("beiscsi_bhs_pool",
+						   phba->pcidev,
+						   sizeof(struct be_cmd_bhs),
+						   64, 0);
+	if (!beiscsi_sess->bhs_pool)
+		goto destroy_sess;
+
 	return cls_session;
+destroy_sess:
+	iscsi_session_teardown(cls_session);
+	return NULL;
 }
 
 /**
@@ -105,18 +98,10 @@ struct iscsi_cls_session *beiscsi_session_create(struct iscsi_endpoint *ep,
  */
 void beiscsi_session_destroy(struct iscsi_cls_session *cls_session)
 {
-	struct iscsi_task *task;
-	struct beiscsi_io_task *io_task;
 	struct iscsi_session *sess = cls_session->dd_data;
-	struct Scsi_Host *shost = iscsi_session_to_shost(cls_session);
-	struct beiscsi_hba *phba = iscsi_host_priv(shost);
+	struct beiscsi_session *beiscsi_sess = sess->dd_data;
 
-	task = sess->cmds[0];
-	io_task = task->dd_data;
-	pci_free_consistent(phba->pcidev,
-			    io_task->alloc_size,
-			    io_task->cmd_bhs,
-			    io_task->bhs_pa.u.a64.address);
+	pci_pool_destroy(beiscsi_sess->bhs_pool);
 	iscsi_session_teardown(cls_session);
 }
 
@@ -133,6 +118,8 @@ beiscsi_conn_create(struct iscsi_cls_session *cls_session, u32 cid)
 	struct iscsi_cls_conn *cls_conn;
 	struct beiscsi_conn *beiscsi_conn;
 	struct iscsi_conn *conn;
+	struct iscsi_session *sess;
+	struct beiscsi_session *beiscsi_sess;
 
 	SE_DEBUG(DBG_LVL_8, "In beiscsi_conn_create ,cid"
 		 "from iscsi layer=%d\n", cid);
@@ -148,6 +135,9 @@ beiscsi_conn_create(struct iscsi_cls_session *cls_session, u32 cid)
 	beiscsi_conn->ep = NULL;
 	beiscsi_conn->phba = phba;
 	beiscsi_conn->conn = conn;
+	sess = cls_session->dd_data;
+	beiscsi_sess = sess->dd_data;
+	beiscsi_conn->beiscsi_sess = beiscsi_sess;
 	return cls_conn;
 }
 
diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index d520fe8..4299a46 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -2880,6 +2880,13 @@ static int beiscsi_alloc_pdu(struct iscsi_task *task, uint8_t opcode)
 	struct hwi_wrb_context *pwrb_context;
 	struct hwi_controller *phwi_ctrlr;
 	itt_t itt;
+	struct beiscsi_session *beiscsi_sess = beiscsi_conn->beiscsi_sess;
+
+	io_task->cmd_bhs = pci_pool_alloc(beiscsi_sess->bhs_pool,
+					  GFP_KERNEL,
+					  &io_task->bhs_pa.u.a64.address);
+	if (!io_task->cmd_bhs)
+		return -ENOMEM;
 
 	io_task->pwrb_handle = alloc_wrb_handle(phba,
 						beiscsi_conn->beiscsi_conn_cid,
@@ -2894,17 +2901,9 @@ static int beiscsi_alloc_pdu(struct iscsi_task *task, uint8_t opcode)
 		spin_lock(&phba->io_sgl_lock);
 		io_task->psgl_handle = alloc_io_sgl_handle(phba);
 		spin_unlock(&phba->io_sgl_lock);
-		if (!io_task->psgl_handle) {
-			phwi_ctrlr = phba->phwi_ctrlr;
-			pwrb_context = &phwi_ctrlr->wrb_context
-					[beiscsi_conn->beiscsi_conn_cid];
-			free_wrb_handle(phba, pwrb_context,
-						io_task->pwrb_handle);
-			io_task->pwrb_handle = NULL;
-			SE_DEBUG(DBG_LVL_1,
-				 "Alloc of SGL_ICD Failed \n");
-			return -ENOMEM;
-		}
+		if (!io_task->psgl_handle)
+			goto free_hndls;
+
 	} else {
 		io_task->scsi_cmnd = NULL;
 		if ((task->hdr->opcode & ISCSI_OPCODE_MASK) == ISCSI_OP_LOGIN) {
@@ -2913,18 +2912,9 @@ static int beiscsi_alloc_pdu(struct iscsi_task *task, uint8_t opcode)
 				io_task->psgl_handle = (struct sgl_handle *)
 						alloc_mgmt_sgl_handle(phba);
 				spin_unlock(&phba->mgmt_sgl_lock);
-				if (!io_task->psgl_handle) {
-					phwi_ctrlr = phba->phwi_ctrlr;
-					pwrb_context =
-					&phwi_ctrlr->wrb_context
-					[beiscsi_conn->beiscsi_conn_cid];
-					free_wrb_handle(phba, pwrb_context,
-							io_task->pwrb_handle);
-					io_task->pwrb_handle = NULL;
-					SE_DEBUG(DBG_LVL_1, "Alloc of "
-						"MGMT_SGL_ICD Failed \n");
-					return -ENOMEM;
-				}
+				if (!io_task->psgl_handle)
+					goto free_hndls;
+
 				beiscsi_conn->login_in_progress = 1;
 				beiscsi_conn->plogin_sgl_handle =
 							io_task->psgl_handle;
@@ -2936,23 +2926,24 @@ static int beiscsi_alloc_pdu(struct iscsi_task *task, uint8_t opcode)
 			spin_lock(&phba->mgmt_sgl_lock);
 			io_task->psgl_handle = alloc_mgmt_sgl_handle(phba);
 			spin_unlock(&phba->mgmt_sgl_lock);
-			if (!io_task->psgl_handle) {
-				phwi_ctrlr = phba->phwi_ctrlr;
-				pwrb_context = &phwi_ctrlr->wrb_context
-					[beiscsi_conn->beiscsi_conn_cid];
-				free_wrb_handle(phba, pwrb_context,
-							io_task->pwrb_handle);
-				io_task->pwrb_handle = NULL;
-				SE_DEBUG(DBG_LVL_1, "Alloc of "
-					 "MGMT_SGL_ICD Failed \n");
-				return -ENOMEM;
-			}
+			if (!io_task->psgl_handle)
+				goto free_hndls;
 		}
 	}
 	itt = (itt_t) cpu_to_be32(((unsigned int)task->itt << 16) |
 			(unsigned int)(io_task->psgl_handle->sgl_index));
 	io_task->cmd_bhs->iscsi_hdr.itt = itt;
 	return 0;
+
+free_hndls:
+	phwi_ctrlr = phba->phwi_ctrlr;
+	pwrb_context = &phwi_ctrlr->wrb_context[beiscsi_conn->beiscsi_conn_cid];
+	free_wrb_handle(phba, pwrb_context, io_task->pwrb_handle);
+	io_task->pwrb_handle = NULL;
+	pci_pool_free(beiscsi_sess->bhs_pool, io_task->cmd_bhs,
+		      io_task->bhs_pa.u.a64.address);
+	SE_DEBUG(DBG_LVL_1, "Alloc of SGL_ICD Failed \n");
+	return -ENOMEM;
 }
 
 static void beiscsi_cleanup_task(struct iscsi_task *task)
@@ -2961,6 +2952,7 @@ static void beiscsi_cleanup_task(struct iscsi_task *task)
 	struct iscsi_conn *conn = task->conn;
 	struct beiscsi_conn *beiscsi_conn = conn->dd_data;
 	struct beiscsi_hba *phba = beiscsi_conn->phba;
+	struct beiscsi_session *beiscsi_sess = beiscsi_conn->beiscsi_sess;
 	struct hwi_wrb_context *pwrb_context;
 	struct hwi_controller *phwi_ctrlr;
 
@@ -2971,6 +2963,11 @@ static void beiscsi_cleanup_task(struct iscsi_task *task)
 		io_task->pwrb_handle = NULL;
 	}
 
+	if (io_task->cmd_bhs) {
+		pci_pool_free(beiscsi_sess->bhs_pool, io_task->cmd_bhs,
+			      io_task->bhs_pa.u.a64.address);
+	}
+
 	if (task->sc) {
 		if (io_task->psgl_handle) {
 			spin_lock(&phba->io_sgl_lock);
@@ -3003,7 +3000,6 @@ static int beiscsi_iotask(struct iscsi_task *task, struct scatterlist *sg,
 	unsigned int doorbell = 0;
 
 	pwrb = io_task->pwrb_handle->pwrb;
-
 	io_task->cmd_bhs->iscsi_hdr.exp_statsn = 0;
 	io_task->bhs_len = sizeof(struct be_cmd_bhs);
 
@@ -3020,7 +3016,6 @@ static int beiscsi_iotask(struct iscsi_task *task, struct scatterlist *sg,
 			      &io_task->cmd_bhs->iscsi_data_pdu, 1);
 		AMAP_SET_BITS(struct amap_iscsi_wrb, type, pwrb, INI_WR_CMD);
 		AMAP_SET_BITS(struct amap_iscsi_wrb, dsp, pwrb, 1);
-
 	} else {
 		SE_DEBUG(DBG_LVL_4, "READ Command \t");
 		AMAP_SET_BITS(struct amap_iscsi_wrb, type, pwrb, INI_RD_CMD);
diff --git a/drivers/scsi/be2iscsi/be_main.h b/drivers/scsi/be2iscsi/be_main.h
index 387e363..53c9b70 100644
--- a/drivers/scsi/be2iscsi/be_main.h
+++ b/drivers/scsi/be2iscsi/be_main.h
@@ -391,7 +391,6 @@ struct beiscsi_io_task {
 	unsigned short cid;
 	unsigned short header_len;
 
-	unsigned int alloc_size;
 	struct be_cmd_bhs *cmd_bhs;
 	struct be_bus_address bhs_pa;
 	unsigned short bhs_len;
-- 
1.6.4

Re: [PATCH 2/2] be2iscsi: Moving to pci_pools v3

[Mike Christie]

On 09/21/2009 09:52 PM, Jayamohan Kallickal wrote:
>   This patch contains changes to use pci_pools for iscsi hdr
> instead of pci_alloc_consistent. Here we alloc and free to pool
> for every IO
>
> v3:
> - Remove cleanup loop in beiscsi_session_destroy
> - Fixup for allocation failure handling in beiscsi_alloc_pdu
> - Removed unused variable in beiscsi_session_destroy.
>
> Signed-off-by: Jayamohan Kallickal<jayamohank@serverengines.com>

Thanks for fixing those issues.

Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>

Re: [PATCH 2/2] be2iscsi: Moving to pci_pools v3

[James Bottomley]

On Wed, 2009-09-23 at 12:04 -0500, Mike Christie wrote:
> On 09/21/2009 09:52 PM, Jayamohan Kallickal wrote:
> >   This patch contains changes to use pci_pools for iscsi hdr
> > instead of pci_alloc_consistent. Here we alloc and free to pool
> > for every IO
> >
> > v3:
> > - Remove cleanup loop in beiscsi_session_destroy
> > - Fixup for allocation failure handling in beiscsi_alloc_pdu
> > - Removed unused variable in beiscsi_session_destroy.
> >
> > Signed-off-by: Jayamohan Kallickal<jayamohank@serverengines.com>
> 
> Thanks for fixing those issues.
> 
> Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>

Actually, this isn't building correctly for me in my 32 bit environment:

drivers/scsi/be2iscsi/be_main.c: In function ‘beiscsi_alloc_pdu’:
drivers/scsi/be2iscsi/be_main.c:2885: warning: passing argument 3 of
‘dma_pool_alloc’ from incompatible pointer type

The problem is this unsigned long long definition of a64: dma_addr_t is
only a long (32 bits) on this platform.

This patch fixes the problem for me ... I'll just fold it in if
everyone's OK with it.

James

---

diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 4299a46..4f1aca3 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -2881,13 +2881,15 @@ static int beiscsi_alloc_pdu(struct iscsi_task *task, uint8_t opcode)
 	struct hwi_controller *phwi_ctrlr;
 	itt_t itt;
 	struct beiscsi_session *beiscsi_sess = beiscsi_conn->beiscsi_sess;
+	dma_addr_t paddr;
 
 	io_task->cmd_bhs = pci_pool_alloc(beiscsi_sess->bhs_pool,
-					  GFP_KERNEL,
-					  &io_task->bhs_pa.u.a64.address);
+					  GFP_KERNEL, &paddr);
+
 	if (!io_task->cmd_bhs)
 		return -ENOMEM;
 
+	io_task->bhs_pa.u.a64.address = paddr;
 	io_task->pwrb_handle = alloc_wrb_handle(phba,
 						beiscsi_conn->beiscsi_conn_cid,
 						task->itt);


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

Re: [PATCH 2/2] be2iscsi: Moving to pci_pools v3

[Mike Christie]

On 09/26/2009 11:30 AM, James Bottomley wrote:
> On Wed, 2009-09-23 at 12:04 -0500, Mike Christie wrote:
>> On 09/21/2009 09:52 PM, Jayamohan Kallickal wrote:
>>>    This patch contains changes to use pci_pools for iscsi hdr
>>> instead of pci_alloc_consistent. Here we alloc and free to pool
>>> for every IO
>>>
>>> v3:
>>> - Remove cleanup loop in beiscsi_session_destroy
>>> - Fixup for allocation failure handling in beiscsi_alloc_pdu
>>> - Removed unused variable in beiscsi_session_destroy.
>>>
>>> Signed-off-by: Jayamohan Kallickal<jayamohank@serverengines.com>
>> Thanks for fixing those issues.
>>
>> Reviewed-by: Mike Christie<michaelc@cs.wisc.edu>
>
> Actually, this isn't building correctly for me in my 32 bit environment:
>
> drivers/scsi/be2iscsi/be_main.c: In function ‘beiscsi_alloc_pdu’:
> drivers/scsi/be2iscsi/be_main.c:2885: warning: passing argument 3 of
> ‘dma_pool_alloc’ from incompatible pointer type
>
> The problem is this unsigned long long definition of a64: dma_addr_t is
> only a long (32 bits) on this platform.
>
> This patch fixes the problem for me ... I'll just fold it in if
> everyone's OK with it.
>
> James
>
> ---
>
> diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
> index 4299a46..4f1aca3 100644
> --- a/drivers/scsi/be2iscsi/be_main.c
> +++ b/drivers/scsi/be2iscsi/be_main.c
> @@ -2881,13 +2881,15 @@ static int beiscsi_alloc_pdu(struct iscsi_task *task, uint8_t opcode)
>   	struct hwi_controller *phwi_ctrlr;
>   	itt_t itt;
>   	struct beiscsi_session *beiscsi_sess = beiscsi_conn->beiscsi_sess;
> +	dma_addr_t paddr;
>
>   	io_task->cmd_bhs = pci_pool_alloc(beiscsi_sess->bhs_pool,
> -					  GFP_KERNEL,
> -					&io_task->bhs_pa.u.a64.address);
> +					  GFP_KERNEL,&paddr);
> +
>   	if (!io_task->cmd_bhs)
>   		return -ENOMEM;
>
> +	io_task->bhs_pa.u.a64.address = paddr;
>   	io_task->pwrb_handle = alloc_wrb_handle(phba,
>   						beiscsi_conn->beiscsi_conn_cid,
>   						task->itt);
>
>

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

Re: [PATCH 2/2] be2iscsi: Moving to pci_pools v3

[Jayamohan Kalickal]

Looks OK to me
-Jay

  _____  
From: James Bottomley [mailto:James.Bottomley@suse.de]
To: Mike Christie [mailto:michaelc@cs.wisc.edu]
Cc: Jayamohan Kalickal [mailto:jayamohank@serverengines.com], linux-scsi@vger.kernel.org
Sent: Sat, 26 Sep 2009 09:30:22 -0700
Subject: Re: [PATCH 2/2] be2iscsi: Moving to pci_pools v3

On Wed, 2009-09-23 at 12:04 -0500, Mike Christie wrote:
> On 09/21/2009 09:52 PM, Jayamohan Kallickal wrote:
> >   This patch contains changes to use pci_pools for iscsi hdr
> > instead of pci_alloc_consistent. Here we alloc and free to pool
> > for every IO
> >
> > v3:
> > - Remove cleanup loop in beiscsi_session_destroy
> > - Fixup for allocation failure handling in beiscsi_alloc_pdu
> > - Removed unused variable in beiscsi_session_destroy.
> >
> > Signed-off-by: Jayamohan Kallickal<jayamohank@serverengines.com>
> 
> Thanks for fixing those issues.
> 
> Reviewed-by: Mike Christie <michaelc@cs.wisc.edu>

Actually, this isn't building correctly for me in my 32 bit environment:

drivers/scsi/be2iscsi/be_main.c: In function ‘beiscsi_alloc_pdu’:
drivers/scsi/be2iscsi/be_main.c:2885: warning: passing argument 3 of
‘dma_pool_alloc’ from incompatible pointer type

The problem is this unsigned long long definition of a64: dma_addr_t is
only a long (32 bits) on this platform.

This patch fixes the problem for me ... I'll just fold it in if
everyone's OK with it.

James

---

diff --git a/drivers/scsi/be2iscsi/be_main.c b/drivers/scsi/be2iscsi/be_main.c
index 4299a46..4f1aca3 100644
--- a/drivers/scsi/be2iscsi/be_main.c
+++ b/drivers/scsi/be2iscsi/be_main.c
@@ -2881,13 +2881,15 @@ static int beiscsi_alloc_pdu(struct iscsi_task *task, uint8_t opcode)
 	struct hwi_controller *phwi_ctrlr;
 	itt_t itt;
 	struct beiscsi_session *beiscsi_sess = beiscsi_conn->beiscsi_sess;
+	dma_addr_t paddr;
 
 	io_task->cmd_bhs = pci_pool_alloc(beiscsi_sess->bhs_pool,
-					  GFP_KERNEL,
-					  &io_task->bhs_pa.u.a64.address);
+					  GFP_KERNEL, &paddr);
+
 	if (!io_task->cmd_bhs)
 		return -ENOMEM;
 
+	io_task->bhs_pa.u.a64.address = paddr;
 	io_task->pwrb_handle = alloc_wrb_handle(phba,
 						beiscsi_conn->beiscsi_conn_cid,
 						task->itt);

___________________________________________________________________________________
This message, together with any attachment(s), contains confidential and proprietary information of
ServerEngines Corporation and is intended only for the designated recipient(s) named above. Any unauthorized
review, printing, retention, copying, disclosure or distribution is strictly prohibited.  If you are not the
intended recipient of this message, please immediately advise the sender by reply email message and
delete all copies of this message and any attachment(s). Thank you.

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