diff mbox series

[1/2] svcrdma: Refactor the creation of listener CMA ID

Message ID 20240531131550.64044-5-cel@kernel.org (mailing list archive)
State New, archived
Headers show
Series Fix ADDR_CHANGE event handling for NFSD | expand

Commit Message

Chuck Lever May 31, 2024, 1:15 p.m. UTC 
From: Chuck Lever <chuck.lever@oracle.com>

In a moment, I will add a second consumer of CMA ID creation in
svcrdma. Refactor so this code can be reused.

Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
---
 net/sunrpc/xprtrdma/svc_rdma_transport.c | 67 ++++++++++++++----------
 1 file changed, 40 insertions(+), 27 deletions(-)

Comments

Guoqing Jiang June 3, 2024, 10:59 a.m. UTC | #1 
On 5/31/24 21:15, cel@kernel.org wrote:
> From: Chuck Lever <chuck.lever@oracle.com>
>
> In a moment, I will add a second consumer of CMA ID creation in
> svcrdma. Refactor so this code can be reused.
>
> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> ---
>   net/sunrpc/xprtrdma/svc_rdma_transport.c | 67 ++++++++++++++----------
>   1 file changed, 40 insertions(+), 27 deletions(-)
>
> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> index 2b1c16b9547d..fa50b7494a0a 100644
> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> @@ -65,6 +65,8 @@
>   
>   static struct svcxprt_rdma *svc_rdma_create_xprt(struct svc_serv *serv,
>   						 struct net *net, int node);
> +static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id,
> +				   struct rdma_cm_event *event);
>   static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
>   					struct net *net,
>   					struct sockaddr *sa, int salen,
> @@ -122,6 +124,41 @@ static void qp_event_handler(struct ib_event *event, void *context)
>   	}
>   }
>   
> +static struct rdma_cm_id *
> +svc_rdma_create_listen_id(struct net *net, struct sockaddr *sap,
> +			  void *context)
> +{
> +	struct rdma_cm_id *listen_id;
> +	int ret;
> +
> +	listen_id = rdma_create_id(net, svc_rdma_listen_handler, context,
> +				   RDMA_PS_TCP, IB_QPT_RC);
> +	if (IS_ERR(listen_id))
> +		return listen_id;

I am wondering if above need to return PTR_ERR(listen_id), and I find 
some callers (in net/rds/, nvme etc)
return PTR_ERR(id) while others (rtrs-srv, ib_isert.c) return 
ERR_PTR(ret) with ret is set to PTR_ERR(id).

Thanks,
Guoqing
Chuck Lever III June 3, 2024, 1:23 p.m. UTC | #2 
On Mon, Jun 03, 2024 at 06:59:13PM +0800, Guoqing Jiang wrote:
> 
> 
> On 5/31/24 21:15, cel@kernel.org wrote:
> > From: Chuck Lever <chuck.lever@oracle.com>
> > 
> > In a moment, I will add a second consumer of CMA ID creation in
> > svcrdma. Refactor so this code can be reused.
> > 
> > Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
> > ---
> >   net/sunrpc/xprtrdma/svc_rdma_transport.c | 67 ++++++++++++++----------
> >   1 file changed, 40 insertions(+), 27 deletions(-)
> > 
> > diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > index 2b1c16b9547d..fa50b7494a0a 100644
> > --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
> > @@ -65,6 +65,8 @@
> >   static struct svcxprt_rdma *svc_rdma_create_xprt(struct svc_serv *serv,
> >   						 struct net *net, int node);
> > +static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id,
> > +				   struct rdma_cm_event *event);
> >   static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
> >   					struct net *net,
> >   					struct sockaddr *sa, int salen,
> > @@ -122,6 +124,41 @@ static void qp_event_handler(struct ib_event *event, void *context)
> >   	}
> >   }
> > +static struct rdma_cm_id *
> > +svc_rdma_create_listen_id(struct net *net, struct sockaddr *sap,
> > +			  void *context)
> > +{
> > +	struct rdma_cm_id *listen_id;
> > +	int ret;
> > +
> > +	listen_id = rdma_create_id(net, svc_rdma_listen_handler, context,
> > +				   RDMA_PS_TCP, IB_QPT_RC);
> > +	if (IS_ERR(listen_id))
> > +		return listen_id;
> 
> I am wondering if above need to return PTR_ERR(listen_id),

PTR_ERR would convert the listen_id error to an integer, but
svc_rdma_create_listen_id() returns a pointer or an ERR_PTR. Thus
using PTR_ERR() would be wrong in this case.


> and I find some
> callers (in net/rds/, nvme etc)
> return PTR_ERR(id) while others (rtrs-srv, ib_isert.c) return ERR_PTR(ret)
> with ret is set to PTR_ERR(id).

These functions use PTR_ERR only when the calling function returns
an int.
Guoqing Jiang June 3, 2024, 2:03 p.m. UTC | #3 
On 6/3/24 21:23, Chuck Lever wrote:
> On Mon, Jun 03, 2024 at 06:59:13PM +0800, Guoqing Jiang wrote:
>>
>> On 5/31/24 21:15, cel@kernel.org wrote:
>>> From: Chuck Lever <chuck.lever@oracle.com>
>>>
>>> In a moment, I will add a second consumer of CMA ID creation in
>>> svcrdma. Refactor so this code can be reused.
>>>
>>> Signed-off-by: Chuck Lever <chuck.lever@oracle.com>
>>> ---
>>>    net/sunrpc/xprtrdma/svc_rdma_transport.c | 67 ++++++++++++++----------
>>>    1 file changed, 40 insertions(+), 27 deletions(-)
>>>
>>> diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> index 2b1c16b9547d..fa50b7494a0a 100644
>>> --- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> +++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
>>> @@ -65,6 +65,8 @@
>>>    static struct svcxprt_rdma *svc_rdma_create_xprt(struct svc_serv *serv,
>>>    						 struct net *net, int node);
>>> +static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id,
>>> +				   struct rdma_cm_event *event);
>>>    static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
>>>    					struct net *net,
>>>    					struct sockaddr *sa, int salen,
>>> @@ -122,6 +124,41 @@ static void qp_event_handler(struct ib_event *event, void *context)
>>>    	}
>>>    }
>>> +static struct rdma_cm_id *
>>> +svc_rdma_create_listen_id(struct net *net, struct sockaddr *sap,
>>> +			  void *context)
>>> +{
>>> +	struct rdma_cm_id *listen_id;
>>> +	int ret;
>>> +
>>> +	listen_id = rdma_create_id(net, svc_rdma_listen_handler, context,
>>> +				   RDMA_PS_TCP, IB_QPT_RC);
>>> +	if (IS_ERR(listen_id))
>>> +		return listen_id;
>> I am wondering if above need to return PTR_ERR(listen_id),
> PTR_ERR would convert the listen_id error to an integer, but
> svc_rdma_create_listen_id() returns a pointer or an ERR_PTR. Thus
> using PTR_ERR() would be wrong in this case.
>
>
>> and I find some
>> callers (in net/rds/, nvme etc)
>> return PTR_ERR(id) while others (rtrs-srv, ib_isert.c) return ERR_PTR(ret)
>> with ret is set to PTR_ERR(id).
> These functions use PTR_ERR only when the calling function returns
> an int.

Thanks for the explanation!

Guoqing
diff mbox series

Patch

diff --git a/net/sunrpc/xprtrdma/svc_rdma_transport.c b/net/sunrpc/xprtrdma/svc_rdma_transport.c
index 2b1c16b9547d..fa50b7494a0a 100644
--- a/net/sunrpc/xprtrdma/svc_rdma_transport.c
+++ b/net/sunrpc/xprtrdma/svc_rdma_transport.c
@@ -65,6 +65,8 @@ 
 
 static struct svcxprt_rdma *svc_rdma_create_xprt(struct svc_serv *serv,
 						 struct net *net, int node);
+static int svc_rdma_listen_handler(struct rdma_cm_id *cma_id,
+				   struct rdma_cm_event *event);
 static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
 					struct net *net,
 					struct sockaddr *sa, int salen,
@@ -122,6 +124,41 @@  static void qp_event_handler(struct ib_event *event, void *context)
 	}
 }
 
+static struct rdma_cm_id *
+svc_rdma_create_listen_id(struct net *net, struct sockaddr *sap,
+			  void *context)
+{
+	struct rdma_cm_id *listen_id;
+	int ret;
+
+	listen_id = rdma_create_id(net, svc_rdma_listen_handler, context,
+				   RDMA_PS_TCP, IB_QPT_RC);
+	if (IS_ERR(listen_id))
+		return listen_id;
+
+	/* Allow both IPv4 and IPv6 sockets to bind a single port
+	 * at the same time.
+	 */
+#if IS_ENABLED(CONFIG_IPV6)
+	ret = rdma_set_afonly(listen_id, 1);
+	if (ret)
+		goto out_destroy;
+#endif
+	ret = rdma_bind_addr(listen_id, sap);
+	if (ret)
+		goto out_destroy;
+
+	ret = rdma_listen(listen_id, RPCRDMA_LISTEN_BACKLOG);
+	if (ret)
+		goto out_destroy;
+
+	return listen_id;
+
+out_destroy:
+	rdma_destroy_id(listen_id);
+	return ERR_PTR(ret);
+}
+
 static struct svcxprt_rdma *svc_rdma_create_xprt(struct svc_serv *serv,
 						 struct net *net, int node)
 {
@@ -307,7 +344,6 @@  static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
 {
 	struct rdma_cm_id *listen_id;
 	struct svcxprt_rdma *cma_xprt;
-	int ret;
 
 	if (sa->sa_family != AF_INET && sa->sa_family != AF_INET6)
 		return ERR_PTR(-EAFNOSUPPORT);
@@ -317,30 +353,13 @@  static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
 	set_bit(XPT_LISTENER, &cma_xprt->sc_xprt.xpt_flags);
 	strcpy(cma_xprt->sc_xprt.xpt_remotebuf, "listener");
 
-	listen_id = rdma_create_id(net, svc_rdma_listen_handler, cma_xprt,
-				   RDMA_PS_TCP, IB_QPT_RC);
+	listen_id = svc_rdma_create_listen_id(net, sa, cma_xprt);
 	if (IS_ERR(listen_id)) {
-		ret = PTR_ERR(listen_id);
-		goto err0;
+		kfree(cma_xprt);
+		return (struct svc_xprt *)listen_id;
 	}
-
-	/* Allow both IPv4 and IPv6 sockets to bind a single port
-	 * at the same time.
-	 */
-#if IS_ENABLED(CONFIG_IPV6)
-	ret = rdma_set_afonly(listen_id, 1);
-	if (ret)
-		goto err1;
-#endif
-	ret = rdma_bind_addr(listen_id, sa);
-	if (ret)
-		goto err1;
 	cma_xprt->sc_cm_id = listen_id;
 
-	ret = rdma_listen(listen_id, RPCRDMA_LISTEN_BACKLOG);
-	if (ret)
-		goto err1;
-
 	/*
 	 * We need to use the address from the cm_id in case the
 	 * caller specified 0 for the port number.
@@ -349,12 +368,6 @@  static struct svc_xprt *svc_rdma_create(struct svc_serv *serv,
 	svc_xprt_set_local(&cma_xprt->sc_xprt, sa, salen);
 
 	return &cma_xprt->sc_xprt;
-
- err1:
-	rdma_destroy_id(listen_id);
- err0:
-	kfree(cma_xprt);
-	return ERR_PTR(ret);
 }
 
 /*