Get rid of transport layer retry count config parameter
Subsystems (2)
Sagi Grimberg
maintainercover 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
Sagi Grimberg
maintainerPATCH 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.
--- drivers/nvme/host/rdma.c | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-)
drivers/nvme/host/rdma.c +3 -6
@@ -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
Sagi Grimberg
maintainerPATCH 2/2 nvme-fabrics: Remove tl_retry_count
In reply to Sagi GrimbergThe 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.
--- drivers/nvme/host/fabrics.c | 14 -------------- drivers/nvme/host/fabrics.h | 3 --- 2 files changed, 17 deletions(-)
drivers/nvme/host/fabrics.c +0 -14
@@ -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
@@ -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
cover Get rid of transport layer retry count config parameter
In reply to Sagi GrimbergQuoted 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?
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.
cover Get rid of transport layer retry count config parameter
In reply to Christoph HellwigQuoted 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
Sagi Grimberg
maintainercover 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).
cover Get rid of transport layer retry count config parameter
Quoted 2 lines · Sagi Grimberg
Completely agree Jason. Lowering the
retry_countis very useful for APM (Automatic Path Migration).
Does this mean you're retracting the patches?
Sagi Grimberg
maintainercover Get rid of transport layer retry count config parameter
Quoted 5 lines
Quoted 3 lines
Completely agree Jason. Lowering the
retry_countis 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...
Sagi Grimberg
maintainercover Get rid of transport layer retry count config parameter
In reply to Christoph HellwigQuoted 5 lines
Quoted 3 lines
Completely agree Jason. Lowering the
retry_countis 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?
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.
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..
Sagi Grimberg
maintainercover 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 :)
cover Get rid of transport layer retry count config parameter
In reply to Christoph HellwigQuoted 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.
Sagi Grimberg
maintainercover 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).