[PATCH v5 0/2] nvme: add controller id and disk name to tracepoints

public inbox for linux-kernel@vger.kernel.org
 help / color / mirror / Atom feed

* [PATCH v5 0/2] nvme: add controller id and disk name to tracepoints
@ 2018-06-27 12:53 Johannes Thumshirn
  2018-06-27 12:53 ` [PATCH v5 1/2] nvme: cache struct nvme_ctrl reference to struct nvme_request Johannes Thumshirn
  2018-06-27 12:53 ` [PATCH v5 2/2] nvme: trace: add disk name to tracepoints Johannes Thumshirn
  0 siblings, 2 replies; 7+ messages in thread
From: Johannes Thumshirn @ 2018-06-27 12:53 UTC (permalink / raw)
  To: Christoph Hellwig, Sagi Grimberg, Keith Busch, James Smart
  Cc: Linux Kernel Mailinglist, Linux NVMe Mailinglist,
	Johannes Thumshirn

Patch one is a preparation patch from Sagi and caches the nvme_ctrl in
the nvme_request. This is not only useful for the tracepoints but for
further development as well.

The second patch adds the controller IDs and if applicable the disk
name to the tracepoints so we can distinguish between the individual
controllers and disks.

The patches are relative to the nvme-4.19 branch.

Changes to v4:
* Move nvme_ctrl caching into init_request callouts
* I decided against renaming the qid tag in the tracepints but opt-in
  for a hwqid once we have all the infor available

Johannes Thumshirn (1):
  nvme: trace: add disk name to tracepoints

Sagi Grimberg (1):
  nvme: cache struct nvme_ctrl reference to struct nvme_request

 drivers/nvme/host/core.c   |  5 +++--
 drivers/nvme/host/fc.c     |  1 +
 drivers/nvme/host/nvme.h   |  1 +
 drivers/nvme/host/pci.c    |  2 ++
 drivers/nvme/host/rdma.c   |  1 +
 drivers/nvme/host/trace.h  | 39 +++++++++++++++++++++++++--------------
 drivers/nvme/target/loop.c |  1 +
 7 files changed, 34 insertions(+), 16 deletions(-)

-- 
2.16.4


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

* [PATCH v5 1/2] nvme: cache struct nvme_ctrl reference to struct nvme_request
  2018-06-27 12:53 [PATCH v5 0/2] nvme: add controller id and disk name to tracepoints Johannes Thumshirn
@ 2018-06-27 12:53 ` Johannes Thumshirn
  2018-06-27 16:40   ` James Smart
  2018-06-27 12:53 ` [PATCH v5 2/2] nvme: trace: add disk name to tracepoints Johannes Thumshirn
  1 sibling, 1 reply; 7+ messages in thread
From: Johannes Thumshirn @ 2018-06-27 12:53 UTC (permalink / raw)
  To: Christoph Hellwig, Sagi Grimberg, Keith Busch, James Smart
  Cc: Linux Kernel Mailinglist, Linux NVMe Mailinglist

From: Sagi Grimberg <sagi@grimberg.me>

We will need to reference the controller in the setup and completion
time for tracing and future traffic based keep alive support.

Signed-off-by: Sagi Grimberg <sagi@grimberg.me>

---
Changes to v4:
- Move caching from nvme_setup_cmd to .init_request (Keith)
---
 drivers/nvme/host/core.c   | 1 +
 drivers/nvme/host/fc.c     | 1 +
 drivers/nvme/host/nvme.h   | 1 +
 drivers/nvme/host/pci.c    | 2 ++
 drivers/nvme/host/rdma.c   | 1 +
 drivers/nvme/target/loop.c | 1 +
 6 files changed, 7 insertions(+)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e541fe268bcf..99dd62c1076c 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -652,6 +652,7 @@ blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req,
 	}
 
 	cmd->common.command_id = req->tag;
+
 	if (ns)
 		trace_nvme_setup_nvm_cmd(req->q->id, cmd);
 	else
diff --git a/drivers/nvme/host/fc.c b/drivers/nvme/host/fc.c
index 41d45a1b5c62..9cc33752539a 100644
--- a/drivers/nvme/host/fc.c
+++ b/drivers/nvme/host/fc.c
@@ -1737,6 +1737,7 @@ nvme_fc_init_request(struct blk_mq_tag_set *set, struct request *rq,
 	int queue_idx = (set == &ctrl->tag_set) ? hctx_idx + 1 : 0;
 	struct nvme_fc_queue *queue = &ctrl->queues[queue_idx];
 
+	nvme_req(rq)->ctrl = &ctrl->ctrl;
 	return __nvme_fc_init_request(ctrl, queue, op, rq, queue->rqcnt++);
 }
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 0c4a33df3b2f..f2249387b60d 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -102,6 +102,7 @@ struct nvme_request {
 	u8			retries;
 	u8			flags;
 	u16			status;
+	struct nvme_ctrl	*ctrl;
 };
 
 /*
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index ba943f211687..8dcae11bbf3a 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -418,6 +418,8 @@ static int nvme_init_request(struct blk_mq_tag_set *set, struct request *req,
 
 	BUG_ON(!nvmeq);
 	iod->nvmeq = nvmeq;
+
+	nvme_req(req)->ctrl = &dev->ctrl;
 	return 0;
 }
 
diff --git a/drivers/nvme/host/rdma.c b/drivers/nvme/host/rdma.c
index 9f5fc7b7fd7f..36344710050b 100644
--- a/drivers/nvme/host/rdma.c
+++ b/drivers/nvme/host/rdma.c
@@ -286,6 +286,7 @@ static int nvme_rdma_init_request(struct blk_mq_tag_set *set,
 	struct ib_device *ibdev = dev->dev;
 	int ret;
 
+	nvme_req(rq)->ctrl = &ctrl->ctrl;
 	ret = nvme_rdma_alloc_qe(ibdev, &req->sqe, sizeof(struct nvme_command),
 			DMA_TO_DEVICE);
 	if (ret)
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index d8d91f04bd7e..af7fbf4132b0 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -227,6 +227,7 @@ static int nvme_loop_init_request(struct blk_mq_tag_set *set,
 {
 	struct nvme_loop_ctrl *ctrl = set->driver_data;
 
+	nvme_req(req)->ctrl = &ctrl->ctrl;
 	return nvme_loop_init_iod(ctrl, blk_mq_rq_to_pdu(req),
 			(set == &ctrl->tag_set) ? hctx_idx + 1 : 0);
 }
-- 
2.16.4


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

* Re: [PATCH v5 1/2] nvme: cache struct nvme_ctrl reference to struct nvme_request
  2018-06-27 12:53 ` [PATCH v5 1/2] nvme: cache struct nvme_ctrl reference to struct nvme_request Johannes Thumshirn
@ 2018-06-27 16:40   ` James Smart
  0 siblings, 0 replies; 7+ messages in thread
From: James Smart @ 2018-06-27 16:40 UTC (permalink / raw)
  To: Johannes Thumshirn, Christoph Hellwig, Sagi Grimberg, Keith Busch
  Cc: Linux Kernel Mailinglist, Linux NVMe Mailinglist



On 6/27/2018 5:53 AM, Johannes Thumshirn wrote:
> From: Sagi Grimberg <sagi@grimberg.me>
>
> We will need to reference the controller in the setup and completion
> time for tracing and future traffic based keep alive support.
>
> Signed-off-by: Sagi Grimberg <sagi@grimberg.me>
>
> ---
> Changes to v4:
> - Move caching from nvme_setup_cmd to .init_request (Keith)
> ---
>   drivers/nvme/host/core.c   | 1 +
>   drivers/nvme/host/fc.c     | 1 +
>   drivers/nvme/host/nvme.h   | 1 +
>   drivers/nvme/host/pci.c    | 2 ++
>   drivers/nvme/host/rdma.c   | 1 +
>   drivers/nvme/target/loop.c | 1 +
>   6 files changed, 7 insertions(+)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index e541fe268bcf..99dd62c1076c 100644
>

good for the fc side...

Reviewed-by:  James Smart  <james.smart@broadcom.com>



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

* [PATCH v5 2/2] nvme: trace: add disk name to tracepoints
  2018-06-27 12:53 [PATCH v5 0/2] nvme: add controller id and disk name to tracepoints Johannes Thumshirn
  2018-06-27 12:53 ` [PATCH v5 1/2] nvme: cache struct nvme_ctrl reference to struct nvme_request Johannes Thumshirn
@ 2018-06-27 12:53 ` Johannes Thumshirn
  2018-06-27 16:37   ` Keith Busch
  1 sibling, 1 reply; 7+ messages in thread
From: Johannes Thumshirn @ 2018-06-27 12:53 UTC (permalink / raw)
  To: Christoph Hellwig, Sagi Grimberg, Keith Busch, James Smart
  Cc: Linux Kernel Mailinglist, Linux NVMe Mailinglist,
	Johannes Thumshirn

Add disk name to tracepoints so we can better distinguish between
individual disks in the trace output and admin commands which
are represented without a disk name.

Signed-off-by: Johannes Thumshirn <jthumshirn@suse.de>
---
 drivers/nvme/host/core.c  |  4 ++--
 drivers/nvme/host/trace.h | 39 +++++++++++++++++++++++++--------------
 2 files changed, 27 insertions(+), 16 deletions(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 99dd62c1076c..ada04870e613 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -654,9 +654,9 @@ blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req,
 	cmd->common.command_id = req->tag;
 
 	if (ns)
-		trace_nvme_setup_nvm_cmd(req->q->id, cmd);
+		trace_nvme_setup_nvm_cmd(req, cmd, ns->disk->disk_name);
 	else
-		trace_nvme_setup_admin_cmd(cmd);
+		trace_nvme_setup_admin_cmd(req, cmd);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(nvme_setup_cmd);
diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
index 01390f0e1671..371a816cd225 100644
--- a/drivers/nvme/host/trace.h
+++ b/drivers/nvme/host/trace.h
@@ -76,9 +76,10 @@ const char *nvme_trace_parse_nvm_cmd(struct trace_seq *p, u8 opcode,
 	nvme_trace_parse_nvm_cmd(p, opcode, cdw10)
 
 TRACE_EVENT(nvme_setup_admin_cmd,
-	    TP_PROTO(struct nvme_command *cmd),
-	    TP_ARGS(cmd),
+	    TP_PROTO(struct request *req, struct nvme_command *cmd),
+	    TP_ARGS(req, cmd),
 	    TP_STRUCT__entry(
+		    __field(int, ctrl_id)
 		    __field(u8, opcode)
 		    __field(u8, flags)
 		    __field(u16, cid)
@@ -86,6 +87,7 @@ TRACE_EVENT(nvme_setup_admin_cmd,
 		    __array(u8, cdw10, 24)
 	    ),
 	    TP_fast_assign(
+		    __entry->ctrl_id = nvme_req(req)->ctrl->cntlid;
 		    __entry->opcode = cmd->common.opcode;
 		    __entry->flags = cmd->common.flags;
 		    __entry->cid = cmd->common.command_id;
@@ -93,17 +95,21 @@ TRACE_EVENT(nvme_setup_admin_cmd,
 		    memcpy(__entry->cdw10, cmd->common.cdw10,
 			   sizeof(__entry->cdw10));
 	    ),
-	    TP_printk(" cmdid=%u, flags=0x%x, meta=0x%llx, cmd=(%s %s)",
-		      __entry->cid, __entry->flags, __entry->metadata,
+	    TP_printk("nvme%d: cmdid=%u, flags=0x%x, meta=0x%llx, cmd=(%s %s)",
+		      __entry->ctrl_id, __entry->cid, __entry->flags,
+		      __entry->metadata,
 		      show_admin_opcode_name(__entry->opcode),
 		      __parse_nvme_admin_cmd(__entry->opcode, __entry->cdw10))
 );
 
 
 TRACE_EVENT(nvme_setup_nvm_cmd,
-	    TP_PROTO(int qid, struct nvme_command *cmd),
-	    TP_ARGS(qid, cmd),
+	    TP_PROTO(struct request *req, struct nvme_command *cmd,
+		     char *disk_name),
+	    TP_ARGS(req, cmd, disk_name),
 	    TP_STRUCT__entry(
+		    __string(name, disk_name)
+		    __field(int, ctrl_id)
 		    __field(int, qid)
 		    __field(u8, opcode)
 		    __field(u8, flags)
@@ -113,7 +119,9 @@ TRACE_EVENT(nvme_setup_nvm_cmd,
 		    __array(u8, cdw10, 24)
 	    ),
 	    TP_fast_assign(
-		    __entry->qid = qid;
+		    __assign_str(name, disk_name);
+		    __entry->ctrl_id = nvme_req(req)->ctrl->cntlid;
+		    __entry->qid = req->q->id;
 		    __entry->opcode = cmd->common.opcode;
 		    __entry->flags = cmd->common.flags;
 		    __entry->cid = cmd->common.command_id;
@@ -122,10 +130,10 @@ TRACE_EVENT(nvme_setup_nvm_cmd,
 		    memcpy(__entry->cdw10, cmd->common.cdw10,
 			   sizeof(__entry->cdw10));
 	    ),
-	    TP_printk("qid=%d, nsid=%u, cmdid=%u, flags=0x%x, meta=0x%llx, cmd=(%s %s)",
-		      __entry->qid, __entry->nsid, __entry->cid,
-		      __entry->flags, __entry->metadata,
-		      show_opcode_name(__entry->opcode),
+	    TP_printk("nvme%d: disk=%s, qid=%d, nsid=%u, cmdid=%u, flags=0x%x, meta=0x%llx, cmd=(%s %s)",
+		      __entry->ctrl_id, __get_str(name), __entry->qid,
+		      __entry->nsid, __entry->cid, __entry->flags,
+		      __entry->metadata, show_opcode_name(__entry->opcode),
 		      __parse_nvme_cmd(__entry->opcode, __entry->cdw10))
 );
 
@@ -133,6 +141,7 @@ TRACE_EVENT(nvme_complete_rq,
 	    TP_PROTO(struct request *req),
 	    TP_ARGS(req),
 	    TP_STRUCT__entry(
+		    __field(int, ctrl_id)
 		    __field(int, qid)
 		    __field(int, cid)
 		    __field(u64, result)
@@ -141,6 +150,7 @@ TRACE_EVENT(nvme_complete_rq,
 		    __field(u16, status)
 	    ),
 	    TP_fast_assign(
+		    __entry->ctrl_id = nvme_req(req)->ctrl->cntlid;
 		    __entry->qid = req->q->id;
 		    __entry->cid = req->tag;
 		    __entry->result = le64_to_cpu(nvme_req(req)->result.u64);
@@ -148,9 +158,10 @@ TRACE_EVENT(nvme_complete_rq,
 		    __entry->flags = nvme_req(req)->flags;
 		    __entry->status = nvme_req(req)->status;
 	    ),
-	    TP_printk("qid=%d, cmdid=%u, res=%llu, retries=%u, flags=0x%x, status=%u",
-		      __entry->qid, __entry->cid, __entry->result,
-		      __entry->retries, __entry->flags, __entry->status)
+	    TP_printk("nvme%d: qid=%d, cmdid=%u, res=%llu, retries=%u, flags=0x%x, status=%u",
+		      __entry->ctrl_id, __entry->qid, __entry->cid,
+		      __entry->result,  __entry->retries, __entry->flags,
+		      __entry->status)
 
 );
 
-- 
2.16.4


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

* Re: [PATCH v5 2/2] nvme: trace: add disk name to tracepoints
  2018-06-27 12:53 ` [PATCH v5 2/2] nvme: trace: add disk name to tracepoints Johannes Thumshirn
@ 2018-06-27 16:37   ` Keith Busch
  2018-06-28  7:48     ` Johannes Thumshirn
  0 siblings, 1 reply; 7+ messages in thread
From: Keith Busch @ 2018-06-27 16:37 UTC (permalink / raw)
  To: Johannes Thumshirn
  Cc: Christoph Hellwig, Sagi Grimberg, Keith Busch, James Smart,
	Linux Kernel Mailinglist, Linux NVMe Mailinglist

On Wed, Jun 27, 2018 at 02:53:24PM +0200, Johannes Thumshirn wrote:
>  	    TP_fast_assign(
> -		    __entry->qid = qid;
> +		    __assign_str(name, disk_name);
> +		    __entry->ctrl_id = nvme_req(req)->ctrl->cntlid;
> +		    __entry->qid = req->q->id;
>  		    __entry->opcode = cmd->common.opcode;
>  		    __entry->flags = cmd->common.flags;
>  		    __entry->cid = cmd->common.command_id;
> @@ -122,10 +130,10 @@ TRACE_EVENT(nvme_setup_nvm_cmd,
>  		    memcpy(__entry->cdw10, cmd->common.cdw10,
>  			   sizeof(__entry->cdw10));
>  	    ),
> -	    TP_printk("qid=%d, nsid=%u, cmdid=%u, flags=0x%x, meta=0x%llx, cmd=(%s %s)",
> -		      __entry->qid, __entry->nsid, __entry->cid,
> -		      __entry->flags, __entry->metadata,
> -		      show_opcode_name(__entry->opcode),
> +	    TP_printk("nvme%d: disk=%s, qid=%d, nsid=%u, cmdid=%u, flags=0x%x, meta=0x%llx, cmd=(%s %s)",
> +		      __entry->ctrl_id, __get_str(name), __entry->qid,
> +		      __entry->nsid, __entry->cid, __entry->flags,
> +		      __entry->metadata, show_opcode_name(__entry->opcode),
>  		      __parse_nvme_cmd(__entry->opcode, __entry->cdw10))
>  );

This looks a bit confusing to me. The ctrl->cntlid you're using in
__entry->ctrl_id isn't a unique value across subsystems, and it will
typically be 0 for all controllers that are the only controller in
their subsystem.

It looks like you want the # assigned to /dev/nvme<#>. Do you want to
use ctrl->instance here instead?

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

* Re: [PATCH v5 2/2] nvme: trace: add disk name to tracepoints
  2018-06-27 16:37   ` Keith Busch
@ 2018-06-28  7:48     ` Johannes Thumshirn
  2018-06-28 20:55       ` Keith Busch
  0 siblings, 1 reply; 7+ messages in thread
From: Johannes Thumshirn @ 2018-06-28  7:48 UTC (permalink / raw)
  To: Keith Busch
  Cc: Sagi Grimberg, James Smart, Linux NVMe Mailinglist,
	Linux Kernel Mailinglist, Keith Busch, Christoph Hellwig

On Wed, Jun 27, 2018 at 10:37:06AM -0600, Keith Busch wrote:
> It looks like you want the # assigned to /dev/nvme<#>. Do you want to
> use ctrl->instance here instead?

Right.

As it'll conflict with your other trace patch do you want me to wait
with the resend?

     Johannes
-- 
Johannes Thumshirn                                          Storage
jthumshirn@suse.de                                +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

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

* Re: [PATCH v5 2/2] nvme: trace: add disk name to tracepoints
  2018-06-28  7:48     ` Johannes Thumshirn
@ 2018-06-28 20:55       ` Keith Busch
  0 siblings, 0 replies; 7+ messages in thread
From: Keith Busch @ 2018-06-28 20:55 UTC (permalink / raw)
  To: Johannes Thumshirn
  Cc: Sagi Grimberg, James Smart, Linux NVMe Mailinglist,
	Linux Kernel Mailinglist, Keith Busch, Christoph Hellwig

On Thu, Jun 28, 2018 at 09:48:24AM +0200, Johannes Thumshirn wrote:
> On Wed, Jun 27, 2018 at 10:37:06AM -0600, Keith Busch wrote:
> > It looks like you want the # assigned to /dev/nvme<#>. Do you want to
> > use ctrl->instance here instead?
> 
> Right.
> 
> As it'll conflict with your other trace patch do you want me to wait
> with the resend?

I tried to merge your disk names but it got substantially different
after I combined the two submission trace points into one (I sort of
prefer having only one). I'm about to send it out, and I hope I was able
to preserve your intention, but please have a look and let me know.

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

end of thread, other threads:[~2018-06-28 20:57 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2018-06-27 12:53 [PATCH v5 0/2] nvme: add controller id and disk name to tracepoints Johannes Thumshirn
2018-06-27 12:53 ` [PATCH v5 1/2] nvme: cache struct nvme_ctrl reference to struct nvme_request Johannes Thumshirn
2018-06-27 16:40   ` James Smart
2018-06-27 12:53 ` [PATCH v5 2/2] nvme: trace: add disk name to tracepoints Johannes Thumshirn
2018-06-27 16:37   ` Keith Busch
2018-06-28  7:48     ` Johannes Thumshirn
2018-06-28 20:55       ` Keith Busch

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox