[PATCH v2] infiniband: free only allocated items

[PATCH v2] infiniband: free only allocated items

[Maninder Singh]

 o If allocation of dm fails, no need to free it.
 o Free only allocated items.

Signed-off-by: Maninder Singh <maninder1.s-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
---
 drivers/infiniband/hw/mlx4/main.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 067a691..253498f 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -2669,17 +2669,15 @@ static void do_slave_init(struct mlx4_ib_dev *ibdev, int slave, int do_init)
 	dm = kcalloc(ports, sizeof(*dm), GFP_ATOMIC);
 	if (!dm) {
 		pr_err("failed to allocate memory for tunneling qp update\n");
-		goto out;
+		return;
 	}
 
 	for (i = 0; i < ports; i++) {
 		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]);
-			}
+			while (--i >= 0)
+				kfree(dm[i]);
 			goto out;
 		}
 	}
-- 
1.7.9.5

--
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

[PATCH v2] infiniband: free only allocated items

[Maninder Singh]

 o If allocation of dm fails, no need to free it.
 o Free only allocated items.

Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
---
 drivers/infiniband/hw/mlx4/main.c |    8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
index 067a691..253498f 100644
--- a/drivers/infiniband/hw/mlx4/main.c
+++ b/drivers/infiniband/hw/mlx4/main.c
@@ -2669,17 +2669,15 @@ static void do_slave_init(struct mlx4_ib_dev *ibdev, int slave, int do_init)
 	dm = kcalloc(ports, sizeof(*dm), GFP_ATOMIC);
 	if (!dm) {
 		pr_err("failed to allocate memory for tunneling qp update\n");
-		goto out;
+		return;
 	}
 
 	for (i = 0; i < ports; i++) {
 		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]);
-			}
+			while (--i >= 0)
+				kfree(dm[i]);
 			goto out;
 		}
 	}
-- 
1.7.9.5

Re: [PATCH v2] infiniband: free only allocated items

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 2069 bytes --]

On 07/08/2015 12:13 AM, Maninder Singh wrote:
>  o If allocation of dm fails, no need to free it.
>  o Free only allocated items.

I've taken the patch, but I reworked your commit message.  The v1
version of the patch had a more correct commit message, but it could
have used a little rework as well.  This is what I entered in the commit
message:

IB/mlx4: Optimize freeing of items on error unwind

On failure, we loop through all possible pointers and test them before
calling kfree.  But really, why even attempt to free items we didn't
allocate when we can easily loop through exactly and only the devices
for which the original memory allocation succeeded and free just those.

Signed-off-by: Maninder Singh <maninder1.s-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
Signed-off-by: Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>


> 
> Signed-off-by: Maninder Singh <maninder1.s-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>
> ---
>  drivers/infiniband/hw/mlx4/main.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index 067a691..253498f 100644
> --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -2669,17 +2669,15 @@ static void do_slave_init(struct mlx4_ib_dev *ibdev, int slave, int do_init)
>  	dm = kcalloc(ports, sizeof(*dm), GFP_ATOMIC);
>  	if (!dm) {
>  		pr_err("failed to allocate memory for tunneling qp update\n");
> -		goto out;
> +		return;
>  	}
>  
>  	for (i = 0; i < ports; i++) {
>  		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]);
> -			}
> +			while (--i >= 0)
> +				kfree(dm[i]);
>  			goto out;
>  		}
>  	}
> 


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]

Re: [PATCH v2] infiniband: free only allocated items

[Doug Ledford]

[-- Attachment #1: Type: text/plain, Size: 1955 bytes --]

On 07/08/2015 12:13 AM, Maninder Singh wrote:
>  o If allocation of dm fails, no need to free it.
>  o Free only allocated items.

I've taken the patch, but I reworked your commit message.  The v1
version of the patch had a more correct commit message, but it could
have used a little rework as well.  This is what I entered in the commit
message:

IB/mlx4: Optimize freeing of items on error unwind

On failure, we loop through all possible pointers and test them before
calling kfree.  But really, why even attempt to free items we didn't
allocate when we can easily loop through exactly and only the devices
for which the original memory allocation succeeded and free just those.

Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
Signed-off-by: Doug Ledford <dledford@redhat.com>


> 
> Signed-off-by: Maninder Singh <maninder1.s@samsung.com>
> ---
>  drivers/infiniband/hw/mlx4/main.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
> index 067a691..253498f 100644
> --- a/drivers/infiniband/hw/mlx4/main.c
> +++ b/drivers/infiniband/hw/mlx4/main.c
> @@ -2669,17 +2669,15 @@ static void do_slave_init(struct mlx4_ib_dev *ibdev, int slave, int do_init)
>  	dm = kcalloc(ports, sizeof(*dm), GFP_ATOMIC);
>  	if (!dm) {
>  		pr_err("failed to allocate memory for tunneling qp update\n");
> -		goto out;
> +		return;
>  	}
>  
>  	for (i = 0; i < ports; i++) {
>  		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]);
> -			}
> +			while (--i >= 0)
> +				kfree(dm[i]);
>  			goto out;
>  		}
>  	}
> 


-- 
Doug Ledford <dledford@redhat.com>
              GPG KeyID: 0E572FDD



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 884 bytes --]