[PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration

[PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration

[Israel Rukshin]

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>
---
 drivers/nvme/host/fabrics.c | 16 +++++++++++++++-
 drivers/nvme/host/fabrics.h |  3 +++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 5838f7cd53ac..dbce7ffe6c2c 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -611,6 +611,7 @@ static const match_table_t opt_tokens = {
 	{ 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,18 @@ 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 || token > 255) {
+				pr_err("Invalid type of service %d\n", token);
+				ret = -EINVAL;
+				goto out;
+			}
+			opts->tos = token;
+			break;
 		default:
 			pr_warn("unknown parameter or missing value '%s' in ctrl creation request\n",
 				p);
@@ -975,7 +989,7 @@ EXPORT_SYMBOL_GPL(nvmf_free_options);
 #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 3044d8b99a24..93f08d77c896 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;
 };
 
 /*
-- 
2.16.3

[PATCH 2/2] nvme-rdma: Add TOS for rdma transport

[Israel Rukshin]

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.

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

Signed-off-by: Israel Rukshin <israelr at mellanox.com>
---
 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 13743b97a315..091f1d2f108f 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -1758,16 +1758,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;
 	}
-- 
2.16.3

[PATCH] nvme-cli/fabrics: Add tos param to connect cmd

[Israel Rukshin]

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

[PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration

[Sagi Grimberg]

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

Can you please have tcp support it too in the patch set (and test
that it works)?

Its simply setsockopt with SO_PRIORITY.

[PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration

[Israel Rukshin]

On 8/5/2019 6:10 PM, Sagi Grimberg 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.
>
> Can you please have tcp support it too in the patch set (and test
> that it works)?
>
> Its simply setsockopt with SO_PRIORITY.

Hi,

Yes sure,? l will add support also for tcp.

Regards,

Israel

[PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration

[James Smart]

On 8/4/2019 2:55 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>
> ---
>   drivers/nvme/host/fabrics.c | 16 +++++++++++++++-
>   drivers/nvme/host/fabrics.h |  3 +++
>   2 files changed, 18 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 5838f7cd53ac..dbce7ffe6c2c 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -611,6 +611,7 @@ static const match_table_t opt_tokens = {
>   	{ 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			}
>   };
>

I think we need to create a framework for transport-specific options.? 
Keeping them at the same flat level as the generic options isn't a great 
idea as they start to grow and always modifying core for a transport 
option isn't good either.

How about we start the convention "<transport_name>:<option>" for an 
option.? The opts struct can have a pointer that is owned by the 
transport.? The transport can supply a parsing function for an option.? 
The core layer can match the transport name in the prefix, then invoke 
the transport handler instead of the generic parsing. Transport handler 
can parse and set it's own value in its own opts struct. Transport can 
validate it has the necessary transport opts as the first thing it does 
in it's create_ctrl routine.

-- james

[PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration

[Sagi Grimberg]

> On 8/4/2019 2:55 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>
>> ---
>> ? drivers/nvme/host/fabrics.c | 16 +++++++++++++++-
>> ? drivers/nvme/host/fabrics.h |? 3 +++
>> ? 2 files changed, 18 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>> index 5838f7cd53ac..dbce7ffe6c2c 100644
>> --- a/drivers/nvme/host/fabrics.c
>> +++ b/drivers/nvme/host/fabrics.c
>> @@ -611,6 +611,7 @@ static const match_table_t opt_tokens = {
>> ????? { 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??????????? }
>> ? };
>>
> 
> I think we need to create a framework for transport-specific options. 
> Keeping them at the same flat level as the generic options isn't a great 
> idea as they start to grow and always modifying core for a transport 
> option isn't good either.

Well, not disagreeing, but I personally think that this can be
applicable to all transports as this is a generic term that happens
to map to a specific marking in ip transports.

Also, I actually think that keeping a single parser simplifies things.
Unless we may run out of short arg options, which in that case it is
beneficial...

> How about we start the convention "<transport_name>:<option>" for an 
> option.? The opts struct can have a pointer that is owned by the 
> transport.? The transport can supply a parsing function for an option. 
> The core layer can match the transport name in the prefix, then invoke 
> the transport handler instead of the generic parsing. Transport handler 
> can parse and set it's own value in its own opts struct. Transport can 
> validate it has the necessary transport opts as the first thing it does 
> in it's create_ctrl routine.

That is one way to go...

I don't have a strong stand here, what do others think?

[PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration

[Max Gurtovoy]

On 8/6/2019 2:17 AM, Sagi Grimberg wrote:
>
>> On 8/4/2019 2:55 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>
>>> ---
>>> ? drivers/nvme/host/fabrics.c | 16 +++++++++++++++-
>>> ? drivers/nvme/host/fabrics.h |? 3 +++
>>> ? 2 files changed, 18 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
>>> index 5838f7cd53ac..dbce7ffe6c2c 100644
>>> --- a/drivers/nvme/host/fabrics.c
>>> +++ b/drivers/nvme/host/fabrics.c
>>> @@ -611,6 +611,7 @@ static const match_table_t opt_tokens = {
>>> ????? { 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??????????? }
>>> ? };
>>>
>>
>> I think we need to create a framework for transport-specific options. 
>> Keeping them at the same flat level as the generic options isn't a 
>> great idea as they start to grow and always modifying core for a 
>> transport option isn't good either.
>
> Well, not disagreeing, but I personally think that this can be
> applicable to all transports as this is a generic term that happens
> to map to a specific marking in ip transports.
>
> Also, I actually think that keeping a single parser simplifies things.
> Unless we may run out of short arg options, which in that case it is
> beneficial...
>
>> How about we start the convention "<transport_name>:<option>" for an 
>> option.? The opts struct can have a pointer that is owned by the 
>> transport.? The transport can supply a parsing function for an 
>> option. The core layer can match the transport name in the prefix, 
>> then invoke the transport handler instead of the generic parsing. 
>> Transport handler can parse and set it's own value in its own opts 
>> struct. Transport can validate it has the necessary transport opts as 
>> the first thing it does in it's create_ctrl routine.
>
> That is one way to go...
>
> I don't have a strong stand here, what do others think?

I think we can re-use the code in the general fabrics and just add 
helpers for each option to decrease the lines of nvmf_parse_options func 
and make it more readable.

I prefer to avoid of parsing per transport.

something like:

case NVMF_OPT_NR_POLL_QUEUES:

 ??? nvmf_parse_nr_poll_queues(opts, args);

 ??? break;

case NVMF_OPT_TOS:

 ??? nvmf_parse_tos(opts, args);

 ??? break;

-Max.

[PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration

[Max Gurtovoy]

The series looks good (sorry for not reviewing internally :))

please add to the v2:

connect example for the kernel commit message (rdma + tcp) and please 
show how the TOS influence on the traffic:

e.g. 80% to ctrl_1 and 20% to ctrl_2.



Reviewed-by: Max Gurtovoy <maxg at mellanox.com>

[PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration

[Israel Rukshin]

On 8/5/2019 6:10 PM, Sagi Grimberg 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.
>
> Can you please have tcp support it too in the patch set (and test
> that it works)?
>
> Its simply setsockopt with SO_PRIORITY.

Hi Sagi,

I implemented your suggestion but it doesn't work for me.

The kernel_setsockopt() succeed and I see that 
queue->sock->sk->sk_priority was changed to the correct value,

but I don't see that the priority was changed on Wireshark.

Do I miss anything?

Here is my change:

diff --git a/drivers/nvme/host/tcp.c b/drivers/nvme/host/tcp.c
index 606b13d..c0efb2a 100644
--- a/drivers/nvme/host/tcp.c
+++ b/drivers/nvme/host/tcp.c
@@ -1301,6 +1301,19 @@ static int nvme_tcp_alloc_queue(struct nvme_ctrl 
*nctrl,
 ???? ??? goto err_sock;
 ???? }

+??? /* Set socket priority */
+??? if (nctrl->opts->tos >= 0) {
+??? ??? opt = nctrl->opts->tos;
+??? ??? ret = kernel_setsockopt(queue->sock, SOL_SOCKET, SO_PRIORITY,
+??? ??? ??? ??? (char *)&opt, sizeof(opt));
+??? ??? if (ret) {
+??? ??? ??? dev_err(nctrl->device,
+??? ??? ??? ??? "failed to set SO_PRIORITY sock opt %d\n", ret);
+??? ??? ??? goto err_sock;
+??? ??? }
+??? }
+
 ???? queue->sock->sk->sk_allocation = GFP_ATOMIC;
 ???? if (!qid)
 ???? ??? n = 0;
-- 


Regards,

Israel

[PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration

[Sagi Grimberg]

> Hi Sagi,
> 
> I implemented your suggestion but it doesn't work for me.
> 
> The kernel_setsockopt() succeed and I see that
> queue->sock->sk->sk_priority was changed to the correct value,
> 
> but I don't see that the priority was changed on Wireshark.
> 
> Do I miss anything?

Should work, can you instrument to see that the skb->priority does
get assigned correctly?

[PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration

[Israel Rukshin]

On 8/7/2019 11:53 PM, Sagi Grimberg wrote:
>
>> Hi Sagi,
>>
>> I implemented your suggestion but it doesn't work for me.
>>
>> The kernel_setsockopt() succeed and I see that
>> queue->sock->sk->sk_priority was changed to the correct value,
>>
>> but I don't see that the priority was changed on Wireshark.
>>
>> Do I miss anything?
>
> Should work, can you instrument to see that the skb->priority does
> get assigned correctly?

I did the following instead and it works! :)

+?????????????? ret = kernel_setsockopt(queue->sock, SOL_IP, IP_TOS,
+?????????????????????????????? (char *)&opt, sizeof(opt));

It affects only the outgoing packets from the initiator side (unlike 
rdma-cm that affects both sides).

I added the same kernel_setsockopt() to the target at 
nvmet_tcp_add_port() and it works on both sides.

Do you think we need to add port configuration on the target side for 
nvmet tcp?

Regards,

Israel

[PATCH 1/2] nvme-fabrics: Add type of service (TOS) configuration

[Sagi Grimberg]

>>> Hi Sagi,
>>>
>>> I implemented your suggestion but it doesn't work for me.
>>>
>>> The kernel_setsockopt() succeed and I see that
>>> queue->sock->sk->sk_priority was changed to the correct value,
>>>
>>> but I don't see that the priority was changed on Wireshark.
>>>
>>> Do I miss anything?
>>
>> Should work, can you instrument to see that the skb->priority does
>> get assigned correctly?
> 
> I did the following instead and it works! :)
> 
> +?????????????? ret = kernel_setsockopt(queue->sock, SOL_IP, IP_TOS,
> +?????????????????????????????? (char *)&opt, sizeof(opt));
> 
> It affects only the outgoing packets from the initiator side (unlike
> rdma-cm that affects both sides).
> 
> I added the same kernel_setsockopt() to the target at
> nvmet_tcp_add_port() and it works on both sides.
> 
> Do you think we need to add port configuration on the target side for
> nvmet tcp?

Absolutely not :)

We should check our received tos and set according to that.

Can you test something like:
--
diff --git a/drivers/nvme/target/tcp.c b/drivers/nvme/target/tcp.c
index 76e43750b9e5..a2f7e1eec440 100644
--- a/drivers/nvme/target/tcp.c
+++ b/drivers/nvme/target/tcp.c
@@ -1415,6 +1415,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;

@@ -1448,6 +1449,15 @@ static int nvmet_tcp_set_queue_sock(struct 
nvmet_tcp_queue *queue)
         sock->sk->sk_write_space = nvmet_tcp_write_space;
         write_unlock_bh(&sock->sk->sk_callback_lock);

+       if (inet->rcv_tos > 0) {
+               int opt = inet->rcv_tos;
+
+               ret = kernel_setsockopt(sock, SOL_IP, IP_TOS,
+                               (char *)&opt, sizeof(opt));
+               if (ret)
+                       return ret;
+       }
+
         return 0;
  }
--