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@xxxxxxxxxxxxxxx > >> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html