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
From: Sagi Grimberg <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
---
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
On 6/27/2018 5:53 AM, Johannes Thumshirn wrote:
> From: Sagi Grimberg <[email protected]>
>
> 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 <[email protected]>
>
> ---
> 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 <[email protected]>
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?
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.
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
[email protected] +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