[openib-general] Re: [PATCH] [CM] 3/6 SDP updates to bind cm_id's to a device
Michael S. Tsirkin
mst at mellanox.co.il
Wed Sep 7 02:14:14 PDT 2005
Hello, Sean!
Thanks for the patch. The changes in SDP seem quite small,
I think this validates that the new API is easy enough to use.
Quoting Sean Hefty <sean.hefty at intel.com>:
> Subject: RE: [PATCH] [CM] 3/6 SDP updates to bind cm_id's to a device
>
> This patch updates SDP to use the new ib_create_cm_id() API.
I wander if the API updates could be split to a separate patch.
> It also replaces the state driven CM callback processing model with
> the more reliable event driven processing model.
Could you please elaborate on why is an event driven model more reliable than
the state driven one?
It certainly seems to require more code: isnt cm_id->state set by CM to
a valid value? It seems that cm needs to track connection state
anyway as per "12.9.2 Invalid State Input Handling", to know which messages
are legal. Is this done? If so why isnt it a good idea to reuse that state
in SDP?
> This patch is for review and is untested.
>
> Signed-off-by: Sean Hefty <sean.hefty at intel.com>
A couple of comments below.
> Index: ulp/sdp/sdp_actv.c
> ===================================================================
> --- ulp/sdp/sdp_actv.c (revision 3295)
> +++ ulp/sdp/sdp_actv.c (working copy)
> @@ -480,7 +480,7 @@ static void sdp_cm_path_complete(u64 id,
> /* XXX set timeout to default value of 14 */
> path->packet_life = 13;
> #endif
> - conn->cm_id = ib_create_cm_id(sdp_cm_event_handler,
> + conn->cm_id = ib_create_cm_id(ca, sdp_cm_event_handler,
> hashent_arg(conn->hashent));
> if (!conn->cm_id) {
> sdp_dbg_warn(conn, "Failed to create CM handle, %d",
> Index: ulp/sdp/sdp_conn.c
> ===================================================================
> --- ulp/sdp/sdp_conn.c (revision 3295)
> +++ ulp/sdp/sdp_conn.c (working copy)
> @@ -1801,11 +1801,29 @@ static void sdp_device_init_one(struct i
> }
> }
>
> + hca->listen_id = ib_create_cm_id(device, sdp_cm_event_handler, hca);
> + if (IS_ERR(hca->listen_id)) {
> + sdp_warn("Error <%ld> creating listen ID on <%s>.",
> + PTR_ERR(hca->listen_id), device->name);
> + goto error;
> + }
> +
> + result = ib_cm_listen(hca->listen_id,
> + cpu_to_be64(SDP_MSG_SERVICE_ID_VALUE),
> + cpu_to_be64(SDP_MSG_SERVICE_ID_MASK));
> + if (result) {
> + sdp_warn("Error <%d> listening for SDP connections", result);
> + goto error;
> + }
> +
> ib_set_client_data(device, &sdp_client, hca);
>
> return;
Can a listen event arrive before we call ib_set_client_data?
> error:
> + if (!IS_ERR(hca->listen_id))
> + ib_destroy_cm_id(hca->listen_id);
> +
> list_for_each_entry_safe(port, tmp, &hca->port_list, list) {
> list_del(&port->list);
> kfree(port);
> @@ -1838,6 +1856,9 @@ static void sdp_device_remove_one(struct
> return;
> }
>
> + if (!IS_ERR(hca->listen_id))
> + ib_destroy_cm_id(hca->listen_id);
> +
> list_for_each_entry_safe(port, tmp, &hca->port_list, list) {
> list_del(&port->list);
> kfree(port);
I'd prefer separate labels for two errors, instead of testing IS_ERR twice.
> @@ -1938,31 +1959,9 @@ int sdp_conn_table_init(int proto_family
> goto error_iocb;
> }
>
> - /*
> - * start listening
> - */
> - dev_root_s.listen_id = ib_create_cm_id(sdp_cm_event_handler,
> - (void *)SDP_DEV_SK_INVALID);
> - if (!dev_root_s.listen_id) {
> - sdp_warn("Failed to create listen connection identifier.");
> - result = -ENOMEM;
> - goto error_conn;
> - }
> -
> - result = ib_cm_listen(dev_root_s.listen_id,
> - cpu_to_be64(SDP_MSG_SERVICE_ID_VALUE),
> - cpu_to_be64(SDP_MSG_SERVICE_ID_MASK));
> - if (result) {
> - sdp_warn("Error <%d> listening for SDP connections", result);
> - goto error_listen;
> -
> - }
> -
> sdp_dbg_init("Started listening for SDP connection requests");
>
> return 0;
> -error_listen:
> - ib_destroy_cm_id(dev_root_s.listen_id);
> error_conn:
> sdp_main_iocb_cleanup();
> error_iocb:
> @@ -2003,8 +2002,4 @@ void sdp_conn_table_clear(void)
> * delete IOCB table
> */
> sdp_main_iocb_cleanup();
> - /*
> - * stop listening
> - */
> - ib_destroy_cm_id(dev_root_s.listen_id);
> }
> Index: ulp/sdp/sdp_event.c
> ===================================================================
> --- ulp/sdp/sdp_event.c (revision 3295)
> +++ ulp/sdp/sdp_event.c (working copy)
> @@ -384,45 +384,46 @@ int sdp_cm_event_handler(struct ib_cm_id
> struct sdp_sock *conn = NULL;
> int result = 0;
>
> - sdp_dbg_ctrl(NULL, "CM state <%d> event <%d> commID <%08x> ID <%d>",
> - cm_id->state, event->event, cm_id->local_id, hashent);
> - /*
> - * lookup the connection, on a REQ_RECV the sk will be empty.
> - */
> - conn = sdp_conn_table_lookup(hashent);
> - if (conn)
> - sdp_conn_lock(conn);
> - else
> - if (cm_id->state != IB_CM_REQ_RCVD) {
> - sdp_dbg_warn(NULL,
> - "No conn <%d> CM state <%d> event <%d>",
> - hashent, cm_id->state, event->event);
> + sdp_dbg_ctrl(NULL, "event <%d> commID <%08x> ID <%d>",
> + event->event, cm_id->local_id, hashent);
> +
> + if (event->event != IB_CM_REQ_RECEIVED) {
> + conn = sdp_conn_table_lookup(hashent);
> + if (conn)
> + sdp_conn_lock(conn);
> + else
> return -EINVAL;
> - }
> + /* Can this fail? Why not just set context = conn? */
> + }
Regarding the question: the first thing we do in sdp_conn_put
is sdp_conn_table_remove, so I think this lookup can fail.
> - switch (cm_id->state) {
> - case IB_CM_REQ_RCVD:
> + switch (event->event) {
> + case IB_CM_REQ_RECEIVED:
> result = sdp_cm_req_handler(cm_id, event);
> break;
> - case IB_CM_REP_RCVD:
> + case IB_CM_REP_RECEIVED:
> result = sdp_cm_rep_handler(cm_id, event, conn);
> break;
> - case IB_CM_IDLE:
> + case IB_CM_REQ_ERROR:
> + case IB_CM_REP_ERROR:
> + case IB_CM_REJ_RECEIVED:
> + case IB_CM_TIMEWAIT_EXIT:
> result = sdp_cm_idle(cm_id, event, conn);
> break;
> - case IB_CM_ESTABLISHED:
> + case IB_CM_RTU_RECEIVED:
> + case IB_CM_USER_ESTABLISHED:
> result = sdp_cm_established(cm_id, event, conn);
> break;
> - case IB_CM_DREQ_RCVD:
> + case IB_CM_DREQ_RECEIVED:
> result = sdp_cm_dreq_rcvd(cm_id, event, conn);
> if (result)
> break;
> /* fall through on success to handle state transition */
> - case IB_CM_TIMEWAIT:
> + case IB_CM_DREQ_ERROR:
> + case IB_CM_DREP_RECEIVED:
> result = sdp_cm_timewait(cm_id, event, conn);
> break;
> default:
> - sdp_dbg_warn(conn, "Unexpected CM state <%d>", cm_id->state);
> + sdp_dbg_warn(conn, "Unhandled CM event <%d>", event->event);
> result = -EINVAL;
> }
> /*
Seems more code. Could you please elaborate on why is this more correct?
> Index: ulp/sdp/sdp_dev.h
> ===================================================================
> --- ulp/sdp/sdp_dev.h (revision 3295)
> +++ ulp/sdp/sdp_dev.h (working copy)
> @@ -161,6 +161,7 @@ struct sdev_hca {
> u32 r_key; /* remote key */
> struct ib_fmr_pool *fmr_pool; /* fast memory for Zcopy */
> struct list_head port_list; /* ports on this HCA */
> + struct ib_cm_id *listen_id;
> };
>
> struct sdev_root {
> @@ -194,10 +195,6 @@ struct sdev_root {
> spinlock_t bind_lock;
> spinlock_t sock_lock;
> spinlock_t listen_lock;
> - /*
> - * SDP wide listen
> - */
> - struct ib_cm_id *listen_id; /* listen handle */
> };
>
> #endif /* _SDP_DEV_H */
>
Thanks,
MST
--
MST
More information about the general
mailing list