connect reject event helpers

RFC PATCH Superseded Steve Wise 5 participants 23 messages Oct 20, 2016 – Oct 24, 2016 2 messages not in the archive

Subsystems (2)

InfiniBand NVMe

lore

Share & tools

lore

Steve Wise

author

2016-10-20 22:40 UTC #0 lore raw

v2 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. I've included a nvme_rdma patch to utilize the new helpers.

Thoughts?

Changes since v1:

- moved reject logic to cm.c and iwcm.c.
- added rdma_consumer_reject() helper.
- added nvme-rdma change to utilize the new helpers.

Steve Wise (3):
rdma_cm: add rdma_reject_msg() helper function
rdma_cm: add rdma_consumer_reject() helper function
nvme-rdma: use rdma_reject_msg() to log connection rejects

 drivers/infiniband/core/cm.c   | 46 ++++++++++++++++++++++++++++++++++++++++++
 drivers/infiniband/core/cma.c  | 26 ++++++++++++++++++++++++
 drivers/infiniband/core/iwcm.c | 18 +++++++++++++++++
 drivers/nvme/host/rdma.c       | 18 ++++++++++-------
 include/rdma/ib_cm.h           | 15 ++++++++++++++
 include/rdma/iw_cm.h           | 15 ++++++++++++++
 include/rdma/rdma_cm.h         | 14 +++++++++++++
 7 files changed, 145 insertions(+), 7 deletions(-)

Signature · 1 line

2.7.0

Steve Wise

author

2016-10-20 22:40 UTC #1 lore raw

RFC PATCH v2 1/3 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   | 46 ++++++++++++++++++++++++++++++++++++++++++
 drivers/infiniband/core/cma.c  | 13 ++++++++++++
 drivers/infiniband/core/iwcm.c | 18 +++++++++++++++++
 include/rdma/ib_cm.h           |  6 ++++++
 include/rdma/iw_cm.h           |  6 ++++++
 include/rdma/rdma_cm.h         |  8 ++++++++
 6 files changed, 97 insertions(+)

drivers/infiniband/core/cm.c +46 -0

index c995255..0918c17 100644
@@ -57,6 +57,52 @@ MODULE_AUTHOR("Sean Hefty");
 MODULE_DESCRIPTION("InfiniBand CM");
 MODULE_LICENSE("Dual BSD/GPL");
 
+static const char * const ib_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__ ib_reject_msg(int reason)
+{
+	size_t index = reason;
+
+	return (index < ARRAY_SIZE(ib_rej_reason_strs) &&
+		ib_rej_reason_strs[index]) ?
+		ib_rej_reason_strs[index] : "unrecognized reason";
+}
+EXPORT_SYMBOL(ib_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 +13 -0

index 5f65a78..7cc7346 100644
@@ -101,6 +101,19 @@ 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 ib_reject_msg(reason);
+
+	if (rdma_protocol_iwarp(id->device, id->port_num))
+		return iw_reject_msg(reason);
+
+	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 +18 -0

index 357624f..588a31d 100644
@@ -59,6 +59,24 @@ MODULE_AUTHOR("Tom Tucker");
 MODULE_DESCRIPTION("iWARP CM");
 MODULE_LICENSE("Dual BSD/GPL");
 
+static const char * const iw_rej_reason_strs[] = {
+	[ECONNRESET]			= "reset by remote host",
+	[ECONNREFUSED]			= "refused by remote application",
+	[ETIMEDOUT]			= "setup timeout",
+};
+
+const char *__attribute_const__ iw_reject_msg(int reason)
+{
+	size_t index = -reason;
+
+	/* iWARP uses negative errnos */
+	index = -index;
+	return (index < ARRAY_SIZE(iw_rej_reason_strs) &&
+		iw_rej_reason_strs[index]) ?
+		iw_rej_reason_strs[index] : "unrecognized reason";
+}
+EXPORT_SYMBOL(iw_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..af193b7 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);
 
+/**
+ * ib_reject_msg - return a pointer to a reject message string.
+ * @reason: Value returned in the REJECT event status field.
+ */
+const char *__attribute_const__ ib_reject_msg(int reason);
+
 #endif /* IB_CM_H */

include/rdma/iw_cm.h +6 -0

index 6d0065c..15b437e 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);
 
+/**
+ * iw_reject_msg - return a pointer to a reject message string.
+ * @reason: Value returned in the REJECT event status field.
+ */
+const char *__attribute_const__ iw_reject_msg(int reason);
+
 #endif /* IW_CM_H */

include/rdma/rdma_cm.h +8 -0

index 81fb1d1..712a70c 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

Christoph Hellwig

maintainer

2016-10-21 12:12 UTC #2 lore raw

RFC PATCH v2 1/3 rdma_cm: add rdma_reject_msg() helper function

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

Quoted 9 lines

drivers/infiniband/core/cm.c +9 -0

+const char *__attribute_const__ ib_reject_msg(int reason)
+{
+	size_t index = reason;
+
+	return (index < ARRAY_SIZE(ib_rej_reason_strs) &&
+		ib_rej_reason_strs[index]) ?
+		ib_rej_reason_strs[index] : "unrecognized reason";
+}
+EXPORT_SYMBOL(ib_reject_msg);

This looks a bit odd, why not something like:

const char *__attribute_const__ ib_reject_msg(int reason)

{
	if (reason >= ARRAY_SIZE(ib_rej_reason_strs) ||
	    !ib_rej_reason_strs[reason])
		return "unrecognized reason";
	return ib_rej_reason_strs[reason];
}

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

Quoted 11 lines

drivers/infiniband/core/iwcm.c +11 -0

+const char *__attribute_const__ iw_reject_msg(int reason)
+{
+	size_t index = -reason;
+
+	/* iWARP uses negative errnos */
+	index = -index;
+	return (index < ARRAY_SIZE(iw_rej_reason_strs) &&
+		iw_rej_reason_strs[index]) ?
+		iw_rej_reason_strs[index] : "unrecognized reason";
+}
+EXPORT_SYMBOL(iw_reject_msg);

Same here:

const char *__attribute_const__ iw_reject_msg(int reason)

{
	/* iWARP uses negative errnos */
	reason = -reason;

	if (reason >= ARRAY_SIZE(iw_rej_reason_strs) ||
	    !iw_rej_reason_strs[reason])
		return "unrecognized reason";
	return iw_rej_reason_strs[reason];
}

Otherwise this looks good and very useful to me.

Steve Wise

author

2016-10-21 14:07 UTC #3 lore raw

RFC PATCH v2 1/3 rdma_cm: add rdma_reject_msg() helper function

Quoted 13 lines

drivers/infiniband/core/cm.c MODULE_AUTHOR("Sean Hefty"); · line 104
Quoted 10 lines
drivers/infiniband/core/cm.c +9 -0
+const char *__attribute_const__ ib_reject_msg(int reason)
+{
+	size_t index = reason;
+
+	return (index < ARRAY_SIZE(ib_rej_reason_strs) &&
+		ib_rej_reason_strs[index]) ?
+		ib_rej_reason_strs[index] : "unrecognized reason";
+}
+EXPORT_SYMBOL(ib_reject_msg);
This looks a bit odd, why not something like:

Quoted 8 lines

const char *__attribute_const__ ib_reject_msg(int reason)

{
	if (reason >= ARRAY_SIZE(ib_rej_reason_strs) ||
	    !ib_rej_reason_strs[reason])
		return "unrecognized reason";
	return ib_rej_reason_strs[reason];
}

I copy/pasted from rdma_event_msg().

Quoted 25 lines

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

Quoted 11 lines

drivers/infiniband/core/iwcm.c +11 -0

+const char *__attribute_const__ iw_reject_msg(int reason)
+{
+	size_t index = -reason;
+
+	/* iWARP uses negative errnos */
+	index = -index;
+	return (index < ARRAY_SIZE(iw_rej_reason_strs) &&
+		iw_rej_reason_strs[index]) ?
+		iw_rej_reason_strs[index] : "unrecognized reason";
+}
+EXPORT_SYMBOL(iw_reject_msg);

Same here:

const char *__attribute_const__ iw_reject_msg(int reason)

{
	/* iWARP uses negative errnos */
	reason = -reason;

	if (reason >= ARRAY_SIZE(iw_rej_reason_strs) ||
	    !iw_rej_reason_strs[reason])
		return "unrecognized reason";
	return iw_rej_reason_strs[reason];
}

Quoted 1 line

Otherwise this looks good and very useful to me.

I will refactor as you suggest. You proposed logic is slightly more readable to me...

Thanks.

Sagi Grimberg

maintainer

2016-10-21 21:43 UTC #4 lore raw

RFC PATCH v2 1/3 rdma_cm: add rdma_reject_msg() helper function

Looks good to me either way,

Reviewed-by Sagi Grimberg <sagi at grimberg.me>

Sagi Grimberg

maintainer

2016-10-21 21:51 UTC #5 lore raw

RFC PATCH v2 1/3 rdma_cm: add rdma_reject_msg() helper function

In reply to Steve Wise

Quoted 28 lines

drivers/infiniband/core/cm.c +27 -0

+static const char * const ib_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",

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

Quoted 10 lines

drivers/infiniband/core/cm.c +10 -0

+	[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__ ib_reject_msg(int reason)

Please call it ibcm_reject_msg()

Quoted 1 line

+const char *__attribute_const__ iw_reject_msg(int reason)

and this iwcm_reject_msg

Steve Wise

author

2016-10-20 22:40 UTC #6 lore raw

RFC PATCH v2 2/3 rdma_cm: add rdma_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/ib_cm.h          |  9 +++++++++
 include/rdma/iw_cm.h          |  9 +++++++++
 include/rdma/rdma_cm.h        |  6 ++++++
 4 files changed, 37 insertions(+)

drivers/infiniband/core/cma.c +13 -0

index 7cc7346..4ec16a3 100644
@@ -114,6 +114,19 @@ const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
 }
 EXPORT_SYMBOL(rdma_reject_msg);
 
+bool rdma_consumer_reject(struct rdma_cm_id *id, int reason)
+{
+	if (rdma_ib_or_roce(id->device, id->port_num))
+		return ib_consumer_reject(reason);
+
+	if (rdma_protocol_iwarp(id->device, id->port_num))
+		return iw_consumer_reject(reason);
+
+	/* FIXME should we WARN_ONCE() here? */
+	return false;
+}
+EXPORT_SYMBOL(rdma_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/ib_cm.h +9 -0

index af193b7..5595529 100644
@@ -609,4 +609,13 @@ int ib_send_cm_sidr_rep(struct ib_cm_id *cm_id,
  */
 const char *__attribute_const__ ib_reject_msg(int reason);
 
+/**
+ * ib_consumer_reject - return true if the user rejected the connection.
+ * @reason: Value returned in the REJECT event status field.
+ */
+static inline bool ib_consumer_reject(int reason)
+{
+	return reason == IB_CM_REJ_CONSUMER_DEFINED;
+};
+
 #endif /* IB_CM_H */

include/rdma/iw_cm.h +9 -0

index 15b437e..1e5bd33 100644
@@ -259,4 +259,13 @@ int iw_cm_init_qp_attr(struct iw_cm_id *cm_id, struct ib_qp_attr *qp_attr,
  */
 const char *__attribute_const__ iw_reject_msg(int reason);
 
+/**
+ * iw_consumer_reject - return true if the consumer rejected the connection.
+ * @reason: Value returned in the REJECT event status field.
+ */
+static inline bool iw_consumer_reject(int reason)
+{
+	return reason == -ECONNREFUSED;
+}
+
 #endif /* IW_CM_H */

include/rdma/rdma_cm.h +6 -0

index 712a70c..058dfbb 100644
@@ -395,5 +395,11 @@ __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_consumer_reject - return true if the consumer rejected the connection.
+ * @id: Communication identifier that received the REJECT event
+ * @reason: Value returned in the REJECT event status field.
+ */
+bool rdma_consumer_reject(struct rdma_cm_id *id, int reason);
 
 #endif /* RDMA_CM_H */

Signature · 1 line

2.7.0

Christoph Hellwig

maintainer

2016-10-21 12:14 UTC #7 lore raw

RFC PATCH v2 2/3 rdma_cm: add rdma_consumer_reject() helper function

Quoted 19 lines · 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/ib_cm.h          |  9 +++++++++
 include/rdma/iw_cm.h          |  9 +++++++++
 include/rdma/rdma_cm.h        |  6 ++++++
 4 files changed, 37 insertions(+)

drivers/infiniband/core/cma.c +0 -0

index 7cc7346..4ec16a3 100644
@@ -114,6 +114,19 @@ const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id,
 }
 EXPORT_SYMBOL(rdma_reject_msg);

drivers/infiniband/core/cma.c const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id, · line 126

Quoted 10 lines

drivers/infiniband/core/cma.c +10 -0

+bool rdma_consumer_reject(struct rdma_cm_id *id, int reason)
+{
+	if (rdma_ib_or_roce(id->device, id->port_num))
+		return ib_consumer_reject(reason);
+
+	if (rdma_protocol_iwarp(id->device, id->port_num))
+		return iw_consumer_reject(reason);
+
+	/* FIXME should we WARN_ONCE() here? */
+	return false;

Yes. Also I'd just inline the ib_consumer_reject and iw_consumer_reject helpers here.

Aso wouldn't it be better named rdma_consumer_is_reject or similar given that we don't reject anything here, but check if the request has been rejected?

Steve Wise

author

2016-10-21 14:09 UTC #8 lore raw

RFC PATCH v2 2/3 rdma_cm: add rdma_consumer_reject() helper function

Quoted 36 lines · Steve Wise

drivers/infiniband/core/cma.c const char *__attribute_const__
Quoted 16 lines
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/ib_cm.h          |  9 +++++++++
 include/rdma/iw_cm.h          |  9 +++++++++
 include/rdma/rdma_cm.h        |  6 ++++++
 4 files changed, 37 insertions(+)
drivers/infiniband/core/cma.c +0 -0
index 7cc7346..4ec16a3 100644
@@ -114,6 +114,19 @@ const char *__attribute_const__
rdma_reject_msg(struct rdma_cm_id *id,
drivers/infiniband/core/cma.c const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id, · line 126
Quoted 13 lines
drivers/infiniband/core/cma.c +10 -0
 }
 EXPORT_SYMBOL(rdma_reject_msg);
 
+bool rdma_consumer_reject(struct rdma_cm_id *id, int reason)
+{
+	if (rdma_ib_or_roce(id->device, id->port_num))
+		return ib_consumer_reject(reason);
+
+	if (rdma_protocol_iwarp(id->device, id->port_num))
+		return iw_consumer_reject(reason);
+
+	/* FIXME should we WARN_ONCE() here? */
+	return false;
Yes. Also I'd just inline the ib_consumer_reject and iw_consumer_reject helpers here.

Quoted 3 lines

Aso wouldn't it be better named rdma_consumer_is_reject or similar given that we don't reject anything here, but check if the request has been rejected?

How about rdma_rejected_by_consumer()?

Parav Pandit

2016-10-21 15:50 UTC #9 lore raw

RFC PATCH v2 2/3 rdma_cm: add rdma_consumer_reject() helper function

Quoted 36 lines · Steve Wise

Quoted 36 lines · Steve Wise
drivers/infiniband/core/cma.c const char *__attribute_const__
Quoted 16 lines
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/ib_cm.h          |  9 +++++++++
 include/rdma/iw_cm.h          |  9 +++++++++
 include/rdma/rdma_cm.h        |  6 ++++++
 4 files changed, 37 insertions(+)
drivers/infiniband/core/cma.c +0 -0
index 7cc7346..4ec16a3 100644
@@ -114,6 +114,19 @@ const char *__attribute_const__
rdma_reject_msg(struct rdma_cm_id *id,
drivers/infiniband/core/cma.c const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id, · line 126
Quoted 13 lines
drivers/infiniband/core/cma.c +10 -0
 }
 EXPORT_SYMBOL(rdma_reject_msg);
 
+bool rdma_consumer_reject(struct rdma_cm_id *id, int reason)
+{
+   if (rdma_ib_or_roce(id->device, id->port_num))
+           return ib_consumer_reject(reason);
+
+   if (rdma_protocol_iwarp(id->device, id->port_num))
+           return iw_consumer_reject(reason);
+
+   /* FIXME should we WARN_ONCE() here? */
+   return false;
Yes. Also I'd just inline the ib_consumer_reject and iw_consumer_reject helpers here.

Quoted 6 lines

Quoted 3 lines
Aso wouldn't it be better named rdma_consumer_is_reject or similar given that we don't reject anything here, but check if the request has been rejected?
How about rdma_rejected_by_consumer()?

How about rdma_reject_by_ulp()?
We have ulp directory holding iser, srp etc.

Sagi Grimberg

maintainer

2016-10-21 21:45 UTC #10 lore raw

RFC PATCH v2 2/3 rdma_cm: add rdma_consumer_reject() helper function

Quoted 4 lines

Quoted 4 lines
Quoted 4 lines
Yes. Also I'd just inline the ib_consumer_reject and iw_consumer_reject helpers here.

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Aso wouldn't it be better named rdma_consumer_is_reject or similar given that we don't reject anything here, but check if the request has been rejected?
How about rdma_rejected_by_consumer()?
How about rdma_reject_by_ulp()?
We have ulp directory holding iser, srp etc.

I like consumer better.

Steve Wise

author

2016-10-22 15:57 UTC #11 lore raw

RFC PATCH v2 2/3 rdma_cm: add rdma_consumer_reject() helper function

In reply to Christoph Hellwig

Quoted 7 lines

drivers/infiniband/core/cma.c const char *__attribute_const__
Quoted 3 lines
drivers/infiniband/core/cma.c +0 -0
@@ -114,6 +114,19 @@ const char *__attribute_const__
rdma_reject_msg(struct rdma_cm_id *id,
Quoted 3 lines
drivers/infiniband/core/cma.c +0 -0
 }
 EXPORT_SYMBOL(rdma_reject_msg);
 

Quoted 14 lines

drivers/infiniband/core/cma.c const char *__attribute_const__ rdma_reject_msg(struct rdma_cm_id *id, · line 126
Quoted 10 lines
drivers/infiniband/core/cma.c +10 -0
+bool rdma_consumer_reject(struct rdma_cm_id *id, int reason)
+{
+	if (rdma_ib_or_roce(id->device, id->port_num))
+		return ib_consumer_reject(reason);
+
+	if (rdma_protocol_iwarp(id->device, id->port_num))
+		return iw_consumer_reject(reason);
+
+	/* FIXME should we WARN_ONCE() here? */
+	return false;
Yes. Also I'd just inline the ib_consumer_reject and iw_consumer_reject helpers here.

Why is that preferred vs the static inline functions in ib_cm.h and iw_cm.h?

Steve Wise

author

2016-10-20 22:40 UTC #12 lore raw

RFC PATCH v2 3/3 nvme-rdma: use rdma_reject_msg() to log connection rejects

In reply to Steve Wise

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

---
 drivers/nvme/host/rdma.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

drivers/nvme/host/rdma.c +11 -7

index 601aecf..6319c26 100644
@@ -1237,18 +1237,22 @@ 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 rdma_cm_id *cm_id = queue->cm_id;
+	int rdma_status = ev->status;
+	short nvme_status = -1;
+
+	if (rdma_consumer_reject(cm_id, rdma_status) &&
+	    ev->param.conn.private_data_len) {
 		struct nvme_rdma_cm_rej *rej =
 			(struct nvme_rdma_cm_rej *)ev->param.conn.private_data;
 
-		dev_err(queue->ctrl->ctrl.device,
-			"Connect rejected, status %d.", le16_to_cpu(rej->sts));
-		/* XXX: Think of something clever to do here... */
-	} else {
-		dev_err(queue->ctrl->ctrl.device,
-			"Connect rejected, no private data.\n");
+		nvme_status = le16_to_cpu(rej->sts);
 	}
 
+	dev_err(queue->ctrl->ctrl.device, "Connect rejected: status %d (%s) "
+		"nvme status %d.\n", rdma_status,
+		rdma_reject_msg(cm_id, rdma_status), nvme_status);
+
 	return -ECONNRESET;
 }

Signature · 1 line

2.7.0

Christoph Hellwig

maintainer

2016-10-21 12:23 UTC #13 lore raw

RFC PATCH v2 3/3 nvme-rdma: use rdma_reject_msg() to log connection rejects

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

Quoted 12 lines · Steve Wise

drivers/nvme/host/rdma.c +6 -0

@@ -1237,18 +1237,22 @@ out_destroy_queue_ib:
 static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
 		struct rdma_cm_event *ev)
 {
+	struct rdma_cm_id *cm_id = queue->cm_id;
+	int rdma_status = ev->status;
+	short nvme_status = -1;
+
+	if (rdma_consumer_reject(cm_id, rdma_status) &&
+	    ev->param.conn.private_data_len) {
 		struct nvme_rdma_cm_rej *rej =
 			(struct nvme_rdma_cm_rej *)ev->param.conn.private_data;

Given the nasty casting issues in the current RDMA/CM API maybe we should actually expand the scope of the rdma_consumer_reject helper to include the above check, e.g. check that there is a private data len and then return a pointer to the private data?

Something like

static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
struct rdma_cm_event *ev)

{
	struct rdma_cm_id *cm_id = queue->cm_id;
	struct nvme_rdma_cm_rej *rej
	short nvme_status = -1;

	rej = rdma_cm_reject_message(ev);
	if (rej)
		nvme_status = le16_to_cpu(rej->sts);

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

Quoted 4 lines

drivers/nvme/host/rdma.c +3 -0

+	dev_err(queue->ctrl->ctrl.device, "Connect rejected: status %d (%s) "
+		"nvme status %d.\n", rdma_status,
+		rdma_reject_msg(cm_id, rdma_status), nvme_status);

And while we're pretty printing the rest it would be nice to pretty print the NVMe status here as well.

Sagi Grimberg

maintainer

2016-10-21 21:48 UTC #14 lore raw

RFC PATCH v2 3/3 nvme-rdma: use rdma_reject_msg() to log connection rejects

Quoted 10 lines

Given the nasty casting issues in the current RDMA/CM API maybe we should actually expand the scope of the rdma_consumer_reject helper to include the above check, e.g. check that there is a private data len and then return a pointer to the private data?
Something like
static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
struct rdma_cm_event *ev)

Quoted 9 lines

{
	struct rdma_cm_id *cm_id = queue->cm_id;
	struct nvme_rdma_cm_rej *rej
	short nvme_status = -1;

	rej = rdma_cm_reject_message(ev);
	if (rej)
		nvme_status = le16_to_cpu(rej->sts);

Looks nicer...

Quoted 7 lines

drivers/nvme/host/rdma.c out_destroy_queue_ib: · line 1254
Quoted 4 lines
drivers/nvme/host/rdma.c +3 -0
+	dev_err(queue->ctrl->ctrl.device, "Connect rejected: status %d (%s) "
+		"nvme status %d.\n", rdma_status,
+		rdma_reject_msg(cm_id, rdma_status), nvme_status);
And while we're pretty printing the rest it would be nice to pretty print the NVMe status here as well.

Would be nice...

Steve Wise

author

2016-10-22 16:12 UTC #15 lore raw

RFC PATCH v2 3/3 nvme-rdma: use rdma_reject_msg() to log connection rejects

In reply to Christoph Hellwig

Quoted 12 lines · Steve Wise

Quoted 10 lines

drivers/nvme/host/rdma.c +6 -0

@@ -1237,18 +1237,22 @@ out_destroy_queue_ib:
 static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
 		struct rdma_cm_event *ev)
 {
+	struct rdma_cm_id *cm_id = queue->cm_id;
+	int rdma_status = ev->status;
+	short nvme_status = -1;
+
+	if (rdma_consumer_reject(cm_id, rdma_status) &&
+	    ev->param.conn.private_data_len) {

Quoted 8 lines

Review out_destroy_queue_ib: · line 1248
Quoted 3 lines
(quoted diff) +0 -0
 		struct nvme_rdma_cm_rej *rej =
 			(struct nvme_rdma_cm_rej *)ev-
 param.conn.private_data;
Given the nasty casting issues in the current RDMA/CM API maybe we should actually expand the scope of the rdma_consumer_reject helper to include the above check, e.g. check that there is a private data len and then return a pointer to the private data?

An application could reject and not provide private data, so I think we need 3 helpers (so far):

rdma_reject_msg() - protocol reject reason string rdma_is_consumer_reject() - true if the peer consumer/ulp rejected rdma_consumer_reject_data() - ptr to any private data

Sound good?

A message that is not in the archive (sent off-list or not yet received): [email protected]

Steve Wise

author

2016-10-24 14:57 UTC #17 lore raw

RFC PATCH v2 3/3 nvme-rdma: use rdma_reject_msg() to log connection rejects

Quoted 6 lines

Quoted 1 line
Given the nasty casting issues in the current RDMA/CM API maybe we
should
Quoted 3 lines
actually expand the scope of the rdma_consumer_reject helper to include the above check, e.g. check that there is a private data len and then return a pointer to the private data?

Quoted 8 lines

An application could reject and not provide private data, so I think we need 3 helpers (so far):
rdma_reject_msg() - protocol reject reason string rdma_is_consumer_reject() - true if the peer consumer/ulp rejected rdma_consumer_reject_data() - ptr to any private data
Sound good?

Or these 3 could be rolled into one uber function that returns true if the consumer rejected, and sets 2 pointers passed in: one to the protocol reject
string, and one to the private data, if any. If the something like like this:

bool rdma_reject_info(struct rdma_cm_id *id, int reason, char **protocol_msg, char **consumer_data)

Kinda ugly, but only one call is needed vs 3 calls to get this info...

'Christoph Hellwig'

maintainer

2016-10-24 15:09 UTC #18 lore raw

RFC PATCH v2 3/3 nvme-rdma: use rdma_reject_msg() to log connection rejects

Quoted 1 line · Steve Wise

Quoted 1 line
rdma_reject_msg() - protocol reject reason string

This one sounds useful on it's own.

Quoted 2 lines

Quoted 2 lines
rdma_is_consumer_reject() - true if the peer consumer/ulp rejected rdma_consumer_reject_data() - ptr to any private data

I see why these are conceptually different, but why do we care if something is a consumer reject except for printing what reject we got (solved by rdma_reject_msg) or for getting consumer reject data if there is any (solved by rdma_consumer_reject_data).

So all three helpers are fine with me, but rdma_is_consumer_reject would be more of a low-level helper that drivers wouldn't use directly, only through rdma_consumer_reject_data.

Sagi Grimberg

maintainer

2016-10-21 21:53 UTC #19 lore raw

v2 cover connect reject event helpers

In reply to Steve Wise

Quoted 10 lines

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. I've included a nvme_rdma patch to utilize the new helpers.
Thoughts?

Hey Steve,

This looks nice and useful. Would be great if you can also help other ULPs that use this (e.g. srp/srpt)

Steve Wise

author

2016-10-21 21:58 UTC #20 lore raw

v2 cover connect reject event helpers

Quoted 10 lines

Quoted 10 lines
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. I've included a nvme_rdma patch to utilize the new helpers.

Quoted 6 lines

Quoted 1 line
Thoughts?
Hey Steve,
This looks nice and useful. Would be great if you can also help other ULPs that use this (e.g. srp/srpt)

can-do!

A message that is not in the archive (sent off-list or not yet received): [email protected]

Steve Wise

author

2016-10-24 17:44 UTC #22 lore raw

v2 cover connect reject event helpers

Quoted 10 lines

Quoted 4 lines
Quoted 4 lines
While reviewing:
http://lists.infradead.org/pipermail/linux-nvme/2016-October/006681.html
Quoted 5 lines
Quoted 5 lines
I decided to propose transport-agnostic helper functions to better handle connection reject event information. I've included a nvme_rdma patch to utilize the new helpers.

Quoted 8 lines

Quoted 6 lines
Quoted 1 line
Thoughts?
Hey Steve,
This looks nice and useful. Would be great if you can also help other ULPs that use this (e.g. srp/srpt)
can-do!

Actually, srp uses the ib_cm directly. Further it already has logic to do various actions based on the IB_CM Reject status. It really doesn't need these helpers. The only one would be ibcm_reject_msg(). Looking at the iser initiator, it logs nothing on a reject event. And nfsrdma doesn't log any details about the reject reason also.

I could add a dev_warn() or something for iser and nfsrdma.

Steve.

Chuck Lever

2016-10-24 17:52 UTC #23 lore raw

v2 cover connect reject event helpers

Quoted 22 lines · Steve Wise

Quoted 11 lines
Quoted 5 lines
Quoted 5 lines
While reviewing:
http://lists.infradead.org/pipermail/linux-nvme/2016-October/006681.html
Quoted 5 lines
Quoted 5 lines
I decided to propose transport-agnostic helper functions to better handle connection reject event information. I've included a nvme_rdma patch to utilize the new helpers.
Quoted 8 lines
Quoted 6 lines
Quoted 1 line
Thoughts?
Hey Steve,
This looks nice and useful. Would be great if you can also help other ULPs that use this (e.g. srp/srpt)
can-do!

Quoted 7 lines

Actually, srp uses the ib_cm directly. Further it already has logic to do various actions based on the IB_CM Reject status. It really doesn't need these helpers. The only one would be ibcm_reject_msg(). Looking at the iser initiator, it logs nothing on a reject event. And nfsrdma doesn't log any details about the reject reason also.
I could add a dev_warn() or something for iser and nfsrdma.

I'm fixing an issue in this area. I'd like to take care of the xprtrdma connection upcall myself once you've got these new APIs introduced.

--
Chuck Lever

Steve Wise

author

2016-10-24 17:57 UTC #24 lore raw

v2 cover connect reject event helpers

Quoted 9 lines

Quoted 9 lines
Quoted 6 lines
Quoted 4 lines
Hey Steve,
This looks nice and useful. Would be great if you can also help other ULPs that use this (e.g. srp/srpt)
can-do!
Actually, srp uses the ib_cm directly. Further it already has logic to do various actions based on the IB_CM Reject status. It really doesn't need

these

Quoted 9 lines

Quoted 5 lines
helpers. The only one would be ibcm_reject_msg(). Looking at the iser initiator, it logs nothing on a reject event. And nfsrdma doesn't log any details about the reject reason also.
I could add a dev_warn() or something for iser and nfsrdma.
I'm fixing an issue in this area. I'd like to take care of the xprtrdma connection upcall myself once you've got these new APIs introduced.

Sure, not a problem!