nvme trace updates
Subsystems (3)
v2 cover nvme trace updates
This patch series based on/inspired by Johannes Thumshirn that improves the nvme trace events.
The one from Sagi is saving the nvme_ctrl in the driver request data so we can reference it in other places, both for this series and future developments.
The others are adding and simplifying the nvme traces so we can see the more useful information to uniquely identify what hardware the trace event is showing, and filter events for specific hardware.
v1 -> v2:
Dropped the blk-mq patch as there were pre-existing ways to get to the desired information without introducing a new API
Changelog typo fixups
Keith Busch (3):
nvme: trace nvme queue identifiers
nvme: trace controller name
nvme: add disk name to tracepoints
Sagi Grimberg (1):
nvme: cache struct nvme_ctrl reference to struct nvme_request
drivers/nvme/host/core.c | 7 +++--- 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.c | 11 +++++++++ drivers/nvme/host/trace.h | 61 ++++++++++++++++++++++------------------------ drivers/nvme/target/loop.c | 1 + 8 files changed, 49 insertions(+), 36 deletions(-)
Signature · 1 line
2.14.3
PATCH v2 1/4 nvme: trace nvme queue identifiers
We can not match a command to its completion based on the command id alone. We need the submitting queue identifier to pair the completion, so this patch adds that to the trace buffer.
This patch is also collapsing the admin and io submission traces into a single one since we don't need to duplicate this and creating unnecessary code branches.
--- drivers/nvme/host/core.c | 7 +++---- drivers/nvme/host/trace.h | 41 ++++++++++------------------------------- 2 files changed, 13 insertions(+), 35 deletions(-)
drivers/nvme/host/core.c +3 -4
@@ -97,6 +97,8 @@ static dev_t nvme_chr_devt;static struct class *nvme_class;static struct class *nvme_subsys_class; +static void nvme_remove_invalid_namespaces (struct nvme_ctrl *ctrl, +unsigned nsid);static void nvme_ns_remove (struct nvme_ns *ns);static int nvme_revalidate_disk (struct gendisk *disk);static void nvme_put_subsystem (struct nvme_subsystem *subsys);
@@ -652,10 +654,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 -trace_nvme_setup_admin_cmd (cmd); +trace_nvme_setup_nvm_cmd (req, cmd);return ret; }EXPORT_SYMBOL_GPL (nvme_setup_cmd);
drivers/nvme/host/trace.h +10 -31
66 lines not on this page · raw message
Signature · 1 line
2.14.3
Re: [PATCHv2 1/4] nvme: trace nvme queue identifiers
drivers/nvme/host/core.c static dev_t nvme_chr_devt; · line 101
Quoted 3 lines · Keith Busch
drivers/nvme/host/core.c +2
-0+static void nvme_remove_invalid_namespaces (struct nvme_ctrl *ctrl, +unsigned nsid);
This looks like a leftover from something different.
Otherwise,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at 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
Christoph Hellwig
maintainerRe: [PATCHv2 1/4] nvme: trace nvme queue identifiers
In reply to Keith BuschQuoted 19 lines · Keith Busch
We can not match a command to its completion based on the command id alone. We need the submitting queue identifier to pair the completion, so this patch adds that to the trace buffer.
This patch is also collapsing the admin and io submission traces into a single one since we don't need to duplicate this and creating unnecessary code branches.
Signed-off-by Keith Busch <[email protected]>--- drivers/nvme/host/core.c | 7 +++---- drivers/nvme/host/trace.h | 41 ++++++++++------------------------------- 2 files changed, 13 insertions(+), 35 deletions(-)drivers/nvme/host/core.c +0
-0
drivers/nvme/host/core.c static dev_t nvme_chr_devt; · line 101
Quoted 5 lines
drivers/nvme/host/core.c +2
-0@@ -97,6 +97,8 @@ static dev_t nvme_chr_devt;static struct class *nvme_class;static struct class *nvme_subsys_class; +static void nvme_remove_invalid_namespaces (struct nvme_ctrl *ctrl, +unsigned nsid);
Huh?
Quoted 57 lines
drivers/nvme/host/core.c +1
-4static void nvme_ns_remove (struct nvme_ns *ns);static int nvme_revalidate_disk (struct gendisk *disk);static void nvme_put_subsystem (struct nvme_subsystem *subsys);@@ -652,10 +654,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 -trace_nvme_setup_admin_cmd (cmd); +trace_nvme_setup_nvm_cmd (req, cmd);return ret; }EXPORT_SYMBOL_GPL (nvme_setup_cmd);drivers/nvme/host/trace.h +2
-27@@ -75,34 +75,9 @@ const char *nvme_trace_parse_nvm_cmd(struct trace_seq *p, u8 opcode,#define __parse_nvme_cmd (opcode, cdw10) \ nvme_trace_parse_nvm_cmd(p, opcode, cdw10) -TRACE_EVENT (nvme_setup_admin_cmd, - TP_PROTO(struct nvme_command *cmd), -TP_ARGS (cmd), -TP_STRUCT__entry ( - __field(u8, opcode) - __field(u8, flags) - __field(u16, cid) - __field(u64, metadata) -__array (u8, cdw10,24 ) - ), -TP_fast_assign ( - __entry->opcode = cmd->common.opcode; - __entry->flags = cmd->common.flags; - __entry->cid = cmd->common.command_id; - __entry->metadata = le64_to_cpu(cmd->common.metadata); -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, -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), + TP_ARGS(req, cmd), TP_STRUCT__entry( __field(int, qid) __field(u8, opcode)
drivers/nvme/host/trace.h TRACE_EVENT(nvme_setup_nvm_cmd, · line 91
Quoted 5 lines
drivers/nvme/host/trace.h +1
-1@@ -113,7 +88,7 @@ TRACE_EVENT(nvme_setup_nvm_cmd,__array (u8, cdw10,24 ) ),TP_fast_assign ( - __entry->qid = qid; + __entry->qid = blk_mq_unique_tag_to_hwq(blk_mq_unique_tag(req)) + !!req->rq_disk;
This looks pretty ugly. I'd much prefer:
if (req->rq_disk) __entry->qid = blk_mq_unique_tag_to_hwq(blk_mq_unique_tag(req)); else __entry->qid = 0;
Also the indentation in the existing code seems weird and not based on tabs. We should fix that up.
drivers/nvme/host/trace.h TRACE_EVENT(nvme_setup_nvm_cmd, · line 109
Quoted 7 lines
drivers/nvme/host/trace.h +6
-0+ __entry->qid ? +show_opcode_name (__entry->opcode) : +show_admin_opcode_name (__entry->opcode), + __entry->qid ? +__parse_nvme_cmd (__entry->opcode, __entry->cdw10) : + __parse_nvme_admin_cmd(__entry->opcode, __entry->cdw10)) );
Can we hide this? E.g. pass the qid to show_opcode_name and then implement show_opcode_name as:
#define show_opcode_name(qid, opcode) \
(qid ? show_admin_opcode_name(opcode) : show_nvm_opcode_name(opcode))
Same for __parse_nvme_cmd.
drivers/nvme/host/trace.h
Quoted 1 line
drivers/nvme/host/trace.h +1
-0+ __entry->qid =blk_mq_unique_tag_to_hwq (blk_mq_unique_tag (req)) + !!req->rq_disk;
Same as above.
In fact I guess we should just have a helper:
static inline u16 nvme_req_to_qid(struct request *req)
{
if (!req->rq_disk)
return 0;
return blk_mq_unique_tag_to_hwq(blk_mq_unique_tag(req));
}PATCH v2 2/4 nvme: cache struct nvme_ctrl reference to struct nvme_request
In reply to Keith BuschFrom: 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.
--- 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 + 5 files changed, 6 insertions(+)
drivers/nvme/host/fc.c +1 -0
@@ -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++); }
drivers/nvme/host/nvme.h +1 -0
@@ -102,6 +102,7 @@ struct nvme_request {u8 retries;u8 flags;u16 status; +struct nvme_ctrl *ctrl; };/*
drivers/nvme/host/pci.c +2 -0
@@ -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 ; }
drivers/nvme/host/rdma.c +1 -0
@@ -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)
drivers/nvme/target/loop.c +1 -0
@@ -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 ); }
Signature · 1 line
2.14.3
Re: [PATCHv2 2/4] nvme: cache struct nvme_ctrl reference to struct nvme_request
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at 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
PATCH v2 3/4 nvme: trace controller name
In reply to Keith BuschThis appends the controller instance to the trace buffer for nvme commands to distinguish what controller is dispatching and completing a command.
--- drivers/nvme/host/trace.h | 17 +++++++++++------ 1 file changed, 11 insertions(+), 6 deletions(-)
drivers/nvme/host/trace.h +11 -6
@@ -79,6 +79,7 @@ TRACE_EVENT(nvme_setup_nvm_cmd, TP_PROTO(struct request *req, struct nvme_command *cmd), TP_ARGS(req, cmd), TP_STRUCT__entry( + __field(int, ctrl_id) __field(int, qid) __field(u8, opcode) __field(u8, flags)
@@ -88,6 +89,7 @@ TRACE_EVENT(nvme_setup_nvm_cmd,__array (u8, cdw10,24 ) ),TP_fast_assign ( + __entry->ctrl_id = nvme_req(req)->ctrl->instance; __entry->qid =blk_mq_unique_tag_to_hwq (blk_mq_unique_tag (req)) + !!req->rq_disk; __entry->opcode = cmd->common.opcode; __entry->flags = cmd->common.flags;
@@ -97,9 +99,9 @@ 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, + TP_printk("nvme%d: qid=%d, cmdid=%u, nsid=%u, flags=0x%x, meta=0x%llx, cmd=(%s %s)", + __entry->ctrl_id, __entry->qid, __entry->nsid, + __entry->cid, __entry->flags, __entry->metadata, __entry->qid ? show_opcode_name(__entry->opcode) : show_admin_opcode_name(__entry->opcode),
@@ -112,6 +114,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)
@@ -120,6 +123,7 @@ TRACE_EVENT(nvme_complete_rq,__field (u16, status) ),TP_fast_assign ( + __entry->ctrl_id = nvme_req(req)->ctrl->instance; __entry->qid =blk_mq_unique_tag_to_hwq (blk_mq_unique_tag (req)) + !!req->rq_disk; __entry->cid = req->tag; __entry->result =le64_to_cpu (nvme_req (req)->result.u64);
@@ -127,9 +131,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) );
Signature · 1 line
2.14.3
Re: [PATCHv2 3/4] nvme: trace controller name
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at 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
PATCH v2 4/4 nvme: add disk name to tracepoints
In reply to Keith BuschThis will print the disk name to the nvme trace for disk based requests so a user can better distinguish traffic to different disks. This allows disk based filters. For example, to see only nvme0n2 traffic:
echo "disk == nvme0n2" > /sys/kernel/debug/tracing/events/nvme/filter
--- drivers/nvme/host/trace.c | 11 +++++++++++ drivers/nvme/host/trace.h | 27 ++++++++++++++++++++------- 2 files changed, 31 insertions(+), 7 deletions(-)
drivers/nvme/host/trace.c +11 -0
@@ -128,3 +128,14 @@ const char *nvme_trace_parse_nvm_cmd(struct trace_seq *p,return nvme_trace_common (p, cdw10); } } + +const char *nvme_trace_disk_name (struct trace_seq *p,char *name) +{ +const char *ret =trace_seq_buffer_ptr (p); + +if (name[0 ] !=0 ) +trace_seq_printf (p,"disk=%-.*s, " ,DISK_NAME_LEN , name); +trace_seq_putc (p,0 ); + +return ret; +}
drivers/nvme/host/trace.h +20 -7
@@ -75,10 +75,15 @@ const char *nvme_trace_parse_nvm_cmd(struct trace_seq *p, u8 opcode,#define __parse_nvme_cmd (opcode, cdw10) \ nvme_trace_parse_nvm_cmd(p, opcode, cdw10) +const char *nvme_trace_disk_name (struct trace_seq *p,char *name); +#define __print_nvme_disk_name (name) \ + nvme_trace_disk_name(p, name) + TRACE_EVENT(nvme_setup_nvm_cmd, TP_PROTO(struct request *req, struct nvme_command *cmd), TP_ARGS(req, cmd), TP_STRUCT__entry( + __array(char, disk, DISK_NAME_LEN) __field(int, ctrl_id) __field(int, qid) __field(u8, opcode)
@@ -98,10 +103,14 @@ TRACE_EVENT(nvme_setup_nvm_cmd, __entry->metadata =le64_to_cpu (cmd->common.metadata);memcpy (__entry->cdw10, cmd->common.cdw10,sizeof (__entry->cdw10)); + req->rq_disk ? +memcpy (__entry->disk, req->rq_disk->disk_name,DISK_NAME_LEN ) : +memset (__entry->disk,0 ,DISK_NAME_LEN ); ), - TP_printk("nvme%d: qid=%d, cmdid=%u, nsid=%u, flags=0x%x, meta=0x%llx, cmd=(%s %s)", - __entry->ctrl_id, __entry->qid, __entry->nsid, - __entry->cid, __entry->flags, __entry->metadata, + TP_printk("nvme%d: %sqid=%d, cmdid=%u, nsid=%u, flags=0x%x, meta=0x%llx, cmd=(%s %s)", + __entry->ctrl_id, __print_nvme_disk_name(__entry->disk), + __entry->qid, __entry->nsid, __entry->cid, + __entry->flags, __entry->metadata, __entry->qid ? show_opcode_name(__entry->opcode) : show_admin_opcode_name(__entry->opcode),
@@ -114,6 +123,7 @@ TRACE_EVENT(nvme_complete_rq, TP_PROTO(struct request *req), TP_ARGS(req), TP_STRUCT__entry( + __array(char, disk, DISK_NAME_LEN) __field(int, ctrl_id) __field(int, qid) __field(int, cid)
@@ -130,11 +140,14 @@ TRACE_EVENT(nvme_complete_rq, __entry->retries =nvme_req (req)->retries; __entry->flags =nvme_req (req)->flags; __entry->status =nvme_req (req)->status; + req->rq_disk ? +memcpy (__entry->disk, req->rq_disk->disk_name,DISK_NAME_LEN ) : +memset (__entry->disk,0 ,DISK_NAME_LEN ); ), -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) +TP_printk ("nvme%d: %sqid=%d, cmdid=%u, res=%llu, retries=%u, flags=0x%x, status=%u" , + __entry->ctrl_id,__print_nvme_disk_name (__entry->disk), + __entry->qid, __entry->cid, __entry->result, + __entry->retries, __entry->flags, __entry->status) );
Signature · 1 line
2.14.3
Re: [PATCHv2 4/4] nvme: add disk name to tracepoints
Looks good,
Reviewed-by: Johannes Thumshirn <jthumshirn at suse.de>
Signature · 6 lines
Johannes Thumshirn Storage jthumshirn at 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
Christoph Hellwig
maintainerRe: [PATCHv2 4/4] nvme: add disk name to tracepoints
In reply to Keith Buschdrivers/nvme/host/trace.c const char *nvme_trace_parse_nvm_cmd(struct trace_seq *p, · line 137
Quoted 6 lines
drivers/nvme/host/trace.c +6
-0+const char *nvme_trace_disk_name (struct trace_seq *p,char *name) +{ +const char *ret =trace_seq_buffer_ptr (p); + +if (name[0 ] !=0 ) +trace_seq_printf (p,"disk=%-.*s, " ,DISK_NAME_LEN , name);
if (*name) trace_seq_printf(p, "disk=%-.*s, ", DISK_NAME_LEN, name);
?
drivers/nvme/host/trace.h TRACE_EVENT(nvme_setup_nvm_cmd, · line 108
Quoted 3 lines
drivers/nvme/host/trace.h +3
-0+ req->rq_disk ? +memcpy (__entry->disk, req->rq_disk->disk_name,DISK_NAME_LEN ) : +memset (__entry->disk,0 ,DISK_NAME_LEN );
Make this a helper please.