linux-nvme.lists.infradead.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/5 V3] nvme: Add type of service (TOS) configuration
@ 2019-08-14 10:19 Israel Rukshin
  2019-08-14 10:19 ` [PATCH] nvme-cli/fabrics: Add tos param to connect cmd Israel Rukshin
                   ` (5 more replies)
  0 siblings, 6 replies; 19+ messages in thread
From: Israel Rukshin @ 2019-08-14 10:19 UTC (permalink / raw)


Hi All,

This series adds TOS configuration for TCP and RDMA transports.
It provides clients the ability to segregate traffic flows for different
type of data.
One of the TOS usage is bandwidth management which allows setting bandwidth
limits for QoS classes, e.g. 80% bandwidth to controllers at QoS class A
and 20% to controllers at QoS class B.

Changes from v2:
 - Clamping tos to 255

Changes from v1:
 - Add TCP TOS configuration

Israel Rukshin (5):
  nvme-fabrics: Add type of service (TOS) configuration
  nvme-rdma: Add TOS for rdma transport
  nvme-tcp: Use struct nvme_ctrl directly
  nvme-tcp: Add TOS for tcp transport
  nvmet-tcp: Add TOS for tcp transport

 drivers/nvme/host/fabrics.c | 20 +++++++++++++++++++-
 drivers/nvme/host/fabrics.h |  3 +++
 drivers/nvme/host/rdma.c    |  6 ++++--
 drivers/nvme/host/tcp.c     | 32 ++++++++++++++++++++++----------
 drivers/nvme/target/tcp.c   | 11 +++++++++++
 5 files changed, 59 insertions(+), 13 deletions(-)

-- 
1.8.3.1

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] nvme-cli/fabrics: Add tos param to connect cmd
  2019-08-14 10:19 [PATCH 0/5 V3] nvme: Add type of service (TOS) configuration Israel Rukshin
@ 2019-08-14 10:19 ` Israel Rukshin
  2019-08-14 17:08   ` Sagi Grimberg
  2019-08-14 10:19 ` [PATCH 1/5] nvme-fabrics: Add type of service (TOS) configuration Israel Rukshin
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Israel Rukshin @ 2019-08-14 10:19 UTC (permalink / raw)


Added 'tos' to 'connect' command so users can specify the type of service.

usage examples:
nvme connect --tos=0 --transport=rdma --traddr=10.0.1.1 --nqn=test-nvme
nvme connect -T 0 -t rdma -a 10.0.1.1 -n test_nvme

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 fabrics.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 333669f..f952722 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -60,6 +60,7 @@ static struct config {
 	int  keep_alive_tmo;
 	int  reconnect_delay;
 	int  ctrl_loss_tmo;
+	int  tos;
 	char *raw;
 	char *device;
 	int  duplicate_connect;
@@ -576,11 +577,12 @@ add_bool_argument(char **argstr, int *max_len, char *arg_str, bool arg)
 }
 
 static int
-add_int_argument(char **argstr, int *max_len, char *arg_str, int arg)
+add_int_argument(char **argstr, int *max_len, char *arg_str, int arg,
+		 bool allow_zero)
 {
 	int len;
 
-	if (arg) {
+	if ((arg && !allow_zero) || (arg != -1 && allow_zero)) {
 		len = snprintf(*argstr, *max_len, ",%s=%d", arg_str, arg);
 		if (len < 0)
 			return -EINVAL;
@@ -640,21 +642,23 @@ static int build_options(char *argstr, int max_len, bool discover)
 		    add_argument(&argstr, &max_len, "hostid", cfg.hostid)) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "nr_io_queues",
-				cfg.nr_io_queues)) ||
+				cfg.nr_io_queues, false)) ||
 	    add_int_argument(&argstr, &max_len, "nr_write_queues",
-				cfg.nr_write_queues) ||
+				cfg.nr_write_queues, false) ||
 	    add_int_argument(&argstr, &max_len, "nr_poll_queues",
-				cfg.nr_poll_queues) ||
+				cfg.nr_poll_queues, false) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "queue_size",
-				cfg.queue_size)) ||
+				cfg.queue_size, false)) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "keep_alive_tmo",
-				cfg.keep_alive_tmo)) ||
+				cfg.keep_alive_tmo, false)) ||
 	    add_int_argument(&argstr, &max_len, "reconnect_delay",
-				cfg.reconnect_delay) ||
+				cfg.reconnect_delay, false) ||
 	    add_int_argument(&argstr, &max_len, "ctrl_loss_tmo",
-				cfg.ctrl_loss_tmo) ||
+				cfg.ctrl_loss_tmo, false) ||
+	    add_int_argument(&argstr, &max_len, "tos",
+				cfg.tos, true) ||
 	    add_bool_argument(&argstr, &max_len, "duplicate_connect",
 				cfg.duplicate_connect) ||
 	    add_bool_argument(&argstr, &max_len, "disable_sqflow",
@@ -749,6 +753,13 @@ retry:
 		p += len;
 	}
 
+	if (cfg.tos != -1) {
+		len = sprintf(p, ",tos=%d", cfg.tos);
+		if (len < 0)
+			return -EINVAL;
+		p += len;
+	}
+
 	if (cfg.keep_alive_tmo && !discover) {
 		len = sprintf(p, ",keep_alive_tmo=%d", cfg.keep_alive_tmo);
 		if (len < 0)
@@ -1065,6 +1076,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
+		{"tos",             'T', "LIST", CFG_INT, &cfg.tos,             required_argument, "type of service" },
 		{"hdr_digest", 'g', "", CFG_NONE, &cfg.hdr_digest, no_argument, "enable transport protocol header digest (TCP transport)" },
 		{"data_digest", 'G', "", CFG_NONE, &cfg.data_digest, no_argument, "enable transport protocol data digest (TCP transport)" },
 		{"nr-io-queues",    'i', "LIST", CFG_INT, &cfg.nr_io_queues,    required_argument, "number of io queues to use (default is core count)" },
@@ -1076,6 +1088,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{NULL},
 	};
 
+	cfg.tos = -1;
 	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
 			sizeof(cfg));
 	if (ret)
@@ -1122,6 +1135,7 @@ int connect(const char *desc, int argc, char **argv)
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
+		{"tos",             'T', "LIST", CFG_INT, &cfg.tos,             required_argument, "type of service" },
 		{"duplicate_connect", 'D', "", CFG_NONE, &cfg.duplicate_connect, no_argument, "allow duplicate connections between same transport host and subsystem port" },
 		{"disable_sqflow", 'd', "", CFG_NONE, &cfg.disable_sqflow, no_argument, "disable controller sq flow control (default false)" },
 		{"hdr_digest", 'g', "", CFG_NONE, &cfg.hdr_digest, no_argument, "enable transport protocol header digest (TCP transport)" },
@@ -1129,6 +1143,7 @@ int connect(const char *desc, int argc, char **argv)
 		{NULL},
 	};
 
+	cfg.tos = -1;
 	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
 			sizeof(cfg));
 	if (ret)
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 1/5] nvme-fabrics: Add type of service (TOS) configuration
  2019-08-14 10:19 [PATCH 0/5 V3] nvme: Add type of service (TOS) configuration Israel Rukshin
  2019-08-14 10:19 ` [PATCH] nvme-cli/fabrics: Add tos param to connect cmd Israel Rukshin
@ 2019-08-14 10:19 ` Israel Rukshin
  2019-08-14 17:06   ` Sagi Grimberg
  2019-08-14 10:19 ` [PATCH 2/5] nvme-rdma: Add TOS for rdma transport Israel Rukshin
                   ` (3 subsequent siblings)
  5 siblings, 1 reply; 19+ messages in thread
From: Israel Rukshin @ 2019-08-14 10:19 UTC (permalink / raw)


TOS is user-defined and needs to be configured via nvme-cli.
It must be set before initiating any traffic and once set the TOS
cannot be changed.

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/fabrics.c | 20 +++++++++++++++++++-
 drivers/nvme/host/fabrics.h |  3 +++
 2 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 1994d5b..d343c22 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -611,6 +611,7 @@ bool __nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
 	{ NVMF_OPT_DATA_DIGEST,		"data_digest"		},
 	{ NVMF_OPT_NR_WRITE_QUEUES,	"nr_write_queues=%d"	},
 	{ NVMF_OPT_NR_POLL_QUEUES,	"nr_poll_queues=%d"	},
+	{ NVMF_OPT_TOS,			"tos=%d"		},
 	{ NVMF_OPT_ERR,			NULL			}
 };
 
@@ -632,6 +633,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 	opts->duplicate_connect = false;
 	opts->hdr_digest = false;
 	opts->data_digest = false;
+	opts->tos = -1; /* < 0 == use transport default */
 
 	options = o = kstrdup(buf, GFP_KERNEL);
 	if (!options)
@@ -856,6 +858,22 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			}
 			opts->nr_poll_queues = token;
 			break;
+		case NVMF_OPT_TOS:
+			if (match_int(args, &token)) {
+				ret = -EINVAL;
+				goto out;
+			}
+			if (token < 0) {
+				pr_err("Invalid type of service %d\n", token);
+				ret = -EINVAL;
+				goto out;
+			}
+			if (token > 255) {
+				pr_warn("Clamping type of service to 255\n");
+				token = 255;
+			}
+			opts->tos = token;
+			break;
 		default:
 			pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
 				p);
@@ -975,7 +993,7 @@ void nvmf_free_options(struct nvmf_ctrl_options *opts)
 #define NVMF_ALLOWED_OPTS	(NVMF_OPT_QUEUE_SIZE | NVMF_OPT_NR_IO_QUEUES | \
 				 NVMF_OPT_KATO | NVMF_OPT_HOSTNQN | \
 				 NVMF_OPT_HOST_ID | NVMF_OPT_DUP_CONNECT |\
-				 NVMF_OPT_DISABLE_SQFLOW)
+				 NVMF_OPT_DISABLE_SQFLOW | NVMF_OPT_TOS)
 
 static struct nvme_ctrl *
 nvmf_create_ctrl(struct device *dev, const char *buf)
diff --git a/drivers/nvme/host/fabrics.h b/drivers/nvme/host/fabrics.h
index 3044d8b..93f08d7 100644
--- a/drivers/nvme/host/fabrics.h
+++ b/drivers/nvme/host/fabrics.h
@@ -55,6 +55,7 @@ enum {
 	NVMF_OPT_DATA_DIGEST	= 1 << 16,
 	NVMF_OPT_NR_WRITE_QUEUES = 1 << 17,
 	NVMF_OPT_NR_POLL_QUEUES = 1 << 18,
+	NVMF_OPT_TOS		= 1 << 19,
 };
 
 /**
@@ -87,6 +88,7 @@ enum {
  * @data_digest: generate/verify data digest (TCP)
  * @nr_write_queues: number of queues for write I/O
  * @nr_poll_queues: number of queues for polling I/O
+ * @tos: type of service
  */
 struct nvmf_ctrl_options {
 	unsigned		mask;
@@ -108,6 +110,7 @@ struct nvmf_ctrl_options {
 	bool			data_digest;
 	unsigned int		nr_write_queues;
 	unsigned int		nr_poll_queues;
+	int			tos;
 };
 
 /*
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 2/5] nvme-rdma: Add TOS for rdma transport
  2019-08-14 10:19 [PATCH 0/5 V3] nvme: Add type of service (TOS) configuration Israel Rukshin
  2019-08-14 10:19 ` [PATCH] nvme-cli/fabrics: Add tos param to connect cmd Israel Rukshin
  2019-08-14 10:19 ` [PATCH 1/5] nvme-fabrics: Add type of service (TOS) configuration Israel Rukshin
@ 2019-08-14 10:19 ` Israel Rukshin
  2019-08-14 10:19 ` [PATCH 3/5] nvme-tcp: Use struct nvme_ctrl directly Israel Rukshin
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 19+ messages in thread
From: Israel Rukshin @ 2019-08-14 10:19 UTC (permalink / raw)


For RDMA transports, TOS is an extension of IB QoS to provide clients
the ability to segregate traffic flows for different type of data.
RDMA CM abstract it for ULPs using rdma_set_service_type().
Internally, each traffic flow is represented by a connection with all of
its independent resources like that of a normal connection, and is
differentiated by service type. In other words, there can be multiple qp
connections between an IP pair and each supports a unique service type.

One of the TOS usage is bandwidth management which allows setting bandwidth
limits for QoS classes, e.g. 80% bandwidth to controllers at QoS class A
and 20% to controllers at QoS class B.

Note: In addition to the TOS configuration, QOS must be configured on the
relevant HCA on the target (send RDMA commands) and initiator to effect
the traffic.

usage example:
nvme connect --tos=0 --transport=rdma --traddr=10.0.1.1 --nqn=test-nvme

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
Reviewed-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index a249db5..9fd3d33 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1541,16 +1541,18 @@ static int nvme_rdma_conn_rejected(struct nvme_rdma_queue *queue,
 
 static int nvme_rdma_addr_resolved(struct nvme_rdma_queue *queue)
 {
+	struct nvme_ctrl *ctrl = &queue->ctrl->ctrl;
 	int ret;
 
 	ret = nvme_rdma_create_queue_ib(queue);
 	if (ret)
 		return ret;
 
+	if (ctrl->opts->tos >= 0)
+		rdma_set_service_type(queue->cm_id, ctrl->opts->tos);
 	ret = rdma_resolve_route(queue->cm_id, NVME_RDMA_CONNECT_TIMEOUT_MS);
 	if (ret) {
-		dev_err(queue->ctrl->ctrl.device,
-			"rdma_resolve_route failed (%d).\n",
+		dev_err(ctrl->device, "rdma_resolve_route failed (%d).\n",
 			queue->cm_error);
 		goto out_destroy_queue;
 	}
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 3/5] nvme-tcp: Use struct nvme_ctrl directly
  2019-08-14 10:19 [PATCH 0/5 V3] nvme: Add type of service (TOS) configuration Israel Rukshin
                   ` (2 preceding siblings ...)
  2019-08-14 10:19 ` [PATCH 2/5] nvme-rdma: Add TOS for rdma transport Israel Rukshin
@ 2019-08-14 10:19 ` Israel Rukshin
  2019-08-14 10:19 ` [PATCH 4/5] nvme-tcp: Add TOS for tcp transport Israel Rukshin
  2019-08-14 10:19 ` [PATCH 5/5] nvmet-tcp: " Israel Rukshin
  5 siblings, 0 replies; 19+ messages in thread
From: Israel Rukshin @ 2019-08-14 10:19 UTC (permalink / raw)


This patch doesn't change any functionality.

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
Reviewed-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/tcp.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 606b13d..feed0dc 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1255,7 +1255,7 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
 	queue->queue_size = queue_size;
 
 	if (qid > 0)
-		queue->cmnd_capsule_len = ctrl->ctrl.ioccsz * 16;
+		queue->cmnd_capsule_len = nctrl->ioccsz * 16;
 	else
 		queue->cmnd_capsule_len = sizeof(struct nvme_command) +
 						NVME_TCP_ADMIN_CCSZ;
@@ -1263,7 +1263,7 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
 	ret = sock_create(ctrl->addr.ss_family, SOCK_STREAM,
 			IPPROTO_TCP, &queue->sock);
 	if (ret) {
-		dev_err(ctrl->ctrl.device,
+		dev_err(nctrl->device,
 			"failed to create socket: %d\n", ret);
 		return ret;
 	}
@@ -1273,7 +1273,7 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
 	ret = kernel_setsockopt(queue->sock, IPPROTO_TCP, TCP_SYNCNT,
 			(char *)&opt, sizeof(opt));
 	if (ret) {
-		dev_err(ctrl->ctrl.device,
+		dev_err(nctrl->device,
 			"failed to set TCP_SYNCNT sock opt %d\n", ret);
 		goto err_sock;
 	}
@@ -1283,7 +1283,7 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
 	ret = kernel_setsockopt(queue->sock, IPPROTO_TCP,
 			TCP_NODELAY, (char *)&opt, sizeof(opt));
 	if (ret) {
-		dev_err(ctrl->ctrl.device,
+		dev_err(nctrl->device,
 			"failed to set TCP_NODELAY sock opt %d\n", ret);
 		goto err_sock;
 	}
@@ -1296,7 +1296,7 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
 	ret = kernel_setsockopt(queue->sock, SOL_SOCKET, SO_LINGER,
 			(char *)&sol, sizeof(sol));
 	if (ret) {
-		dev_err(ctrl->ctrl.device,
+		dev_err(nctrl->device,
 			"failed to set SO_LINGER sock opt %d\n", ret);
 		goto err_sock;
 	}
@@ -1314,11 +1314,11 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
 	queue->pdu_offset = 0;
 	sk_set_memalloc(queue->sock->sk);
 
-	if (ctrl->ctrl.opts->mask & NVMF_OPT_HOST_TRADDR) {
+	if (nctrl->opts->mask & NVMF_OPT_HOST_TRADDR) {
 		ret = kernel_bind(queue->sock, (struct sockaddr *)&ctrl->src_addr,
 			sizeof(ctrl->src_addr));
 		if (ret) {
-			dev_err(ctrl->ctrl.device,
+			dev_err(nctrl->device,
 				"failed to bind queue %d socket %d\n",
 				qid, ret);
 			goto err_sock;
@@ -1330,7 +1330,7 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
 	if (queue->hdr_digest || queue->data_digest) {
 		ret = nvme_tcp_alloc_crypto(queue);
 		if (ret) {
-			dev_err(ctrl->ctrl.device,
+			dev_err(nctrl->device,
 				"failed to allocate queue %d crypto\n", qid);
 			goto err_sock;
 		}
@@ -1344,13 +1344,13 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
 		goto err_crypto;
 	}
 
-	dev_dbg(ctrl->ctrl.device, "connecting queue %d\n",
+	dev_dbg(nctrl->device, "connecting queue %d\n",
 			nvme_tcp_queue_id(queue));
 
 	ret = kernel_connect(queue->sock, (struct sockaddr *)&ctrl->addr,
 		sizeof(ctrl->addr), 0);
 	if (ret) {
-		dev_err(ctrl->ctrl.device,
+		dev_err(nctrl->device,
 			"failed to connect socket: %d\n", ret);
 		goto err_rcv_pdu;
 	}
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 4/5] nvme-tcp: Add TOS for tcp transport
  2019-08-14 10:19 [PATCH 0/5 V3] nvme: Add type of service (TOS) configuration Israel Rukshin
                   ` (3 preceding siblings ...)
  2019-08-14 10:19 ` [PATCH 3/5] nvme-tcp: Use struct nvme_ctrl directly Israel Rukshin
@ 2019-08-14 10:19 ` Israel Rukshin
  2019-08-14 10:19 ` [PATCH 5/5] nvmet-tcp: " Israel Rukshin
  5 siblings, 0 replies; 19+ messages in thread
From: Israel Rukshin @ 2019-08-14 10:19 UTC (permalink / raw)


TOS provide clients the ability to segregate traffic flows for
different type of data.
One of the TOS usage is bandwidth management which allows setting bandwidth
limits for QoS classes, e.g. 80% bandwidth to controllers at QoS class A
and 20% to controllers at QoS class B.

usage example:
nvme connect --tos=0 --transport=tcp --traddr=10.0.1.1 --nqn=test-nvme

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
Reviewed-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/tcp.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index feed0dc..229017f 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1301,6 +1301,18 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl *nctrl,
 		goto err_sock;
 	}
 
+	/* Set socket type of service */
+	if (nctrl->opts->tos >= 0) {
+		opt = nctrl->opts->tos;
+		ret = kernel_setsockopt(queue->sock, SOL_IP, IP_TOS,
+				(char *)&opt, sizeof(opt));
+		if (ret) {
+			dev_err(nctrl->device,
+				"failed to set IP_TOS sock opt %d\n", ret);
+			goto err_sock;
+		}
+	}
+
 	queue->sock->sk->sk_allocation = GFP_ATOMIC;
 	if (!qid)
 		n = 0;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 5/5] nvmet-tcp: Add TOS for tcp transport
  2019-08-14 10:19 [PATCH 0/5 V3] nvme: Add type of service (TOS) configuration Israel Rukshin
                   ` (4 preceding siblings ...)
  2019-08-14 10:19 ` [PATCH 4/5] nvme-tcp: Add TOS for tcp transport Israel Rukshin
@ 2019-08-14 10:19 ` Israel Rukshin
  5 siblings, 0 replies; 19+ messages in thread
From: Israel Rukshin @ 2019-08-14 10:19 UTC (permalink / raw)


Set the outgoing packets type of service (TOS) according to the
receiving TOS.

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Suggested-by: Sagi Grimberg <sagi at grimberg.me>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
Reviewed-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/tcp.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 69b83fa..fdf10ee 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1410,6 +1410,7 @@ static void nvmet_tcp_state_change(struct sock *sk)
 static int nvmet_tcp_set_queue_sock(struct nvmet_tcp_queue *queue)
 {
 	struct socket *sock = queue->sock;
+	struct inet_sock *inet = inet_sk(sock->sk);
 	struct linger sol = { .l_onoff = 1, .l_linger = 0 };
 	int ret;
 
@@ -1433,6 +1434,16 @@ static int nvmet_tcp_set_queue_sock(struct nvmet_tcp_queue *queue)
 	if (ret)
 		return ret;
 
+	/* Set socket type of service */
+	if (inet->rcv_tos > 0) {
+		int tos = inet->rcv_tos;
+
+		ret = kernel_setsockopt(sock, SOL_IP, IP_TOS,
+				(char *)&tos, sizeof(tos));
+		if (ret)
+			return ret;
+	}
+
 	write_lock_bh(&sock->sk->sk_callback_lock);
 	sock->sk->sk_user_data = queue;
 	queue->data_ready = sock->sk->sk_data_ready;
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH 1/5] nvme-fabrics: Add type of service (TOS) configuration
  2019-08-14 10:19 ` [PATCH 1/5] nvme-fabrics: Add type of service (TOS) configuration Israel Rukshin
@ 2019-08-14 17:06   ` Sagi Grimberg
  2019-08-15  8:54     ` Max Gurtovoy
  0 siblings, 1 reply; 19+ messages in thread
From: Sagi Grimberg @ 2019-08-14 17:06 UTC (permalink / raw)




On 8/14/19 3:19 AM, Israel Rukshin wrote:
> TOS is user-defined and needs to be configured via nvme-cli.
> It must be set before initiating any traffic and once set the TOS
> cannot be changed.
> 
> Signed-off-by: Israel Rukshin <israelr at mellanox.com>
> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
> ---
>   drivers/nvme/host/fabrics.c | 20 +++++++++++++++++++-
>   drivers/nvme/host/fabrics.h |  3 +++
>   2 files changed, 22 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 1994d5b..d343c22 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -611,6 +611,7 @@ bool __nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
>   	{ NVMF_OPT_DATA_DIGEST,		"data_digest"		},
>   	{ NVMF_OPT_NR_WRITE_QUEUES,	"nr_write_queues=%d"	},
>   	{ NVMF_OPT_NR_POLL_QUEUES,	"nr_poll_queues=%d"	},
> +	{ NVMF_OPT_TOS,			"tos=%d"		},
>   	{ NVMF_OPT_ERR,			NULL			}
>   };
>   
> @@ -632,6 +633,7 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
>   	opts->duplicate_connect = false;
>   	opts->hdr_digest = false;
>   	opts->data_digest = false;
> +	opts->tos = -1; /* < 0 == use transport default */
>   
>   	options = o = kstrdup(buf, GFP_KERNEL);
>   	if (!options)
> @@ -856,6 +858,22 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
>   			}
>   			opts->nr_poll_queues = token;
>   			break;
> +		case NVMF_OPT_TOS:
> +			if (match_int(args, &token)) {
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			if (token < 0) {
> +				pr_err("Invalid type of service %d\n", token);
> +				ret = -EINVAL;
> +				goto out;
> +			}
> +			if (token > 255) {
> +				pr_warn("Clamping type of service to 255\n");
> +				token = 255;
> +			}

Again, if tos is an abstract opaque, this needs to happen in the
individual transports, not in the generic part.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] nvme-cli/fabrics: Add tos param to connect cmd
  2019-08-14 10:19 ` [PATCH] nvme-cli/fabrics: Add tos param to connect cmd Israel Rukshin
@ 2019-08-14 17:08   ` Sagi Grimberg
  2019-08-15  8:41     ` Max Gurtovoy
  0 siblings, 1 reply; 19+ messages in thread
From: Sagi Grimberg @ 2019-08-14 17:08 UTC (permalink / raw)



> Added 'tos' to 'connect' command so users can specify the type of service.
> 
> usage examples:
> nvme connect --tos=0 --transport=rdma --traddr=10.0.1.1 --nqn=test-nvme
> nvme connect -T 0 -t rdma -a 10.0.1.1 -n test_nvme

Still don't see how this is handled if the kernel does not support
tos yet.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] nvme-cli/fabrics: Add tos param to connect cmd
  2019-08-14 17:08   ` Sagi Grimberg
@ 2019-08-15  8:41     ` Max Gurtovoy
  2019-08-15 15:18       ` James Smart
  0 siblings, 1 reply; 19+ messages in thread
From: Max Gurtovoy @ 2019-08-15  8:41 UTC (permalink / raw)



On 8/14/2019 8:08 PM, Sagi Grimberg wrote:
>
>> Added 'tos' to 'connect' command so users can specify the type of 
>> service.
>>
>> usage examples:
>> nvme connect --tos=0 --transport=rdma --traddr=10.0.1.1 --nqn=test-nvme
>> nvme connect -T 0 -t rdma -a 10.0.1.1 -n test_nvme
>
> Still don't see how this is handled if the kernel does not support
> tos yet.

how any other feature we added to connect command will act with old 
kernel ? (e.g. queue_size/nr_queues)

it will fail to get token "token = match_token(p, opt_tokens, args);" 
and return? -EINVAL.

Israel checked that.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH 1/5] nvme-fabrics: Add type of service (TOS) configuration
  2019-08-14 17:06   ` Sagi Grimberg
@ 2019-08-15  8:54     ` Max Gurtovoy
  0 siblings, 0 replies; 19+ messages in thread
From: Max Gurtovoy @ 2019-08-15  8:54 UTC (permalink / raw)



On 8/14/2019 8:06 PM, Sagi Grimberg wrote:
>
>
> On 8/14/19 3:19 AM, Israel Rukshin wrote:
>> TOS is user-defined and needs to be configured via nvme-cli.
>> It must be set before initiating any traffic and once set the TOS
>> cannot be changed.
>>
>> Signed-off-by: Israel Rukshin <israelr at mellanox.com>
>> Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
>> ---
>> ? drivers/nvme/host/fabrics.c | 20 +++++++++++++++++++-
>> ? drivers/nvme/host/fabrics.h |? 3 +++
>> ? 2 files changed, 22 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index 1994d5b..d343c22 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -611,6 +611,7 @@ bool __nvmf_check_ready(struct nvme_ctrl *ctrl, 
>> struct request *rq,
>> ????? { NVMF_OPT_DATA_DIGEST,??????? "data_digest"??????? },
>> ????? { NVMF_OPT_NR_WRITE_QUEUES,??? "nr_write_queues=%d"??? },
>> ????? { NVMF_OPT_NR_POLL_QUEUES,??? "nr_poll_queues=%d"??? },
>> +??? { NVMF_OPT_TOS,??????????? "tos=%d"??????? },
>> ????? { NVMF_OPT_ERR,??????????? NULL??????????? }
>> ? };
>> ? @@ -632,6 +633,7 @@ static int nvmf_parse_options(struct 
>> nvmf_ctrl_options *opts,
>> ????? opts->duplicate_connect = false;
>> ????? opts->hdr_digest = false;
>> ????? opts->data_digest = false;
>> +??? opts->tos = -1; /* < 0 == use transport default */
>> ? ????? options = o = kstrdup(buf, GFP_KERNEL);
>> ????? if (!options)
>> @@ -856,6 +858,22 @@ static int nvmf_parse_options(struct 
>> nvmf_ctrl_options *opts,
>> ????????????? }
>> ????????????? opts->nr_poll_queues = token;
>> ????????????? break;
>> +??????? case NVMF_OPT_TOS:
>> +??????????? if (match_int(args, &token)) {
>> +??????????????? ret = -EINVAL;
>> +??????????????? goto out;
>> +??????????? }
>> +??????????? if (token < 0) {
>> +??????????????? pr_err("Invalid type of service %d\n", token);
>> +??????????????? ret = -EINVAL;
>> +??????????????? goto out;
>> +??????????? }
>> +??????????? if (token > 255) {
>> +??????????????? pr_warn("Clamping type of service to 255\n");
>> +??????????????? token = 255;
>> +??????????? }
>
> Again, if tos is an abstract opaque, this needs to happen in the
> individual transports, not in the generic part.

right, we'll add it to .allowed_opts in the rdma.c and tcp.c and remove 
from NVMF_ALLOWED_OPTS in V4.

We'll leave the parsing in the common code.

-Max.

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] nvme-cli/fabrics: Add tos param to connect cmd
  2019-08-15  8:41     ` Max Gurtovoy
@ 2019-08-15 15:18       ` James Smart
  2019-08-15 17:13         ` Sagi Grimberg
  0 siblings, 1 reply; 19+ messages in thread
From: James Smart @ 2019-08-15 15:18 UTC (permalink / raw)


On 8/15/2019 1:41 AM, Max Gurtovoy wrote:
>
> On 8/14/2019 8:08 PM, Sagi Grimberg wrote:
>>
>>> Added 'tos' to 'connect' command so users can specify the type of 
>>> service.
>>>
>>> usage examples:
>>> nvme connect --tos=0 --transport=rdma --traddr=10.0.1.1 --nqn=test-nvme
>>> nvme connect -T 0 -t rdma -a 10.0.1.1 -n test_nvme
>>
>> Still don't see how this is handled if the kernel does not support
>> tos yet.
>
> how any other feature we added to connect command will act with old 
> kernel ? (e.g. queue_size/nr_queues)
>
> it will fail to get token "token = match_token(p, opt_tokens, args);" 
> and return? -EINVAL.
>
> Israel checked that.

I think what Sagi's getting at is: it would be really nice to not fail 
the whole connect request if an argument can't be parsed. E.g. a newer 
cli, used on an older kernel, should be functional with or without the 
tos argument.

If so- as I doubt the cli knows what was invalid or which argument was 
invalid and it would be really odd (especially over time) to back out 
arguments and try again - we need to update the kernel to warn that an 
argument was not parsed and was ignored, but attempt to use what it does 
recognize and not return failure to the cli.? Sagi ?

-- james

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] nvme-cli/fabrics: Add tos param to connect cmd
  2019-08-15 15:18       ` James Smart
@ 2019-08-15 17:13         ` Sagi Grimberg
  0 siblings, 0 replies; 19+ messages in thread
From: Sagi Grimberg @ 2019-08-15 17:13 UTC (permalink / raw)



>>>> Added 'tos' to 'connect' command so users can specify the type of 
>>>> service.
>>>>
>>>> usage examples:
>>>> nvme connect --tos=0 --transport=rdma --traddr=10.0.1.1 --nqn=test-nvme
>>>> nvme connect -T 0 -t rdma -a 10.0.1.1 -n test_nvme
>>>
>>> Still don't see how this is handled if the kernel does not support
>>> tos yet.
>>
>> how any other feature we added to connect command will act with old 
>> kernel ? (e.g. queue_size/nr_queues)
>>
>> it will fail to get token "token = match_token(p, opt_tokens, args);" 
>> and return? -EINVAL.
>>
>> Israel checked that.
> 
> I think what Sagi's getting at is: it would be really nice to not fail 
> the whole connect request if an argument can't be parsed. E.g. a newer 
> cli, used on an older kernel, should be functional with or without the 
> tos argument.
> 
> If so- as I doubt the cli knows what was invalid or which argument was 
> invalid and it would be really odd (especially over time) to back out 
> arguments and try again - we need to update the kernel to warn that an 
> argument was not parsed and was ignored, but attempt to use what it does 
> recognize and not return failure to the cli.? Sagi ?

Actually, I think its fine. I got it confused with another case where
nvme-cli generates this argument on its own from the discovery log
page (like the sqflow_disable thing).

I think we're fine here, we can ignore my comment...

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] nvme-cli/fabrics: Add tos param to connect cmd
  2019-08-18  9:08 [PATCH 0/5 V5] nvme: Add type of service (TOS) configuration Israel Rukshin
@ 2019-08-18  9:08 ` Israel Rukshin
  0 siblings, 0 replies; 19+ messages in thread
From: Israel Rukshin @ 2019-08-18  9:08 UTC (permalink / raw)


Added 'tos' to 'connect' command so users can specify the type of service.

usage examples:
nvme connect --tos=0 --transport=rdma --traddr=10.0.1.1 --nqn=test-nvme
nvme connect -T 0 -t rdma -a 10.0.1.1 -n test_nvme

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 fabrics.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 333669f..f952722 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -60,6 +60,7 @@ static struct config {
 	int  keep_alive_tmo;
 	int  reconnect_delay;
 	int  ctrl_loss_tmo;
+	int  tos;
 	char *raw;
 	char *device;
 	int  duplicate_connect;
@@ -576,11 +577,12 @@ add_bool_argument(char **argstr, int *max_len, char *arg_str, bool arg)
 }
 
 static int
-add_int_argument(char **argstr, int *max_len, char *arg_str, int arg)
+add_int_argument(char **argstr, int *max_len, char *arg_str, int arg,
+		 bool allow_zero)
 {
 	int len;
 
-	if (arg) {
+	if ((arg && !allow_zero) || (arg != -1 && allow_zero)) {
 		len = snprintf(*argstr, *max_len, ",%s=%d", arg_str, arg);
 		if (len < 0)
 			return -EINVAL;
@@ -640,21 +642,23 @@ static int build_options(char *argstr, int max_len, bool discover)
 		    add_argument(&argstr, &max_len, "hostid", cfg.hostid)) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "nr_io_queues",
-				cfg.nr_io_queues)) ||
+				cfg.nr_io_queues, false)) ||
 	    add_int_argument(&argstr, &max_len, "nr_write_queues",
-				cfg.nr_write_queues) ||
+				cfg.nr_write_queues, false) ||
 	    add_int_argument(&argstr, &max_len, "nr_poll_queues",
-				cfg.nr_poll_queues) ||
+				cfg.nr_poll_queues, false) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "queue_size",
-				cfg.queue_size)) ||
+				cfg.queue_size, false)) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "keep_alive_tmo",
-				cfg.keep_alive_tmo)) ||
+				cfg.keep_alive_tmo, false)) ||
 	    add_int_argument(&argstr, &max_len, "reconnect_delay",
-				cfg.reconnect_delay) ||
+				cfg.reconnect_delay, false) ||
 	    add_int_argument(&argstr, &max_len, "ctrl_loss_tmo",
-				cfg.ctrl_loss_tmo) ||
+				cfg.ctrl_loss_tmo, false) ||
+	    add_int_argument(&argstr, &max_len, "tos",
+				cfg.tos, true) ||
 	    add_bool_argument(&argstr, &max_len, "duplicate_connect",
 				cfg.duplicate_connect) ||
 	    add_bool_argument(&argstr, &max_len, "disable_sqflow",
@@ -749,6 +753,13 @@ retry:
 		p += len;
 	}
 
+	if (cfg.tos != -1) {
+		len = sprintf(p, ",tos=%d", cfg.tos);
+		if (len < 0)
+			return -EINVAL;
+		p += len;
+	}
+
 	if (cfg.keep_alive_tmo && !discover) {
 		len = sprintf(p, ",keep_alive_tmo=%d", cfg.keep_alive_tmo);
 		if (len < 0)
@@ -1065,6 +1076,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
+		{"tos",             'T', "LIST", CFG_INT, &cfg.tos,             required_argument, "type of service" },
 		{"hdr_digest", 'g', "", CFG_NONE, &cfg.hdr_digest, no_argument, "enable transport protocol header digest (TCP transport)" },
 		{"data_digest", 'G', "", CFG_NONE, &cfg.data_digest, no_argument, "enable transport protocol data digest (TCP transport)" },
 		{"nr-io-queues",    'i', "LIST", CFG_INT, &cfg.nr_io_queues,    required_argument, "number of io queues to use (default is core count)" },
@@ -1076,6 +1088,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{NULL},
 	};
 
+	cfg.tos = -1;
 	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
 			sizeof(cfg));
 	if (ret)
@@ -1122,6 +1135,7 @@ int connect(const char *desc, int argc, char **argv)
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
+		{"tos",             'T', "LIST", CFG_INT, &cfg.tos,             required_argument, "type of service" },
 		{"duplicate_connect", 'D', "", CFG_NONE, &cfg.duplicate_connect, no_argument, "allow duplicate connections between same transport host and subsystem port" },
 		{"disable_sqflow", 'd', "", CFG_NONE, &cfg.disable_sqflow, no_argument, "disable controller sq flow control (default false)" },
 		{"hdr_digest", 'g', "", CFG_NONE, &cfg.hdr_digest, no_argument, "enable transport protocol header digest (TCP transport)" },
@@ -1129,6 +1143,7 @@ int connect(const char *desc, int argc, char **argv)
 		{NULL},
 	};
 
+	cfg.tos = -1;
 	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
 			sizeof(cfg));
 	if (ret)
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH] nvme-cli/fabrics: Add tos param to connect cmd
  2019-08-15 13:33 [PATCH 0/5 V4] nvme: Add type of service (TOS) configuration Israel Rukshin
@ 2019-08-15 13:33 ` Israel Rukshin
  0 siblings, 0 replies; 19+ messages in thread
From: Israel Rukshin @ 2019-08-15 13:33 UTC (permalink / raw)


Added 'tos' to 'connect' command so users can specify the type of service.

usage examples:
nvme connect --tos=0 --transport=rdma --traddr=10.0.1.1 --nqn=test-nvme
nvme connect -T 0 -t rdma -a 10.0.1.1 -n test_nvme

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 fabrics.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 333669f..f952722 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -60,6 +60,7 @@ static struct config {
 	int  keep_alive_tmo;
 	int  reconnect_delay;
 	int  ctrl_loss_tmo;
+	int  tos;
 	char *raw;
 	char *device;
 	int  duplicate_connect;
@@ -576,11 +577,12 @@ add_bool_argument(char **argstr, int *max_len, char *arg_str, bool arg)
 }
 
 static int
-add_int_argument(char **argstr, int *max_len, char *arg_str, int arg)
+add_int_argument(char **argstr, int *max_len, char *arg_str, int arg,
+		 bool allow_zero)
 {
 	int len;
 
-	if (arg) {
+	if ((arg && !allow_zero) || (arg != -1 && allow_zero)) {
 		len = snprintf(*argstr, *max_len, ",%s=%d", arg_str, arg);
 		if (len < 0)
 			return -EINVAL;
@@ -640,21 +642,23 @@ static int build_options(char *argstr, int max_len, bool discover)
 		    add_argument(&argstr, &max_len, "hostid", cfg.hostid)) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "nr_io_queues",
-				cfg.nr_io_queues)) ||
+				cfg.nr_io_queues, false)) ||
 	    add_int_argument(&argstr, &max_len, "nr_write_queues",
-				cfg.nr_write_queues) ||
+				cfg.nr_write_queues, false) ||
 	    add_int_argument(&argstr, &max_len, "nr_poll_queues",
-				cfg.nr_poll_queues) ||
+				cfg.nr_poll_queues, false) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "queue_size",
-				cfg.queue_size)) ||
+				cfg.queue_size, false)) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "keep_alive_tmo",
-				cfg.keep_alive_tmo)) ||
+				cfg.keep_alive_tmo, false)) ||
 	    add_int_argument(&argstr, &max_len, "reconnect_delay",
-				cfg.reconnect_delay) ||
+				cfg.reconnect_delay, false) ||
 	    add_int_argument(&argstr, &max_len, "ctrl_loss_tmo",
-				cfg.ctrl_loss_tmo) ||
+				cfg.ctrl_loss_tmo, false) ||
+	    add_int_argument(&argstr, &max_len, "tos",
+				cfg.tos, true) ||
 	    add_bool_argument(&argstr, &max_len, "duplicate_connect",
 				cfg.duplicate_connect) ||
 	    add_bool_argument(&argstr, &max_len, "disable_sqflow",
@@ -749,6 +753,13 @@ retry:
 		p += len;
 	}
 
+	if (cfg.tos != -1) {
+		len = sprintf(p, ",tos=%d", cfg.tos);
+		if (len < 0)
+			return -EINVAL;
+		p += len;
+	}
+
 	if (cfg.keep_alive_tmo && !discover) {
 		len = sprintf(p, ",keep_alive_tmo=%d", cfg.keep_alive_tmo);
 		if (len < 0)
@@ -1065,6 +1076,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
+		{"tos",             'T', "LIST", CFG_INT, &cfg.tos,             required_argument, "type of service" },
 		{"hdr_digest", 'g', "", CFG_NONE, &cfg.hdr_digest, no_argument, "enable transport protocol header digest (TCP transport)" },
 		{"data_digest", 'G', "", CFG_NONE, &cfg.data_digest, no_argument, "enable transport protocol data digest (TCP transport)" },
 		{"nr-io-queues",    'i', "LIST", CFG_INT, &cfg.nr_io_queues,    required_argument, "number of io queues to use (default is core count)" },
@@ -1076,6 +1088,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{NULL},
 	};
 
+	cfg.tos = -1;
 	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
 			sizeof(cfg));
 	if (ret)
@@ -1122,6 +1135,7 @@ int connect(const char *desc, int argc, char **argv)
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
+		{"tos",             'T', "LIST", CFG_INT, &cfg.tos,             required_argument, "type of service" },
 		{"duplicate_connect", 'D', "", CFG_NONE, &cfg.duplicate_connect, no_argument, "allow duplicate connections between same transport host and subsystem port" },
 		{"disable_sqflow", 'd', "", CFG_NONE, &cfg.disable_sqflow, no_argument, "disable controller sq flow control (default false)" },
 		{"hdr_digest", 'g', "", CFG_NONE, &cfg.hdr_digest, no_argument, "enable transport protocol header digest (TCP transport)" },
@@ -1129,6 +1143,7 @@ int connect(const char *desc, int argc, char **argv)
 		{NULL},
 	};
 
+	cfg.tos = -1;
 	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
 			sizeof(cfg));
 	if (ret)
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH] nvme-cli/fabrics: Add tos param to connect cmd
  2019-08-13 16:43   ` Sagi Grimberg
@ 2019-08-14  7:42     ` Israel Rukshin
  0 siblings, 0 replies; 19+ messages in thread
From: Israel Rukshin @ 2019-08-14  7:42 UTC (permalink / raw)


On 8/13/2019 7:43 PM, Sagi Grimberg wrote:
>
>> Added 'tos' to 'connect' command so users can specify the type of 
>> service.
>>
>> usage examples:
>> nvme connect --tos=0 --transport=rdma --traddr=10.0.1.1 --nqn=test-nvme
>> nvme connect -T 0 -t rdma -a 10.0.1.1 -n test_nvme
>>
>
> What if you run this with a kernel that doesn't support tos yet?
You will get:
nvme_fabrics: unknown parameter or missing value 'tos=0' in ctrl 
creation request

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] nvme-cli/fabrics: Add tos param to connect cmd
  2019-08-13 13:17 ` [PATCH] nvme-cli/fabrics: Add tos param to connect cmd Israel Rukshin
@ 2019-08-13 16:43   ` Sagi Grimberg
  2019-08-14  7:42     ` Israel Rukshin
  0 siblings, 1 reply; 19+ messages in thread
From: Sagi Grimberg @ 2019-08-13 16:43 UTC (permalink / raw)



> Added 'tos' to 'connect' command so users can specify the type of service.
> 
> usage examples:
> nvme connect --tos=0 --transport=rdma --traddr=10.0.1.1 --nqn=test-nvme
> nvme connect -T 0 -t rdma -a 10.0.1.1 -n test_nvme
> 

What if you run this with a kernel that doesn't support tos yet?

^ permalink raw reply	[flat|nested] 19+ messages in thread

* [PATCH] nvme-cli/fabrics: Add tos param to connect cmd
  2019-08-13 13:17 [PATCH 0/5 V2] nvme: Add type of service (TOS) configuration Israel Rukshin
@ 2019-08-13 13:17 ` Israel Rukshin
  2019-08-13 16:43   ` Sagi Grimberg
  0 siblings, 1 reply; 19+ messages in thread
From: Israel Rukshin @ 2019-08-13 13:17 UTC (permalink / raw)


Added 'tos' to 'connect' command so users can specify the type of service.

usage examples:
nvme connect --tos=0 --transport=rdma --traddr=10.0.1.1 --nqn=test-nvme
nvme connect -T 0 -t rdma -a 10.0.1.1 -n test_nvme

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
Reviewed-by: Max Gurtovoy <maxg at mellanox.com>
---
 fabrics.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 333669f..f952722 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -60,6 +60,7 @@ static struct config {
 	int  keep_alive_tmo;
 	int  reconnect_delay;
 	int  ctrl_loss_tmo;
+	int  tos;
 	char *raw;
 	char *device;
 	int  duplicate_connect;
@@ -576,11 +577,12 @@ add_bool_argument(char **argstr, int *max_len, char *arg_str, bool arg)
 }
 
 static int
-add_int_argument(char **argstr, int *max_len, char *arg_str, int arg)
+add_int_argument(char **argstr, int *max_len, char *arg_str, int arg,
+		 bool allow_zero)
 {
 	int len;
 
-	if (arg) {
+	if ((arg && !allow_zero) || (arg != -1 && allow_zero)) {
 		len = snprintf(*argstr, *max_len, ",%s=%d", arg_str, arg);
 		if (len < 0)
 			return -EINVAL;
@@ -640,21 +642,23 @@ static int build_options(char *argstr, int max_len, bool discover)
 		    add_argument(&argstr, &max_len, "hostid", cfg.hostid)) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "nr_io_queues",
-				cfg.nr_io_queues)) ||
+				cfg.nr_io_queues, false)) ||
 	    add_int_argument(&argstr, &max_len, "nr_write_queues",
-				cfg.nr_write_queues) ||
+				cfg.nr_write_queues, false) ||
 	    add_int_argument(&argstr, &max_len, "nr_poll_queues",
-				cfg.nr_poll_queues) ||
+				cfg.nr_poll_queues, false) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "queue_size",
-				cfg.queue_size)) ||
+				cfg.queue_size, false)) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "keep_alive_tmo",
-				cfg.keep_alive_tmo)) ||
+				cfg.keep_alive_tmo, false)) ||
 	    add_int_argument(&argstr, &max_len, "reconnect_delay",
-				cfg.reconnect_delay) ||
+				cfg.reconnect_delay, false) ||
 	    add_int_argument(&argstr, &max_len, "ctrl_loss_tmo",
-				cfg.ctrl_loss_tmo) ||
+				cfg.ctrl_loss_tmo, false) ||
+	    add_int_argument(&argstr, &max_len, "tos",
+				cfg.tos, true) ||
 	    add_bool_argument(&argstr, &max_len, "duplicate_connect",
 				cfg.duplicate_connect) ||
 	    add_bool_argument(&argstr, &max_len, "disable_sqflow",
@@ -749,6 +753,13 @@ retry:
 		p += len;
 	}
 
+	if (cfg.tos != -1) {
+		len = sprintf(p, ",tos=%d", cfg.tos);
+		if (len < 0)
+			return -EINVAL;
+		p += len;
+	}
+
 	if (cfg.keep_alive_tmo && !discover) {
 		len = sprintf(p, ",keep_alive_tmo=%d", cfg.keep_alive_tmo);
 		if (len < 0)
@@ -1065,6 +1076,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
+		{"tos",             'T', "LIST", CFG_INT, &cfg.tos,             required_argument, "type of service" },
 		{"hdr_digest", 'g', "", CFG_NONE, &cfg.hdr_digest, no_argument, "enable transport protocol header digest (TCP transport)" },
 		{"data_digest", 'G', "", CFG_NONE, &cfg.data_digest, no_argument, "enable transport protocol data digest (TCP transport)" },
 		{"nr-io-queues",    'i', "LIST", CFG_INT, &cfg.nr_io_queues,    required_argument, "number of io queues to use (default is core count)" },
@@ -1076,6 +1088,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{NULL},
 	};
 
+	cfg.tos = -1;
 	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
 			sizeof(cfg));
 	if (ret)
@@ -1122,6 +1135,7 @@ int connect(const char *desc, int argc, char **argv)
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
+		{"tos",             'T', "LIST", CFG_INT, &cfg.tos,             required_argument, "type of service" },
 		{"duplicate_connect", 'D', "", CFG_NONE, &cfg.duplicate_connect, no_argument, "allow duplicate connections between same transport host and subsystem port" },
 		{"disable_sqflow", 'd', "", CFG_NONE, &cfg.disable_sqflow, no_argument, "disable controller sq flow control (default false)" },
 		{"hdr_digest", 'g', "", CFG_NONE, &cfg.hdr_digest, no_argument, "enable transport protocol header digest (TCP transport)" },
@@ -1129,6 +1143,7 @@ int connect(const char *desc, int argc, char **argv)
 		{NULL},
 	};
 
+	cfg.tos = -1;
 	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
 			sizeof(cfg));
 	if (ret)
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

* [PATCH] nvme-cli/fabrics: Add tos param to connect cmd
  2019-08-04  9:55 [PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration Israel Rukshin
@ 2019-08-04  9:55 ` Israel Rukshin
  0 siblings, 0 replies; 19+ messages in thread
From: Israel Rukshin @ 2019-08-04  9:55 UTC (permalink / raw)


Added 'tos' to 'connect' command so users can specify the type of service.

usage examples:
nvme connect --tos=0 --transport=rdma --traddr=10.0.1.1 --nqn=test-nvme
nvme connect -T 0 -t rdma -a 10.0.1.1 -n test_nvme

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
---
 fabrics.c | 33 ++++++++++++++++++++++++---------
 1 file changed, 24 insertions(+), 9 deletions(-)

diff --git a/fabrics.c b/fabrics.c
index 333669f..f952722 100644
--- a/fabrics.c
+++ b/fabrics.c
@@ -60,6 +60,7 @@ static struct config {
 	int  keep_alive_tmo;
 	int  reconnect_delay;
 	int  ctrl_loss_tmo;
+	int  tos;
 	char *raw;
 	char *device;
 	int  duplicate_connect;
@@ -576,11 +577,12 @@ add_bool_argument(char **argstr, int *max_len, char *arg_str, bool arg)
 }
 
 static int
-add_int_argument(char **argstr, int *max_len, char *arg_str, int arg)
+add_int_argument(char **argstr, int *max_len, char *arg_str, int arg,
+		 bool allow_zero)
 {
 	int len;
 
-	if (arg) {
+	if ((arg && !allow_zero) || (arg != -1 && allow_zero)) {
 		len = snprintf(*argstr, *max_len, ",%s=%d", arg_str, arg);
 		if (len < 0)
 			return -EINVAL;
@@ -640,21 +642,23 @@ static int build_options(char *argstr, int max_len, bool discover)
 		    add_argument(&argstr, &max_len, "hostid", cfg.hostid)) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "nr_io_queues",
-				cfg.nr_io_queues)) ||
+				cfg.nr_io_queues, false)) ||
 	    add_int_argument(&argstr, &max_len, "nr_write_queues",
-				cfg.nr_write_queues) ||
+				cfg.nr_write_queues, false) ||
 	    add_int_argument(&argstr, &max_len, "nr_poll_queues",
-				cfg.nr_poll_queues) ||
+				cfg.nr_poll_queues, false) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "queue_size",
-				cfg.queue_size)) ||
+				cfg.queue_size, false)) ||
 	    (!discover &&
 	      add_int_argument(&argstr, &max_len, "keep_alive_tmo",
-				cfg.keep_alive_tmo)) ||
+				cfg.keep_alive_tmo, false)) ||
 	    add_int_argument(&argstr, &max_len, "reconnect_delay",
-				cfg.reconnect_delay) ||
+				cfg.reconnect_delay, false) ||
 	    add_int_argument(&argstr, &max_len, "ctrl_loss_tmo",
-				cfg.ctrl_loss_tmo) ||
+				cfg.ctrl_loss_tmo, false) ||
+	    add_int_argument(&argstr, &max_len, "tos",
+				cfg.tos, true) ||
 	    add_bool_argument(&argstr, &max_len, "duplicate_connect",
 				cfg.duplicate_connect) ||
 	    add_bool_argument(&argstr, &max_len, "disable_sqflow",
@@ -749,6 +753,13 @@ retry:
 		p += len;
 	}
 
+	if (cfg.tos != -1) {
+		len = sprintf(p, ",tos=%d", cfg.tos);
+		if (len < 0)
+			return -EINVAL;
+		p += len;
+	}
+
 	if (cfg.keep_alive_tmo && !discover) {
 		len = sprintf(p, ",keep_alive_tmo=%d", cfg.keep_alive_tmo);
 		if (len < 0)
@@ -1065,6 +1076,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
+		{"tos",             'T', "LIST", CFG_INT, &cfg.tos,             required_argument, "type of service" },
 		{"hdr_digest", 'g', "", CFG_NONE, &cfg.hdr_digest, no_argument, "enable transport protocol header digest (TCP transport)" },
 		{"data_digest", 'G', "", CFG_NONE, &cfg.data_digest, no_argument, "enable transport protocol data digest (TCP transport)" },
 		{"nr-io-queues",    'i', "LIST", CFG_INT, &cfg.nr_io_queues,    required_argument, "number of io queues to use (default is core count)" },
@@ -1076,6 +1088,7 @@ int discover(const char *desc, int argc, char **argv, bool connect)
 		{NULL},
 	};
 
+	cfg.tos = -1;
 	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
 			sizeof(cfg));
 	if (ret)
@@ -1122,6 +1135,7 @@ int connect(const char *desc, int argc, char **argv)
 		{"keep-alive-tmo",  'k', "LIST", CFG_INT, &cfg.keep_alive_tmo,  required_argument, "keep alive timeout period in seconds" },
 		{"reconnect-delay", 'c', "LIST", CFG_INT, &cfg.reconnect_delay, required_argument, "reconnect timeout period in seconds" },
 		{"ctrl-loss-tmo",   'l', "LIST", CFG_INT, &cfg.ctrl_loss_tmo,   required_argument, "controller loss timeout period in seconds" },
+		{"tos",             'T', "LIST", CFG_INT, &cfg.tos,             required_argument, "type of service" },
 		{"duplicate_connect", 'D', "", CFG_NONE, &cfg.duplicate_connect, no_argument, "allow duplicate connections between same transport host and subsystem port" },
 		{"disable_sqflow", 'd', "", CFG_NONE, &cfg.disable_sqflow, no_argument, "disable controller sq flow control (default false)" },
 		{"hdr_digest", 'g', "", CFG_NONE, &cfg.hdr_digest, no_argument, "enable transport protocol header digest (TCP transport)" },
@@ -1129,6 +1143,7 @@ int connect(const char *desc, int argc, char **argv)
 		{NULL},
 	};
 
+	cfg.tos = -1;
 	ret = argconfig_parse(argc, argv, desc, command_line_options, &cfg,
 			sizeof(cfg));
 	if (ret)
-- 
1.8.3.1

^ permalink raw reply related	[flat|nested] 19+ messages in thread

end of thread, other threads:[~2019-08-18  9:08 UTC | newest]

Thread overview: 19+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-14 10:19 [PATCH 0/5 V3] nvme: Add type of service (TOS) configuration Israel Rukshin
2019-08-14 10:19 ` [PATCH] nvme-cli/fabrics: Add tos param to connect cmd Israel Rukshin
2019-08-14 17:08   ` Sagi Grimberg
2019-08-15  8:41     ` Max Gurtovoy
2019-08-15 15:18       ` James Smart
2019-08-15 17:13         ` Sagi Grimberg
2019-08-14 10:19 ` [PATCH 1/5] nvme-fabrics: Add type of service (TOS) configuration Israel Rukshin
2019-08-14 17:06   ` Sagi Grimberg
2019-08-15  8:54     ` Max Gurtovoy
2019-08-14 10:19 ` [PATCH 2/5] nvme-rdma: Add TOS for rdma transport Israel Rukshin
2019-08-14 10:19 ` [PATCH 3/5] nvme-tcp: Use struct nvme_ctrl directly Israel Rukshin
2019-08-14 10:19 ` [PATCH 4/5] nvme-tcp: Add TOS for tcp transport Israel Rukshin
2019-08-14 10:19 ` [PATCH 5/5] nvmet-tcp: " Israel Rukshin
  -- strict thread matches above, loose matches on Subject: below --
2019-08-18  9:08 [PATCH 0/5 V5] nvme: Add type of service (TOS) configuration Israel Rukshin
2019-08-18  9:08 ` [PATCH] nvme-cli/fabrics: Add tos param to connect cmd Israel Rukshin
2019-08-15 13:33 [PATCH 0/5 V4] nvme: Add type of service (TOS) configuration Israel Rukshin
2019-08-15 13:33 ` [PATCH] nvme-cli/fabrics: Add tos param to connect cmd Israel Rukshin
2019-08-13 13:17 [PATCH 0/5 V2] nvme: Add type of service (TOS) configuration Israel Rukshin
2019-08-13 13:17 ` [PATCH] nvme-cli/fabrics: Add tos param to connect cmd Israel Rukshin
2019-08-13 16:43   ` Sagi Grimberg
2019-08-14  7:42     ` Israel Rukshin
2019-08-04  9:55 [PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration Israel Rukshin
2019-08-04  9:55 ` [PATCH] nvme-cli/fabrics: Add tos param to connect cmd Israel Rukshin

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).