On 12/10/2015 11:47 AM, l...@leon.nu 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 majord...@vger.kernel.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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
