[PATCH RFC v2 0/3] connect reject event helpers

[PATCH RFC v2 0/3] connect reject event helpers

[Steve Wise]

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(-)

-- 
2.7.0

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

[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>
---
 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(+)

diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c
index c995255..0918c17 100644
--- a/drivers/infiniband/core/cm.c
+++ b/drivers/infiniband/core/cm.c
@@ -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);
 
diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 5f65a78..7cc7346 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -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);
 
diff --git a/drivers/infiniband/core/iwcm.c b/drivers/infiniband/core/iwcm.c
index 357624f..588a31d 100644
--- a/drivers/infiniband/core/iwcm.c
+++ b/drivers/infiniband/core/iwcm.c
@@ -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},
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index 92a7d85..af193b7 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -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 */
diff --git a/include/rdma/iw_cm.h b/include/rdma/iw_cm.h
index 6d0065c..15b437e 100644
--- a/include/rdma/iw_cm.h
+++ b/include/rdma/iw_cm.h
@@ -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 */
diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h
index 81fb1d1..712a70c 100644
--- a/include/rdma/rdma_cm.h
+++ b/include/rdma/rdma_cm.h
@@ -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 */
-- 
2.7.0

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

[Christoph Hellwig]

> +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];
}

> +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.

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

[Steve Wise]

> 
> > +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];
> }
>

I copy/pasted from rdma_event_msg().

 
> > +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.

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

Thanks.

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

[Sagi Grimberg]

Looks good to me either way,

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

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

[Sagi Grimberg]

> +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)

Please call it ibcm_reject_msg()


> +const char *__attribute_const__ iw_reject_msg(int reason)

and this iwcm_reject_msg

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

[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(+)

diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
index 7cc7346..4ec16a3 100644
--- a/drivers/infiniband/core/cma.c
+++ b/drivers/infiniband/core/cma.c
@@ -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);
 
diff --git a/include/rdma/ib_cm.h b/include/rdma/ib_cm.h
index af193b7..5595529 100644
--- a/include/rdma/ib_cm.h
+++ b/include/rdma/ib_cm.h
@@ -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 */
diff --git a/include/rdma/iw_cm.h b/include/rdma/iw_cm.h
index 15b437e..1e5bd33 100644
--- a/include/rdma/iw_cm.h
+++ b/include/rdma/iw_cm.h
@@ -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 */
diff --git a/include/rdma/rdma_cm.h b/include/rdma/rdma_cm.h
index 712a70c..058dfbb 100644
--- a/include/rdma/rdma_cm.h
+++ b/include/rdma/rdma_cm.h
@@ -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 */
-- 
2.7.0

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

[Christoph Hellwig]

On Thu, Oct 20, 2016@03:40:26PM -0700, Steve Wise wrote:
> 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(+)
> 
> diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> index 7cc7346..4ec16a3 100644
> --- a/drivers/infiniband/core/cma.c
> +++ b/drivers/infiniband/core/cma.c
> @@ -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;

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?

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

[Steve Wise]

> 
> On Thu, Oct 20, 2016@03:40:26PM -0700, Steve Wise wrote:
> > 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(+)
> >
> > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
> > index 7cc7346..4ec16a3 100644
> > --- a/drivers/infiniband/core/cma.c
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -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;
> 
> 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?

How about rdma_rejected_by_consumer()?

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

[Parav Pandit]

On Fri, Oct 21, 2016@7:39 PM, Steve Wise <swise@opengridcomputing.com> wrote:
>>
>> On Thu, Oct 20, 2016@03:40:26PM -0700, Steve Wise wrote:
>> > 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(+)
>> >
>> > diff --git a/drivers/infiniband/core/cma.c b/drivers/infiniband/core/cma.c
>> > index 7cc7346..4ec16a3 100644
>> > --- a/drivers/infiniband/core/cma.c
>> > +++ b/drivers/infiniband/core/cma.c
>> > @@ -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;
>>
>> 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?
>
> How about rdma_rejected_by_consumer()?
>
How about rdma_reject_by_ulp()?
We have ulp directory holding iser, srp etc.

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

[Sagi Grimberg]

>>> 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?
>>
>> How about rdma_rejected_by_consumer()?
>>
> How about rdma_reject_by_ulp()?
> We have ulp directory holding iser, srp etc.

I like consumer better.

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

[Steve Wise]

> > --- a/drivers/infiniband/core/cma.c
> > +++ b/drivers/infiniband/core/cma.c
> > @@ -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;
> 
> 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?

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

[Steve Wise]

Signed-off-by: Steve Wise <swise at opengridcomputing.com>
---
 drivers/nvme/host/rdma.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 601aecf..6319c26 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -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;
 }
 
-- 
2.7.0

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

[Christoph Hellwig]

On Thu, Oct 20, 2016@03:40:29PM -0700, Steve Wise wrote:
> @@ -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);

>  
> +	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.

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

[Sagi Grimberg]

> 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);
>

Looks nicer...

>>
>> +	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...

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

[Steve Wise]

> 
> On Thu, Oct 20, 2016@03:40:29PM -0700, Steve Wise wrote:
> > @@ -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?

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?

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

[Steve Wise]

> > 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?

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...

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

['Christoph Hellwig']

On Mon, Oct 24, 2016@09:57:50AM -0500, Steve Wise wrote:
> > rdma_reject_msg() - protocol reject reason string

This one sounds useful on it's own.

> > 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.

[PATCH RFC v2 0/3] connect reject event helpers

[Sagi Grimberg]

> 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)

[PATCH RFC v2 0/3] connect reject event helpers

[Steve Wise]

> 
> 
> > 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)

can-do!

[PATCH RFC v2 0/3] connect reject event helpers

[Steve Wise]

> >
> > > 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)
> 
> 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.

[PATCH RFC v2 0/3] connect reject event helpers

[Chuck Lever]

> On Oct 24, 2016,@1:44 PM, Steve Wise <swise@opengridcomputing.com> wrote:
> 
>>> 
>>>> 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)
>> 
>> 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.

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

[PATCH RFC v2 0/3] connect reject event helpers

[Steve Wise]

> >>> 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.
> 
> 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!