Skip to contentSkip to conversation

nvme-rdma: move nvme cm status helper to .h file

PATCH New Max Gurtovoy 4 participants 8 messages Feb 20, 2017 – Feb 21, 2017

Subsystems (2)
NVMe NVMe target
lore
Share & tools
lore

Max Gurtovoy

author
#0 lore raw

PATCH v2 1/2 nvme-rdma: move nvme cm status helper to .h file

This will enable the usage for nvme rdma target. Also move from a lookup array to a switch statement.

Signed-off-by Max Gurtovoy <maxg at mellanox.com>
Reviewed-by Parav Pandit <parav at mellanox.com>

---

Changes from v1:
- updated the code in the switch label to be in a new line
- updated the changelog

---
 drivers/nvme/host/rdma.c  |   22 ----------------------
 include/linux/nvme-rdma.h |   24 ++++++++++++++++++++++++
 2 files changed, 24 insertions(+), 22 deletions(-)
drivers/nvme/host/rdma.c +0 -22
index 96285ef..b3e1071 100644
@@ -42,28 +42,6 @@
 
 #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 nvme_rdma_cm_status_strs[index];
-	else
-		return "unrecognized reason";
-};
-
 /*
  * We handle AEN commands ourselves and don't even let the
  * block layer know about them.
include/linux/nvme-rdma.h +24 -0
index bf240a3..a72fd04 100644
@@ -29,6 +29,30 @@ enum nvme_rdma_cm_status {
 	NVME_RDMA_CM_INVALID_ORD	= 0x08,
 };
 
+static inline const char *nvme_rdma_cm_msg(enum nvme_rdma_cm_status status)
+{
+	switch (status) {
+	case NVME_RDMA_CM_INVALID_LEN:
+		return "invalid length";
+	case NVME_RDMA_CM_INVALID_RECFMT:
+		return "invalid record format";
+	case NVME_RDMA_CM_INVALID_QID:
+		return "invalid queue ID";
+	case NVME_RDMA_CM_INVALID_HSQSIZE:
+		return "invalid host SQ size";
+	case NVME_RDMA_CM_INVALID_HRQSIZE:
+		return "invalid host RQ size";
+	case NVME_RDMA_CM_NO_RSC:
+		return "resource not found";
+	case NVME_RDMA_CM_INVALID_IRD:
+		return "invalid IRD";
+	case NVME_RDMA_CM_INVALID_ORD:
+		return "Invalid ORD";
+	default:
+		return "unrecognized reason";
+	}
+}
+
 /**
  * struct nvme_rdma_cm_req - rdma connect request
  *
Signature · 1 line
1.7.1
author
#1 lore raw

PATCH v2 2/2 nvmet-rdma: use nvme cm status helper

Also remove redundant debug prints.

Signed-off-by Max Gurtovoy <maxg at mellanox.com>
Reviewed-by Parav Pandit <parav at mellanox.com>
Reviewed-by Christoph Hellwig <hch at lst.de>

---

Changes from v1:
- Add Christoph as reviewer

---
 drivers/nvme/target/rdma.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)
drivers/nvme/target/rdma.c +3 -2
index 6099022..5d189c0 100644
@@ -1041,6 +1041,9 @@ static int nvmet_rdma_cm_reject(struct rdma_cm_id *cm_id,
 {
 	struct nvme_rdma_cm_rej rej;
 
+	pr_debug("rejecting connect request: status %d (%s)\n",
+		 status, nvme_rdma_cm_msg(status));
+
 	rej.recfmt = cpu_to_le16(NVME_RDMA_CM_FMT_1_0);
 	rej.sts = cpu_to_le16(status);
 

@@ -1135,7 +1138,6 @@ static int nvmet_rdma_cm_reject(struct rdma_cm_id *cm_id,
 out_free_queue:
 	kfree(queue);
 out_reject:
-	pr_debug("rejecting connect request with status code %d\n", ret);
 	nvmet_rdma_cm_reject(cm_id, ret);
 	return NULL;
 }

@@ -1188,7 +1190,6 @@ static int nvmet_rdma_queue_connect(struct rdma_cm_id *cm_id,
 
 	ndev = nvmet_rdma_find_get_device(cm_id);
 	if (!ndev) {
-		pr_err("no client data!\n");
 		nvmet_rdma_cm_reject(cm_id, NVME_RDMA_CM_NO_RSC);
 		return -ECONNREFUSED;
 	}
Signature · 1 line
1.7.1
#3 lore raw

PATCH v2 1/2 nvme-rdma: move nvme cm status helper to .h file

In reply to Max Gurtovoy
Quoted 2 lines · Max Gurtovoy

This will enable the usage for nvme rdma target. Also move from a lookup array to a switch statement.

Hello Max,

In general un-inlining code that is not in the hot path is wrong because this increases the size of the kernel and because this increases the time needed to build the kernel. Would it have been possible to keep the nvme_rdma_cm_msg() definition in a .c file?

Thanks,

Bart.

author
#4 lore raw

PATCH v2 1/2 nvme-rdma: move nvme cm status helper to .h file

Hi Bart,

Currently we don't have any .c file to put this function implementation without creating a code duplication and module dependencies. I think that this new solution is better than code duplication, right ?

thanks,
Max.

Quoted 15 lines · Bart Van Assche
Quoted 2 lines · Max Gurtovoy

This will enable the usage for nvme rdma target. Also move from a lookup array to a switch statement.

Hello Max,

In general un-inlining code that is not in the hot path is wrong because this increases the size of the kernel and because this increases the time needed to build the kernel. Would it have been possible to keep the nvme_rdma_cm_msg() definition in a .c file?

Thanks,

Bart.

#5 lore raw

PATCH v2 1/2 nvme-rdma: move nvme cm status helper to .h file

Quoted 5 lines · Max Gurtovoy

Hi Bart,

Currently we don't have any .c file to put this function implementation without creating a code duplication and module dependencies. I think that this new solution is better than code duplication, right ?

Yes, it is - especially given that it's just a bit of constant and not much code anyway, and each new module would take up at least a page of memory.

author
#6 lore raw

PATCH v2 1/2 nvme-rdma: move nvme cm status helper to .h file

Quoted 11 lines · On 2/21/2017 11:27 AM, [email protected] wrote:
Quoted 5 lines · Max Gurtovoy

Hi Bart,

Currently we don't have any .c file to put this function implementation without creating a code duplication and module dependencies. I think that this new solution is better than code duplication, right ?

Yes, it is - especially given that it's just a bit of constant and not much code anyway, and each new module would take up at least a page of memory.

Maybe we'll need it in the future, but for now let's take it as is.

Sagi,
can you take it to nvme-4.11 ?

Max.

maintainer
#7 lore raw

PATCH v2 1/2 nvme-rdma: move nvme cm status helper to .h file

Quoted 2 lines

Sagi,
can you take it to nvme-4.11 ?

Yea, queued, thanks.