2018-06-29 06:22:36

by Keith Busch

[permalink] [raw]
Subject: [PATCHv2 0/4] 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(-)

--
2.14.3



2018-06-29 06:22:35

by Keith Busch

[permalink] [raw]
Subject: [PATCHv2 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.

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(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index 46df030b2c3f..398a5fb26603 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -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);
diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
index 01390f0e1671..42c99445dd96 100644
--- a/drivers/nvme/host/trace.h
+++ b/drivers/nvme/host/trace.h
@@ -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)
@@ -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;
__entry->opcode = cmd->common.opcode;
__entry->flags = cmd->common.flags;
__entry->cid = cmd->common.command_id;
@@ -125,8 +100,12 @@ TRACE_EVENT(nvme_setup_nvm_cmd,
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),
- __parse_nvme_cmd(__entry->opcode, __entry->cdw10))
+ __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))
);

TRACE_EVENT(nvme_complete_rq,
@@ -141,7 +120,7 @@ TRACE_EVENT(nvme_complete_rq,
__field(u16, status)
),
TP_fast_assign(
- __entry->qid = req->q->id;
+ __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);
__entry->retries = nvme_req(req)->retries;
--
2.14.3


2018-06-29 06:23:03

by Keith Busch

[permalink] [raw]
Subject: [PATCHv2 2/4] nvme: cache struct nvme_ctrl reference to struct nvme_request

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]>
---
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(+)

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 9544625c0b7d..ade55977d432 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.14.3


2018-06-29 06:23:09

by Keith Busch

[permalink] [raw]
Subject: [PATCHv2 3/4] nvme: trace controller name

This appends the controller instance to the trace buffer for nvme commands
to distinguish what controller is dispatching and completing a command.

Signed-off-by: Keith Busch <[email protected]>
---
drivers/nvme/host/trace.h | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
index 42c99445dd96..a99b2929794a 100644
--- a/drivers/nvme/host/trace.h
+++ b/drivers/nvme/host/trace.h
@@ -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)

);

--
2.14.3


2018-06-29 06:23:48

by Keith Busch

[permalink] [raw]
Subject: [PATCHv2 4/4] nvme: add disk name to tracepoints

This 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

Signed-off-by: Keith Busch <[email protected]>
---
drivers/nvme/host/trace.c | 11 +++++++++++
drivers/nvme/host/trace.h | 27 ++++++++++++++++++++-------
2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
index 41944bbef835..796a5bcf67b8 100644
--- a/drivers/nvme/host/trace.c
+++ b/drivers/nvme/host/trace.c
@@ -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;
+}
diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
index a99b2929794a..d3522c373e5e 100644
--- a/drivers/nvme/host/trace.h
+++ b/drivers/nvme/host/trace.h
@@ -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)

);

--
2.14.3


2018-06-29 07:28:52

by Johannes Thumshirn

[permalink] [raw]
Subject: Re: [PATCHv2 2/4] nvme: cache struct nvme_ctrl reference to struct nvme_request

Looks good,
Reviewed-by: Johannes Thumshirn <[email protected]>
--
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

2018-06-29 07:29:41

by Johannes Thumshirn

[permalink] [raw]
Subject: Re: [PATCHv2 1/4] nvme: trace nvme queue identifiers

On Thu, Jun 28, 2018 at 03:49:53PM -0600, Keith Busch wrote:
>
> +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 <[email protected]>
--
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

2018-06-29 07:32:52

by Johannes Thumshirn

[permalink] [raw]
Subject: Re: [PATCHv2 4/4] nvme: add disk name to tracepoints

Looks good,
Reviewed-by: Johannes Thumshirn <[email protected]>
--
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

2018-06-29 07:58:21

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCHv2 1/4] nvme: trace nvme queue identifiers

On Thu, Jun 28, 2018 at 03:49:53PM -0600, Keith Busch wrote:
> 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(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index 46df030b2c3f..398a5fb26603 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -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?

> 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);
> diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
> index 01390f0e1671..42c99445dd96 100644
> --- a/drivers/nvme/host/trace.h
> +++ b/drivers/nvme/host/trace.h
> @@ -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)
> @@ -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.

> + __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.

> + __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));
}

2018-06-29 08:02:22

by Christoph Hellwig

[permalink] [raw]
Subject: Re: [PATCHv2 4/4] nvme: add disk name to tracepoints

> +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);

?

> + 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.

2018-06-29 09:44:12

by Johannes Thumshirn

[permalink] [raw]
Subject: Re: [PATCHv2 3/4] nvme: trace controller name

Looks good,
Reviewed-by: Johannes Thumshirn <[email protected]>
--
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