[PATCH 0/5] rds: trivial patches

[PATCH 0/5] rds: trivial patches

[Zhu Yanjun]

Hi, all 

The 5 patches are trivial patches.

Thanks a lot.

Zhu Yanjun (5):
  rds: tcp: release the created connection
  rds: rdma: fix memory leak error
  rds: remove unnecessary returned value check
  rds: remove the unused variable
  rds: ib: add the static type to the functions

 net/rds/af_rds.c     |  5 +----
 net/rds/ib_mr.h      |  2 --
 net/rds/ib_rdma.c    | 12 +++++-------
 net/rds/ib_recv.c    |  3 +--
 net/rds/rdma.c       |  7 +++++++
 net/rds/rds.h        |  2 +-
 net/rds/stats.c      |  3 +--
 net/rds/tcp_listen.c |  1 +
 8 files changed, 17 insertions(+), 18 deletions(-)

-- 
2.7.4

[PATCH 1/5] rds: tcp: release the created connection

[Zhu Yanjun]

When some error occurs, the created connection should be destroyed.

Signed-off-by: Zhu Yanjun <yanjun.zhu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 net/rds/tcp_listen.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
index 5076788..58aa5bc 100644
--- a/net/rds/tcp_listen.c
+++ b/net/rds/tcp_listen.c
@@ -196,6 +196,7 @@ int rds_tcp_accept_one(struct socket *sock)
 rst_nsk:
 	/* reset the newly returned accept sock and bail */
 	kernel_sock_shutdown(new_sock, SHUT_RDWR);
+	rds_conn_destroy(conn);
 	ret = 0;
 out:
 	if (rs_tcp)
-- 
2.7.4

--
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 2/5] rds: rdma: fix memory leak error

[Zhu Yanjun]

In the function __rds_rdma_map, the allocated memory and other
resources should be freed when some error occurs.

Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
---
 net/rds/rdma.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/net/rds/rdma.c b/net/rds/rdma.c
index f06fac4..ad9b6bd 100644
--- a/net/rds/rdma.c
+++ b/net/rds/rdma.c
@@ -253,6 +253,8 @@ static int __rds_rdma_map(struct rds_sock *rs, struct rds_get_mr_args *args,
 	sg = kcalloc(nents, sizeof(*sg), GFP_KERNEL);
 	if (!sg) {
 		ret = -ENOMEM;
+		for (i = 0; i < nents; i++)
+			put_page(pages[i]);
 		goto out;
 	}
 	WARN_ON(!nents);
@@ -293,6 +295,11 @@ static int __rds_rdma_map(struct rds_sock *rs, struct rds_get_mr_args *args,
 		*cookie_ret = cookie;
 
 	if (args->cookie_addr && put_user(cookie, (u64 __user *)(unsigned long) args->cookie_addr)) {
+		for (i = 0; i < nents; i++)
+			put_page(sg_page(&sg[i]));
+		kfree(sg);
+		mr->r_trans->free_mr(mr->r_trans_private, 1);
+		mr->r_trans_private = NULL;
 		ret = -EFAULT;
 		goto out;
 	}
-- 
2.7.4

[PATCH 3/5] rds: remove unnecessary returned value check

[Zhu Yanjun]

In the function rds_stats_init, the returned value
is always zero. As such, it is not necessary to check it.
And change the return type to void.

Signed-off-by: Zhu Yanjun <yanjun.zhu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 net/rds/af_rds.c | 5 +----
 net/rds/rds.h    | 2 +-
 net/rds/stats.c  | 3 +--
 3 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/net/rds/af_rds.c b/net/rds/af_rds.c
index b405f77..a696096 100644
--- a/net/rds/af_rds.c
+++ b/net/rds/af_rds.c
@@ -658,9 +658,7 @@ static int rds_init(void)
 	ret = rds_sysctl_init();
 	if (ret)
 		goto out_threads;
-	ret = rds_stats_init();
-	if (ret)
-		goto out_sysctl;
+	rds_stats_init();
 	ret = proto_register(&rds_proto, 1);
 	if (ret)
 		goto out_stats;
@@ -677,7 +675,6 @@ static int rds_init(void)
 	proto_unregister(&rds_proto);
 out_stats:
 	rds_stats_exit();
-out_sysctl:
 	rds_sysctl_exit();
 out_threads:
 	rds_threads_exit();
diff --git a/net/rds/rds.h b/net/rds/rds.h
index 82d38cc..82a8856 100644
--- a/net/rds/rds.h
+++ b/net/rds/rds.h
@@ -870,7 +870,7 @@ DECLARE_PER_CPU_SHARED_ALIGNED(struct rds_statistics, rds_stats);
 	put_cpu();					\
 } while (0)
 #define rds_stats_add(member, count) rds_stats_add_which(rds_stats, member, count)
-int rds_stats_init(void);
+void rds_stats_init(void);
 void rds_stats_exit(void);
 void rds_stats_info_copy(struct rds_info_iterator *iter,
 			 uint64_t *values, const char *const *names,
diff --git a/net/rds/stats.c b/net/rds/stats.c
index 73be187..e83b4bb 100644
--- a/net/rds/stats.c
+++ b/net/rds/stats.c
@@ -145,8 +145,7 @@ void rds_stats_exit(void)
 	rds_info_deregister_func(RDS_INFO_COUNTERS, rds_stats_info);
 }
 
-int rds_stats_init(void)
+void rds_stats_init(void)
 {
 	rds_info_register_func(RDS_INFO_COUNTERS, rds_stats_info);
-	return 0;
 }
-- 
2.7.4

--
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 4/5] rds: remove the unused variable

[Zhu Yanjun]

In the function rds_ib_recv_path, the variable ret is not used.

Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
---
 net/rds/ib_recv.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/net/rds/ib_recv.c b/net/rds/ib_recv.c
index e10624a..be8d2f2 100644
--- a/net/rds/ib_recv.c
+++ b/net/rds/ib_recv.c
@@ -1023,7 +1023,6 @@ int rds_ib_recv_path(struct rds_conn_path *cp)
 {
 	struct rds_connection *conn = cp->cp_conn;
 	struct rds_ib_connection *ic = conn->c_transport_data;
-	int ret = 0;
 
 	rdsdebug("conn %p\n", conn);
 	if (rds_conn_up(conn)) {
@@ -1031,7 +1030,7 @@ int rds_ib_recv_path(struct rds_conn_path *cp)
 		rds_ib_recv_refill(conn, 0, GFP_KERNEL);
 	}
 
-	return ret;
+	return 0;
 }
 
 int rds_ib_recv_init(void)
-- 
2.7.4

[PATCH 5/5] rds: ib: add the static type to the functions

[Zhu Yanjun]

The functions rds_ib_flush_mr_pool and rds_ib_reuse_mr
are called in the local file. As such, the static type
is added to limit them in the local file.

And the function rds_ib_flush_mr_pool always returns zero.
As such, change the return type to void.

Signed-off-by: Zhu Yanjun <yanjun.zhu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
---
 net/rds/ib_mr.h   |  2 --
 net/rds/ib_rdma.c | 12 +++++-------
 2 files changed, 5 insertions(+), 9 deletions(-)

diff --git a/net/rds/ib_mr.h b/net/rds/ib_mr.h
index 0ea4ab0..a3a9676 100644
--- a/net/rds/ib_mr.h
+++ b/net/rds/ib_mr.h
@@ -125,8 +125,6 @@ void rds_ib_mr_exit(void);
 void __rds_ib_teardown_mr(struct rds_ib_mr *);
 void rds_ib_teardown_mr(struct rds_ib_mr *);
 struct rds_ib_mr *rds_ib_alloc_fmr(struct rds_ib_device *, int);
-struct rds_ib_mr *rds_ib_reuse_mr(struct rds_ib_mr_pool *);
-int rds_ib_flush_mr_pool(struct rds_ib_mr_pool *, int, struct rds_ib_mr **);
 struct rds_ib_mr *rds_ib_reg_fmr(struct rds_ib_device *, struct scatterlist *,
 				 unsigned long, u32 *);
 struct rds_ib_mr *rds_ib_try_reuse_ibmr(struct rds_ib_mr_pool *);
diff --git a/net/rds/ib_rdma.c b/net/rds/ib_rdma.c
index 977f698..1bd3aba 100644
--- a/net/rds/ib_rdma.c
+++ b/net/rds/ib_rdma.c
@@ -179,7 +179,7 @@ void rds_ib_get_mr_info(struct rds_ib_device *rds_ibdev, struct rds_info_rdma_co
 	iinfo->rdma_mr_size = pool_1m->fmr_attr.max_pages;
 }
 
-struct rds_ib_mr *rds_ib_reuse_mr(struct rds_ib_mr_pool *pool)
+static struct rds_ib_mr *rds_ib_reuse_mr(struct rds_ib_mr_pool *pool)
 {
 	struct rds_ib_mr *ibmr = NULL;
 	struct llist_node *ret;
@@ -336,8 +336,8 @@ static void list_to_llist_nodes(struct rds_ib_mr_pool *pool,
  * If the number of MRs allocated exceeds the limit, we also try
  * to free as many MRs as needed to get back to this limit.
  */
-int rds_ib_flush_mr_pool(struct rds_ib_mr_pool *pool,
-			 int free_all, struct rds_ib_mr **ibmr_ret)
+static void rds_ib_flush_mr_pool(struct rds_ib_mr_pool *pool,
+				 int free_all, struct rds_ib_mr **ibmr_ret)
 {
 	struct rds_ib_mr *ibmr;
 	struct llist_node *clean_nodes;
@@ -358,7 +358,7 @@ int rds_ib_flush_mr_pool(struct rds_ib_mr_pool *pool,
 			if (ibmr) {
 				*ibmr_ret = ibmr;
 				finish_wait(&pool->flush_wait, &wait);
-				goto out_nolock;
+				return;
 			}
 
 			prepare_to_wait(&pool->flush_wait, &wait,
@@ -370,7 +370,7 @@ int rds_ib_flush_mr_pool(struct rds_ib_mr_pool *pool,
 			if (ibmr) {
 				*ibmr_ret = ibmr;
 				finish_wait(&pool->flush_wait, &wait);
-				goto out_nolock;
+				return;
 			}
 		}
 		finish_wait(&pool->flush_wait, &wait);
@@ -433,8 +433,6 @@ int rds_ib_flush_mr_pool(struct rds_ib_mr_pool *pool,
 	mutex_unlock(&pool->flush_lock);
 	if (waitqueue_active(&pool->flush_wait))
 		wake_up(&pool->flush_wait);
-out_nolock:
-	return 0;
 }
 
 struct rds_ib_mr *rds_ib_try_reuse_ibmr(struct rds_ib_mr_pool *pool)
-- 
2.7.4

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

Re: [PATCH 1/5] rds: tcp: release the created connection

[Sowmini Varadhan]

On (03/27/17 03:06), Zhu Yanjun wrote:
> Date: Mon, 27 Mar 2017 03:06:26 -0400
> From: Zhu Yanjun <yanjun.zhu@oracle.com>
> To: yanjun.zhu@oracle.com, santosh.shilimkar@oracle.com,
>  netdev@vger.kernel.org, linux-rdma@vger.kernel.org,
>  rds-devel@oss.oracle.com, junxiao.bi@oracle.com, joe.jin@oracle.com
> Subject: [PATCH 1/5] rds: tcp: release the created connection
> X-Mailer: git-send-email 2.7.4
> 
> When some error occurs, the created connection should be destroyed.

No please dont do this.

This is the case when there are duelling connections. We want
to reset the new (accept sock) and leave the old socket in place.

How did you test this? Did you test it with network namespaces?

--Sowmini


>  net/rds/tcp_listen.c | 1 +
> 
> diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
> index 5076788..58aa5bc 100644
> --- a/net/rds/tcp_listen.c
> +++ b/net/rds/tcp_listen.c
> @@ -196,6 +196,7 @@ int rds_tcp_accept_one(struct socket *sock)
>  rst_nsk:
>  	/* reset the newly returned accept sock and bail */
>  	kernel_sock_shutdown(new_sock, SHUT_RDWR);
> +	rds_conn_destroy(conn);
>  	ret = 0;
>  out:
>  	if (rs_tcp)
> -- 
> 2.7.4
> 

Re: [PATCH 1/5] rds: tcp: release the created connection

[Yanjun Zhu]

On 2017/3/27 15:37, Sowmini Varadhan wrote:
> On (03/27/17 03:06), Zhu Yanjun wrote:
>> Date: Mon, 27 Mar 2017 03:06:26 -0400
>> From: Zhu Yanjun <yanjun.zhu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org>
>> To: yanjun.zhu-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org, santosh.shilimkar-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org,
>>   netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org, linux-rdma-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
>>   rds-devel-N0ozoZBvEnrZJqsBc5GL+g@public.gmane.org, junxiao.bi-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org, joe.jin-QHcLZuEGTsvQT0dZR+AlfA@public.gmane.org
>> Subject: [PATCH 1/5] rds: tcp: release the created connection
>> X-Mailer: git-send-email 2.7.4
>>
>> When some error occurs, the created connection should be destroyed.
> No please dont do this.
>
> This is the case when there are duelling connections. We want
> to reset the new (accept sock) and leave the old socket in place.
>
> How did you test this? Did you test it with network namespaces?
Sorry. I just made simple test. It seems that it worked well. Would you 
like to show me some test about this patch?

Thanks a lot.
Zhu Yanjun
>
> --Sowmini
>
>
>>   net/rds/tcp_listen.c | 1 +
>>
>> diff --git a/net/rds/tcp_listen.c b/net/rds/tcp_listen.c
>> index 5076788..58aa5bc 100644
>> --- a/net/rds/tcp_listen.c
>> +++ b/net/rds/tcp_listen.c
>> @@ -196,6 +196,7 @@ int rds_tcp_accept_one(struct socket *sock)
>>   rst_nsk:
>>   	/* reset the newly returned accept sock and bail */
>>   	kernel_sock_shutdown(new_sock, SHUT_RDWR);
>> +	rds_conn_destroy(conn);
>>   	ret = 0;
>>   out:
>>   	if (rs_tcp)
>> -- 
>> 2.7.4
>>
> --
> 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

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

Re: [PATCH 1/5] rds: tcp: release the created connection

[Sowmini Varadhan]

On (03/27/17 16:05), Yanjun Zhu wrote:
> Sorry. I just made simple test. It seems that it worked well. Would you like
> to show me some test about this patch?
> 

Your patch is a non-trivial bug. Dont do this.

Please read the comments above the "goto rst_nsk" in that function.
Also note the comments above rds_conn_destroy.

Thanks
--Sowmini