kfree(NULL) is safe and this check is probably not required
Signed-off-by: Maninder Singh <[email protected]>
Reviewed-by: Akhilesh Kumar <[email protected]>
---
drivers/infiniband/hw/mlx4/main.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 067a691..4e60b39 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -2676,10 +2676,8 @@ static void do_slave_init(struct mlx4_ib_dev *ibdev, int slave, int do_init)
dm[i] = kmalloc(sizeof (struct mlx4_ib_demux_work), GFP_ATOMIC);
if (!dm[i]) {
pr_err("failed to allocate memory for tunneling qp update work struct\n");
- for (i = 0; i < dev->caps.num_ports; i++) {
- if (dm[i])
- kfree(dm[i]);
- }
+ for (i = 0; i < dev->caps.num_ports; i++)
+ kfree(dm[i]);
goto out;
}
}
--
1.7.9.5
> On Jun 26, 2015, at 3:09 AM, Maninder Singh <[email protected]> wrote:
>
> kfree(NULL) is safe and this check is probably not required
>
> Signed-off-by: Maninder Singh <[email protected]>
> Reviewed-by: Akhilesh Kumar <[email protected]>
> ---
> drivers/infiniband/hw/mlx4/main.c | 6 ++----
> 1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index 067a691..4e60b39 100644
> --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -2676,10 +2676,8 @@ static void do_slave_init(struct mlx4_ib_dev *ibdev, int slave, int do_init)
> dm[i] = kmalloc(sizeof (struct mlx4_ib_demux_work), GFP_ATOMIC);
> if (!dm[i]) {
> pr_err("failed to allocate memory for tunneling qp update work struct\n");
> - for (i = 0; i < dev->caps.num_ports; i++) {
> - if (dm[i])
> - kfree(dm[i]);
> - }
> + for (i = 0; i < dev->caps.num_ports; i++)
> + kfree(dm[i]);
> goto out;
> }
> }
> --
> 1.7.9.5
>
If you are going to change this, you might as well make it 100% correct:
i—-;
while (i >= 0)
kfree(dm[i]);
Then you don’t have to worry about whether kfree works on NULL, every item you free will be guaranteed to be non-NULL.
—
Doug Ledford <[email protected]>
GPG Key ID: 0E572FDD
Hello,
>> + for (i = 0; i < dev->caps.num_ports; i++)
>> + kfree(dm[i]);
>> goto out;
>> }
>> }
>> --
>> 1.7.9.5
>>
>
>If you are going to change this, you might as well make it 100% correct:
>
>i?-;
>while (i >= 0)
> kfree(dm[i]);
>
>Then you don?t have to worry about whether kfree works on NULL, every item you free will be guaranteed to be non-NULL.
Thanks for suggestion :)
Sent new patch with described changes, I was thinking one more thing.
In below code :-
if (!ibdev->sriov.is_going_down)
queue_work(ibdev->sriov.demux[i].ud_wq, &dm[i]->work);
spin_unlock_irqrestore(&ibdev->sriov.going_down_lock, flags);
}
out:
kfree(dm);
return;
dm is freed after queue_work, is it correct to free dm when other dm[i] are allocated ? i did not get it.
Thanks
Maninder
------------????{.n?+???????+%?????ݶ??w??{.n?+????{??G?????{ay? ʇڙ?,j??f???h?????????z_??(?階?ݢj"???m??????G????????????&???~???iO???z??v?^?m???? ????????I?
On 07/08/2015 12:23 AM, Maninder Singh wrote:
> Hello,
>
>>> + for (i = 0; i < dev->caps.num_ports; i++)
>>> + kfree(dm[i]);
>>> goto out;
>>> }
>>> }
>>> --
>>> 1.7.9.5
>>>
>>
>> If you are going to change this, you might as well make it 100% correct:
>>
>> i—-;
>> while (i >= 0)
>> kfree(dm[i]);
>>
>> Then you don’t have to worry about whether kfree works on NULL, every item you free will be guaranteed to be non-NULL.
> Thanks for suggestion :)
> Sent new patch with described changes, I was thinking one more thing.
>
> In below code :-
> if (!ibdev->sriov.is_going_down)
> queue_work(ibdev->sriov.demux[i].ud_wq, &dm[i]->work);
> spin_unlock_irqrestore(&ibdev->sriov.going_down_lock, flags);
> }
> out:
> kfree(dm);
> return;
>
> dm is freed after queue_work, is it correct to free dm when other dm[i] are allocated ? i did not get it.
The dm is just there to give an easy way to refer to a variable number
of work structs. The flow is supposed to be something like this:
alloc(dm)
for(i=0;i<num_qps;i++)
dm[i] == alloc(work item);
for(i=0;i<num_qps;i++)
init dm[i] work item
queue dm[i] work item
free(dm)
In this scenario, all of the dm[i] items should be queued to delayed
work. When that work completes, it should then free these structs. So,
yes, the dm variable itself is just a temporary means of keeping all
those work struct pointers together. However, your question caused me
to look closely here, and I see that there is a bug. In particular, if
we check the sriov.is_going_down and as a result *don't* queue a work
item, then we end up leaking that work struct. In addition, I think
there is room to optimize this routine considerably. I'll post a patch
for that in a minute.
--
Doug Ledford <[email protected]>
GPG KeyID: 0E572FDD
This is correct as is.
Acked-by: Christoph Lameter <[email protected]>