All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v2 0/4] traffic based keep alive support (TP 4024)
@ 2018-10-30 17:43 Sagi Grimberg
  2018-10-30 17:43 ` [PATCH v2 1/4] nvme: introduce ctrl attributes enumeration Sagi Grimberg
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Sagi Grimberg @ 2018-10-30 17:43 UTC (permalink / raw)


Traffic Based Keep Alive TP (4024) was ratified recently. The technical
proposal introduces a mode where keep alive commands may be omitted
based on the presence of admin or io traffic during the keep alive timeout
period. The rational is that normal command execution is sufficient
indication of host, controller and transport health so the keep alive command
is redundant.

Moreover, There is no way to guarantee in the various transports that a
keep alive command will not be starved by heavy traffic, causing the
keep alive timeout to expire. There were a number bug reports on
linux nvme community on this exact issue.

The implementation simply a controller-wide indicator that is cleared
upon reset of the keep alive timer and set when a command (target) or
completion (host) is processed. When the keep alive timer expires, we
check if any command/completion were processed, we restart the keep
alive timer even if a keep alive command was not executed.

Changes from v1:
- omit comp_seen update for controllers that don't support keep alive

Sagi Grimberg (4):
  nvme: introduce ctrl attributes enumeration
  nvmet: support for traffic based keep-alive
  nvme-core: cache controller attributes
  nvme-core: support traffic based keep-alive based on controller
    support

 drivers/nvme/host/core.c        | 14 ++++++++++++++
 drivers/nvme/host/nvme.h        |  2 ++
 drivers/nvme/target/admin-cmd.c |  3 ++-
 drivers/nvme/target/core.c      | 12 ++++++++++++
 drivers/nvme/target/nvmet.h     |  2 ++
 include/linux/nvme.h            |  5 +++++
 6 files changed, 37 insertions(+), 1 deletion(-)

-- 
2.17.1

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

* [PATCH v2 1/4] nvme: introduce ctrl attributes enumeration
  2018-10-30 17:43 [PATCH v2 0/4] traffic based keep alive support (TP 4024) Sagi Grimberg
@ 2018-10-30 17:43 ` Sagi Grimberg
  2018-10-30 20:32   ` Chaitanya Kulkarni
  2018-11-02  8:00   ` Hannes Reinecke
  2018-10-30 17:43 ` [PATCH v2 2/4] nvmet: support for traffic based keep-alive Sagi Grimberg
                   ` (2 subsequent siblings)
  3 siblings, 2 replies; 11+ messages in thread
From: Sagi Grimberg @ 2018-10-30 17:43 UTC (permalink / raw)


We are growing more controller attributes, so use
a proper enumeration for it. For now just add the
128-bit hostid which we support.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/admin-cmd.c | 2 +-
 include/linux/nvme.h            | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 1179f6314323..30778ffc46f5 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -304,7 +304,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 
 	/* XXX: figure out what to do about RTD3R/RTD3 */
 	id->oaes = cpu_to_le32(NVMET_AEN_CFG_OPTIONAL);
-	id->ctratt = cpu_to_le32(1 << 0);
+	id->ctratt = cpu_to_le32(NVME_CTRL_ATTR_HID_128_BIT);
 
 	id->oacs = 0;
 
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 818dbe9331be..753c83a5c01f 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -198,6 +198,10 @@ enum {
 	NVME_PS_FLAGS_NON_OP_STATE	= 1 << 1,
 };
 
+enum nvme_ctrl_attr {
+	NVME_CTRL_ATTR_HID_128_BIT	= (1 << 0),
+};
+
 struct nvme_id_ctrl {
 	__le16			vid;
 	__le16			ssvid;
-- 
2.17.1

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

* [PATCH v2 2/4] nvmet: support for traffic based keep-alive
  2018-10-30 17:43 [PATCH v2 0/4] traffic based keep alive support (TP 4024) Sagi Grimberg
  2018-10-30 17:43 ` [PATCH v2 1/4] nvme: introduce ctrl attributes enumeration Sagi Grimberg
@ 2018-10-30 17:43 ` Sagi Grimberg
  2018-11-02  8:04   ` Hannes Reinecke
  2018-10-30 17:43 ` [PATCH v2 3/4] nvme-core: cache controller attributes Sagi Grimberg
  2018-10-30 17:43 ` [PATCH v2 4/4] nvme-core: support traffic based keep-alive based on controller support Sagi Grimberg
  3 siblings, 1 reply; 11+ messages in thread
From: Sagi Grimberg @ 2018-10-30 17:43 UTC (permalink / raw)


A controller that supports traffic based keep-alive can restart
its keep alive timer even if keep-alive was not issued in the
kato period but other admin or io commands was. For each command
set ctrl->cmd_seen to true, and when keep-alive timer expires,
if any commands were seen, resched ka_work instead of escalating
to a fatal error.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/target/admin-cmd.c |  3 ++-
 drivers/nvme/target/core.c      | 12 ++++++++++++
 drivers/nvme/target/nvmet.h     |  2 ++
 include/linux/nvme.h            |  1 +
 4 files changed, 17 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
index 30778ffc46f5..c9c6d25a3ec2 100644
--- a/drivers/nvme/target/admin-cmd.c
+++ b/drivers/nvme/target/admin-cmd.c
@@ -304,7 +304,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
 
 	/* XXX: figure out what to do about RTD3R/RTD3 */
 	id->oaes = cpu_to_le32(NVMET_AEN_CFG_OPTIONAL);
-	id->ctratt = cpu_to_le32(NVME_CTRL_ATTR_HID_128_BIT);
+	id->ctratt = cpu_to_le32(NVME_CTRL_ATTR_HID_128_BIT |
+		NVME_CTRL_ATTR_TBKAS);
 
 	id->oacs = 0;
 
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 0acdff9e6842..ebe951a05712 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -298,6 +298,15 @@ static void nvmet_keep_alive_timer(struct work_struct *work)
 {
 	struct nvmet_ctrl *ctrl = container_of(to_delayed_work(work),
 			struct nvmet_ctrl, ka_work);
+	bool cmd_seen = ctrl->cmd_seen;
+
+	ctrl->cmd_seen = false;
+	if (cmd_seen) {
+		pr_debug("ctrl %d traffic-based keep-alive timer expired, reschedule\n",
+			ctrl->cntlid);
+		schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
+		return;
+	}
 
 	pr_err("ctrl %d keep-alive timer (%d seconds) expired!\n",
 		ctrl->cntlid, ctrl->kato);
@@ -700,6 +709,9 @@ bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
 		goto fail;
 	}
 
+	if (sq->ctrl)
+		sq->ctrl->cmd_seen = true;
+
 	return true;
 
 fail:
diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index 08f7b57a1203..c5255e743f63 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -154,6 +154,8 @@ struct nvmet_ctrl {
 	struct nvmet_cq		**cqs;
 	struct nvmet_sq		**sqs;
 
+	bool			cmd_seen;
+
 	struct mutex		lock;
 	u64			cap;
 	u32			cc;
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index 753c83a5c01f..429c4cf90899 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -200,6 +200,7 @@ enum {
 
 enum nvme_ctrl_attr {
 	NVME_CTRL_ATTR_HID_128_BIT	= (1 << 0),
+	NVME_CTRL_ATTR_TBKAS		= (1 << 6),
 };
 
 struct nvme_id_ctrl {
-- 
2.17.1

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

* [PATCH v2 3/4] nvme-core: cache controller attributes
  2018-10-30 17:43 [PATCH v2 0/4] traffic based keep alive support (TP 4024) Sagi Grimberg
  2018-10-30 17:43 ` [PATCH v2 1/4] nvme: introduce ctrl attributes enumeration Sagi Grimberg
  2018-10-30 17:43 ` [PATCH v2 2/4] nvmet: support for traffic based keep-alive Sagi Grimberg
@ 2018-10-30 17:43 ` Sagi Grimberg
  2018-10-30 20:27   ` Chaitanya Kulkarni
  2018-11-02  8:05   ` Hannes Reinecke
  2018-10-30 17:43 ` [PATCH v2 4/4] nvme-core: support traffic based keep-alive based on controller support Sagi Grimberg
  3 siblings, 2 replies; 11+ messages in thread
From: Sagi Grimberg @ 2018-10-30 17:43 UTC (permalink / raw)


We get the controller attributes in identify, cache
them as we'll need them for traffic based keep alive
support.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 1 +
 drivers/nvme/host/nvme.h | 1 +
 2 files changed, 2 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 65c42448e904..ef609239c9c5 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -2409,6 +2409,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 	ctrl->sgls = le32_to_cpu(id->sgls);
 	ctrl->kas = le16_to_cpu(id->kas);
 	ctrl->max_namespaces = le32_to_cpu(id->mnan);
+	ctrl->ctratt = le32_to_cpu(id->ctratt);
 
 	if (id->rtd3e) {
 		/* us -> s */
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 9fefba039d1e..4dd7535caf1b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -193,6 +193,7 @@ struct nvme_ctrl {
 	u8 apsta;
 	u32 oaes;
 	u32 aen_result;
+	u32 ctratt;
 	unsigned int shutdown_timeout;
 	unsigned int kato;
 	bool subsystem;
-- 
2.17.1

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

* [PATCH v2 4/4] nvme-core: support traffic based keep-alive based on controller support
  2018-10-30 17:43 [PATCH v2 0/4] traffic based keep alive support (TP 4024) Sagi Grimberg
                   ` (2 preceding siblings ...)
  2018-10-30 17:43 ` [PATCH v2 3/4] nvme-core: cache controller attributes Sagi Grimberg
@ 2018-10-30 17:43 ` Sagi Grimberg
  2018-11-02  8:06   ` Hannes Reinecke
  3 siblings, 1 reply; 11+ messages in thread
From: Sagi Grimberg @ 2018-10-30 17:43 UTC (permalink / raw)


If the controller supports traffic based keep alive, we restart
the keep alive timer if any admin or io commands was completed
during the kato period. This prevents a possible starvation of
keep alive commands in the presence of heavy traffic as in such
case, we already have a health indication from the host perspective.

Only set a comp_seen indicator in case the controller supports keep
alive to minimize the overhead for pci controllers.

Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
---
 drivers/nvme/host/core.c | 13 +++++++++++++
 drivers/nvme/host/nvme.h |  1 +
 2 files changed, 14 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index ef609239c9c5..938478d1e904 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -251,6 +251,9 @@ void nvme_complete_rq(struct request *req)
 
 	trace_nvme_complete_rq(req);
 
+	if (nvme_req(req)->ctrl->kas)
+		nvme_req(req)->ctrl->comp_seen = true;
+
 	if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) {
 		if ((req->cmd_flags & REQ_NVME_MPATH) &&
 		    blk_path_error(status)) {
@@ -841,6 +844,7 @@ static void nvme_keep_alive_end_io(struct request *rq, blk_status_t status)
 		return;
 	}
 
+	ctrl->comp_seen = false;
 	schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
 }
 
@@ -865,6 +869,15 @@ static void nvme_keep_alive_work(struct work_struct *work)
 {
 	struct nvme_ctrl *ctrl = container_of(to_delayed_work(work),
 			struct nvme_ctrl, ka_work);
+	bool comp_seen = ctrl->comp_seen;
+
+	if ((ctrl->ctratt & NVME_CTRL_ATTR_TBKAS) && comp_seen) {
+		dev_dbg(ctrl->device,
+			"traffic based keep-alive timer expired, reschedule\n");
+		ctrl->comp_seen = false;
+		schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
+		return;
+	}
 
 	if (nvme_keep_alive(ctrl)) {
 		/* allocation failure, reset the controller */
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 4dd7535caf1b..6206d026d53b 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -145,6 +145,7 @@ enum nvme_ctrl_state {
 };
 
 struct nvme_ctrl {
+	bool comp_seen;
 	enum nvme_ctrl_state state;
 	bool identified;
 	spinlock_t lock;
-- 
2.17.1

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

* [PATCH v2 3/4] nvme-core: cache controller attributes
  2018-10-30 17:43 ` [PATCH v2 3/4] nvme-core: cache controller attributes Sagi Grimberg
@ 2018-10-30 20:27   ` Chaitanya Kulkarni
  2018-11-02  8:05   ` Hannes Reinecke
  1 sibling, 0 replies; 11+ messages in thread
From: Chaitanya Kulkarni @ 2018-10-30 20:27 UTC (permalink / raw)


Can we have a commit log line length till 72 ? can be done at the time of applying patch.

Looks good.
Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>.

?On 10/30/18, 10:43 AM, "Linux-nvme on behalf of Sagi Grimberg" <linux-nvme-bounces@lists.infradead.org on behalf of sagi@grimberg.me> wrote:

    We get the controller attributes in identify, cache
    them as we'll need them for traffic based keep alive
    support.
    
    Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
    ---
     drivers/nvme/host/core.c | 1 +
     drivers/nvme/host/nvme.h | 1 +
     2 files changed, 2 insertions(+)
    
    diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
    index 65c42448e904..ef609239c9c5 100644
    --- a/drivers/nvme/host/core.c
    +++ b/drivers/nvme/host/core.c
    @@ -2409,6 +2409,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
     	ctrl->sgls = le32_to_cpu(id->sgls);
     	ctrl->kas = le16_to_cpu(id->kas);
     	ctrl->max_namespaces = le32_to_cpu(id->mnan);
    +	ctrl->ctratt = le32_to_cpu(id->ctratt);
     
     	if (id->rtd3e) {
     		/* us -> s */
    diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
    index 9fefba039d1e..4dd7535caf1b 100644
    --- a/drivers/nvme/host/nvme.h
    +++ b/drivers/nvme/host/nvme.h
    @@ -193,6 +193,7 @@ struct nvme_ctrl {
     	u8 apsta;
     	u32 oaes;
     	u32 aen_result;
    +	u32 ctratt;
     	unsigned int shutdown_timeout;
     	unsigned int kato;
     	bool subsystem;
    -- 
    2.17.1
    
    
    _______________________________________________
    Linux-nvme mailing list
    Linux-nvme at lists.infradead.org
    http://lists.infradead.org/mailman/listinfo/linux-nvme
    

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

* [PATCH v2 1/4] nvme: introduce ctrl attributes enumeration
  2018-10-30 17:43 ` [PATCH v2 1/4] nvme: introduce ctrl attributes enumeration Sagi Grimberg
@ 2018-10-30 20:32   ` Chaitanya Kulkarni
  2018-11-02  8:00   ` Hannes Reinecke
  1 sibling, 0 replies; 11+ messages in thread
From: Chaitanya Kulkarni @ 2018-10-30 20:32 UTC (permalink / raw)


Looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni at wdc.com>.

?On 10/30/18, 10:44 AM, "Linux-nvme on behalf of Sagi Grimberg" <linux-nvme-bounces@lists.infradead.org on behalf of sagi@grimberg.me> wrote:

    We are growing more controller attributes, so use
    a proper enumeration for it. For now just add the
    128-bit hostid which we support.
    
    Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
    ---
     drivers/nvme/target/admin-cmd.c | 2 +-
     include/linux/nvme.h            | 4 ++++
     2 files changed, 5 insertions(+), 1 deletion(-)
    
    diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
    index 1179f6314323..30778ffc46f5 100644
    --- a/drivers/nvme/target/admin-cmd.c
    +++ b/drivers/nvme/target/admin-cmd.c
    @@ -304,7 +304,7 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
     
     	/* XXX: figure out what to do about RTD3R/RTD3 */
     	id->oaes = cpu_to_le32(NVMET_AEN_CFG_OPTIONAL);
    -	id->ctratt = cpu_to_le32(1 << 0);
    +	id->ctratt = cpu_to_le32(NVME_CTRL_ATTR_HID_128_BIT);
     
     	id->oacs = 0;
     
    diff --git a/include/linux/nvme.h b/include/linux/nvme.h
    index 818dbe9331be..753c83a5c01f 100644
    --- a/include/linux/nvme.h
    +++ b/include/linux/nvme.h
    @@ -198,6 +198,10 @@ enum {
     	NVME_PS_FLAGS_NON_OP_STATE	= 1 << 1,
     };
     
    +enum nvme_ctrl_attr {
    +	NVME_CTRL_ATTR_HID_128_BIT	= (1 << 0),
    +};
    +
     struct nvme_id_ctrl {
     	__le16			vid;
     	__le16			ssvid;
    -- 
    2.17.1
    
    
    _______________________________________________
    Linux-nvme mailing list
    Linux-nvme at lists.infradead.org
    http://lists.infradead.org/mailman/listinfo/linux-nvme
    

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

* [PATCH v2 1/4] nvme: introduce ctrl attributes enumeration
  2018-10-30 17:43 ` [PATCH v2 1/4] nvme: introduce ctrl attributes enumeration Sagi Grimberg
  2018-10-30 20:32   ` Chaitanya Kulkarni
@ 2018-11-02  8:00   ` Hannes Reinecke
  1 sibling, 0 replies; 11+ messages in thread
From: Hannes Reinecke @ 2018-11-02  8:00 UTC (permalink / raw)


On 10/30/18 6:43 PM, Sagi Grimberg wrote:
> We are growing more controller attributes, so use
> a proper enumeration for it. For now just add the
> 128-bit hostid which we support.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/target/admin-cmd.c | 2 +-
>   include/linux/nvme.h            | 4 ++++
>   2 files changed, 5 insertions(+), 1 deletion(-)
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

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

* [PATCH v2 2/4] nvmet: support for traffic based keep-alive
  2018-10-30 17:43 ` [PATCH v2 2/4] nvmet: support for traffic based keep-alive Sagi Grimberg
@ 2018-11-02  8:04   ` Hannes Reinecke
  0 siblings, 0 replies; 11+ messages in thread
From: Hannes Reinecke @ 2018-11-02  8:04 UTC (permalink / raw)


On 10/30/18 6:43 PM, Sagi Grimberg wrote:
> A controller that supports traffic based keep-alive can restart
> its keep alive timer even if keep-alive was not issued in the
> kato period but other admin or io commands was. For each command
> set ctrl->cmd_seen to true, and when keep-alive timer expires,
> if any commands were seen, resched ka_work instead of escalating
> to a fatal error.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/target/admin-cmd.c |  3 ++-
>   drivers/nvme/target/core.c      | 12 ++++++++++++
>   drivers/nvme/target/nvmet.h     |  2 ++
>   include/linux/nvme.h            |  1 +
>   4 files changed, 17 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/nvme/target/admin-cmd.c b/drivers/nvme/target/admin-cmd.c
> index 30778ffc46f5..c9c6d25a3ec2 100644
> --- a/drivers/nvme/target/admin-cmd.c
> +++ b/drivers/nvme/target/admin-cmd.c
> @@ -304,7 +304,8 @@ static void nvmet_execute_identify_ctrl(struct nvmet_req *req)
>   
>   	/* XXX: figure out what to do about RTD3R/RTD3 */
>   	id->oaes = cpu_to_le32(NVMET_AEN_CFG_OPTIONAL);
> -	id->ctratt = cpu_to_le32(NVME_CTRL_ATTR_HID_128_BIT);
> +	id->ctratt = cpu_to_le32(NVME_CTRL_ATTR_HID_128_BIT |
> +		NVME_CTRL_ATTR_TBKAS);
>   
>   	id->oacs = 0;
>   
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 0acdff9e6842..ebe951a05712 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -298,6 +298,15 @@ static void nvmet_keep_alive_timer(struct work_struct *work)
>   {
>   	struct nvmet_ctrl *ctrl = container_of(to_delayed_work(work),
>   			struct nvmet_ctrl, ka_work);
> +	bool cmd_seen = ctrl->cmd_seen;
> +
> +	ctrl->cmd_seen = false;
> +	if (cmd_seen) {
> +		pr_debug("ctrl %d traffic-based keep-alive timer expired, reschedule\n",
> +			ctrl->cntlid);
> +		schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
> +		return;
> +	}
>   
>   	pr_err("ctrl %d keep-alive timer (%d seconds) expired!\n",
>   		ctrl->cntlid, ctrl->kato);

Please rephrase that. For TBKA 'expired' means that everything is 
working, whereas for all other cases 'expired' signals an error.

Maybe just 'reschedule traffic-based keep-alive timer' ?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

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

* [PATCH v2 3/4] nvme-core: cache controller attributes
  2018-10-30 17:43 ` [PATCH v2 3/4] nvme-core: cache controller attributes Sagi Grimberg
  2018-10-30 20:27   ` Chaitanya Kulkarni
@ 2018-11-02  8:05   ` Hannes Reinecke
  1 sibling, 0 replies; 11+ messages in thread
From: Hannes Reinecke @ 2018-11-02  8:05 UTC (permalink / raw)


On 10/30/18 6:43 PM, Sagi Grimberg wrote:
> We get the controller attributes in identify, cache
> them as we'll need them for traffic based keep alive
> support.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/core.c | 1 +
>   drivers/nvme/host/nvme.h | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 65c42448e904..ef609239c9c5 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -2409,6 +2409,7 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
>   	ctrl->sgls = le32_to_cpu(id->sgls);
>   	ctrl->kas = le16_to_cpu(id->kas);
>   	ctrl->max_namespaces = le32_to_cpu(id->mnan);
> +	ctrl->ctratt = le32_to_cpu(id->ctratt);
>   
>   	if (id->rtd3e) {
>   		/* us -> s */
> diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
> index 9fefba039d1e..4dd7535caf1b 100644
> --- a/drivers/nvme/host/nvme.h
> +++ b/drivers/nvme/host/nvme.h
> @@ -193,6 +193,7 @@ struct nvme_ctrl {
>   	u8 apsta;
>   	u32 oaes;
>   	u32 aen_result;
> +	u32 ctratt;
>   	unsigned int shutdown_timeout;
>   	unsigned int kato;
>   	bool subsystem;
> 
Reviewed-by: Hannes Reinecke <hare at suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

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

* [PATCH v2 4/4] nvme-core: support traffic based keep-alive based on controller support
  2018-10-30 17:43 ` [PATCH v2 4/4] nvme-core: support traffic based keep-alive based on controller support Sagi Grimberg
@ 2018-11-02  8:06   ` Hannes Reinecke
  0 siblings, 0 replies; 11+ messages in thread
From: Hannes Reinecke @ 2018-11-02  8:06 UTC (permalink / raw)


On 10/30/18 6:43 PM, Sagi Grimberg wrote:
> If the controller supports traffic based keep alive, we restart
> the keep alive timer if any admin or io commands was completed
> during the kato period. This prevents a possible starvation of
> keep alive commands in the presence of heavy traffic as in such
> case, we already have a health indication from the host perspective.
> 
> Only set a comp_seen indicator in case the controller supports keep
> alive to minimize the overhead for pci controllers.
> 
> Signed-off-by: Sagi Grimberg <sagi at grimberg.me>
> ---
>   drivers/nvme/host/core.c | 13 +++++++++++++
>   drivers/nvme/host/nvme.h |  1 +
>   2 files changed, 14 insertions(+)
> 
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index ef609239c9c5..938478d1e904 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -251,6 +251,9 @@ void nvme_complete_rq(struct request *req)
>   
>   	trace_nvme_complete_rq(req);
>   
> +	if (nvme_req(req)->ctrl->kas)
> +		nvme_req(req)->ctrl->comp_seen = true;
> +
>   	if (unlikely(status != BLK_STS_OK && nvme_req_needs_retry(req))) {
>   		if ((req->cmd_flags & REQ_NVME_MPATH) &&
>   		    blk_path_error(status)) {
> @@ -841,6 +844,7 @@ static void nvme_keep_alive_end_io(struct request *rq, blk_status_t status)
>   		return;
>   	}
>   
> +	ctrl->comp_seen = false;
>   	schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
>   }
>   
> @@ -865,6 +869,15 @@ static void nvme_keep_alive_work(struct work_struct *work)
>   {
>   	struct nvme_ctrl *ctrl = container_of(to_delayed_work(work),
>   			struct nvme_ctrl, ka_work);
> +	bool comp_seen = ctrl->comp_seen;
> +
> +	if ((ctrl->ctratt & NVME_CTRL_ATTR_TBKAS) && comp_seen) {
> +		dev_dbg(ctrl->device,
> +			"traffic based keep-alive timer expired, reschedule\n");
> +		ctrl->comp_seen = false;
> +		schedule_delayed_work(&ctrl->ka_work, ctrl->kato * HZ);
> +		return;
> +	}
>   
>   	if (nvme_keep_alive(ctrl)) {
>   		/* allocation failure, reset the controller */
Same here; I'd prefer 'reschedule traffic based keep-alive timer'.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare at suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 N?rnberg
GF: F. Imend?rffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG N?rnberg)

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

end of thread, other threads:[~2018-11-02  8:06 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-10-30 17:43 [PATCH v2 0/4] traffic based keep alive support (TP 4024) Sagi Grimberg
2018-10-30 17:43 ` [PATCH v2 1/4] nvme: introduce ctrl attributes enumeration Sagi Grimberg
2018-10-30 20:32   ` Chaitanya Kulkarni
2018-11-02  8:00   ` Hannes Reinecke
2018-10-30 17:43 ` [PATCH v2 2/4] nvmet: support for traffic based keep-alive Sagi Grimberg
2018-11-02  8:04   ` Hannes Reinecke
2018-10-30 17:43 ` [PATCH v2 3/4] nvme-core: cache controller attributes Sagi Grimberg
2018-10-30 20:27   ` Chaitanya Kulkarni
2018-11-02  8:05   ` Hannes Reinecke
2018-10-30 17:43 ` [PATCH v2 4/4] nvme-core: support traffic based keep-alive based on controller support Sagi Grimberg
2018-11-02  8:06   ` Hannes Reinecke

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.