All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] IB/usnic: Handle 0 counts in resource allocation
@ 2015-12-09 18:42 Nelson Escobar
       [not found] ` <1449686539-29959-6-git-send-email-neescoba-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Nelson Escobar @ 2015-12-09 18:42 UTC (permalink / raw)
  To: linux-rdma-u79uwXL29TY76Z2rM5mHXA; +Cc: Nelson Escobar

Signed-off-by: Dave Goodell <dgoodell-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Reese Faucette <rfaucett-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
Reviewed-by: Xuyang Wang <xuywang-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
Signed-off-by: Nelson Escobar <neescoba-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
---
 drivers/infiniband/hw/usnic/usnic_vnic.c | 54 +++++++++++++++++---------------
 1 file changed, 29 insertions(+), 25 deletions(-)

diff --git a/drivers/infiniband/hw/usnic/usnic_vnic.c b/drivers/infiniband/hw/usnic/usnic_vnic.c
index 66de93f..8875107 100644
--- a/drivers/infiniband/hw/usnic/usnic_vnic.c
+++ b/drivers/infiniband/hw/usnic/usnic_vnic.c
@@ -237,7 +237,7 @@ usnic_vnic_get_resources(struct usnic_vnic *vnic, enum usnic_vnic_res_type type,
 	struct usnic_vnic_res *res;
 	int i;
 
-	if (usnic_vnic_res_free_cnt(vnic, type) < cnt || cnt < 1 || !owner)
+	if (usnic_vnic_res_free_cnt(vnic, type) < cnt || cnt < 0 || !owner)
 		return ERR_PTR(-EINVAL);
 
 	ret = kzalloc(sizeof(*ret), GFP_ATOMIC);
@@ -247,26 +247,28 @@ usnic_vnic_get_resources(struct usnic_vnic *vnic, enum usnic_vnic_res_type type,
 		return ERR_PTR(-ENOMEM);
 	}
 
-	ret->res = kzalloc(sizeof(*(ret->res))*cnt, GFP_ATOMIC);
-	if (!ret->res) {
-		usnic_err("Failed to allocate resources for %s. Out of memory\n",
-				usnic_vnic_pci_name(vnic));
-		kfree(ret);
-		return ERR_PTR(-ENOMEM);
-	}
+	if (cnt > 0) {
+		ret->res = kcalloc(cnt, sizeof(*(ret->res)), GFP_ATOMIC);
+		if (!ret->res) {
+			usnic_err("Failed to allocate resources for %s. Out of memory\n",
+					usnic_vnic_pci_name(vnic));
+			kfree(ret);
+			return ERR_PTR(-ENOMEM);
+		}
 
-	spin_lock(&vnic->res_lock);
-	src = &vnic->chunks[type];
-	for (i = 0; i < src->cnt && ret->cnt < cnt; i++) {
-		res = src->res[i];
-		if (!res->owner) {
-			src->free_cnt--;
-			res->owner = owner;
-			ret->res[ret->cnt++] = res;
+		spin_lock(&vnic->res_lock);
+		src = &vnic->chunks[type];
+		for (i = 0; i < src->cnt && ret->cnt < cnt; i++) {
+			res = src->res[i];
+			if (!res->owner) {
+				src->free_cnt--;
+				res->owner = owner;
+				ret->res[ret->cnt++] = res;
+			}
 		}
-	}
 
-	spin_unlock(&vnic->res_lock);
+		spin_unlock(&vnic->res_lock);
+	}
 	ret->type = type;
 	ret->vnic = vnic;
 	WARN_ON(ret->cnt != cnt);
@@ -281,14 +283,16 @@ void usnic_vnic_put_resources(struct usnic_vnic_res_chunk *chunk)
 	int i;
 	struct usnic_vnic *vnic = chunk->vnic;
 
-	spin_lock(&vnic->res_lock);
-	while ((i = --chunk->cnt) >= 0) {
-		res = chunk->res[i];
-		chunk->res[i] = NULL;
-		res->owner = NULL;
-		vnic->chunks[res->type].free_cnt++;
+	if (chunk->cnt > 0) {
+		spin_lock(&vnic->res_lock);
+		while ((i = --chunk->cnt) >= 0) {
+			res = chunk->res[i];
+			chunk->res[i] = NULL;
+			res->owner = NULL;
+			vnic->chunks[res->type].free_cnt++;
+		}
+		spin_unlock(&vnic->res_lock);
 	}
-	spin_unlock(&vnic->res_lock);
 
 	kfree(chunk->res);
 	kfree(chunk);
-- 
2.4.3

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

^ permalink raw reply related	[flat|nested] 7+ messages in thread

* Re: [PATCH] IB/usnic: Handle 0 counts in resource allocation
       [not found] ` <1449686539-29959-6-git-send-email-neescoba-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
@ 2015-12-10  6:47   ` Leon Romanovsky
       [not found]     ` <20151210064702.GC8662-2ukJVAZIZ/Y@public.gmane.org>
  2015-12-23 19:48   ` Doug Ledford
  1 sibling, 1 reply; 7+ messages in thread
From: Leon Romanovsky @ 2015-12-10  6:47 UTC (permalink / raw)
  To: Nelson Escobar; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Wed, Dec 09, 2015 at 10:42:19AM -0800, Nelson Escobar wrote:
> -	if (usnic_vnic_res_free_cnt(vnic, type) < cnt || cnt < 1 || !owner)
> +	if (usnic_vnic_res_free_cnt(vnic, type) < cnt || cnt < 0 || !owner)
Before this change you returned EINVAL if no free_cnt were available,
now you will continue. is this behaviour expected?

>  		return ERR_PTR(-EINVAL);
>  
>  	ret = kzalloc(sizeof(*ret), GFP_ATOMIC);
> @@ -247,26 +247,28 @@ usnic_vnic_get_resources(struct usnic_vnic *vnic, enum usnic_vnic_res_type type,
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> -	ret->res = kzalloc(sizeof(*(ret->res))*cnt, GFP_ATOMIC);
> -	if (!ret->res) {
> -		usnic_err("Failed to allocate resources for %s. Out of memory\n",
> -				usnic_vnic_pci_name(vnic));
> -		kfree(ret);
> -		return ERR_PTR(-ENOMEM);
> -	}
> +	if (cnt > 0) {
> +		ret->res = kcalloc(cnt, sizeof(*(ret->res)), GFP_ATOMIC);
> +		if (!ret->res) {
> +			usnic_err("Failed to allocate resources for %s. Out of memory\n",
> +					usnic_vnic_pci_name(vnic));
You don't need to print OOM messages, failure in memory allocation very hard to miss.
> +			kfree(ret);
> +			return ERR_PTR(-ENOMEM);
> +		}
>  
> -	spin_lock(&vnic->res_lock);
> -	src = &vnic->chunks[type];
> -	for (i = 0; i < src->cnt && ret->cnt < cnt; i++) {
> -		res = src->res[i];
> -		if (!res->owner) {
> -			src->free_cnt--;
> -			res->owner = owner;
> -			ret->res[ret->cnt++] = res;
> +		spin_lock(&vnic->res_lock);
> +		src = &vnic->chunks[type];
> +		for (i = 0; i < src->cnt && ret->cnt < cnt; i++) {
> +			res = src->res[i];
> +			if (!res->owner) {
> +				src->free_cnt--;
It will be negative, because of skip usnic_vnic_res_free_cnt check
before.
> +				res->owner = owner;
> +				ret->res[ret->cnt++] = res;
> +			}
>  		}
> -	}
>  
> -	spin_unlock(&vnic->res_lock);
> +		spin_unlock(&vnic->res_lock);
> +	}
>  	ret->type = type;
>  	ret->vnic = vnic;
>  	WARN_ON(ret->cnt != cnt);
> @@ -281,14 +283,16 @@ void usnic_vnic_put_resources(struct usnic_vnic_res_chunk *chunk)
>  	int i;
>  	struct usnic_vnic *vnic = chunk->vnic;
>  
> -	spin_lock(&vnic->res_lock);
> -	while ((i = --chunk->cnt) >= 0) {
> -		res = chunk->res[i];
> -		chunk->res[i] = NULL;
> -		res->owner = NULL;
> -		vnic->chunks[res->type].free_cnt++;
> +	if (chunk->cnt > 0) {
> +		spin_lock(&vnic->res_lock);
> +		while ((i = --chunk->cnt) >= 0) {
> +			res = chunk->res[i];
> +			chunk->res[i] = NULL;
> +			res->owner = NULL;
> +			vnic->chunks[res->type].free_cnt++;
> +		}
> +		spin_unlock(&vnic->res_lock);
>  	}
> -	spin_unlock(&vnic->res_lock);
>  
>  	kfree(chunk->res);
>  	kfree(chunk);
> -- 
> 2.4.3
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] IB/usnic: Handle 0 counts in resource allocation
       [not found]     ` <20151210064702.GC8662-2ukJVAZIZ/Y@public.gmane.org>
@ 2015-12-10 19:22       ` Nelson Escobar
       [not found]         ` <5669D0F0.7010104-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Nelson Escobar @ 2015-12-10 19:22 UTC (permalink / raw)
  To: leon-2ukJVAZIZ/Y; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

On 12/9/2015 10:47 PM, Leon Romanovsky wrote:
> On Wed, Dec 09, 2015 at 10:42:19AM -0800, Nelson Escobar wrote:
>> -	if (usnic_vnic_res_free_cnt(vnic, type) < cnt || cnt < 1 || !owner)
>> +	if (usnic_vnic_res_free_cnt(vnic, type) < cnt || cnt < 0 || !owner)
> Before this change you returned EINVAL if no free_cnt were available,
> now you will continue. is this behaviour expected?
Yes.  If cnt is 0, then no resources are being requested, so it is OK if
there are no resources available.
> 
>>  		return ERR_PTR(-EINVAL);
>>  
>>  	ret = kzalloc(sizeof(*ret), GFP_ATOMIC);
>> @@ -247,26 +247,28 @@ usnic_vnic_get_resources(struct usnic_vnic *vnic, enum usnic_vnic_res_type type,
>>  		return ERR_PTR(-ENOMEM);
>>  	}
>>  
>> -	ret->res = kzalloc(sizeof(*(ret->res))*cnt, GFP_ATOMIC);
>> -	if (!ret->res) {
>> -		usnic_err("Failed to allocate resources for %s. Out of memory\n",
>> -				usnic_vnic_pci_name(vnic));
>> -		kfree(ret);
>> -		return ERR_PTR(-ENOMEM);
>> -	}
>> +	if (cnt > 0) {
>> +		ret->res = kcalloc(cnt, sizeof(*(ret->res)), GFP_ATOMIC);
>> +		if (!ret->res) {
>> +			usnic_err("Failed to allocate resources for %s. Out of memory\n",
>> +					usnic_vnic_pci_name(vnic));
> You don't need to print OOM messages, failure in memory allocation very hard to miss.
OOM messages are hard to miss, but this message is already in upstream
and outside the scope of this patch.
>> +			kfree(ret);
>> +			return ERR_PTR(-ENOMEM);
>> +		}
>>  
>> -	spin_lock(&vnic->res_lock);
>> -	src = &vnic->chunks[type];
>> -	for (i = 0; i < src->cnt && ret->cnt < cnt; i++) {
>> -		res = src->res[i];
>> -		if (!res->owner) {
>> -			src->free_cnt--;
>> -			res->owner = owner;
>> -			ret->res[ret->cnt++] = res;
>> +		spin_lock(&vnic->res_lock);
>> +		src = &vnic->chunks[type];
>> +		for (i = 0; i < src->cnt && ret->cnt < cnt; i++) {
>> +			res = src->res[i];
>> +			if (!res->owner) {
>> +				src->free_cnt--;
> It will be negative, because of skip usnic_vnic_res_free_cnt check
> before.
We are inside the 'if (cnt > 0)' clause here, so the previous
usnic_vnic_res_free_cnt check wasn't skipped.
>> +				res->owner = owner;
>> +				ret->res[ret->cnt++] = res;
>> +			}
>>  		}
>> -	}
>>  
>> -	spin_unlock(&vnic->res_lock);
>> +		spin_unlock(&vnic->res_lock);
>> +	}
>>  	ret->type = type;
>>  	ret->vnic = vnic;
>>  	WARN_ON(ret->cnt != cnt);
>> @@ -281,14 +283,16 @@ void usnic_vnic_put_resources(struct usnic_vnic_res_chunk *chunk)
>>  	int i;
>>  	struct usnic_vnic *vnic = chunk->vnic;
>>  
>> -	spin_lock(&vnic->res_lock);
>> -	while ((i = --chunk->cnt) >= 0) {
>> -		res = chunk->res[i];
>> -		chunk->res[i] = NULL;
>> -		res->owner = NULL;
>> -		vnic->chunks[res->type].free_cnt++;
>> +	if (chunk->cnt > 0) {
>> +		spin_lock(&vnic->res_lock);
>> +		while ((i = --chunk->cnt) >= 0) {
>> +			res = chunk->res[i];
>> +			chunk->res[i] = NULL;
>> +			res->owner = NULL;
>> +			vnic->chunks[res->type].free_cnt++;
>> +		}
>> +		spin_unlock(&vnic->res_lock);
>>  	}
>> -	spin_unlock(&vnic->res_lock);
>>  
>>  	kfree(chunk->res);
>>  	kfree(chunk);
>> -- 
>> 2.4.3
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] IB/usnic: Handle 0 counts in resource allocation
       [not found]         ` <5669D0F0.7010104-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
@ 2015-12-10 19:47           ` leon-2ukJVAZIZ/Y
       [not found]             ` <20151210194734.GF8662-2ukJVAZIZ/Y@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: leon-2ukJVAZIZ/Y @ 2015-12-10 19:47 UTC (permalink / raw)
  To: Nelson Escobar; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

On Thu, Dec 10, 2015 at 11:22:24AM -0800, Nelson Escobar wrote:
> On 12/9/2015 10:47 PM, Leon Romanovsky wrote:
> > On Wed, Dec 09, 2015 at 10:42:19AM -0800, Nelson Escobar wrote:
> >> -	if (usnic_vnic_res_free_cnt(vnic, type) < cnt || cnt < 1 || !owner)
> >> +	if (usnic_vnic_res_free_cnt(vnic, type) < cnt || cnt < 0 || !owner)
> > Before this change you returned EINVAL if no free_cnt were available,
> > now you will continue. is this behaviour expected?
> Yes.  If cnt is 0, then no resources are being requested, so it is OK if
> there are no resources available.
I afraid that you missed the point.
Old code:
usnic_vnic_res_free_cnt(vnic, type) == 0 and cnt == 1 will return EINVAL
New code
snic_vnic_res_free_cnt(vnic, type) == 0 and cnt == 1 will pass and will
pass te "if (cnt > 0)" check below and will decrease free_cnt variable
to be below zero.

Is this behavior expected?
> > 
> >>  		return ERR_PTR(-EINVAL);
> >>  
> >>  	ret = kzalloc(sizeof(*ret), GFP_ATOMIC);
> >> @@ -247,26 +247,28 @@ usnic_vnic_get_resources(struct usnic_vnic *vnic, enum usnic_vnic_res_type type,
> >>  		return ERR_PTR(-ENOMEM);
> >>  	}
> >>  
> >> -	ret->res = kzalloc(sizeof(*(ret->res))*cnt, GFP_ATOMIC);
> >> -	if (!ret->res) {
> >> -		usnic_err("Failed to allocate resources for %s. Out of memory\n",
> >> -				usnic_vnic_pci_name(vnic));
> >> -		kfree(ret);
> >> -		return ERR_PTR(-ENOMEM);
> >> -	}
> >> +	if (cnt > 0) {
> >> +		ret->res = kcalloc(cnt, sizeof(*(ret->res)), GFP_ATOMIC);
> >> +		if (!ret->res) {
> >> +			usnic_err("Failed to allocate resources for %s. Out of memory\n",
> >> +					usnic_vnic_pci_name(vnic));
> > You don't need to print OOM messages, failure in memory allocation very hard to miss.
> OOM messages are hard to miss, but this message is already in upstream
> and outside the scope of this patch.
It is worth to fix, especially if you are changing these exact lines.
> >> +			kfree(ret);
> >> +			return ERR_PTR(-ENOMEM);
> >> +		}
> >>  
> >> -	spin_lock(&vnic->res_lock);
> >> -	src = &vnic->chunks[type];
> >> -	for (i = 0; i < src->cnt && ret->cnt < cnt; i++) {
> >> -		res = src->res[i];
> >> -		if (!res->owner) {
> >> -			src->free_cnt--;
> >> -			res->owner = owner;
> >> -			ret->res[ret->cnt++] = res;
> >> +		spin_lock(&vnic->res_lock);
> >> +		src = &vnic->chunks[type];
> >> +		for (i = 0; i < src->cnt && ret->cnt < cnt; i++) {
> >> +			res = src->res[i];
> >> +			if (!res->owner) {
> >> +				src->free_cnt--;
> > It will be negative, because of skip usnic_vnic_res_free_cnt check
> > before.
> We are inside the 'if (cnt > 0)' clause here, so the previous
> usnic_vnic_res_free_cnt check wasn't skipped.
See above.

> >> +				res->owner = owner;
> >> +				ret->res[ret->cnt++] = res;
> >> +			}
> >>  		}
> >> -	}
> >>  
> >> -	spin_unlock(&vnic->res_lock);
> >> +		spin_unlock(&vnic->res_lock);
> >> +	}
> >>  	ret->type = type;
> >>  	ret->vnic = vnic;
> >>  	WARN_ON(ret->cnt != cnt);
> >> @@ -281,14 +283,16 @@ void usnic_vnic_put_resources(struct usnic_vnic_res_chunk *chunk)
> >>  	int i;
> >>  	struct usnic_vnic *vnic = chunk->vnic;
> >>  
> >> -	spin_lock(&vnic->res_lock);
> >> -	while ((i = --chunk->cnt) >= 0) {
> >> -		res = chunk->res[i];
> >> -		chunk->res[i] = NULL;
> >> -		res->owner = NULL;
> >> -		vnic->chunks[res->type].free_cnt++;
> >> +	if (chunk->cnt > 0) {
> >> +		spin_lock(&vnic->res_lock);
> >> +		while ((i = --chunk->cnt) >= 0) {
> >> +			res = chunk->res[i];
> >> +			chunk->res[i] = NULL;
> >> +			res->owner = NULL;
> >> +			vnic->chunks[res->type].free_cnt++;
> >> +		}
> >> +		spin_unlock(&vnic->res_lock);
> >>  	}
> >> -	spin_unlock(&vnic->res_lock);
> >>  
> >>  	kfree(chunk->res);
> >>  	kfree(chunk);
> >> -- 
> >> 2.4.3
> >>
> >> --
> >> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> >> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
> >> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] IB/usnic: Handle 0 counts in resource allocation
       [not found]             ` <20151210194734.GF8662-2ukJVAZIZ/Y@public.gmane.org>
@ 2015-12-10 21:46               ` Nelson Escobar
       [not found]                 ` <5669F2A7.8060502-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
  0 siblings, 1 reply; 7+ messages in thread
From: Nelson Escobar @ 2015-12-10 21:46 UTC (permalink / raw)
  To: leon-2ukJVAZIZ/Y; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA



On 12/10/2015 11:47 AM, leon-2ukJVAZIZ/Y@public.gmane.org wrote:
> On Thu, Dec 10, 2015 at 11:22:24AM -0800, Nelson Escobar wrote:
>> On 12/9/2015 10:47 PM, Leon Romanovsky wrote:
>>> On Wed, Dec 09, 2015 at 10:42:19AM -0800, Nelson Escobar wrote:
>>>> -	if (usnic_vnic_res_free_cnt(vnic, type) < cnt || cnt < 1 || !owner)
>>>> +	if (usnic_vnic_res_free_cnt(vnic, type) < cnt || cnt < 0 || !owner)
>>> Before this change you returned EINVAL if no free_cnt were available,
>>> now you will continue. is this behaviour expected?
>> Yes.  If cnt is 0, then no resources are being requested, so it is OK if
>> there are no resources available.
> I afraid that you missed the point.
Thanks for looking at the code.  I am still not understanding your point.
> Old code:
> usnic_vnic_res_free_cnt(vnic, type) == 0 and cnt == 1 will return EINVAL
Yes:
	if (0 < 1 || 1 < 1 || !owner)
		return -EINVAL;
> New code
> snic_vnic_res_free_cnt(vnic, type) == 0 and cnt == 1 will pass and will
> pass te "if (cnt > 0)" check below and will decrease free_cnt variable
> to be below zero.
This I don't understand.  The following still fails with -EINVAL.
	if (0 < 1 || 1 < 0 || !owner)
		return -EINVAL;
> 
> Is this behavior expected?
>>>
>>>>  		return ERR_PTR(-EINVAL);
>>>>  
>>>>  	ret = kzalloc(sizeof(*ret), GFP_ATOMIC);
>>>> @@ -247,26 +247,28 @@ usnic_vnic_get_resources(struct usnic_vnic *vnic, enum usnic_vnic_res_type type,
>>>>  		return ERR_PTR(-ENOMEM);
>>>>  	}
>>>>  
>>>> -	ret->res = kzalloc(sizeof(*(ret->res))*cnt, GFP_ATOMIC);
>>>> -	if (!ret->res) {
>>>> -		usnic_err("Failed to allocate resources for %s. Out of memory\n",
>>>> -				usnic_vnic_pci_name(vnic));
>>>> -		kfree(ret);
>>>> -		return ERR_PTR(-ENOMEM);
>>>> -	}
>>>> +	if (cnt > 0) {
>>>> +		ret->res = kcalloc(cnt, sizeof(*(ret->res)), GFP_ATOMIC);
>>>> +		if (!ret->res) {
>>>> +			usnic_err("Failed to allocate resources for %s. Out of memory\n",
>>>> +					usnic_vnic_pci_name(vnic));
>>> You don't need to print OOM messages, failure in memory allocation very hard to miss.
>> OOM messages are hard to miss, but this message is already in upstream
>> and outside the scope of this patch.
> It is worth to fix, especially if you are changing these exact lines.
>>>> +			kfree(ret);
>>>> +			return ERR_PTR(-ENOMEM);
>>>> +		}
>>>>  
>>>> -	spin_lock(&vnic->res_lock);
>>>> -	src = &vnic->chunks[type];
>>>> -	for (i = 0; i < src->cnt && ret->cnt < cnt; i++) {
>>>> -		res = src->res[i];
>>>> -		if (!res->owner) {
>>>> -			src->free_cnt--;
>>>> -			res->owner = owner;
>>>> -			ret->res[ret->cnt++] = res;
>>>> +		spin_lock(&vnic->res_lock);
>>>> +		src = &vnic->chunks[type];
>>>> +		for (i = 0; i < src->cnt && ret->cnt < cnt; i++) {
>>>> +			res = src->res[i];
>>>> +			if (!res->owner) {
>>>> +				src->free_cnt--;
>>> It will be negative, because of skip usnic_vnic_res_free_cnt check
>>> before.
>> We are inside the 'if (cnt > 0)' clause here, so the previous
>> usnic_vnic_res_free_cnt check wasn't skipped.
> See above.
> 
>>>> +				res->owner = owner;
>>>> +				ret->res[ret->cnt++] = res;
>>>> +			}
>>>>  		}
>>>> -	}
>>>>  
>>>> -	spin_unlock(&vnic->res_lock);
>>>> +		spin_unlock(&vnic->res_lock);
>>>> +	}
>>>>  	ret->type = type;
>>>>  	ret->vnic = vnic;
>>>>  	WARN_ON(ret->cnt != cnt);
>>>> @@ -281,14 +283,16 @@ void usnic_vnic_put_resources(struct usnic_vnic_res_chunk *chunk)
>>>>  	int i;
>>>>  	struct usnic_vnic *vnic = chunk->vnic;
>>>>  
>>>> -	spin_lock(&vnic->res_lock);
>>>> -	while ((i = --chunk->cnt) >= 0) {
>>>> -		res = chunk->res[i];
>>>> -		chunk->res[i] = NULL;
>>>> -		res->owner = NULL;
>>>> -		vnic->chunks[res->type].free_cnt++;
>>>> +	if (chunk->cnt > 0) {
>>>> +		spin_lock(&vnic->res_lock);
>>>> +		while ((i = --chunk->cnt) >= 0) {
>>>> +			res = chunk->res[i];
>>>> +			chunk->res[i] = NULL;
>>>> +			res->owner = NULL;
>>>> +			vnic->chunks[res->type].free_cnt++;
>>>> +		}
>>>> +		spin_unlock(&vnic->res_lock);
>>>>  	}
>>>> -	spin_unlock(&vnic->res_lock);
>>>>  
>>>>  	kfree(chunk->res);
>>>>  	kfree(chunk);
>>>> -- 
>>>> 2.4.3
>>>>
>>>> --
>>>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>>>> the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
>>>> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] IB/usnic: Handle 0 counts in resource allocation
       [not found]                 ` <5669F2A7.8060502-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
@ 2015-12-12  7:29                   ` Leon Romanovsky
  0 siblings, 0 replies; 7+ messages in thread
From: Leon Romanovsky @ 2015-12-12  7:29 UTC (permalink / raw)
  To: Nelson Escobar; +Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA

> Thanks for looking at the code.  I am still not understanding your point.
>> Old code:
>> usnic_vnic_res_free_cnt(vnic, type) == 0 and cnt == 1 will return EINVAL
> Yes:
>         if (0 < 1 || 1 < 1 || !owner)
>                 return -EINVAL;
>> New code
>> snic_vnic_res_free_cnt(vnic, type) == 0 and cnt == 1 will pass and will
>> pass te "if (cnt > 0)" check below and will decrease free_cnt variable
>> to be below zero.
> This I don't understand.  The following still fails with -EINVAL.
>         if (0 < 1 || 1 < 0 || !owner)
>                 return -EINVAL;

Thank you for clarifying it. I don't know why I missed first comparison.
Sorry for bothering you.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply	[flat|nested] 7+ messages in thread

* Re: [PATCH] IB/usnic: Handle 0 counts in resource allocation
       [not found] ` <1449686539-29959-6-git-send-email-neescoba-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
  2015-12-10  6:47   ` Leon Romanovsky
@ 2015-12-23 19:48   ` Doug Ledford
  1 sibling, 0 replies; 7+ messages in thread
From: Doug Ledford @ 2015-12-23 19:48 UTC (permalink / raw)
  To: Nelson Escobar, linux-rdma-u79uwXL29TY76Z2rM5mHXA

[-- Attachment #1: Type: text/plain, Size: 650 bytes --]

On 12/09/2015 01:42 PM, Nelson Escobar wrote:
> Signed-off-by: Dave Goodell <dgoodell-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Reese Faucette <rfaucett-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
> Reviewed-by: Xuyang Wang <xuywang-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
> Signed-off-by: Nelson Escobar <neescoba-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
> ---
>  drivers/infiniband/hw/usnic/usnic_vnic.c | 54 +++++++++++++++++---------------
>  1 file changed, 29 insertions(+), 25 deletions(-)

Thanks, applied.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

^ permalink raw reply	[flat|nested] 7+ messages in thread

end of thread, other threads:[~2015-12-23 19:48 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-12-09 18:42 [PATCH] IB/usnic: Handle 0 counts in resource allocation Nelson Escobar
     [not found] ` <1449686539-29959-6-git-send-email-neescoba-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
2015-12-10  6:47   ` Leon Romanovsky
     [not found]     ` <20151210064702.GC8662-2ukJVAZIZ/Y@public.gmane.org>
2015-12-10 19:22       ` Nelson Escobar
     [not found]         ` <5669D0F0.7010104-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
2015-12-10 19:47           ` leon-2ukJVAZIZ/Y
     [not found]             ` <20151210194734.GF8662-2ukJVAZIZ/Y@public.gmane.org>
2015-12-10 21:46               ` Nelson Escobar
     [not found]                 ` <5669F2A7.8060502-FYB4Gu1CFyUAvxtiuMwx3w@public.gmane.org>
2015-12-12  7:29                   ` Leon Romanovsky
2015-12-23 19:48   ` Doug Ledford

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.