Skip to contentSkip to conversation

connect reject event helpers

PATCH Superseded Steve Wise 5 participants 27 messages Oct 24, 2016 – Oct 25, 2016

Subsystems (5)
Networking InfiniBand NVMe RDS - reliable datagram sockets +1 more subsystems

v3 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Steve Wise

author
#0 lore raw

v3 cover connect reject event helpers

While reviewing:

http://lists.infradead.org/pipermail/linux-nvme/2016-October/006681.html

I decided to propose transport-agnostic helper functions to better handle connection reject event information. Included are patches for nvme_rdma, ib_ser, and rds_rdma to utilize the new helpers.

Changes since v2:

- reworked ibcm/iwcm_reject_msg() as per Christoph's recommendation
- use ibcm_ and iwcm_ prefix instead of ib_ and iw_ for reject_msg funcs
- change rdma_consumer_reject() to rdma_is_consumer_reject()
- add rdma_consumer_reject_data() helper function to return private data/len
- use new helpers in nvme_rdma, ib_iser, and rdma_rds
- in nvme_rdma, add strings for nvme_rdma_cm_status values

---

Steve Wise (6):
rdma_cm: add rdma_reject_msg() helper function
rdma_cm: add rdma_is_consumer_reject() helper function
rdma_cm: add rdma_consumer_reject_data helper function
nvme-rdma: use rdma connection reject helper functions
ib_iser: log the connection reject message
rds_rdma: log the connection reject message

 drivers/infiniband/core/cm.c             | 48 ++++++++++++++++++++++++++++++++
 drivers/infiniband/core/cma.c            | 43 ++++++++++++++++++++++++++++
 drivers/infiniband/core/iwcm.c           | 21 ++++++++++++++
 drivers/infiniband/ulp/iser/iser_verbs.c |  5 +++-
 drivers/nvme/host/rdma.c                 | 46 ++++++++++++++++++++++++------
 include/rdma/ib_cm.h                     |  6 ++++
 include/rdma/iw_cm.h                     |  6 ++++
 include/rdma/rdma_cm.h                   | 25 +++++++++++++++++
 net/rds/rdma_transport.c                 |  5 +++-
 9 files changed, 195 insertions(+), 10 deletions(-)
Signature · 1 line
2.7.0
author
#1 lore raw

PATCH v3 1/6 rdma_cm: add rdma_reject_msg() helper function

rdma_reject_msg() returns a pointer to a string message associated with the transport reject reason codes.

Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 drivers/infiniband/core/cm.c   | 48 ++++++++++++++++++++++++++++++++++++++++++
 drivers/infiniband/core/cma.c  | 14 ++++++++++++
 drivers/infiniband/core/iwcm.c | 21 ++++++++++++++++++
 include/rdma/ib_cm.h           |  6 ++++++
 include/rdma/iw_cm.h           |  6 ++++++
 include/rdma/rdma_cm.h         |  8 +++++++
 6 files changed, 103 insertions(+)
drivers/infiniband/core/cm.c +48 -0
index c995255..98bcc9c 100644
@@ -57,6 +57,54 @@ MODULE_AUTHOR("Sean Hefty");
 MODULE_DESCRIPTION("InfiniBand CM");
 MODULE_LICENSE("Dual BSD/GPL");
 
+static const char * const ibcm_rej_reason_strs[] = {
+	[IB_CM_REJ_NO_QP]			= "no qp",
+	[IB_CM_REJ_NO_EEC]			= "no eec",
+	[IB_CM_REJ_NO_RESOURCES]		= "no resources",
+	[IB_CM_REJ_TIMEOUT]			= "timeout",
+	[IB_CM_REJ_UNSUPPORTED]			= "unsupported",
+	[IB_CM_REJ_INVALID_COMM_ID]		= "invalid comm id",
+	[IB_CM_REJ_INVALID_COMM_INSTANCE]	= "invalid comm instance",
+	[IB_CM_REJ_INVALID_SERVICE_ID]		= "invalid service id",
+	[IB_CM_REJ_INVALID_TRANSPORT_TYPE]	= "invalid transport type",
+	[IB_CM_REJ_STALE_CONN]			= "stale conn",
+	[IB_CM_REJ_RDC_NOT_EXIST]		= "rdc not exist",
+	[IB_CM_REJ_INVALID_GID]			= "invalid gid",
+	[IB_CM_REJ_INVALID_LID]			= "invalid lid",
+	[IB_CM_REJ_INVALID_SL]			= "invalid sl",
+	[IB_CM_REJ_INVALID_TRAFFIC_CLASS]	= "invalid traffic class",
+	[IB_CM_REJ_INVALID_HOP_LIMIT]		= "invalid hop limit",
+	[IB_CM_REJ_INVALID_PACKET_RATE]		= "invalid packet rate",
+	[IB_CM_REJ_INVALID_ALT_GID]		= "invalid alt gid",
+	[IB_CM_REJ_INVALID_ALT_LID]		= "invalid alt lid",
+	[IB_CM_REJ_INVALID_ALT_SL]		= "invalid alt sl",
+	[IB_CM_REJ_INVALID_ALT_TRAFFIC_CLASS]	= "invalid alt traffic class",
+	[IB_CM_REJ_INVALID_ALT_HOP_LIMIT]	= "invalid alt hop limit",
+	[IB_CM_REJ_INVALID_ALT_PACKET_RATE]	= "invalid alt packet rate",
+	[IB_CM_REJ_PORT_CM_REDIRECT]		= "port cm redirect",
+	[IB_CM_REJ_PORT_REDIRECT]		= "port redirect",
+	[IB_CM_REJ_INVALID_MTU]			= "invalid mtu",
+	[IB_CM_REJ_INSUFFICIENT_RESP_RESOURCES]	= "insufficient resp resources",
+	[IB_CM_REJ_CONSUMER_DEFINED]		= "consumer defined",
+	[IB_CM_REJ_INVALID_RNR_RETRY]		= "invalid rnr retry",
+	[IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID]	= "duplicate local comm id",
+	[IB_CM_REJ_INVALID_CLASS_VERSION]	= "invalid class version",
+	[IB_CM_REJ_INVALID_FLOW_LABEL]		= "invalid flow label",
+	[IB_CM_REJ_INVALID_ALT_FLOW_LABEL]	= "invalid alt flow label",
+};
+
+const char *__attribute_const__ ibcm_reject_msg(int reason)
+{
+	size_t index = reason;
+
+	if (index >= ARRAY_SIZE(ibcm_rej_reason_strs) ||
+	    !ibcm_rej_reason_strs[index])
+		return "unrecognized reason";
+	else
+		return ibcm_rej_reason_strs[index];
+}
+EXPORT_SYMBOL(ibcm_reject_msg);
+
 static void cm_add_one(struct ib_device *device);
 static void cm_remove_one(struct ib_device *device, void *client_data);
drivers/infiniband/core/cma.c +14 -0
index 5f65a78..831e94b 100644
@@ -101,6 +101,20 @@ const char *__attribute_const__ rdma_event_msg(enum rdma_cm_event_type event)
 }
 EXPORT_SYMBOL(rdma_event_msg);
 
+const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
+						int reason)
+{
+	if (rdma_ib_or_roce(id->device, id->port_num))
+		return ibcm_reject_msg(reason);
+
+	if (rdma_protocol_iwarp(id->device, id->port_num))
+		return iwcm_reject_msg(reason);
+
+	WARN_ON_ONCE(1);
+	return "unrecognized reason";
+}
+EXPORT_SYMBOL(rdma_reject_msg);
+
 static void cma_add_one(struct ib_device *device);
 static void cma_remove_one(struct ib_device *device, void *client_data);
drivers/infiniband/core/iwcm.c +21 -0
index 357624f..8d01253 100644
@@ -59,6 +59,27 @@ MODULE_AUTHOR("Tom Tucker");
 MODULE_DESCRIPTION("iWARP CM");
 MODULE_LICENSE("Dual BSD/GPL");
 
+static const char * const iwcm_rej_reason_strs[] = {
+	[ECONNRESET]			= "reset by remote host",
+	[ECONNREFUSED]			= "refused by remote application",
+	[ETIMEDOUT]			= "setup timeout",
+};
+
+const char *__attribute_const__ iwcm_reject_msg(int reason)
+{
+	size_t index;
+
+	/* iWARP uses negative errnos */
+	index = -reason;
+
+	if (index >= ARRAY_SIZE(iwcm_rej_reason_strs) ||
+	    !iwcm_rej_reason_strs[index])
+		return "unrecognized reason";
+	else
+		return iwcm_rej_reason_strs[index];
+}
+EXPORT_SYMBOL(iwcm_reject_msg);
+
 static struct ibnl_client_cbs iwcm_nl_cb_table[] = {
 	[RDMA_NL_IWPM_REG_PID] = {.dump = iwpm_register_pid_cb},
 	[RDMA_NL_IWPM_ADD_MAPPING] = {.dump = iwpm_add_mapping_cb},
include/rdma/ib_cm.h +6 -0
index 92a7d85..b49258b 100644
@@ -603,4 +603,10 @@ struct ib_cm_sidr_rep_param {
 int ib_send_cm_sidr_rep(struct ib_cm_id *cm_id,
 			struct ib_cm_sidr_rep_param *param);
 
+/**
+ * ibcm_reject_msg - return a pointer to a reject message string.
+ * @reason: Value returned in the REJECT event status field.
+ */
+const char *__attribute_const__ ibcm_reject_msg(int reason);
+
 #endif /* IB_CM_H */
include/rdma/iw_cm.h +6 -0
index 6d0065c..5cd7701 100644
@@ -253,4 +253,10 @@ int iw_cm_disconnect(struct iw_cm_id *cm_id, int abrupt);
 int iw_cm_init_qp_attr(struct iw_cm_id *cm_id, struct ib_qp_attr *qp_attr,
 		       int *qp_attr_mask);
 
+/**
+ * iwcm_reject_msg - return a pointer to a reject message string.
+ * @reason: Value returned in the REJECT event status field.
+ */
+const char *__attribute_const__ iwcm_reject_msg(int reason);
+
 #endif /* IW_CM_H */
include/rdma/rdma_cm.h +8 -0
index 81fb1d1..f11a768 100644
@@ -388,4 +388,12 @@ int rdma_set_afonly(struct rdma_cm_id *id, int afonly);
  */
 __be64 rdma_get_service_id(struct rdma_cm_id *id, struct sockaddr *addr);
 
+/**
+ * rdma_reject_msg - return a pointer to a reject message string.
+ * @id: Communication identifier that received the REJECT event.
+ * @reason: Value returned in the REJECT event status field.
+ */
+const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
+						int reason);
+
 #endif /* RDMA_CM_H */
Signature · 1 line
2.7.0
maintainer
#2 lore raw

PATCH v3 1/6 rdma_cm: add rdma_reject_msg() helper function

Looks good,

Reviewed-by Sagi Grimberg <sagi at grimbeg.me>
maintainer
#3 lore raw

PATCH v3 1/6 rdma_cm: add rdma_reject_msg() helper function

In reply to Steve Wise
Quoted 4 lines · Steve Wise

rdma_reject_msg() returns a pointer to a string message associated with the transport reject reason codes.

Signed-off-by Steve Wise <swise at opengridcomputing.com>

Looks good,

Reviewed-by Christoph Hellwig <hch at lst.de>

(and /me notes the difference in error numbers between iWarp and the IB universe.. Stunning)

#4 lore raw

PATCH v3 1/6 rdma_cm: add rdma_reject_msg() helper function

In reply to Steve Wise

drivers/infiniband/core/cm.c MODULE_AUTHOR("Sean Hefty"); · line 71

Quoted 1 line · Steve Wise
drivers/infiniband/core/cm.c +1 -0
+	[IB_CM_REJ_RDC_NOT_EXIST]		= "rdc not exist",

Please change this into "RDC does not exist".

Quoted 12 lines
drivers/infiniband/core/cm.c +12 -0
+	[IB_CM_REJ_INVALID_GID]			= "invalid gid",
+	[IB_CM_REJ_INVALID_LID]			= "invalid lid",
+	[IB_CM_REJ_INVALID_SL]			= "invalid sl",
+	[IB_CM_REJ_INVALID_TRAFFIC_CLASS]	= "invalid traffic class",
+	[IB_CM_REJ_INVALID_HOP_LIMIT]		= "invalid hop limit",
+	[IB_CM_REJ_INVALID_PACKET_RATE]		= "invalid packet rate",
+	[IB_CM_REJ_INVALID_ALT_GID]		= "invalid alt gid",
+	[IB_CM_REJ_INVALID_ALT_LID]		= "invalid alt lid",
+	[IB_CM_REJ_INVALID_ALT_SL]		= "invalid alt sl",
+	[IB_CM_REJ_INVALID_ALT_TRAFFIC_CLASS]	= "invalid alt traffic class",
+	[IB_CM_REJ_INVALID_ALT_HOP_LIMIT]	= "invalid alt hop limit",
+	[IB_CM_REJ_INVALID_ALT_PACKET_RATE]	= "invalid alt packet rate",

drivers/infiniband/core/cm.c MODULE_AUTHOR("Sean Hefty"); · line 93

Quoted 10 lines
drivers/infiniband/core/cm.c +10 -0
+	[IB_CM_REJ_PORT_CM_REDIRECT]		= "port cm redirect",
+	[IB_CM_REJ_PORT_REDIRECT]		= "port redirect",
+	[IB_CM_REJ_INVALID_MTU]			= "invalid mtu",
+	[IB_CM_REJ_INSUFFICIENT_RESP_RESOURCES]	= "insufficient resp resources",
+	[IB_CM_REJ_CONSUMER_DEFINED]		= "consumer defined",
+	[IB_CM_REJ_INVALID_RNR_RETRY]		= "invalid rnr retry",
+	[IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID]	= "duplicate local comm id",
+	[IB_CM_REJ_INVALID_CLASS_VERSION]	= "invalid class version",
+	[IB_CM_REJ_INVALID_FLOW_LABEL]		= "invalid flow label",
+	[IB_CM_REJ_INVALID_ALT_FLOW_LABEL]	= "invalid alt flow label",

Other error messages capitalize GID, LID, CM, ID, RNR and SL so please do this here too.

drivers/infiniband/core/cm.c MODULE_AUTHOR("Sean Hefty"); · line 105

Quoted 10 lines
drivers/infiniband/core/cm.c +10 -0
+const char *__attribute_const__ ibcm_reject_msg(int reason)
+{
+	size_t index = reason;
+
+	if (index >= ARRAY_SIZE(ibcm_rej_reason_strs) ||
+	    !ibcm_rej_reason_strs[index])
+		return "unrecognized reason";
+	else
+		return ibcm_rej_reason_strs[index];
+}

Please consider using positive logic - this means negating the if-condition and swapping the if- and else-parts.

drivers/infiniband/core/cma.c const char *__attribute_const__ rdma_event_msg(enum rdma_cm_event_type event) · line 114

Quoted 11 lines
drivers/infiniband/core/cma.c +11 -0
+const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
+						int reason)
+{
+	if (rdma_ib_or_roce(id->device, id->port_num))
+		return ibcm_reject_msg(reason);
+
+	if (rdma_protocol_iwarp(id->device, id->port_num))
+		return iwcm_reject_msg(reason);
+
+	WARN_ON_ONCE(1);
+	return "unrecognized reason";

Have you considered to return "unrecognized transport" here instead?

drivers/infiniband/core/iwcm.c MODULE_AUTHOR("Tom Tucker"); · line 80

Quoted 13 lines
drivers/infiniband/core/iwcm.c +13 -0
+const char *__attribute_const__ iwcm_reject_msg(int reason)
+{
+	size_t index;
+
+	/* iWARP uses negative errnos */
+	index = -reason;
+
+	if (index >= ARRAY_SIZE(iwcm_rej_reason_strs) ||
+	    !iwcm_rej_reason_strs[index])
+		return "unrecognized reason";
+	else
+		return iwcm_rej_reason_strs[index];
+}

Also for this function, please consider using positive logic.

Thanks,

Bart.

author
#5 lore raw

PATCH v3 1/6 rdma_cm: add rdma_reject_msg() helper function

Quoted 10 lines

-----Original Message-----
From: Bart Van Assche [mailto:bart.vanassche at sandisk.com]
Sent: Tuesday, October 25, 2016 12:58 PM
To: Steve Wise; dledford at redhat.com; sean.hefty at intel.com
Cc: linux-rdma at vger.kernel.org; linux-nvme at lists.infradead.org; sagi at grimberg.me; hch at lst.de; axboe at fb.com; santosh.shilimkar at oracle.com
Subject: Re: [PATCH v3 1/6] rdma_cm: add rdma_reject_msg() helper function

Quoted 5 lines

drivers/infiniband/core/cm.c MODULE_AUTHOR("Sean Hefty"); · line 71

Quoted 1 line · Steve Wise
drivers/infiniband/core/cm.c +1 -0
+	[IB_CM_REJ_RDC_NOT_EXIST]		= "rdc not exist",

Please change this into "RDC does not exist".

ok

Quoted 21 lines
Quoted 6 lines
drivers/infiniband/core/cm.c +6 -0
+	[IB_CM_REJ_INVALID_GID]			= "invalid gid",
+	[IB_CM_REJ_INVALID_LID]			= "invalid lid",
+	[IB_CM_REJ_INVALID_SL]			= "invalid sl",
+	[IB_CM_REJ_INVALID_TRAFFIC_CLASS]	= "invalid traffic class",
+	[IB_CM_REJ_INVALID_HOP_LIMIT]		= "invalid hop limit",
+	[IB_CM_REJ_INVALID_PACKET_RATE]		= "invalid packet rate",

drivers/infiniband/core/cm.c MODULE_AUTHOR("Sean Hefty");

Quoted 10 lines
drivers/infiniband/core/cm.c +10 -0
+	[IB_CM_REJ_INVALID_ALT_GID]		= "invalid alt gid",
+	[IB_CM_REJ_INVALID_ALT_LID]		= "invalid alt lid",
+	[IB_CM_REJ_INVALID_ALT_SL]		= "invalid alt sl",
+	[IB_CM_REJ_INVALID_ALT_TRAFFIC_CLASS]	= "invalid alt traffic class",
+	[IB_CM_REJ_INVALID_ALT_HOP_LIMIT]	= "invalid alt hop limit",
+	[IB_CM_REJ_INVALID_ALT_PACKET_RATE]	= "invalid alt packet rate",
+	[IB_CM_REJ_PORT_CM_REDIRECT]		= "port cm redirect",
+	[IB_CM_REJ_PORT_REDIRECT]		= "port redirect",
+	[IB_CM_REJ_INVALID_MTU]			= "invalid mtu",
+	[IB_CM_REJ_INSUFFICIENT_RESP_RESOURCES]	= "insufficient resp

resources",

drivers/infiniband/core/cm.c MODULE_AUTHOR("Sean Hefty");

Quoted 3 lines
drivers/infiniband/core/cm.c +3 -0
+	[IB_CM_REJ_CONSUMER_DEFINED]		= "consumer defined",
+	[IB_CM_REJ_INVALID_RNR_RETRY]		= "invalid rnr retry",
+	[IB_CM_REJ_DUPLICATE_LOCAL_COMM_ID]	= "duplicate local comm

id",

Quoted 7 lines

drivers/infiniband/core/cm.c MODULE_AUTHOR("Sean Hefty"); · line 93

Quoted 3 lines
drivers/infiniband/core/cm.c +3 -0
+	[IB_CM_REJ_INVALID_CLASS_VERSION]	= "invalid class version",
+	[IB_CM_REJ_INVALID_FLOW_LABEL]		= "invalid flow label",
+	[IB_CM_REJ_INVALID_ALT_FLOW_LABEL]	= "invalid alt flow label",

Other error messages capitalize GID, LID, CM, ID, RNR and SL so please do this here too.

ok

Quoted 5 lines
Quoted 5 lines
drivers/infiniband/core/cm.c +5 -0
+const char *__attribute_const__ ibcm_reject_msg(int reason)
+{
+	size_t index = reason;
+
+	if (index >= ARRAY_SIZE(ibcm_rej_reason_strs) ||
Quoted 8 lines

drivers/infiniband/core/cm.c MODULE_AUTHOR("Sean Hefty"); · line 105

Quoted 5 lines
drivers/infiniband/core/cm.c +5 -0
+	    !ibcm_rej_reason_strs[index])
+		return "unrecognized reason";
+	else
+		return ibcm_rej_reason_strs[index];
+}

Please consider using positive logic - this means negating the if-condition and swapping the if- and else-parts.

yea that might be even clearer.

Quoted 15 lines

drivers/infiniband/core/cma.c const char *__attribute_const__ rdma_event_msg(enum rdma_cm_event_type event) · line 114

Quoted 12 lines
drivers/infiniband/core/cma.c +11 -0
+const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
+						int reason)
+{
+	if (rdma_ib_or_roce(id->device, id->port_num))
+		return ibcm_reject_msg(reason);
+
+	if (rdma_protocol_iwarp(id->device, id->port_num))
+		return iwcm_reject_msg(reason);
+
+	WARN_ON_ONCE(1);
+	return "unrecognized reason";

Have you considered to return "unrecognized transport" here instead?

I can do that.

Quoted 19 lines

drivers/infiniband/core/iwcm.c MODULE_AUTHOR("Tom Tucker"); · line 80

Quoted 13 lines
drivers/infiniband/core/iwcm.c +13 -0
+const char *__attribute_const__ iwcm_reject_msg(int reason)
+{
+	size_t index;
+
+	/* iWARP uses negative errnos */
+	index = -reason;
+
+	if (index >= ARRAY_SIZE(iwcm_rej_reason_strs) ||
+	    !iwcm_rej_reason_strs[index])
+		return "unrecognized reason";
+	else
+		return iwcm_rej_reason_strs[index];
+}

Also for this function, please consider using positive logic.

Thanks,

Bart.

author
#6 lore raw

PATCH v3 2/6 rdma_cm: add rdma_is_consumer_reject() helper function

In reply to Steve Wise

Return true if the peer consumer application rejected the connection attempt.

Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 drivers/infiniband/core/cma.c | 13 +++++++++++++
 include/rdma/rdma_cm.h        |  7 +++++++
 2 files changed, 20 insertions(+)
drivers/infiniband/core/cma.c +13 -0
index 831e94b..0d88c9d 100644
@@ -115,6 +115,19 @@ const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
 }
 EXPORT_SYMBOL(rdma_reject_msg);
 
+bool rdma_is_consumer_reject(struct rdma_cm_id *id, int reason)
+{
+	if (rdma_ib_or_roce(id->device, id->port_num))
+		return reason == IB_CM_REJ_CONSUMER_DEFINED;
+
+	if (rdma_protocol_iwarp(id->device, id->port_num))
+		return reason == -ECONNREFUSED;
+
+	WARN_ON_ONCE(1);
+	return false;
+}
+EXPORT_SYMBOL(rdma_is_consumer_reject);
+
 static void cma_add_one(struct ib_device *device);
 static void cma_remove_one(struct ib_device *device, void *client_data);
include/rdma/rdma_cm.h +7 -0
index f11a768..62039c2 100644
@@ -395,5 +395,12 @@ __be64 rdma_get_service_id(struct rdma_cm_id *id, struct sockaddr *addr);
  */
 const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
 						int reason);
+/**
+ * rdma_is_consumer_reject - return true if the consumer rejected the connect
+ *                           request.
+ * @id: Communication identifier that received the REJECT event.
+ * @reason: Value returned in the REJECT event status field.
+ */
+bool rdma_is_consumer_reject(struct rdma_cm_id *id, int reason);
 
 #endif /* RDMA_CM_H */
Signature · 1 line
2.7.0
maintainer
#7 lore raw

PATCH v3 2/6 rdma_cm: add rdma_is_consumer_reject() helper function

Looks good,

Reviewed-by Sagi Grimberg <sagi at grimbeg.me>
maintainer
#8 lore raw

PATCH v3 2/6 rdma_cm: add rdma_is_consumer_reject() helper function

In reply to Steve Wise
Quoted 4 lines · Steve Wise

Return true if the peer consumer application rejected the connection attempt.

Signed-off-by Steve Wise <swise at opengridcomputing.com>

Looks good,

Reviewed-by Christoph Hellwig <hch at lst.de>
author
#9 lore raw

PATCH v3 3/6 rdma_cm: add rdma_consumer_reject_data helper function

In reply to Steve Wise

rdma_consumer_reject_data() will return the private data pointer and length if any is available.

Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 drivers/infiniband/core/cma.c | 16 ++++++++++++++++
 include/rdma/rdma_cm.h        | 10 ++++++++++
 2 files changed, 26 insertions(+)
drivers/infiniband/core/cma.c +16 -0
index 0d88c9d..2536d42 100644
@@ -128,6 +128,22 @@ bool rdma_is_consumer_reject(struct rdma_cm_id *id, int reason)
 }
 EXPORT_SYMBOL(rdma_is_consumer_reject);
 
+const void *rdma_consumer_reject_data(struct rdma_cm_id *id,
+				      struct rdma_cm_event *ev, u8 *data_len)
+{
+	void *p;
+
+	if (rdma_is_consumer_reject(id, ev->status)) {
+		*data_len = ev->param.conn.private_data_len;
+		p = ev->param.conn.private_data;
+	} else {
+		*data_len = 0;
+		p = NULL;
+	}
+	return p;
+}
+EXPORT_SYMBOL(rdma_consumer_reject_data);
+
 static void cma_add_one(struct ib_device *device);
 static void cma_remove_one(struct ib_device *device, void *client_data);
include/rdma/rdma_cm.h +10 -0
index 62039c2..d3968b5 100644
@@ -403,4 +403,14 @@ const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
  */
 bool rdma_is_consumer_reject(struct rdma_cm_id *id, int reason);
 
+/**
+ * rdma_consumer_reject_data - return the consumer reject private data and
+ *			       length, if any.
+ * @id: Communication identifier that received the REJECT event.
+ * @ev: RDMA CM reject event.
+ * @data_len: Pointer to the resulting length of the consumer data.
+ */
+const void *rdma_consumer_reject_data(struct rdma_cm_id *id,
+				      struct rdma_cm_event *ev, u8 *data_len);
+
 #endif /* RDMA_CM_H */
Signature · 1 line
2.7.0
maintainer
#10 lore raw

PATCH v3 3/6 rdma_cm: add rdma_consumer_reject_data helper function

Looks good,

Reviewed-by Sagi Grimberg <sagi at grimbeg.me>
maintainer
#11 lore raw

PATCH v3 3/6 rdma_cm: add rdma_consumer_reject_data helper function

In reply to Steve Wise
Quoted 4 lines · Steve Wise

rdma_consumer_reject_data() will return the private data pointer and length if any is available.

Signed-off-by Steve Wise <swise at opengridcomputing.com>

Looks good,

Reviewed-by Christoph Hellwig <hch at lst.de>
author
#12 lore raw

PATCH v3 4/6 nvme-rdma: use rdma connection reject helper functions

In reply to Steve Wise

Also add nvme cm status strings and use them.

Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 drivers/nvme/host/rdma.c | 46 ++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 38 insertions(+), 8 deletions(-)
drivers/nvme/host/rdma.c +38 -8
index fbdb226..b6a6b50 100644
@@ -43,6 +43,28 @@
 
 #define NVME_RDMA_MAX_INLINE_SEGMENTS	1
 
+static const char *const nvme_rdma_cm_status_strs[] = {
+	[NVME_RDMA_CM_INVALID_LEN]	= "invalid length",
+	[NVME_RDMA_CM_INVALID_RECFMT]	= "invalid record format",
+	[NVME_RDMA_CM_INVALID_QID]	= "invalid queue id",
+	[NVME_RDMA_CM_INVALID_HSQSIZE]	= "invalid host sq size",
+	[NVME_RDMA_CM_INVALID_HRQSIZE]	= "invalid host rq size",
+	[NVME_RDMA_CM_NO_RSC]		= "resource not found",
+	[NVME_RDMA_CM_INVALID_IRD]	= "invalid ird",
+	[NVME_RDMA_CM_INVALID_ORD]	= "Invalid ord",
+};
+
+static const char *nvme_rdma_cm_msg(enum nvme_rdma_cm_status status)
+{
+	size_t index = status;
+
+	if (index >= ARRAY_SIZE(nvme_rdma_cm_status_strs) ||
+	    !nvme_rdma_cm_status_strs[index])
+		return "unrecognized reason";
+	else
+		return nvme_rdma_cm_status_strs[index];
+};
+
 /*
  * We handle AEN commands ourselves and don't even let the
  * block layer know about them.

@@ -1222,17 +1244,25 @@ out_destroy_queue_ib:
 static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
 		struct rdma_cm_event *ev)
 {
-	if (ev->param.conn.private_data_len) {
-		struct nvme_rdma_cm_rej *rej =
-			(struct nvme_rdma_cm_rej *)ev->param.conn.private_data;
+	struct rdma_cm_id *cm_id = queue->cm_id;
+	int status = ev->status;
+	const char *rej_msg;
+	struct nvme_rdma_cm_rej *rej_data;
+	u8 rej_data_len;
+
+	rej_msg = rdma_reject_msg(cm_id, status);
+	rej_data = (struct nvme_rdma_cm_rej *)
+		   rdma_consumer_reject_data(cm_id, ev, &rej_data_len);
+
+	if (rej_data && rej_data_len >= sizeof(u16)) {
+		u16 sts = le16_to_cpu(rej_data->sts);
 
 		dev_err(queue->ctrl->ctrl.device,
-			"Connect rejected, status %d.", le16_to_cpu(rej->sts));
-		/* XXX: Think of something clever to do here... */
-	} else {
+		      "Connect rejected: status %d (%s) nvme status %d (%s).\n",
+		      status, rej_msg, sts, nvme_rdma_cm_msg(sts));
+	} else
 		dev_err(queue->ctrl->ctrl.device,
-			"Connect rejected, no private data.\n");
-	}
+			"Connect rejected: status %d (%s).\n", status, rej_msg);
 
 	return -ECONNRESET;
 }
Signature · 1 line
2.7.0
maintainer
#13 lore raw

PATCH v3 4/6 nvme-rdma: use rdma connection reject helper functions

drivers/nvme/host/rdma.c line 55

Quoted 11 lines
drivers/nvme/host/rdma.c +10 -0
+static const char *const nvme_rdma_cm_status_strs[] = {
+	[NVME_RDMA_CM_INVALID_LEN]	= "invalid length",
+	[NVME_RDMA_CM_INVALID_RECFMT]	= "invalid record format",
+	[NVME_RDMA_CM_INVALID_QID]	= "invalid queue id",
+	[NVME_RDMA_CM_INVALID_HSQSIZE]	= "invalid host sq size",
+	[NVME_RDMA_CM_INVALID_HRQSIZE]	= "invalid host rq size",
+	[NVME_RDMA_CM_NO_RSC]		= "resource not found",
+	[NVME_RDMA_CM_INVALID_IRD]	= "invalid ird",
+	[NVME_RDMA_CM_INVALID_ORD]	= "Invalid ord",
+};

I think it'd be better to move them to include/linux/nvme-rdma.h and use them for logging on the target side too.

author
#14 lore raw

PATCH v3 4/6 nvme-rdma: use rdma connection reject helper functions

Quoted 5 lines
Quoted 5 lines
drivers/nvme/host/rdma.c +5 -0
+static const char *const nvme_rdma_cm_status_strs[] = {
+	[NVME_RDMA_CM_INVALID_LEN]	= "invalid length",
+	[NVME_RDMA_CM_INVALID_RECFMT]	= "invalid record format",
+	[NVME_RDMA_CM_INVALID_QID]	= "invalid queue id",
+	[NVME_RDMA_CM_INVALID_HSQSIZE]	= "invalid host sq size",
Quoted 9 lines

drivers/nvme/host/rdma.c line 55

Quoted 5 lines
drivers/nvme/host/rdma.c +5 -0
+	[NVME_RDMA_CM_INVALID_HRQSIZE]	= "invalid host rq size",
+	[NVME_RDMA_CM_NO_RSC]		= "resource not found",
+	[NVME_RDMA_CM_INVALID_IRD]	= "invalid ird",
+	[NVME_RDMA_CM_INVALID_ORD]	= "Invalid ord",
+};

I think it'd be better to move them to include/linux/nvme-rdma.h and use them for logging on the target side too.

You want to put this in nvme-rdma.h as-is so the memory for this is allocated in each .c file that includes it?

maintainer
#15 lore raw

PATCH v3 4/6 nvme-rdma: use rdma connection reject helper functions

In reply to Sagi Grimberg
Quoted 2 lines · Sagi Grimberg

I think it'd be better to move them to include/linux/nvme-rdma.h and use them for logging on the target side too.

Hmm, that would mean they get compiled into every source file. Also on the target side we can have free-form messages for whatever the exact rejection reason was, no need to limit us to the enum.

maintainer
#16 lore raw

PATCH v3 4/6 nvme-rdma: use rdma connection reject helper functions

Quoted 6 lines · Sagi Grimberg
Quoted 2 lines

I think it'd be better to move them to include/linux/nvme-rdma.h and use them for logging on the target side too.

Hmm, that would mean they get compiled into every source file.

"every source file" is exactly two...

Quoted 2 lines

Also on the target side we can have free-form messages for whatever the exact rejection reason was, no need to limit us to the enum.

Thought it'd be nice to use the same verbosity on both ends, just a suggestion though...

maintainer
#17 lore raw

PATCH v3 4/6 nvme-rdma: use rdma connection reject helper functions

In reply to Steve Wise
Quoted 3 lines · Steve Wise

Also add nvme cm status strings and use them.

Signed-off-by Steve Wise <swise at opengridcomputing.com>

Looks good,

Reviewed-by Christoph Hellwig <hch at lst.de>
#18 lore raw

PATCH v3 4/6 nvme-rdma: use rdma connection reject helper functions

In reply to Steve Wise

drivers/nvme/host/rdma.c line 55

Quoted 10 lines · Steve Wise
drivers/nvme/host/rdma.c +10 -0
+static const char *const nvme_rdma_cm_status_strs[] = {
+	[NVME_RDMA_CM_INVALID_LEN]	= "invalid length",
+	[NVME_RDMA_CM_INVALID_RECFMT]	= "invalid record format",
+	[NVME_RDMA_CM_INVALID_QID]	= "invalid queue id",
+	[NVME_RDMA_CM_INVALID_HSQSIZE]	= "invalid host sq size",
+	[NVME_RDMA_CM_INVALID_HRQSIZE]	= "invalid host rq size",
+	[NVME_RDMA_CM_NO_RSC]		= "resource not found",
+	[NVME_RDMA_CM_INVALID_IRD]	= "invalid ird",
+	[NVME_RDMA_CM_INVALID_ORD]	= "Invalid ord",
+};

Please capitalize abbreviations (SQ, RQ, ID, IRD and ORD).

drivers/nvme/host/rdma.c line 66

Quoted 10 lines
drivers/nvme/host/rdma.c +10 -0
+static const char *nvme_rdma_cm_msg(enum nvme_rdma_cm_status status)
+{
+	size_t index = status;
+
+	if (index >= ARRAY_SIZE(nvme_rdma_cm_status_strs) ||
+	    !nvme_rdma_cm_status_strs[index])
+		return "unrecognized reason";
+	else
+		return nvme_rdma_cm_status_strs[index];
+};

Please consider rewriting this if-statement such that the recognized reason scenario occurs first.

drivers/nvme/host/rdma.c out_destroy_queue_ib: · line 1255

Quoted 3 lines
drivers/nvme/host/rdma.c +3 -0
+	rej_msg = rdma_reject_msg(cm_id, status);
+	rej_data = (struct nvme_rdma_cm_rej *)
+		   rdma_consumer_reject_data(cm_id, ev, &rej_data_len);

Please reorder the patches in this patch series such that it doesn't break a bisect. rdma_consumer_reject_data() should be introduced before it is used.

Thanks,

Bart.

author
#19 lore raw

PATCH v3 4/6 nvme-rdma: use rdma connection reject helper functions

Quoted 6 lines · Steve Wise
Quoted 4 lines
drivers/nvme/host/rdma.c +4 -0
+static const char *const nvme_rdma_cm_status_strs[] = {
+	[NVME_RDMA_CM_INVALID_LEN]	= "invalid length",
+	[NVME_RDMA_CM_INVALID_RECFMT]	= "invalid record format",
+	[NVME_RDMA_CM_INVALID_QID]	= "invalid queue id",
Quoted 9 lines

drivers/nvme/host/rdma.c line 55

Quoted 6 lines
drivers/nvme/host/rdma.c +6 -0
+	[NVME_RDMA_CM_INVALID_HSQSIZE]	= "invalid host sq size",
+	[NVME_RDMA_CM_INVALID_HRQSIZE]	= "invalid host rq size",
+	[NVME_RDMA_CM_NO_RSC]		= "resource not found",
+	[NVME_RDMA_CM_INVALID_IRD]	= "invalid ird",
+	[NVME_RDMA_CM_INVALID_ORD]	= "Invalid ord",
+};

Please capitalize abbreviations (SQ, RQ, ID, IRD and ORD).

will do.

Quoted 5 lines
Quoted 5 lines
drivers/nvme/host/rdma.c +5 -0
+static const char *nvme_rdma_cm_msg(enum nvme_rdma_cm_status status)
+{
+	size_t index = status;
+
+	if (index >= ARRAY_SIZE(nvme_rdma_cm_status_strs) ||
Quoted 9 lines

drivers/nvme/host/rdma.c line 66

Quoted 5 lines
drivers/nvme/host/rdma.c +5 -0
+	    !nvme_rdma_cm_status_strs[index])
+		return "unrecognized reason";
+	else
+		return nvme_rdma_cm_status_strs[index];
+};

Please consider rewriting this if-statement such that the recognized reason scenario occurs first.

sure.

Quoted 8 lines

drivers/nvme/host/rdma.c out_destroy_queue_ib: · line 1255

Quoted 3 lines
drivers/nvme/host/rdma.c +3 -0
+	rej_msg = rdma_reject_msg(cm_id, status);
+	rej_data = (struct nvme_rdma_cm_rej *)
+		   rdma_consumer_reject_data(cm_id, ev, &rej_data_len);

Please reorder the patches in this patch series such that it doesn't break a bisect. rdma_consumer_reject_data() should be introduced before it is used.

rdma_consumer_reject_data() is introduced in patch 3.

#20 lore raw

PATCH v3 4/6 nvme-rdma: use rdma connection reject helper functions

Quoted 14 lines · Steve Wise
Quoted 13 lines · Steve Wise
Quoted 5 lines
drivers/nvme/host/rdma.c +3 -0
+	rej_msg = rdma_reject_msg(cm_id, status);
+	rej_data = (struct nvme_rdma_cm_rej *)
+		???rdma_consumer_reject_data(cm_id, ev,

&rej_data_len);

Please reorder the patches in this patch series such that it doesn't
break a bisect. rdma_consumer_reject_data() should be introduced before
it is used.

Quoted 1 line

rdma_consumer_reject_data() is introduced in patch 3.

Hello Steve,

Thanks for the feedback. Apparently my e-mail client displayed the patches in the wrong order ...

Bart.

author
#21 lore raw

PATCH v3 5/6 ib_iser: log the connection reject message

In reply to Steve Wise
Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 drivers/infiniband/ulp/iser/iser_verbs.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
drivers/infiniband/ulp/iser/iser_verbs.c +4 -1
index 1b49453..7970757 100644
@@ -906,11 +906,14 @@ static int iser_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *eve
 	case RDMA_CM_EVENT_ESTABLISHED:
 		iser_connected_handler(cma_id, event->param.conn.private_data);
 		break;
+	case RDMA_CM_EVENT_REJECTED:
+		iser_info("Connection rejected: %s\n",
+			 rdma_reject_msg(cma_id, event->status));
+		/*FALLTHROUGH*/
 	case RDMA_CM_EVENT_ADDR_ERROR:
 	case RDMA_CM_EVENT_ROUTE_ERROR:
 	case RDMA_CM_EVENT_CONNECT_ERROR:
 	case RDMA_CM_EVENT_UNREACHABLE:
-	case RDMA_CM_EVENT_REJECTED:
 		iser_connect_error(cma_id);
 		break;
 	case RDMA_CM_EVENT_DISCONNECTED:
Signature · 1 line
2.7.0
maintainer
#22 lore raw

PATCH v3 5/6 ib_iser: log the connection reject message

drivers/infiniband/ulp/iser/iser_verbs.c static int iser_cma_handler(struct rdma_cm_id *cma_id, struct rdma_cm_event *eve · line 912

Quoted 4 lines
drivers/infiniband/ulp/iser/iser_verbs.c +4 -0
+	case RDMA_CM_EVENT_REJECTED:
+		iser_info("Connection rejected: %s\n",
+			 rdma_reject_msg(cma_id, event->status));
+		/*FALLTHROUGH*/

NIT: /* FALLTHROUGH */

Otherwise:

Acked-by Sagi Grimberg <sagi at grimberg.me>
author
#23 lore raw

PATCH v3 6/6 rds_rdma: log the connection reject message

In reply to Steve Wise
Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 net/rds/rdma_transport.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
net/rds/rdma_transport.c +4 -1
index 345f090..6bd2814 100644
@@ -100,11 +100,14 @@ int rds_rdma_cm_event_handler(struct rdma_cm_id *cm_id,
 		trans->cm_connect_complete(conn, event);
 		break;
 
+	case RDMA_CM_EVENT_REJECTED:
+		rdsdebug("Connection rejected: %s\n",
+			 rdma_reject_msg(cm_id, event->status));
+		/*FALLTHROUGH*/
 	case RDMA_CM_EVENT_ADDR_ERROR:
 	case RDMA_CM_EVENT_ROUTE_ERROR:
 	case RDMA_CM_EVENT_CONNECT_ERROR:
 	case RDMA_CM_EVENT_UNREACHABLE:
-	case RDMA_CM_EVENT_REJECTED:
 	case RDMA_CM_EVENT_DEVICE_REMOVAL:
 	case RDMA_CM_EVENT_ADDR_CHANGE:
 		if (conn)
Signature · 1 line
2.7.0
#24 lore raw

PATCH v3 6/6 rds_rdma: log the connection reject message

Quoted 10 lines · Steve Wise
Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 net/rds/rdma_transport.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
net/rds/rdma_transport.c +0 -0
index 345f090..6bd2814 100644

net/rds/rdma_transport.c int rds_rdma_cm_event_handler(struct rdma_cm_id *cm_id, · line 105

Quoted 6 lines
net/rds/rdma_transport.c +3 -0
@@ -100,11 +100,14 @@ int rds_rdma_cm_event_handler(struct rdma_cm_id *cm_id,
 		trans->cm_connect_complete(conn, event);
 		break;
 
+	case RDMA_CM_EVENT_REJECTED:
+		rdsdebug("Connection rejected: %s\n",
+			 rdma_reject_msg(cm_id, event->status));

For current code above is fine but I will need the other helper while adding few connection cases where ARI field carries information for the reason for the reject.

I assume, rdma_consumer_reject_data() is meant to be used for that ?

The patch looks fine to me.
FWIW, Acked-by: Santosh Shilimkar <santosh.shilimkar at oracle.com>

author
#25 lore raw

PATCH v3 6/6 rds_rdma: log the connection reject message

Quoted 25 lines

-----Original Message-----
From: Santosh Shilimkar [mailto:santosh.shilimkar at oracle.com]
Sent: Tuesday, October 25, 2016 10:52 AM
To: Steve Wise; dledford at redhat.com; sean.hefty at intel.com
Cc: linux-rdma at vger.kernel.org; bart.vanassche at sandisk.com; linux- nvme at lists.infradead.org; sagi at grimberg.me; hch at lst.de; axboe at fb.com
Subject: Re: [PATCH v3 6/6] rds_rdma: log the connection reject message

net/rds/rdma_transport.c int rds_rdma_cm_event_handler(struct rdma_cm_id

Quoted 10 lines · Steve Wise
Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 net/rds/rdma_transport.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
net/rds/rdma_transport.c +0 -0
index 345f090..6bd2814 100644
@@ -100,11 +100,14 @@ int rds_rdma_cm_event_handler(struct rdma_cm_id

*cm_id,

Quoted 3 lines
net/rds/rdma_transport.c +0 -0
 		trans->cm_connect_complete(conn, event);
 		break;
Quoted 8 lines

net/rds/rdma_transport.c int rds_rdma_cm_event_handler(struct rdma_cm_id *cm_id, · line 105

Quoted 3 lines
net/rds/rdma_transport.c +3 -0
+	case RDMA_CM_EVENT_REJECTED:
+		rdsdebug("Connection rejected: %s\n",
+			 rdma_reject_msg(cm_id, event->status));

For current code above is fine but I will need the other helper while adding few connection cases where ARI field carries information for the reason for the reject.

I assume, rdma_consumer_reject_data() is meant to be used for that ?

Yep. You can see the nvme_rdma patch in this series for an example.

Quoted 3 lines

The patch looks fine to me.
FWIW, Acked-by: Santosh Shilimkar <santosh.shilimkar at oracle.com>

#26 lore raw

PATCH v3 6/6 rds_rdma: log the connection reject message

Quoted 32 lines · Steve Wise
Quoted 32 lines

-----Original Message-----
From: Santosh Shilimkar [mailto:santosh.shilimkar at oracle.com]
Sent: Tuesday, October 25, 2016 10:52 AM
To: Steve Wise; dledford at redhat.com; sean.hefty at intel.com
Cc: linux-rdma at vger.kernel.org; bart.vanassche at sandisk.com; linux- nvme at lists.infradead.org; sagi at grimberg.me; hch at lst.de; axboe at fb.com
Subject: Re: [PATCH v3 6/6] rds_rdma: log the connection reject message

net/rds/rdma_transport.c int rds_rdma_cm_event_handler(struct rdma_cm_id

Quoted 10 lines · Steve Wise
Signed-off-by Steve Wise <swise at opengridcomputing.com>
---
 net/rds/rdma_transport.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
net/rds/rdma_transport.c +0 -0
index 345f090..6bd2814 100644
@@ -100,11 +100,14 @@ int rds_rdma_cm_event_handler(struct rdma_cm_id

*cm_id,

net/rds/rdma_transport.c int rds_rdma_cm_event_handler(struct rdma_cm_id *cm_id, · line 105

Quoted 6 lines
net/rds/rdma_transport.c +3 -0
 		trans->cm_connect_complete(conn, event);
 		break;
 
+	case RDMA_CM_EVENT_REJECTED:
+		rdsdebug("Connection rejected: %s\n",
+			 rdma_reject_msg(cm_id, event->status));

For current code above is fine but I will need the other helper while adding few connection cases where ARI field carries information for the reason for the reject.

Quoted 4 lines
Quoted 1 line

I assume, rdma_consumer_reject_data() is meant to be used for that ?

Yep. You can see the nvme_rdma patch in this series for an example.

Cool. Thanks for info Steve.

regards,
Santosh