Skip to contentSkip to conversation

Get rid of transport layer retry count config parameter

PATCH New Sagi Grimberg 5 participants 15 messages Jun 22, 2016 – Jul 20, 2016

Subsystems (2)
lore
Share & tools
lore

Sagi Grimberg

author maintainer
#0 lore raw

cover Get rid of transport layer retry count config parameter

This parameter was added in order to support a proper timeout for error recovery before the spec defined a periodic keep-alive.

Now that we have periodic keep-alive, we don't need a user configurable transport layer retry count, the keep-alive timeout is sufficient, transports can retry for as long as they see fit.

Sagi Grimberg (2):
nvme-rdma: Don't use tl_retry_count
nvme-fabrics: Remove tl_retry_count

 drivers/nvme/host/fabrics.c | 14 --------------
 drivers/nvme/host/fabrics.h |  3 ---
 drivers/nvme/host/rdma.c    |  9 +++------
 3 files changed, 3 insertions(+), 23 deletions(-)
Signature · 1 line
1.9.1
author maintainer
#1 lore raw

PATCH 1/2 nvme-rdma: Don't use tl_retry_count

Always use the maximum qp retry count as the
error recovery timeout is dictated from the nvme
keep-alive.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/rdma.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/nvme/host/rdma.c +3 -6
index e1205c0d36e4..b939f89ad936 100644
@@ -117,7 +117,6 @@ struct nvme_rdma_ctrl {
 	u32			queue_count;
 
 	/* other member variables */
-	unsigned short		tl_retry_count;
 	struct blk_mq_tag_set	tag_set;
 	struct work_struct	delete_work;
 	struct work_struct	reset_work;
@@ -1268,8 +1267,8 @@ static int nvme_rdma_route_resolved(struct nvme_rdma_queue *queue)
 	param.flow_control = 1;
 
 	param.responder_resources = queue->device->dev->attrs.max_qp_rd_atom;
-	/* rdma_cm will clamp down to max QP retry count (7) */
-	param.retry_count = ctrl->tl_retry_count;
+	/* maximum retry count */
+	param.retry_count = 7;
 	param.rnr_retry_count = 7;
 	param.private_data = &priv;
 	param.private_data_len = sizeof(priv);
@@ -1898,7 +1897,6 @@ static struct nvme_ctrl *nvme_rdma_create_ctrl(struct device *dev,
 
 	ctrl->queue_count = opts->nr_io_queues + 1; /* +1 for admin queue */
 	ctrl->ctrl.sqsize = opts->queue_size;
-	ctrl->tl_retry_count = opts->tl_retry_count;
 	ctrl->ctrl.kato = opts->kato;
 
 	ret = -ENOMEM;
@@ -1975,8 +1973,7 @@ out_free_ctrl:
 static struct nvmf_transport_ops nvme_rdma_transport = {
 	.name		= "rdma",
 	.required_opts	= NVMF_OPT_TRADDR,
-	.allowed_opts	= NVMF_OPT_TRSVCID | NVMF_OPT_TL_RETRY_COUNT |
-			  NVMF_OPT_RECONNECT_DELAY,
+	.allowed_opts	= NVMF_OPT_TRSVCID | NVMF_OPT_RECONNECT_DELAY,
 	.create_ctrl	= nvme_rdma_create_ctrl,
 };
 
Signature · 1 line
1.9.1
author maintainer
#2 lore raw

PATCH 2/2 nvme-fabrics: Remove tl_retry_count

In reply to Sagi Grimberg

The timeout before error recovery logic kicks in is dictated by the nvme keep-alive, so we don't really need a transport layer retry count. transports can retry for as much as they like.

Signed-off-by Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/fabrics.c | 14 --------------
 drivers/nvme/host/fabrics.h |  3 ---
 2 files changed, 17 deletions(-)
drivers/nvme/host/fabrics.c +0 -14
index b86b6379ef0c..0a5a4c4684bc 100644
@@ -503,7 +503,6 @@ static const match_table_t opt_tokens = {
 	{ NVMF_OPT_NQN,			"nqn=%s"		},
 	{ NVMF_OPT_QUEUE_SIZE,		"queue_size=%d"		},
 	{ NVMF_OPT_NR_IO_QUEUES,	"nr_io_queues=%d"	},
-	{ NVMF_OPT_TL_RETRY_COUNT,	"tl_retry_count=%d"	},
 	{ NVMF_OPT_RECONNECT_DELAY,	"reconnect_delay=%d"	},
 	{ NVMF_OPT_KATO,		"keep_alive_tmo=%d"	},
 	{ NVMF_OPT_HOSTNQN,		"hostnqn=%s"		},
@@ -521,7 +520,6 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 	/* Set defaults */
 	opts->queue_size = NVMF_DEF_QUEUE_SIZE;
 	opts->nr_io_queues = num_online_cpus();
-	opts->tl_retry_count = 2;
 	opts->reconnect_delay = NVMF_DEF_RECONNECT_DELAY;
 
 	options = o = kstrdup(buf, GFP_KERNEL);
@@ -605,18 +603,6 @@ static int nvmf_parse_options(struct nvmf_ctrl_options *opts,
 			opts->nr_io_queues = min_t(unsigned int,
 					num_online_cpus(), token);
 			break;
-		case NVMF_OPT_TL_RETRY_COUNT:
-			if (match_int(args, &token)) {
-				ret = -EINVAL;
-				goto out;
-			}
-			if (token < 0) {
-				pr_err("Invalid tl_retry_count %d\n", token);
-				ret = -EINVAL;
-				goto out;
-			}
-			opts->tl_retry_count = token;
-			break;
 		case NVMF_OPT_KATO:
 			if (match_int(args, &token)) {
 				ret = -EINVAL;
drivers/nvme/host/fabrics.h +0 -3
index b54067404963..89df52c8be97 100644
@@ -69,8 +69,6 @@ enum {
  * @trsvcid:	network port used for host-controller communication.
  * @queue_size: Number of IO queue elements.
  * @nr_io_queues: Number of controller IO queues that will be established.
- * @tl_retry_count: Number of transport layer retries for a fabric queue before
- *		     kicking upper layer(s) error recovery.
  * @reconnect_delay: Time between two consecutive reconnect attempts.
  * @discovery_nqn: indicates if the subsysnqn is the well-known discovery NQN.
  * @kato:	Keep-alive timeout.
@@ -84,7 +82,6 @@ struct nvmf_ctrl_options {
 	char			*trsvcid;
 	size_t			queue_size;
 	unsigned int		nr_io_queues;
-	unsigned short		tl_retry_count;
 	unsigned int		reconnect_delay;
 	bool			discovery_nqn;
 	unsigned int		kato;
Signature · 1 line
1.9.1
#3 lore raw

cover Get rid of transport layer retry count config parameter

In reply to Sagi Grimberg
Quoted 6 lines · Sagi Grimberg

This parameter was added in order to support a proper timeout for error recovery before the spec defined a periodic keep-alive.

Now that we have periodic keep-alive, we don't need a user configurable transport layer retry count, the keep-alive timeout is sufficient, transports can retry for as long as they see fit.

Isn't there some IB protocol level rationale for a low retry count in various fabric setups?

#4 lore raw

cover Get rid of transport layer retry count config parameter

Quoted 10 lines
Quoted 7 lines

This parameter was added in order to support a proper timeout for error recovery before the spec defined a periodic keep-alive.

Now that we have periodic keep-alive, we don't need a user configurable transport layer retry count, the keep-alive timeout is sufficient, transports can retry for as long as they see fit.

Isn't there some IB protocol level rationale for a low retry count in various fabric setups?

None that I know of... The QP retry count determines the time it would take to fail a send/read/write.. The retry_count value is multiplied with the packet timeout (which is a result of an IB specific computation - managed by the CM).

It's useful when one needs to limit the time until a send fails in order to kick error recovery (useful for srp which doesn't implement periodic keep-alive), but since nvme does, I don't see the reason why RDMA or any other transport should expose this configuration as the keep-alive timeout exists for that.

#5 lore raw

cover Get rid of transport layer retry count config parameter

In reply to Christoph Hellwig
Quoted 10 lines · Christoph Hellwig
Quoted 6 lines · Sagi Grimberg

This parameter was added in order to support a proper timeout for error recovery before the spec defined a periodic keep-alive.

Now that we have periodic keep-alive, we don't need a user configurable transport layer retry count, the keep-alive timeout is sufficient, transports can retry for as long as they see fit.

Isn't there some IB protocol level rationale for a low retry count in various fabric setups?

IIRC the retry count is part of what drives the APM switch over, so APM configuration should use a lower value.

Jason

author maintainer
#6 lore raw

cover Get rid of transport layer retry count config parameter

Quoted 8 lines
Quoted 8 lines
Quoted 7 lines

This parameter was added in order to support a proper timeout for error recovery before the spec defined a periodic keep-alive.

Now that we have periodic keep-alive, we don't need a user configurable transport layer retry count, the keep-alive timeout is sufficient, transports can retry for as long as they see fit.

Quoted 5 lines
Quoted 2 lines

Isn't there some IB protocol level rationale for a low retry count in various fabric setups?

IIRC the retry count is part of what drives the APM switch over, so APM configuration should use a lower value.

Completely agree Jason. Lowering the retry_count is very useful for APM (Automatic Path Migration).

#7 lore raw

cover Get rid of transport layer retry count config parameter

Quoted 2 lines · Sagi Grimberg

Completely agree Jason. Lowering the retry_count is very useful for APM (Automatic Path Migration).

Does this mean you're retracting the patches?

author maintainer
#8 lore raw

cover Get rid of transport layer retry count config parameter

Quoted 5 lines
Quoted 3 lines

Completely agree Jason. Lowering the retry_count is very useful for APM (Automatic Path Migration).

Does this mean you're retracting the patches?

I'm not, were not using APM anywhere in nvme-rdma. multipathing is done at a higher level than the transport.

Do you see a reason to keep this? I'm not too enthusiast with leaving configs that aren't absolutely needed. As mentioned this config was added to add a fast-fail functionality before we defined the periodic keep-alive...

author maintainer
#9 lore raw

cover Get rid of transport layer retry count config parameter

In reply to Christoph Hellwig
Quoted 5 lines
Quoted 3 lines

Completely agree Jason. Lowering the retry_count is very useful for APM (Automatic Path Migration).

Does this mean you're retracting the patches?

No, we never use APM in nvme-rdma, so I don't see a good reason why we should keep it around....

Can I get a review on this btw?

#10 lore raw

cover Get rid of transport layer retry count config parameter

Quoted 1 line · Sagi Grimberg

Can I get a review on this btw?

Jens already merged that patch after I pinged him last week.

#11 lore raw

cover Get rid of transport layer retry count config parameter

Quoted 4 lines · Christoph Hellwig
Quoted 1 line · Sagi Grimberg

Can I get a review on this btw?

Jens already merged that patch after I pinged him last week.

s/patch/series/. Time to havea coffee..

author maintainer
#12 lore raw

cover Get rid of transport layer retry count config parameter

Quoted 4 lines
Quoted 2 lines

Jens already merged that patch after I pinged him last week.

s/patch/series/. Time to havea coffee..

Thanks Christoph and Jens :)

#13 lore raw

cover Get rid of transport layer retry count config parameter

In reply to Christoph Hellwig
Quoted 10 lines · Christoph Hellwig
Quoted 6 lines · Sagi Grimberg

This parameter was added in order to support a proper timeout for error recovery before the spec defined a periodic keep-alive.

Now that we have periodic keep-alive, we don't need a user configurable transport layer retry count, the keep-alive timeout is sufficient, transports can retry for as long as they see fit.

Isn't there some IB protocol level rationale for a low retry count in various fabric setups?

The IB spec defines an end-to-end credit mechanism for RC connections. So if the transport layer is reliable (InfiniBand, RoCE with DCB enabled) setting the retry count high enough is only needed to avoid connection shutdown due to brief cable disconnect/reconnect events.

Bart.

author maintainer
#14 lore raw

cover Get rid of transport layer retry count config parameter

Quoted 5 lines

The IB spec defines an end-to-end credit mechanism for RC connections. So if the transport layer is reliable (InfiniBand, RoCE with DCB enabled) setting the retry count high enough is only needed to avoid connection shutdown due to brief cable disconnect/reconnect events.

Right, this is why I think the driver can use whatever it sees fit (we have a keep-alive mechanism for fast-fail functionality).