* Questions regarding CMA
@ 2011-07-14 7:58 Jack Morgenstein
[not found] ` <201107141058.29879.jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
0 siblings, 1 reply; 7+ messages in thread
From: Jack Morgenstein @ 2011-07-14 7:58 UTC (permalink / raw)
To: Sean Hefty
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, dotanb-VPRAkNaXOzVS1MOuV/RT9w
I am currently reviewing/cleaning up our CMA LAP patches for submission.
I have several questions regarding the file cma.c, which I would like you to clarify for me.
1. In procedure cma_ib_listen (and elsewhere), if the call to ib_create_cm_id fails,
id_priv->cm_id.ib is left with the (non-zero) error value instead of NULL.
However, if there is a failure later in the procedure, you set id_priv->cm_id.ib to NULL.
Is there a reason for this difference in behavior?
(code snippet from current code is below)
int cma_ib_listen(struct rdma_id_
id_priv->cm_id.ib = ib_create_cm_id(id_priv->id.device, cma_req_handler,
id_priv);
if (IS_ERR(id_priv->cm_id.ib))
return PTR_ERR(id_priv->cm_id.ib);
....
if (ret) {
ib_destroy_cm_id(id_priv->cm_id.ib);
id_priv->cm_id.ib = NULL;
}
2. procedure cma_has_cm_dev looks as follows:
static int cma_has_cm_dev(struct rdma_id_private *id_priv)
{
return (id_priv->id.device && id_priv->cm_id.ib);
}
Shouldn't the line be:
return (id_priv->id.device && id_priv->cm_id.ib &&
!IS_ERR(id_priv->cm_id.ib));
3. There are several places where the value of id_priv->cm_id.ib
is checked to be not NULL.
Wouldn't it be better to call cma_has_cm_dev in these places
(when cma_has_cm_dev has been fixed, as I suggested in 2 above).
Please consider the patch below as a starting point (I did not touch the iwarp code).
Please let me know (ASAP) what you think.
(I still leave a window here where id_priv->cm_id.ib is ERR. Is this a problem?
Would it be better to use a local variable instead of id_priv->cm_id.ib, and
only assign to id_priv->cm_id.ib when all the error checks have passed?
This would leave a window where a successful cm_id creation is not immediately assigned
to the CMA object -- would this be a problem?).
Thanks!
-Jack
====================================================================
--- cma.c 2011-07-13 09:54:09.000000000 +0300
+++ cma_fixed.c 2011-07-14 10:51:23.000000000 +0300
@@ -424,7 +424,8 @@ static int cma_disable_callback(struct r
static int cma_has_cm_dev(struct rdma_id_private *id_priv)
{
- return (id_priv->id.device && id_priv->cm_id.ib);
+ return (id_priv->id.device && id_priv->cm_id.ib &&
+ !IS_ERR(id_priv->cm_id.ib));
}
struct rdma_cm_id *rdma_create_id(rdma_cm_event_handler event_handler,
@@ -658,7 +659,7 @@ int rdma_init_qp_attr(struct rdma_cm_id
id_priv = container_of(id, struct rdma_id_private, id);
switch (rdma_node_get_transport(id_priv->id.device->node_type)) {
case RDMA_TRANSPORT_IB:
- if (!id_priv->cm_id.ib || cma_is_ud_ps(id_priv->id.ps))
+ if (!cma_has_cm_dev(id_priv) || cma_is_ud_ps(id_priv->id.ps))
ret = cma_ib_init_qp_attr(id_priv, qp_attr, qp_attr_mask);
else
ret = ib_cm_init_qp_attr(id_priv->cm_id.ib, qp_attr,
@@ -918,7 +919,7 @@ void rdma_destroy_id(struct rdma_cm_id *
if (id_priv->cma_dev) {
switch (rdma_node_get_transport(id_priv->id.device->node_type)) {
case RDMA_TRANSPORT_IB:
- if (id_priv->cm_id.ib && !IS_ERR(id_priv->cm_id.ib))
+ if (cma_has_cm_dev(id_priv))
ib_destroy_cm_id(id_priv->cm_id.ib);
break;
case RDMA_TRANSPORT_IWARP:
@@ -1471,8 +1472,10 @@ static int cma_ib_listen(struct rdma_id_
id_priv->cm_id.ib = ib_create_cm_id(id_priv->id.device, cma_req_handler,
id_priv);
- if (IS_ERR(id_priv->cm_id.ib))
- return PTR_ERR(id_priv->cm_id.ib);
+ if (IS_ERR(id_priv->cm_id.ib)) {
+ ret = PTR_ERR(id_priv->cm_id.ib);
+ goto out;
+ }
addr = (struct sockaddr *) &id_priv->id.route.addr.src_addr;
svc_id = cma_get_service_id(id_priv->id.ps, addr);
@@ -1482,9 +1485,10 @@ static int cma_ib_listen(struct rdma_id_
cma_set_compare_data(id_priv->id.ps, addr, &compare_data);
ret = ib_cm_listen(id_priv->cm_id.ib, svc_id, 0, &compare_data);
}
-
+out:
if (ret) {
- ib_destroy_cm_id(id_priv->cm_id.ib);
+ if (!IS_ERR(id_priv->cm_id.ib))
+ ib_destroy_cm_id(id_priv->cm_id.ib);
id_priv->cm_id.ib = NULL;
}
@@ -2454,11 +2458,12 @@ static int cma_resolve_ib_udp(struct rdm
req.max_cm_retries = CMA_MAX_CM_RETRIES;
ret = ib_send_cm_sidr_req(id_priv->cm_id.ib, &req);
+out:
if (ret) {
- ib_destroy_cm_id(id_priv->cm_id.ib);
+ if (!IS_ERR(id_priv->cm_id.ib))
+ ib_destroy_cm_id(id_priv->cm_id.ib);
id_priv->cm_id.ib = NULL;
}
-out:
kfree(req.private_data);
return ret;
}
@@ -2516,8 +2521,9 @@ static int cma_connect_ib(struct rdma_id
ret = ib_send_cm_req(id_priv->cm_id.ib, &req);
out:
- if (ret && !IS_ERR(id_priv->cm_id.ib)) {
- ib_destroy_cm_id(id_priv->cm_id.ib);
+ if (ret) {
+ if (!IS_ERR(id_priv->cm_id.ib))
+ ib_destroy_cm_id(id_priv->cm_id.ib);
id_priv->cm_id.ib = NULL;
}
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* RE: Questions regarding CMA
[not found] ` <201107141058.29879.jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
@ 2011-07-14 17:46 ` Hefty, Sean
[not found] ` <1828884A29C6694DAF28B7E6B8A8237302E14C-P5GAC/sN6hmkrb+BlOpmy7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
0 siblings, 1 reply; 7+ messages in thread
From: Hefty, Sean @ 2011-07-14 17:46 UTC (permalink / raw)
To: Jack Morgenstein
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, dotanb-VPRAkNaXOzVS1MOuV/RT9w
> 1. In procedure cma_ib_listen (and elsewhere), if the call to ib_create_cm_id
> fails,
> id_priv->cm_id.ib is left with the (non-zero) error value instead of NULL.
> However, if there is a failure later in the procedure, you set id_priv-
> >cm_id.ib to NULL.
>
> Is there a reason for this difference in behavior?
> (code snippet from current code is below)
> int cma_ib_listen(struct rdma_id_
>
> id_priv->cm_id.ib = ib_create_cm_id(id_priv->id.device,
> cma_req_handler,
> id_priv);
> if (IS_ERR(id_priv->cm_id.ib))
> return PTR_ERR(id_priv->cm_id.ib);
This looks like a bug.
>
> ....
> if (ret) {
> ib_destroy_cm_id(id_priv->cm_id.ib);
> id_priv->cm_id.ib = NULL;
> }
>
>
> 2. procedure cma_has_cm_dev looks as follows:
>
> static int cma_has_cm_dev(struct rdma_id_private *id_priv)
> {
> return (id_priv->id.device && id_priv->cm_id.ib);
> }
>
> Shouldn't the line be:
> return (id_priv->id.device && id_priv->cm_id.ib &&
> !IS_ERR(id_priv->cm_id.ib));
Given the current code, adding IS_ERR makes sense, but see below. Thinking about this, we shouldn't need to check id_priv->id.device. If we have id_priv->cm_id.ib, then the device pointer must be valid.
(fyi: cma_has_cm_dev() was added by commit 6c719f5c6c823901fac2d46b83db5a69ba7e9152. It replaced a state check with the above device check to handle device removal.)
> 3. There are several places where the value of id_priv->cm_id.ib
> is checked to be not NULL.
>
> Wouldn't it be better to call cma_has_cm_dev in these places
> (when cma_has_cm_dev has been fixed, as I suggested in 2 above).
Although it's a minor performance gain, I'm leaning towards keeping id_priv->cm_id.ib = NULL on failure, either by resetting it or using a local variable. cma_has_cm_dev() can then be replaced by checking id_priv->cm_id.ib for non-NULL, and checks for IS_ERR(id_priv->cm_id.ib) are removed.
> Please consider the patch below as a starting point (I did not touch the iwarp
> code).
> Please let me know (ASAP) what you think.
> (I still leave a window here where id_priv->cm_id.ib is ERR. Is this a
> problem?
> Would it be better to use a local variable instead of id_priv->cm_id.ib, and
> only assign to id_priv->cm_id.ib when all the error checks have passed?
> This would leave a window where a successful cm_id creation is not
> immediately assigned
> to the CMA object -- would this be a problem?).
Without adding more synchronization, we need to ensure that id_priv->cm_id.ib is set before a user can receive any callbacks. This restricts our use of a local variable to the create_cm_id calls only. See cma_connect_iw() for an example of using this approach.
Thanks,
- Sean
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: Questions regarding CMA
[not found] ` <1828884A29C6694DAF28B7E6B8A8237302E14C-P5GAC/sN6hmkrb+BlOpmy7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
@ 2011-07-17 7:08 ` Jack Morgenstein
2011-07-17 10:46 ` [PATCH] rdma_cm: avoid assigning an IS_ERR value to cm id pointer in CMA id object Jack Morgenstein
1 sibling, 0 replies; 7+ messages in thread
From: Jack Morgenstein @ 2011-07-17 7:08 UTC (permalink / raw)
To: Hefty, Sean
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, dotanb-VPRAkNaXOzVS1MOuV/RT9w
On Thursday 14 July 2011 20:46, Hefty, Sean wrote:
> Although it's a minor performance gain, I'm leaning towards keeping id_priv->cm_id.ib = NULL on failure, either by resetting it or using a local variable. cma_has_cm_dev() can then be replaced by checking id_priv->cm_id.ib for non-NULL, and checks for IS_ERR(id_priv->cm_id.ib) are removed.
>
> > Please consider the patch below as a starting point (I did not touch the iwarp
> > code).
> > Please let me know (ASAP) what you think.
> > (I still leave a window here where id_priv->cm_id.ib is ERR. Is this a
> > problem?
> > Would it be better to use a local variable instead of id_priv->cm_id.ib, and
> > only assign to id_priv->cm_id.ib when all the error checks have passed?
> > This would leave a window where a successful cm_id creation is not
> > immediately assigned
> > to the CMA object -- would this be a problem?).
>
> Without adding more synchronization, we need to ensure that id_priv->cm_id.ib is set before a user can receive any callbacks.
> This restricts our use of a local variable to the create_cm_id calls only.
> See cma_connect_iw() for an example of using this approach.
>
Agreed. That way we never have to test for IS_ERR. I'll send a patch.
-Jack
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH] rdma_cm: avoid assigning an IS_ERR value to cm id pointer in CMA id object
[not found] ` <1828884A29C6694DAF28B7E6B8A8237302E14C-P5GAC/sN6hmkrb+BlOpmy7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
2011-07-17 7:08 ` Jack Morgenstein
@ 2011-07-17 10:46 ` Jack Morgenstein
[not found] ` <201107171346.48065.jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
1 sibling, 1 reply; 7+ messages in thread
From: Jack Morgenstein @ 2011-07-17 10:46 UTC (permalink / raw)
To: Hefty, Sean
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, dotanb-VPRAkNaXOzVS1MOuV/RT9w
Avoid assigning an IS_ERR value to the cm id pointer in the cma_id object.
This fixes a few anomalies in the error flow, and eliminates the need to
test for the IS_ERR value every time we wish to determine if the cma_id object
has a cm device associated with it.
Also, eliminate the now-unnecessary procedure cma_has_cm_dev (we can check directly
for the existence of the device pointer -- for a non-NULL check, makes no difference
if it is the iwarp or the ib pointer).
Finally, I suggest a few code changes here to improve coding consistency.
Signed-off-by: Jack Morgenstein <jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
---
Sean,
Here is the patch I said I would submit to you.
I got rid of the cma_has_cm_dev() procedure, but if you think that it should be kept,
but simply do the ib-pointer check only (to keep ib out of the rdma level), that is OK with me.
Jack
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index b6a33b3..85ce489 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -406,11 +406,6 @@ static int cma_disable_callback(struct rdma_id_private *id_priv,
return 0;
}
-static int cma_has_cm_dev(struct rdma_id_private *id_priv)
-{
- return (id_priv->id.device && id_priv->cm_id.ib);
-}
-
struct rdma_cm_id *rdma_create_id(rdma_cm_event_handler event_handler,
void *context, enum rdma_port_space ps,
enum ib_qp_type qp_type)
@@ -920,11 +915,11 @@ void rdma_destroy_id(struct rdma_cm_id *id)
if (id_priv->cma_dev) {
switch (rdma_node_get_transport(id_priv->id.device->node_type)) {
case RDMA_TRANSPORT_IB:
- if (id_priv->cm_id.ib && !IS_ERR(id_priv->cm_id.ib))
+ if (id_priv->cm_id.ib)
ib_destroy_cm_id(id_priv->cm_id.ib);
break;
case RDMA_TRANSPORT_IWARP:
- if (id_priv->cm_id.iw && !IS_ERR(id_priv->cm_id.iw))
+ if (id_priv->cm_id.iw)
iw_destroy_cm_id(id_priv->cm_id.iw);
break;
default:
@@ -1085,12 +1080,12 @@ static struct rdma_id_private *cma_new_conn_id(struct rdma_cm_id *listen_id,
if (cma_get_net_info(ib_event->private_data, listen_id->ps,
&ip_ver, &port, &src, &dst))
- goto err;
+ return NULL;
id = rdma_create_id(listen_id->event_handler, listen_id->context,
listen_id->ps, ib_event->param.req_rcvd.qp_type);
if (IS_ERR(id))
- goto err;
+ return NULL;
cma_save_net_info(&id->route.addr, &listen_id->route.addr,
ip_ver, port, src, dst);
@@ -1100,7 +1095,7 @@ static struct rdma_id_private *cma_new_conn_id(struct rdma_cm_id *listen_id,
rt->path_rec = kmalloc(sizeof *rt->path_rec * rt->num_paths,
GFP_KERNEL);
if (!rt->path_rec)
- goto destroy_id;
+ goto err;
rt->path_rec[0] = *ib_event->param.req_rcvd.primary_path;
if (rt->num_paths == 2)
@@ -1114,7 +1109,7 @@ static struct rdma_id_private *cma_new_conn_id(struct rdma_cm_id *listen_id,
ret = rdma_translate_ip((struct sockaddr *) &rt->addr.src_addr,
&rt->addr.dev_addr);
if (ret)
- goto destroy_id;
+ goto err;
}
rdma_addr_set_dgid(&rt->addr.dev_addr, &rt->path_rec[0].dgid);
@@ -1122,9 +1117,8 @@ static struct rdma_id_private *cma_new_conn_id(struct rdma_cm_id *listen_id,
id_priv->state = RDMA_CM_CONNECT;
return id_priv;
-destroy_id:
- rdma_destroy_id(id);
err:
+ rdma_destroy_id(id);
return NULL;
}
@@ -1468,13 +1462,15 @@ static int cma_ib_listen(struct rdma_id_private *id_priv)
{
struct ib_cm_compare_data compare_data;
struct sockaddr *addr;
+ struct ib_cm_id *id;
__be64 svc_id;
int ret;
- id_priv->cm_id.ib = ib_create_cm_id(id_priv->id.device, cma_req_handler,
- id_priv);
- if (IS_ERR(id_priv->cm_id.ib))
- return PTR_ERR(id_priv->cm_id.ib);
+ id = ib_create_cm_id(id_priv->id.device, cma_req_handler, id_priv);
+ if (IS_ERR(id))
+ return PTR_ERR(id);
+
+ id_priv->cm_id.ib = id;
addr = (struct sockaddr *) &id_priv->id.route.addr.src_addr;
svc_id = cma_get_service_id(id_priv->id.ps, addr);
@@ -1497,12 +1493,15 @@ static int cma_iw_listen(struct rdma_id_private *id_priv, int backlog)
{
int ret;
struct sockaddr_in *sin;
+ struct iw_cm_id *id;
- id_priv->cm_id.iw = iw_create_cm_id(id_priv->id.device,
- iw_conn_req_handler,
- id_priv);
- if (IS_ERR(id_priv->cm_id.iw))
- return PTR_ERR(id_priv->cm_id.iw);
+ id = iw_create_cm_id(id_priv->id.device,
+ iw_conn_req_handler,
+ id_priv);
+ if (IS_ERR(id))
+ return PTR_ERR(id);
+
+ id_priv->cm_id.iw = id;
sin = (struct sockaddr_in *) &id_priv->id.route.addr.src_addr;
id_priv->cm_id.iw->local_addr = *sin;
@@ -2484,6 +2483,7 @@ static int cma_resolve_ib_udp(struct rdma_id_private *id_priv,
{
struct ib_cm_sidr_req_param req;
struct rdma_route *route;
+ struct ib_cm_id *id;
int ret;
req.private_data_len = sizeof(struct cma_hdr) +
@@ -2501,12 +2501,13 @@ static int cma_resolve_ib_udp(struct rdma_id_private *id_priv,
if (ret)
goto out;
- id_priv->cm_id.ib = ib_create_cm_id(id_priv->id.device,
- cma_sidr_rep_handler, id_priv);
- if (IS_ERR(id_priv->cm_id.ib)) {
- ret = PTR_ERR(id_priv->cm_id.ib);
+ id = ib_create_cm_id(id_priv->id.device, cma_sidr_rep_handler,
+ id_priv);
+ if (IS_ERR(id)) {
+ ret = PTR_ERR(id);
goto out;
}
+ id_priv->cm_id.ib = id;
req.path = route->path_rec;
req.service_id = cma_get_service_id(id_priv->id.ps,
@@ -2530,6 +2531,7 @@ static int cma_connect_ib(struct rdma_id_private *id_priv,
struct ib_cm_req_param req;
struct rdma_route *route;
void *private_data;
+ struct ib_cm_id *id;
int offset, ret;
memset(&req, 0, sizeof req);
@@ -2543,12 +2545,12 @@ static int cma_connect_ib(struct rdma_id_private *id_priv,
memcpy(private_data + offset, conn_param->private_data,
conn_param->private_data_len);
- id_priv->cm_id.ib = ib_create_cm_id(id_priv->id.device, cma_ib_handler,
- id_priv);
- if (IS_ERR(id_priv->cm_id.ib)) {
- ret = PTR_ERR(id_priv->cm_id.ib);
+ id = ib_create_cm_id(id_priv->id.device, cma_ib_handler, id_priv);
+ if (IS_ERR(id)) {
+ ret = PTR_ERR(id);
goto out;
}
+ id_priv->cm_id.ib = id;
route = &id_priv->id.route;
ret = cma_format_hdr(private_data, id_priv->id.ps, route);
@@ -2577,7 +2579,7 @@ static int cma_connect_ib(struct rdma_id_private *id_priv,
ret = ib_send_cm_req(id_priv->cm_id.ib, &req);
out:
- if (ret && !IS_ERR(id_priv->cm_id.ib)) {
+ if (ret && !IS_ERR(id)) {
ib_destroy_cm_id(id_priv->cm_id.ib);
id_priv->cm_id.ib = NULL;
}
@@ -2595,10 +2597,8 @@ static int cma_connect_iw(struct rdma_id_private *id_priv,
struct iw_cm_conn_param iw_param;
cm_id = iw_create_cm_id(id_priv->id.device, cma_iw_handler, id_priv);
- if (IS_ERR(cm_id)) {
- ret = PTR_ERR(cm_id);
- goto out;
- }
+ if (IS_ERR(cm_id))
+ return PTR_ERR(cm_id);
id_priv->cm_id.iw = cm_id;
@@ -2622,7 +2622,7 @@ static int cma_connect_iw(struct rdma_id_private *id_priv,
iw_param.qpn = conn_param->qp_num;
ret = iw_cm_connect(cm_id, &iw_param);
out:
- if (ret && !IS_ERR(cm_id)) {
+ if (ret) {
iw_destroy_cm_id(cm_id);
id_priv->cm_id.iw = NULL;
}
@@ -2795,7 +2795,7 @@ int rdma_notify(struct rdma_cm_id *id, enum ib_event_type event)
int ret;
id_priv = container_of(id, struct rdma_id_private, id);
- if (!cma_has_cm_dev(id_priv))
+ if (!id_priv->cm_id.ib)
return -EINVAL;
switch (id->device->node_type) {
@@ -2817,7 +2817,7 @@ int rdma_reject(struct rdma_cm_id *id, const void *private_data,
int ret;
id_priv = container_of(id, struct rdma_id_private, id);
- if (!cma_has_cm_dev(id_priv))
+ if (!id_priv->cm_id.ib)
return -EINVAL;
switch (rdma_node_get_transport(id->device->node_type)) {
@@ -2848,7 +2848,7 @@ int rdma_disconnect(struct rdma_cm_id *id)
int ret;
id_priv = container_of(id, struct rdma_id_private, id);
- if (!cma_has_cm_dev(id_priv))
+ if (!id_priv->cm_id.ib)
return -EINVAL;
switch (rdma_node_get_transport(id->device->node_type)) {
--
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* RE: [PATCH] rdma_cm: avoid assigning an IS_ERR value to cm id pointer in CMA id object
[not found] ` <201107171346.48065.jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
@ 2011-07-18 16:35 ` Hefty, Sean
[not found] ` <1828884A29C6694DAF28B7E6B8A8237302E6B9-P5GAC/sN6hmkrb+BlOpmy7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
0 siblings, 1 reply; 7+ messages in thread
From: Hefty, Sean @ 2011-07-18 16:35 UTC (permalink / raw)
To: Jack Morgenstein
Cc: linux-rdma-u79uwXL29TY76Z2rM5mHXA, dotanb-VPRAkNaXOzVS1MOuV/RT9w
> Avoid assigning an IS_ERR value to the cm id pointer in the cma_id object.
> This fixes a few anomalies in the error flow, and eliminates the need to
> test for the IS_ERR value every time we wish to determine if the cma_id object
> has a cm device associated with it.
>
> Also, eliminate the now-unnecessary procedure cma_has_cm_dev (we can check
> directly
> for the existence of the device pointer -- for a non-NULL check, makes no
> difference
> if it is the iwarp or the ib pointer).
>
> Finally, I suggest a few code changes here to improve coding consistency.
>
> Signed-off-by: Jack Morgenstein <jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
>
> ---
> Sean,
> Here is the patch I said I would submit to you.
> I got rid of the cma_has_cm_dev() procedure, but if you think that it should
> be kept,
> but simply do the ib-pointer check only (to keep ib out of the rdma level),
> that is OK with me.
Thanks for doing this. The patch looks good to me with one nit:
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index b6a33b3..85ce489 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -2577,7 +2579,7 @@ static int cma_connect_ib(struct rdma_id_private
> *id_priv,
>
> ret = ib_send_cm_req(id_priv->cm_id.ib, &req);
> out:
> - if (ret && !IS_ERR(id_priv->cm_id.ib)) {
> + if (ret && !IS_ERR(id)) {
> ib_destroy_cm_id(id_priv->cm_id.ib);
I would change the above line to ib_destroy_cm_id(id) to align the destroy call with the if statement.
- Sean
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rdma_cm: avoid assigning an IS_ERR value to cm id pointer in CMA id object
[not found] ` <1828884A29C6694DAF28B7E6B8A8237302E6B9-P5GAC/sN6hmkrb+BlOpmy7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
@ 2011-07-18 16:39 ` Roland Dreier
[not found] ` <CAL1RGDWn_wqK3-0NM4NG1=wHw9_Q-2TT2goB_H_3RtyPEdS7aQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
0 siblings, 1 reply; 7+ messages in thread
From: Roland Dreier @ 2011-07-18 16:39 UTC (permalink / raw)
To: Hefty, Sean
Cc: Jack Morgenstein, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
dotanb-VPRAkNaXOzVS1MOuV/RT9w
On Mon, Jul 18, 2011 at 9:35 AM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
>> - if (ret && !IS_ERR(id_priv->cm_id.ib)) {
>> + if (ret && !IS_ERR(id)) {
>> ib_destroy_cm_id(id_priv->cm_id.ib);
>
> I would change the above line to ib_destroy_cm_id(id) to align the destroy call with the if statement.
OK, I'll grab this and make that suggested change as I merge it... no
need to resend.
- R.
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] rdma_cm: avoid assigning an IS_ERR value to cm id pointer in CMA id object
[not found] ` <CAL1RGDWn_wqK3-0NM4NG1=wHw9_Q-2TT2goB_H_3RtyPEdS7aQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
@ 2011-07-19 6:45 ` Jack Morgenstein
0 siblings, 0 replies; 7+ messages in thread
From: Jack Morgenstein @ 2011-07-19 6:45 UTC (permalink / raw)
To: Roland Dreier
Cc: Hefty, Sean, linux-rdma-u79uwXL29TY76Z2rM5mHXA,
dotanb-VPRAkNaXOzVS1MOuV/RT9w
On Monday 18 July 2011 19:39, Roland Dreier wrote:
> On Mon, Jul 18, 2011 at 9:35 AM, Hefty, Sean <sean.hefty-ral2JQCrhuEAvxtiuMwx3w@public.gmane.org> wrote:
> >> - if (ret && !IS_ERR(id_priv->cm_id.ib)) {
> >> + if (ret && !IS_ERR(id)) {
> >> ib_destroy_cm_id(id_priv->cm_id.ib);
> >
> > I would change the above line to ib_destroy_cm_id(id) to align the destroy call with the if statement.
>
> OK, I'll grab this and make that suggested change as I merge it... no
> need to resend.
>
> - R.
Thanks, Roland!
-Jack
--
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
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2011-07-19 6:45 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-07-14 7:58 Questions regarding CMA Jack Morgenstein
[not found] ` <201107141058.29879.jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
2011-07-14 17:46 ` Hefty, Sean
[not found] ` <1828884A29C6694DAF28B7E6B8A8237302E14C-P5GAC/sN6hmkrb+BlOpmy7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
2011-07-17 7:08 ` Jack Morgenstein
2011-07-17 10:46 ` [PATCH] rdma_cm: avoid assigning an IS_ERR value to cm id pointer in CMA id object Jack Morgenstein
[not found] ` <201107171346.48065.jackm-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>
2011-07-18 16:35 ` Hefty, Sean
[not found] ` <1828884A29C6694DAF28B7E6B8A8237302E6B9-P5GAC/sN6hmkrb+BlOpmy7fspsVTdybXVpNB7YpNyf8@public.gmane.org>
2011-07-18 16:39 ` Roland Dreier
[not found] ` <CAL1RGDWn_wqK3-0NM4NG1=wHw9_Q-2TT2goB_H_3RtyPEdS7aQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
2011-07-19 6:45 ` Jack Morgenstein
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.