* Re: [PATCH 1/1] infiniband: Remove redundant NULL check before kfree
@ 2015-07-08 4:23 Maninder Singh
2015-07-08 22:24 ` Doug Ledford
0 siblings, 1 reply; 5+ messages in thread
From: Maninder Singh @ 2015-07-08 4:23 UTC (permalink / raw)
To: Doug Ledford
Cc: Sean Hefty, Or Gerlitz, David Miller, roland, Matan Barak,
Moni Shoua, jackm, Yishai Hadas, eranbe, Ira Weiny, linux-rdma,
linux-kernel, PANKAJ MISHRA
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=windows-1252, Size: 992 bytes --]
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 dont 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Ç+·®+%Ëÿ±éݶ\x17¥wÿº{.nÇ+·¥{±þG«éÿ{ayº\x1dÊÚë,j\a¢f£¢·hïêÿêçz_è®\x03(éÝ¢j"ú\x1a¶^[m§ÿÿ¾\a«þG«éÿ¢¸?¨èÚ&£ø§~á¶iOæ¬z·vØ^\x14\x04\x1a¶^[m§ÿÿÃ\fÿ¶ìÿ¢¸?I¥
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] infiniband: Remove redundant NULL check before kfree
2015-07-08 4:23 [PATCH 1/1] infiniband: Remove redundant NULL check before kfree Maninder Singh
@ 2015-07-08 22:24 ` Doug Ledford
0 siblings, 0 replies; 5+ messages in thread
From: Doug Ledford @ 2015-07-08 22:24 UTC (permalink / raw)
To: maninder1.s
Cc: Sean Hefty, Or Gerlitz, David Miller, roland, Matan Barak,
Moni Shoua, jackm, Yishai Hadas, eranbe, Ira Weiny, linux-rdma,
linux-kernel, PANKAJ MISHRA
[-- Attachment #1: Type: text/plain, Size: 1912 bytes --]
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 <dledford@redhat.com>
GPG KeyID: 0E572FDD
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] infiniband: Remove redundant NULL check before kfree
2015-06-26 7:09 Maninder Singh
2015-07-07 18:53 ` Doug Ledford
@ 2015-07-09 17:06 ` Christoph Lameter
1 sibling, 0 replies; 5+ messages in thread
From: Christoph Lameter @ 2015-07-09 17:06 UTC (permalink / raw)
To: Maninder Singh
Cc: dledford, sean.hefty, ogerlitz, davem, roland, matanb, monis,
jackm, yishaih, eranbe, ira.weiny, linux-rdma, linux-kernel,
pankaj.m
This is correct as is.
Acked-by: Christoph Lameter <cl@linux.com>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH 1/1] infiniband: Remove redundant NULL check before kfree
2015-06-26 7:09 Maninder Singh
@ 2015-07-07 18:53 ` Doug Ledford
2015-07-09 17:06 ` Christoph Lameter
1 sibling, 0 replies; 5+ messages in thread
From: Doug Ledford @ 2015-07-07 18:53 UTC (permalink / raw)
To: Maninder Singh
Cc: Sean Hefty, Or Gerlitz, David Miller, roland, Matan Barak,
Moni Shoua, jackm, Yishai Hadas, eranbe, Ira Weiny, linux-rdma,
linux-kernel, pankaj.m
[-- Attachment #1: Type: text/plain, Size: 1400 bytes --]
> On Jun 26, 2015, at 3:09 AM, Maninder Singh <maninder1.s@samsung.com> wrote:
>
> kfree(NULL) is safe and this check is probably not required
>
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> Reviewed-by: Akhilesh Kumar <akhilesh.k@samsung.com>
> ---
> 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 <dledford@redhat.com>
GPG Key ID: 0E572FDD
[-- Attachment #2: Message signed with OpenPGP using GPGMail --]
[-- Type: application/pgp-signature, Size: 842 bytes --]
^ permalink raw reply [flat|nested] 5+ messages in thread
* [PATCH 1/1] infiniband: Remove redundant NULL check before kfree
@ 2015-06-26 7:09 Maninder Singh
2015-07-07 18:53 ` Doug Ledford
2015-07-09 17:06 ` Christoph Lameter
0 siblings, 2 replies; 5+ messages in thread
From: Maninder Singh @ 2015-06-26 7:09 UTC (permalink / raw)
To: dledford, sean.hefty, ogerlitz, davem, roland, matanb, monis,
jackm, yishaih, eranbe, ira.weiny, linux-rdma, linux-kernel
Cc: pankaj.m, Maninder Singh
kfree(NULL) is safe and this check is probably not required
Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
Reviewed-by: Akhilesh Kumar <akhilesh.k@samsung.com>
---
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
^ permalink raw reply related [flat|nested] 5+ messages in thread
end of thread, other threads:[~2015-07-09 17:06 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2015-07-08 4:23 [PATCH 1/1] infiniband: Remove redundant NULL check before kfree Maninder Singh
2015-07-08 22:24 ` Doug Ledford
-- strict thread matches above, loose matches on Subject: below --
2015-06-26 7:09 Maninder Singh
2015-07-07 18:53 ` Doug Ledford
2015-07-09 17:06 ` Christoph Lameter
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).