[PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info

[PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info

[kxie]

[PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info.

From: Karen Xie <kxie@chelsio.com>

Remove gl_skb from cxgbi_ddp_info as it is only used by cxgb3i.

Signed-off-by: Karen Xie <kxie@chelsio.com>
---
 drivers/scsi/cxgbi/cxgb3i/cxgb3i.c |   51 +++++-------------------------------
 drivers/scsi/cxgbi/cxgb4i/cxgb4i.c |    2 -
 drivers/scsi/cxgbi/libcxgbi.c      |   15 +----------
 drivers/scsi/cxgbi/libcxgbi.h      |    3 --
 4 files changed, 8 insertions(+), 63 deletions(-)

diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
index a129a17..e2362b9 100644
--- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
+++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
@@ -1108,10 +1108,11 @@ static int ddp_set_map(struct cxgbi_sock *csk, struct cxgbi_pagepod_hdr *hdr,
 		csk, idx, npods, gl);
 
 	for (i = 0; i < npods; i++, idx++, pm_addr += PPOD_SIZE) {
-		struct sk_buff *skb = ddp->gl_skb[idx];
+		struct sk_buff *skb = alloc_wr(sizeof(struct ulp_mem_io) +
+						PPOD_SIZE, 0, GFP_ATOMIC);
 
-		/* hold on to the skb until we clear the ddp mapping */
-		skb_get(skb);
+		if (!skb)
+			return -ENOMEM;
 
 		ulp_mem_io_set_hdr(skb, pm_addr);
 		cxgbi_ddp_ppod_set((struct cxgbi_pagepod *)(skb->head +
@@ -1136,56 +1137,20 @@ static void ddp_clear_map(struct cxgbi_hba *chba, unsigned int tag,
 		cdev, idx, npods, tag);
 
 	for (i = 0; i < npods; i++, idx++, pm_addr += PPOD_SIZE) {
-		struct sk_buff *skb = ddp->gl_skb[idx];
+		struct sk_buff *skb = alloc_wr(sizeof(struct ulp_mem_io) +
+						PPOD_SIZE, 0, GFP_ATOMIC);
 
 		if (!skb) {
-			pr_err("tag 0x%x, 0x%x, %d/%u, skb NULL.\n",
+			pr_err("tag 0x%x, 0x%x, %d/%u, skb OOM.\n",
 				tag, idx, i, npods);
 			continue;
 		}
-		ddp->gl_skb[idx] = NULL;
-		memset(skb->head + sizeof(struct ulp_mem_io), 0, PPOD_SIZE);
 		ulp_mem_io_set_hdr(skb, pm_addr);
 		skb->priority = CPL_PRIORITY_CONTROL;
 		cxgb3_ofld_send(cdev->lldev, skb);
 	}
 }
 
-static void ddp_free_gl_skb(struct cxgbi_ddp_info *ddp, int idx, int cnt)
-{
-	int i;
-
-	log_debug(1 << CXGBI_DBG_DDP,
-		"ddp 0x%p, idx %d, cnt %d.\n", ddp, idx, cnt);
-
-	for (i = 0; i < cnt; i++, idx++)
-		if (ddp->gl_skb[idx]) {
-			kfree_skb(ddp->gl_skb[idx]);
-			ddp->gl_skb[idx] = NULL;
-		}
-}
-
-static int ddp_alloc_gl_skb(struct cxgbi_ddp_info *ddp, int idx,
-				   int cnt, gfp_t gfp)
-{
-	int i;
-
-	log_debug(1 << CXGBI_DBG_DDP,
-		"ddp 0x%p, idx %d, cnt %d.\n", ddp, idx, cnt);
-
-	for (i = 0; i < cnt; i++) {
-		struct sk_buff *skb = alloc_wr(sizeof(struct ulp_mem_io) +
-						PPOD_SIZE, 0, gfp);
-		if (skb)
-			ddp->gl_skb[idx + i] = skb;
-		else {
-			ddp_free_gl_skb(ddp, idx, i);
-			return -ENOMEM;
-		}
-	}
-	return 0;
-}
-
 static int ddp_setup_conn_pgidx(struct cxgbi_sock *csk,
 				       unsigned int tid, int pg_idx, bool reply)
 {
@@ -1316,8 +1281,6 @@ static int cxgb3i_ddp_init(struct cxgbi_device *cdev)
 	}
 	tdev->ulp_iscsi = ddp;
 
-	cdev->csk_ddp_free_gl_skb = ddp_free_gl_skb;
-	cdev->csk_ddp_alloc_gl_skb = ddp_alloc_gl_skb;
 	cdev->csk_ddp_setup_digest = ddp_setup_conn_digest;
 	cdev->csk_ddp_setup_pgidx = ddp_setup_conn_pgidx;
 	cdev->csk_ddp_set = ddp_set_map;
diff --git a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
index 8c04fad..5b1f078 100644
--- a/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
+++ b/drivers/scsi/cxgbi/cxgb4i/cxgb4i.c
@@ -1425,8 +1425,6 @@ static int cxgb4i_ddp_init(struct cxgbi_device *cdev)
 	cxgbi_ddp_page_size_factor(pgsz_factor);
 	cxgb4_iscsi_init(lldi->ports[0], tagmask, pgsz_factor);
 
-	cdev->csk_ddp_free_gl_skb = NULL;
-	cdev->csk_ddp_alloc_gl_skb = NULL;
 	cdev->csk_ddp_setup_digest = ddp_setup_conn_digest;
 	cdev->csk_ddp_setup_pgidx = ddp_setup_conn_pgidx;
 	cdev->csk_ddp_set = ddp_set_map;
diff --git a/drivers/scsi/cxgbi/libcxgbi.c b/drivers/scsi/cxgbi/libcxgbi.c
index 17dcada..bfeb0b6 100644
--- a/drivers/scsi/cxgbi/libcxgbi.c
+++ b/drivers/scsi/cxgbi/libcxgbi.c
@@ -1277,12 +1277,6 @@ static int ddp_tag_reserve(struct cxgbi_sock *csk, unsigned int tid,
 		return idx;
 	}
 
-	if (cdev->csk_ddp_alloc_gl_skb) {
-		err = cdev->csk_ddp_alloc_gl_skb(ddp, idx, npods, gfp);
-		if (err < 0)
-			goto unmark_entries;
-	}
-
 	tag = cxgbi_ddp_tag_base(tformat, sw_tag);
 	tag |= idx << PPOD_IDX_SHIFT;
 
@@ -1293,11 +1287,8 @@ static int ddp_tag_reserve(struct cxgbi_sock *csk, unsigned int tid,
 	hdr.page_offset = htonl(gl->offset);
 
 	err = cdev->csk_ddp_set(csk, &hdr, idx, npods, gl);
-	if (err < 0) {
-		if (cdev->csk_ddp_free_gl_skb)
-			cdev->csk_ddp_free_gl_skb(ddp, idx, npods);
+	if (err < 0)
 		goto unmark_entries;
-	}
 
 	ddp->idx_last = idx;
 	log_debug(1 << CXGBI_DBG_DDP,
@@ -1363,8 +1354,6 @@ static void ddp_destroy(struct kref *kref)
 					>> PPOD_PAGES_SHIFT;
 			pr_info("cdev 0x%p, ddp %d + %d.\n", cdev, i, npods);
 			kfree(gl);
-			if (cdev->csk_ddp_free_gl_skb)
-				cdev->csk_ddp_free_gl_skb(ddp, i, npods);
 			i += npods;
 		} else
 			i++;
@@ -1407,8 +1396,6 @@ int cxgbi_ddp_init(struct cxgbi_device *cdev,
 		return -ENOMEM;
 	}
 	ddp->gl_map = (struct cxgbi_gather_list **)(ddp + 1);
-	ddp->gl_skb = (struct sk_buff **)(((char *)ddp->gl_map) +
-				ppmax * sizeof(struct cxgbi_gather_list *));
 	cdev->ddp = ddp;
 
 	spin_lock_init(&ddp->map_lock);
diff --git a/drivers/scsi/cxgbi/libcxgbi.h b/drivers/scsi/cxgbi/libcxgbi.h
index c57d59d..23cbc58 100644
--- a/drivers/scsi/cxgbi/libcxgbi.h
+++ b/drivers/scsi/cxgbi/libcxgbi.h
@@ -131,7 +131,6 @@ struct cxgbi_ddp_info {
 	unsigned int rsvd_tag_mask;
 	spinlock_t map_lock;
 	struct cxgbi_gather_list **gl_map;
-	struct sk_buff **gl_skb;
 };
 
 #define DDP_PGIDX_MAX		4
@@ -536,8 +535,6 @@ struct cxgbi_device {
 	struct cxgbi_ddp_info *ddp;
 
 	void (*dev_ddp_cleanup)(struct cxgbi_device *);
-	void (*csk_ddp_free_gl_skb)(struct cxgbi_ddp_info *, int, int);
-	int (*csk_ddp_alloc_gl_skb)(struct cxgbi_ddp_info *, int, int, gfp_t);
 	int (*csk_ddp_set)(struct cxgbi_sock *, struct cxgbi_pagepod_hdr *,
 				unsigned int, unsigned int,
 				struct cxgbi_gather_list *);

Re: [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info

[Mike Christie]

On 01/07/2011 04:45 PM, kxie@chelsio.com wrote:
> [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info.
>
> From: Karen Xie<kxie@chelsio.com>
>
> Remove gl_skb from cxgbi_ddp_info as it is only used by cxgb3i.
>
> Signed-off-by: Karen Xie<kxie@chelsio.com>
> ---
>   drivers/scsi/cxgbi/cxgb3i/cxgb3i.c |   51 +++++-------------------------------
>   drivers/scsi/cxgbi/cxgb4i/cxgb4i.c |    2 -
>   drivers/scsi/cxgbi/libcxgbi.c      |   15 +----------
>   drivers/scsi/cxgbi/libcxgbi.h      |    3 --
>   4 files changed, 8 insertions(+), 63 deletions(-)
>
> diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
> index a129a17..e2362b9 100644
> --- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
> +++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
> @@ -1108,10 +1108,11 @@ static int ddp_set_map(struct cxgbi_sock *csk, struct cxgbi_pagepod_hdr *hdr,
>   		csk, idx, npods, gl);
>
>   	for (i = 0; i<  npods; i++, idx++, pm_addr += PPOD_SIZE) {
> -		struct sk_buff *skb = ddp->gl_skb[idx];
> +		struct sk_buff *skb = alloc_wr(sizeof(struct ulp_mem_io) +
> +						PPOD_SIZE, 0, GFP_ATOMIC);


I think you want to try to avoid lots of little GFP_ATOMIC allocations 
in the main IO path, because it probably is bad for performance and 
because they can fail and you can be stuck with no mem but other 
allocations needing to write data out.

Did you want to just make each driver allocate a pool/map, then allocate 
from that pool/map in these places (cxgb4i does a similar skb allocation 
at these points right?)?

Re: [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info

[Mike Christie]

On 01/07/2011 05:42 PM, Mike Christie wrote:
> On 01/07/2011 04:45 PM, kxie@chelsio.com wrote:
>> [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info.
>>
>> From: Karen Xie<kxie@chelsio.com>
>>
>> Remove gl_skb from cxgbi_ddp_info as it is only used by cxgb3i.
>>
>> Signed-off-by: Karen Xie<kxie@chelsio.com>
>> ---
>> drivers/scsi/cxgbi/cxgb3i/cxgb3i.c | 51
>> +++++-------------------------------
>> drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 2 -
>> drivers/scsi/cxgbi/libcxgbi.c | 15 +----------
>> drivers/scsi/cxgbi/libcxgbi.h | 3 --
>> 4 files changed, 8 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>> b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>> index a129a17..e2362b9 100644
>> --- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>> +++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>> @@ -1108,10 +1108,11 @@ static int ddp_set_map(struct cxgbi_sock *csk,
>> struct cxgbi_pagepod_hdr *hdr,
>> csk, idx, npods, gl);
>>
>> for (i = 0; i< npods; i++, idx++, pm_addr += PPOD_SIZE) {
>> - struct sk_buff *skb = ddp->gl_skb[idx];
>> + struct sk_buff *skb = alloc_wr(sizeof(struct ulp_mem_io) +
>> + PPOD_SIZE, 0, GFP_ATOMIC);
>
>
> I think you want to try to avoid lots of little GFP_ATOMIC allocations
> in the main IO path, because it probably is bad for performance and
> because they can fail and you can be stuck with no mem but other
> allocations needing to write data out.
>
> Did you want to just make each driver allocate a pool/map, then allocate
> from that pool/map in these places (cxgb4i does a similar skb allocation
> at these points right?)?
>

Oh yeah, is the complication that the cxgb3i driver uses 1 size for the 
objects but cxgb4i is variable?

RE: [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info

[Karen Xie]

Hi, Mike,

Yes, the message size is different between cxgb3i and cxgb4i. I am not
using the pool mainly because this is on the tx side and tx skb
recycling does not seem to be utilized much.

Karen

-----Original Message-----
From: Mike Christie [mailto:michaelc@cs.wisc.edu] 
Sent: Friday, January 07, 2011 3:48 PM
To: open-iscsi@googlegroups.com
Cc: Karen Xie; linux-scsi@vger.kernel.org;
James.Bottomley@HansenPartnership.com
Subject: Re: [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info

On 01/07/2011 05:42 PM, Mike Christie wrote:
> On 01/07/2011 04:45 PM, kxie@chelsio.com wrote:
>> [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info.
>>
>> From: Karen Xie<kxie@chelsio.com>
>>
>> Remove gl_skb from cxgbi_ddp_info as it is only used by cxgb3i.
>>
>> Signed-off-by: Karen Xie<kxie@chelsio.com>
>> ---
>> drivers/scsi/cxgbi/cxgb3i/cxgb3i.c | 51
>> +++++-------------------------------
>> drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 2 -
>> drivers/scsi/cxgbi/libcxgbi.c | 15 +----------
>> drivers/scsi/cxgbi/libcxgbi.h | 3 --
>> 4 files changed, 8 insertions(+), 63 deletions(-)
>>
>> diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>> b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>> index a129a17..e2362b9 100644
>> --- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>> +++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>> @@ -1108,10 +1108,11 @@ static int ddp_set_map(struct cxgbi_sock
*csk,
>> struct cxgbi_pagepod_hdr *hdr,
>> csk, idx, npods, gl);
>>
>> for (i = 0; i< npods; i++, idx++, pm_addr += PPOD_SIZE) {
>> - struct sk_buff *skb = ddp->gl_skb[idx];
>> + struct sk_buff *skb = alloc_wr(sizeof(struct ulp_mem_io) +
>> + PPOD_SIZE, 0, GFP_ATOMIC);
>
>
> I think you want to try to avoid lots of little GFP_ATOMIC allocations
> in the main IO path, because it probably is bad for performance and
> because they can fail and you can be stuck with no mem but other
> allocations needing to write data out.
>
> Did you want to just make each driver allocate a pool/map, then
allocate
> from that pool/map in these places (cxgb4i does a similar skb
allocation
> at these points right?)?
>

Oh yeah, is the complication that the cxgb3i driver uses 1 size for the 
objects but cxgb4i is variable?

Re: [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info

[Mike Christie]

On 01/07/2011 06:09 PM, Karen Xie wrote:
> Hi, Mike,
>
> Yes, the message size is different between cxgb3i and cxgb4i. I am not

I am not sure if we are talking about the same thing. Mostly I am 
confused a little :)

For pool I just mean gl_skb. So I mean cxgb3i could make its own pool 
like it is now. Basically just move gl_skb to some cxgb3i struct and mv 
ddp_alloc_gl_skb related code to cxgb3i.

Then cxgb4i could do something similar if possible. Below when I was 
asking about variable sizes I meant that for cxgb4i it seemed harder 
because ddp_set_map->ddp_ppod_write_idata->alloc_wr allocates the skb 
based on wr_len.

> using the pool mainly because this is on the tx side and tx skb
> recycling does not seem to be utilized much.
>

What pool are you talking about? Some sort of skb pool from the network 
layer?

> Karen
>
> -----Original Message-----
> From: Mike Christie [mailto:michaelc@cs.wisc.edu]
> Sent: Friday, January 07, 2011 3:48 PM
> To: open-iscsi@googlegroups.com
> Cc: Karen Xie; linux-scsi@vger.kernel.org;
> James.Bottomley@HansenPartnership.com
> Subject: Re: [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info
>
> On 01/07/2011 05:42 PM, Mike Christie wrote:
>> On 01/07/2011 04:45 PM, kxie@chelsio.com wrote:
>>> [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info.
>>>
>>> From: Karen Xie<kxie@chelsio.com>
>>>
>>> Remove gl_skb from cxgbi_ddp_info as it is only used by cxgb3i.
>>>
>>> Signed-off-by: Karen Xie<kxie@chelsio.com>
>>> ---
>>> drivers/scsi/cxgbi/cxgb3i/cxgb3i.c | 51
>>> +++++-------------------------------
>>> drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 2 -
>>> drivers/scsi/cxgbi/libcxgbi.c | 15 +----------
>>> drivers/scsi/cxgbi/libcxgbi.h | 3 --
>>> 4 files changed, 8 insertions(+), 63 deletions(-)
>>>
>>> diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>>> b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>>> index a129a17..e2362b9 100644
>>> --- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>>> +++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>>> @@ -1108,10 +1108,11 @@ static int ddp_set_map(struct cxgbi_sock
> *csk,
>>> struct cxgbi_pagepod_hdr *hdr,
>>> csk, idx, npods, gl);
>>>
>>> for (i = 0; i<  npods; i++, idx++, pm_addr += PPOD_SIZE) {
>>> - struct sk_buff *skb = ddp->gl_skb[idx];
>>> + struct sk_buff *skb = alloc_wr(sizeof(struct ulp_mem_io) +
>>> + PPOD_SIZE, 0, GFP_ATOMIC);
>>
>>
>> I think you want to try to avoid lots of little GFP_ATOMIC allocations
>> in the main IO path, because it probably is bad for performance and
>> because they can fail and you can be stuck with no mem but other
>> allocations needing to write data out.
>>
>> Did you want to just make each driver allocate a pool/map, then
> allocate
>> from that pool/map in these places (cxgb4i does a similar skb
> allocation
>> at these points right?)?
>>
>
> Oh yeah, is the complication that the cxgb3i driver uses 1 size for the
> objects but cxgb4i is variable?
> --
> 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] cxgbi: get rid of gl_skb in cxgbi_ddp_info

[Karen Xie]

Hi, Mike,

What I mean is even if cxgb3i maintains a pool of skbs (equivalent of
gl_skbs). Once they are handed down to the network layer cxgb3 they will
still be freed by the network driver (on the tx path).

Karen

-----Original Message-----
From: Mike Christie [mailto:michaelc@cs.wisc.edu] 
Sent: Friday, January 07, 2011 8:23 PM
To: Karen Xie
Cc: open-iscsi@googlegroups.com; linux-scsi@vger.kernel.org;
James.Bottomley@HansenPartnership.com
Subject: Re: [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info

On 01/07/2011 06:09 PM, Karen Xie wrote:
> Hi, Mike,
>
> Yes, the message size is different between cxgb3i and cxgb4i. I am not

I am not sure if we are talking about the same thing. Mostly I am 
confused a little :)

For pool I just mean gl_skb. So I mean cxgb3i could make its own pool 
like it is now. Basically just move gl_skb to some cxgb3i struct and mv 
ddp_alloc_gl_skb related code to cxgb3i.

Then cxgb4i could do something similar if possible. Below when I was 
asking about variable sizes I meant that for cxgb4i it seemed harder 
because ddp_set_map->ddp_ppod_write_idata->alloc_wr allocates the skb 
based on wr_len.

> using the pool mainly because this is on the tx side and tx skb
> recycling does not seem to be utilized much.
>

What pool are you talking about? Some sort of skb pool from the network 
layer?

> Karen
>
> -----Original Message-----
> From: Mike Christie [mailto:michaelc@cs.wisc.edu]
> Sent: Friday, January 07, 2011 3:48 PM
> To: open-iscsi@googlegroups.com
> Cc: Karen Xie; linux-scsi@vger.kernel.org;
> James.Bottomley@HansenPartnership.com
> Subject: Re: [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info
>
> On 01/07/2011 05:42 PM, Mike Christie wrote:
>> On 01/07/2011 04:45 PM, kxie@chelsio.com wrote:
>>> [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info.
>>>
>>> From: Karen Xie<kxie@chelsio.com>
>>>
>>> Remove gl_skb from cxgbi_ddp_info as it is only used by cxgb3i.
>>>
>>> Signed-off-by: Karen Xie<kxie@chelsio.com>
>>> ---
>>> drivers/scsi/cxgbi/cxgb3i/cxgb3i.c | 51
>>> +++++-------------------------------
>>> drivers/scsi/cxgbi/cxgb4i/cxgb4i.c | 2 -
>>> drivers/scsi/cxgbi/libcxgbi.c | 15 +----------
>>> drivers/scsi/cxgbi/libcxgbi.h | 3 --
>>> 4 files changed, 8 insertions(+), 63 deletions(-)
>>>
>>> diff --git a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>>> b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>>> index a129a17..e2362b9 100644
>>> --- a/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>>> +++ b/drivers/scsi/cxgbi/cxgb3i/cxgb3i.c
>>> @@ -1108,10 +1108,11 @@ static int ddp_set_map(struct cxgbi_sock
> *csk,
>>> struct cxgbi_pagepod_hdr *hdr,
>>> csk, idx, npods, gl);
>>>
>>> for (i = 0; i<  npods; i++, idx++, pm_addr += PPOD_SIZE) {
>>> - struct sk_buff *skb = ddp->gl_skb[idx];
>>> + struct sk_buff *skb = alloc_wr(sizeof(struct ulp_mem_io) +
>>> + PPOD_SIZE, 0, GFP_ATOMIC);
>>
>>
>> I think you want to try to avoid lots of little GFP_ATOMIC
allocations
>> in the main IO path, because it probably is bad for performance and
>> because they can fail and you can be stuck with no mem but other
>> allocations needing to write data out.
>>
>> Did you want to just make each driver allocate a pool/map, then
> allocate
>> from that pool/map in these places (cxgb4i does a similar skb
> allocation
>> at these points right?)?
>>
>
> Oh yeah, is the complication that the cxgb3i driver uses 1 size for
the
> objects but cxgb4i is variable?
> --
> 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] cxgbi: get rid of gl_skb in cxgbi_ddp_info

[Mike Christie]

On 01/10/2011 11:52 AM, Karen Xie wrote:
> Hi, Mike,
>
> What I mean is even if cxgb3i maintains a pool of skbs (equivalent of
> gl_skbs). Once they are handed down to the network layer cxgb3 they will
> still be freed by the network driver (on the tx path).
>

So hows does it work now? It seems ddp_alloc_gl_skb is called at device 
setup time, then when IO is queued/xmited we run ddp_set_map which 
allocates from gl_skbs. If when you transmit the IO, the driver frees 
the skb, how does gl_skb get replenished?

RE: [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info

[Karen Xie]

Yes, that's bug this patch is fixing too. The current implementation
does not replenish the gl_skb after the initial allocation.

I am working on an update which utilizing a pool of skbs, and those skbs
would be used not only for the ddp setups (like the current gl_skb) but
also for the connection management like the rx credit returns. So this
skb pool will not reside in cxgbi_ddp_info, so still need to get rid of
gl_skb. I guess this patch can be viewed as an interim fix.

-----Original Message-----
From: Mike Christie [mailto:michaelc-hcNo3dDEHLuVc3sceRu5cw@public.gmane.org] 
Sent: Monday, January 10, 2011 11:57 AM
To: Karen Xie
Cc: open-iscsi-/JYPxA39Uh5TLH3MbocFFw@public.gmane.org; linux-scsi-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org
Subject: Re: [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info

On 01/10/2011 11:52 AM, Karen Xie wrote:
> Hi, Mike,
>
> What I mean is even if cxgb3i maintains a pool of skbs (equivalent of
> gl_skbs). Once they are handed down to the network layer cxgb3 they
will
> still be freed by the network driver (on the tx path).
>

So hows does it work now? It seems ddp_alloc_gl_skb is called at device 
setup time, then when IO is queued/xmited we run ddp_set_map which 
allocates from gl_skbs. If when you transmit the IO, the driver frees 
the skb, how does gl_skb get replenished?

-- 
You received this message because you are subscribed to the Google Groups "open-iscsi" group.
To post to this group, send email to open-iscsi-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
To unsubscribe from this group, send email to open-iscsi+unsubscribe-/JYPxA39Uh5TLH3MbocFF+G/Ez6ZCGd0@public.gmane.org
For more options, visit this group at http://groups.google.com/group/open-iscsi?hl=en.

Re: [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info

[Mike Christie]

On 01/10/2011 04:57 PM, Karen Xie wrote:
> Yes, that's bug this patch is fixing too. The current implementation
> does not replenish the gl_skb after the initial allocation.
>
> I am working on an update which utilizing a pool of skbs, and those skbs
> would be used not only for the ddp setups (like the current gl_skb) but
> also for the connection management like the rx credit returns. So this
> skb pool will not reside in cxgbi_ddp_info, so still need to get rid of
> gl_skb. I guess this patch can be viewed as an interim fix.
>

Ok with me then.

Re: [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info

[Mike Christie]

On 01/07/2011 04:45 PM, kxie@chelsio.com wrote:
> [PATCH 2/2] cxgbi: get rid of gl_skb in cxgbi_ddp_info.
>
> From: Karen Xie<kxie@chelsio.com>
>
> Remove gl_skb from cxgbi_ddp_info as it is only used by cxgb3i.
>
> Signed-off-by: Karen Xie<kxie@chelsio.com>


Ok for now.

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