Re: [PATCH] IB/usnic: Handle 0 counts in resource allocation

["leon-2ukJVAZIZ/Y@public.gmane.org" <leon-2ukJVAZIZ/Y@public.gmane.org>]

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