| Message ID | 60243a2ce17e08cdc93600b9998698dbd7f83306.1477003235.git.swise@opengridcomputing.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
On Thu, Oct 20, 2016 at 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. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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... -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > On Thu, Oct 20, 2016 at 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? -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> > 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... -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Oct 24, 2016 at 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. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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; }
Signed-off-by: Steve Wise <swise@opengridcomputing.com> --- drivers/nvme/host/rdma.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)