diff mbox

[2/3] IB/srpt: allocate ib_qp_attr on the stack

Message ID 1482922813-8392-3-git-send-email-maxg@mellanox.com (mailing list archive)
State Superseded
Headers show

Commit Message

Max Gurtovoy Dec. 28, 2016, 11 a.m. UTC 
No reason to allocate it dynamically.

Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
---
 drivers/infiniband/ulp/srpt/ib_srpt.c |   18 +++++++-----------
 1 files changed, 7 insertions(+), 11 deletions(-)

Comments

Yuval Shaia Dec. 28, 2016, 11:06 a.m. UTC | #1 
One minor comment in line but w/ or w/o it:

Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>

On Wed, Dec 28, 2016 at 01:00:12PM +0200, Max Gurtovoy wrote:
> No reason to allocate it dynamically.
> 
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c |   18 +++++++-----------
>  1 files changed, 7 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 0b1f69e..aa18d74 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -984,24 +984,20 @@ static int srpt_get_desc_tbl(struct srpt_send_ioctx *ioctx,
>   */
>  static int srpt_init_ch_qp(struct srpt_rdma_ch *ch, struct ib_qp *qp)
>  {
> -	struct ib_qp_attr *attr;
> +	struct ib_qp_attr attr;
>  	int ret;
>  
> -	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> -	if (!attr)
> -		return -ENOMEM;
> -
> -	attr->qp_state = IB_QPS_INIT;
> -	attr->qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
> +	memset(&attr, 0, sizeof(attr));
> +	attr.qp_state = IB_QPS_INIT;
> +	attr.qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
>  	    IB_ACCESS_REMOTE_WRITE;
> -	attr->port_num = ch->sport->port;
> -	attr->pkey_index = 0;
> +	attr.port_num = ch->sport->port;
> +	attr.pkey_index = 0;

Correct me if i'm wrong but this looks like redundant since we zeroed the
entire attr object few lines ago.

>  
> -	ret = ib_modify_qp(qp, attr,
> +	ret = ib_modify_qp(qp, &attr,
>  			   IB_QP_STATE | IB_QP_ACCESS_FLAGS | IB_QP_PORT |
>  			   IB_QP_PKEY_INDEX);
>  
> -	kfree(attr);
>  	return ret;
>  }
>  
> -- 
> 1.7.1
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Max Gurtovoy Dec. 28, 2016, 11:38 a.m. UTC | #2 
On 12/28/2016 1:06 PM, Yuval Shaia wrote:
> One minor comment in line but w/ or w/o it:
>
> Reviewed-by: Yuval Shaia <yuval.shaia@oracle.com>
>
> On Wed, Dec 28, 2016 at 01:00:12PM +0200, Max Gurtovoy wrote:
>> No reason to allocate it dynamically.
>>
>> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
>> ---
>>  drivers/infiniband/ulp/srpt/ib_srpt.c |   18 +++++++-----------
>>  1 files changed, 7 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> index 0b1f69e..aa18d74 100644
>> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
>> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
>> @@ -984,24 +984,20 @@ static int srpt_get_desc_tbl(struct srpt_send_ioctx *ioctx,
>>   */
>>  static int srpt_init_ch_qp(struct srpt_rdma_ch *ch, struct ib_qp *qp)
>>  {
>> -	struct ib_qp_attr *attr;
>> +	struct ib_qp_attr attr;
>>  	int ret;
>>
>> -	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
>> -	if (!attr)
>> -		return -ENOMEM;
>> -
>> -	attr->qp_state = IB_QPS_INIT;
>> -	attr->qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
>> +	memset(&attr, 0, sizeof(attr));
>> +	attr.qp_state = IB_QPS_INIT;
>> +	attr.qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
>>  	    IB_ACCESS_REMOTE_WRITE;
>> -	attr->port_num = ch->sport->port;
>> -	attr->pkey_index = 0;
>> +	attr.port_num = ch->sport->port;
>> +	attr.pkey_index = 0;
>
> Correct me if i'm wrong but this looks like redundant since we zeroed the
> entire attr object few lines ago.
>

Yes this is redundant but I think it's implies the flags you'll pass 
ib_modify_qp. don't realy mind,

Bart, your call :)

>>
>> -	ret = ib_modify_qp(qp, attr,
>> +	ret = ib_modify_qp(qp, &attr,
>>  			   IB_QP_STATE | IB_QP_ACCESS_FLAGS | IB_QP_PORT |
>>  			   IB_QP_PKEY_INDEX);
>>
>> -	kfree(attr);
>>  	return ret;
>>  }
>>
>> --
>> 1.7.1
>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
>> the body of a message to majordomo@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 majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Bart Van Assche Dec. 28, 2016, 11:39 a.m. UTC | #3 
T24gV2VkLCAyMDE2LTEyLTI4IGF0IDEzOjAwICswMjAwLCBNYXggR3VydG92b3kgd3JvdGU6DQo+
IE5vIHJlYXNvbiB0byBhbGxvY2F0ZSBpdCBkeW5hbWljYWxseS4NCg0KU2FtZSBjb21tZW50IGhl
cmU6IHN0cnVjdCBpYl9xcF9hdHRyIGlzIGFsbG9jYXRlZCBvbiB0aGUgc3RhY2sNCnRvIGtlZXAg
c3RhY2sgdXNhZ2UgbG93Lg0KDQpCYXJ0Lg0K
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Leon Romanovsky Dec. 28, 2016, 12:57 p.m. UTC | #4 
On Wed, Dec 28, 2016 at 01:00:12PM +0200, Max Gurtovoy wrote:
> No reason to allocate it dynamically.
>
> Signed-off-by: Max Gurtovoy <maxg@mellanox.com>
> ---
>  drivers/infiniband/ulp/srpt/ib_srpt.c |   18 +++++++-----------
>  1 files changed, 7 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
> index 0b1f69e..aa18d74 100644
> --- a/drivers/infiniband/ulp/srpt/ib_srpt.c
> +++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
> @@ -984,24 +984,20 @@ static int srpt_get_desc_tbl(struct srpt_send_ioctx *ioctx,
>   */
>  static int srpt_init_ch_qp(struct srpt_rdma_ch *ch, struct ib_qp *qp)
>  {
> -	struct ib_qp_attr *attr;
> +	struct ib_qp_attr attr;
>  	int ret;
>
> -	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
> -	if (!attr)
> -		return -ENOMEM;
> -
> -	attr->qp_state = IB_QPS_INIT;
> -	attr->qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
> +	memset(&attr, 0, sizeof(attr));

This line will not needed if you initialize to zero this struct attr at the beginning of function.


> +	attr.qp_state = IB_QPS_INIT;
> +	attr.qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
>  	    IB_ACCESS_REMOTE_WRITE;
> -	attr->port_num = ch->sport->port;
> -	attr->pkey_index = 0;
> +	attr.port_num = ch->sport->port;
> +	attr.pkey_index = 0;
>
> -	ret = ib_modify_qp(qp, attr,
> +	ret = ib_modify_qp(qp, &attr,
>  			   IB_QP_STATE | IB_QP_ACCESS_FLAGS | IB_QP_PORT |
>  			   IB_QP_PKEY_INDEX);
>
> -	kfree(attr);
>  	return ret;
>  }
>
> --
> 1.7.1
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 0b1f69e..aa18d74 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -984,24 +984,20 @@  static int srpt_get_desc_tbl(struct srpt_send_ioctx *ioctx,
  */
 static int srpt_init_ch_qp(struct srpt_rdma_ch *ch, struct ib_qp *qp)
 {
-	struct ib_qp_attr *attr;
+	struct ib_qp_attr attr;
 	int ret;
 
-	attr = kzalloc(sizeof(*attr), GFP_KERNEL);
-	if (!attr)
-		return -ENOMEM;
-
-	attr->qp_state = IB_QPS_INIT;
-	attr->qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
+	memset(&attr, 0, sizeof(attr));
+	attr.qp_state = IB_QPS_INIT;
+	attr.qp_access_flags = IB_ACCESS_LOCAL_WRITE | IB_ACCESS_REMOTE_READ |
 	    IB_ACCESS_REMOTE_WRITE;
-	attr->port_num = ch->sport->port;
-	attr->pkey_index = 0;
+	attr.port_num = ch->sport->port;
+	attr.pkey_index = 0;
 
-	ret = ib_modify_qp(qp, attr,
+	ret = ib_modify_qp(qp, &attr,
 			   IB_QP_STATE | IB_QP_ACCESS_FLAGS | IB_QP_PORT |
 			   IB_QP_PKEY_INDEX);
 
-	kfree(attr);
 	return ret;
 }