[PATCH V2 0/2] nvme-trace: Add support for fabrics command

[PATCH V2 0/2] nvme-trace: Add support for fabrics command

[Minwoo Im]

  The first one introduces a new inline function to check whether the
given nvme command is for fabrics or not.  We do the checks in many
places so that we can have it as an inline.

  The second patch is the actual commit to support fabrics command
tracing with updating review comments on previous V1.  Actually it has
been updated entirely, but keep it tracked for the history and make this
patch V2.

I would really appreciate it if they are reviewed.

Thanks,

Changes to V1:
  - fabrics commands should also be decoded, not just showing that it's
    a fabrics command. (Christoph)
  - do not make it within nvme admin commands (Chaitanya)

Minwoo Im (2):
  nvme: Introduce nvme_is_fabrics to check fabrics cmd
  nvme-trace: Add support to trace fabrics command

 drivers/nvme/host/core.c          |  7 +++-
 drivers/nvme/host/fabrics.c       |  2 +-
 drivers/nvme/host/trace.c         | 67 +++++++++++++++++++++++++++++++
 drivers/nvme/host/trace.h         | 37 +++++++++++++++++
 drivers/nvme/target/core.c        |  2 +-
 drivers/nvme/target/fabrics-cmd.c |  2 +-
 drivers/nvme/target/fc.c          |  2 +-
 include/linux/nvme.h              |  7 +++-
 8 files changed, 120 insertions(+), 6 deletions(-)

-- 
2.17.1

[PATCH 1/2] nvme: Introduce nvme_is_fabrics to check fabrics cmd

[Minwoo Im]

This patch introduce a nvme_is_fabrics() inline function to check
whether or not the given command structure is for fabrics.

Cc: Keith Busch <keith.busch at intel.com>
Cc: Jens Axboe <axboe at fb.com>
Cc: Christoph Hellwig <hch at lst.de>
Cc: Sagi Grimberg <sagi at grimberg.me>
Cc: James Smart <james.smart at broadcom.com>
Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 drivers/nvme/host/fabrics.c       | 2 +-
 drivers/nvme/target/core.c        | 2 +-
 drivers/nvme/target/fabrics-cmd.c | 2 +-
 drivers/nvme/target/fc.c          | 2 +-
 include/linux/nvme.h              | 7 ++++++-
 5 files changed, 10 insertions(+), 5 deletions(-)

diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
index 592d1e61ef7e..931995f2dbc3 100644
--- a/drivers/nvme/host/fabrics.c
+++ b/drivers/nvme/host/fabrics.c
@@ -578,7 +578,7 @@ bool __nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
 	switch (ctrl->state) {
 	case NVME_CTRL_NEW:
 	case NVME_CTRL_CONNECTING:
-		if (req->cmd->common.opcode == nvme_fabrics_command &&
+		if (nvme_is_fabrics(req->cmd) &&
 		    req->cmd->fabrics.fctype == nvme_fabrics_type_connect)
 			return true;
 		break;
diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
index 7734a6acff85..da2ea97042af 100644
--- a/drivers/nvme/target/core.c
+++ b/drivers/nvme/target/core.c
@@ -871,7 +871,7 @@ bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
 		status = nvmet_parse_connect_cmd(req);
 	else if (likely(req->sq->qid != 0))
 		status = nvmet_parse_io_cmd(req);
-	else if (req->cmd->common.opcode == nvme_fabrics_command)
+	else if (nvme_is_fabrics(req->cmd))
 		status = nvmet_parse_fabrics_cmd(req);
 	else if (req->sq->ctrl->subsys->type == NVME_NQN_DISC)
 		status = nvmet_parse_discovery_cmd(req);
diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
index 3b9f79aba98f..d16b55ffe79f 100644
--- a/drivers/nvme/target/fabrics-cmd.c
+++ b/drivers/nvme/target/fabrics-cmd.c
@@ -268,7 +268,7 @@ u16 nvmet_parse_connect_cmd(struct nvmet_req *req)
 {
 	struct nvme_command *cmd = req->cmd;
 
-	if (cmd->common.opcode != nvme_fabrics_command) {
+	if (!nvme_is_fabrics(cmd)) {
 		pr_err("invalid command 0x%x on unconnected queue.\n",
 			cmd->fabrics.opcode);
 		req->error_loc = offsetof(struct nvme_common_command, opcode);
diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
index 508661af0f50..a59c5a013a5c 100644
--- a/drivers/nvme/target/fc.c
+++ b/drivers/nvme/target/fc.c
@@ -1806,7 +1806,7 @@ nvmet_fc_prep_fcp_rsp(struct nvmet_fc_tgtport *tgtport,
 	 */
 	rspcnt = atomic_inc_return(&fod->queue->zrspcnt);
 	if (!(rspcnt % fod->queue->ersp_ratio) ||
-	    sqe->opcode == nvme_fabrics_command ||
+	    nvme_is_fabrics((struct nvme_command *) sqe) ||
 	    xfr_length != fod->req.transfer_len ||
 	    (le16_to_cpu(cqe->status) & 0xFFFE) || cqewd[0] || cqewd[1] ||
 	    (sqe->flags & (NVME_CMD_FUSE_FIRST | NVME_CMD_FUSE_SECOND)) ||
diff --git a/include/linux/nvme.h b/include/linux/nvme.h
index c40720cb59ac..ab5e9392b42d 100644
--- a/include/linux/nvme.h
+++ b/include/linux/nvme.h
@@ -1165,6 +1165,11 @@ struct nvme_command {
 	};
 };
 
+static inline bool nvme_is_fabrics(struct nvme_command *cmd)
+{
+	return cmd->common.opcode == nvme_fabrics_command;
+}
+
 struct nvme_error_slot {
 	__le64		error_count;
 	__le16		sqid;
@@ -1186,7 +1191,7 @@ static inline bool nvme_is_write(struct nvme_command *cmd)
 	 *
 	 * Why can't we simply have a Fabrics In and Fabrics out command?
 	 */
-	if (unlikely(cmd->common.opcode == nvme_fabrics_command))
+	if (unlikely(nvme_is_fabrics(cmd)))
 		return cmd->fabrics.fctype & 1;
 	return cmd->common.opcode & 1;
 }
-- 
2.17.1

[PATCH 1/2] nvme: Introduce nvme_is_fabrics to check fabrics cmd

[Chaitanya Kulkarni]

I'm not sure if we need change right now, reason :-

Existing code is pretty straight forward and we are not having
any complicated logic in the helper function. I'll let the
maintainers decide this.

On 05/06/2019 12:47 PM, Minwoo Im wrote:
> This patch introduce a nvme_is_fabrics() inline function to check
> whether or not the given command structure is for fabrics.
>
> Cc: Keith Busch <keith.busch at intel.com>
> Cc: Jens Axboe <axboe at fb.com>
> Cc: Christoph Hellwig <hch at lst.de>
> Cc: Sagi Grimberg <sagi at grimberg.me>
> Cc: James Smart <james.smart at broadcom.com>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>   drivers/nvme/host/fabrics.c       | 2 +-
>   drivers/nvme/target/core.c        | 2 +-
>   drivers/nvme/target/fabrics-cmd.c | 2 +-
>   drivers/nvme/target/fc.c          | 2 +-
>   include/linux/nvme.h              | 7 ++++++-
>   5 files changed, 10 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/nvme/host/fabrics.c b/drivers/nvme/host/fabrics.c
> index 592d1e61ef7e..931995f2dbc3 100644
> --- a/drivers/nvme/host/fabrics.c
> +++ b/drivers/nvme/host/fabrics.c
> @@ -578,7 +578,7 @@ bool __nvmf_check_ready(struct nvme_ctrl *ctrl, struct request *rq,
>   	switch (ctrl->state) {
>   	case NVME_CTRL_NEW:
>   	case NVME_CTRL_CONNECTING:
> -		if (req->cmd->common.opcode == nvme_fabrics_command &&
> +		if (nvme_is_fabrics(req->cmd) &&
>   		    req->cmd->fabrics.fctype == nvme_fabrics_type_connect)
>   			return true;
>   		break;
> diff --git a/drivers/nvme/target/core.c b/drivers/nvme/target/core.c
> index 7734a6acff85..da2ea97042af 100644
> --- a/drivers/nvme/target/core.c
> +++ b/drivers/nvme/target/core.c
> @@ -871,7 +871,7 @@ bool nvmet_req_init(struct nvmet_req *req, struct nvmet_cq *cq,
>   		status = nvmet_parse_connect_cmd(req);
>   	else if (likely(req->sq->qid != 0))
>   		status = nvmet_parse_io_cmd(req);
> -	else if (req->cmd->common.opcode == nvme_fabrics_command)
> +	else if (nvme_is_fabrics(req->cmd))
>   		status = nvmet_parse_fabrics_cmd(req);
>   	else if (req->sq->ctrl->subsys->type == NVME_NQN_DISC)
>   		status = nvmet_parse_discovery_cmd(req);
> diff --git a/drivers/nvme/target/fabrics-cmd.c b/drivers/nvme/target/fabrics-cmd.c
> index 3b9f79aba98f..d16b55ffe79f 100644
> --- a/drivers/nvme/target/fabrics-cmd.c
> +++ b/drivers/nvme/target/fabrics-cmd.c
> @@ -268,7 +268,7 @@ u16 nvmet_parse_connect_cmd(struct nvmet_req *req)
>   {
>   	struct nvme_command *cmd = req->cmd;
>
> -	if (cmd->common.opcode != nvme_fabrics_command) {
> +	if (!nvme_is_fabrics(cmd)) {
>   		pr_err("invalid command 0x%x on unconnected queue.\n",
>   			cmd->fabrics.opcode);
>   		req->error_loc = offsetof(struct nvme_common_command, opcode);
> diff --git a/drivers/nvme/target/fc.c b/drivers/nvme/target/fc.c
> index 508661af0f50..a59c5a013a5c 100644
> --- a/drivers/nvme/target/fc.c
> +++ b/drivers/nvme/target/fc.c
> @@ -1806,7 +1806,7 @@ nvmet_fc_prep_fcp_rsp(struct nvmet_fc_tgtport *tgtport,
>   	 */
>   	rspcnt = atomic_inc_return(&fod->queue->zrspcnt);
>   	if (!(rspcnt % fod->queue->ersp_ratio) ||
> -	    sqe->opcode == nvme_fabrics_command ||
> +	    nvme_is_fabrics((struct nvme_command *) sqe) ||
>   	    xfr_length != fod->req.transfer_len ||
>   	    (le16_to_cpu(cqe->status) & 0xFFFE) || cqewd[0] || cqewd[1] ||
>   	    (sqe->flags & (NVME_CMD_FUSE_FIRST | NVME_CMD_FUSE_SECOND)) ||
> diff --git a/include/linux/nvme.h b/include/linux/nvme.h
> index c40720cb59ac..ab5e9392b42d 100644
> --- a/include/linux/nvme.h
> +++ b/include/linux/nvme.h
> @@ -1165,6 +1165,11 @@ struct nvme_command {
>   	};
>   };
>
> +static inline bool nvme_is_fabrics(struct nvme_command *cmd)
> +{
> +	return cmd->common.opcode == nvme_fabrics_command;
> +}
> +
>   struct nvme_error_slot {
>   	__le64		error_count;
>   	__le16		sqid;
> @@ -1186,7 +1191,7 @@ static inline bool nvme_is_write(struct nvme_command *cmd)
>   	 *
>   	 * Why can't we simply have a Fabrics In and Fabrics out command?
>   	 */
> -	if (unlikely(cmd->common.opcode == nvme_fabrics_command))
> +	if (unlikely(nvme_is_fabrics(cmd)))
>   		return cmd->fabrics.fctype & 1;
>   	return cmd->common.opcode & 1;
>   }
>

[PATCH 1/2] nvme: Introduce nvme_is_fabrics to check fabrics cmd

[Minwoo Im]

Chaitanya,

Thanks for your kindly review on this.

> I'm not sure if we need change right now, reason :-
> 
> Existing code is pretty straight forward and we are not having

Yeah, the code looks readable.  But I thought it would be nicer if
it can be removed to reduce duplications in code.

> any complicated logic in the helper function. I'll let the

In fact, this is why I have introduced an inline function here.  If it
has complicated logic inside, I would not introduce it. :)

> maintainers decide this.

Thanks, again.

[PATCH 1/2] nvme: Introduce nvme_is_fabrics to check fabrics cmd

[Christoph Hellwig]

On Tue, May 07, 2019@07:59:38AM +0900, Minwoo Im wrote:
> Chaitanya,
> 
> Thanks for your kindly review on this.
> 
> > I'm not sure if we need change right now, reason :-
> > 
> > Existing code is pretty straight forward and we are not having
> 
> Yeah, the code looks readable.  But I thought it would be nicer if
> it can be removed to reduce duplications in code.
> 
> > any complicated logic in the helper function. I'll let the
> 
> In fact, this is why I have introduced an inline function here.  If it
> has complicated logic inside, I would not introduce it. :)

I think I'm ok with this check.  We keep growing a few more of those,
so a little helper like this might be useful.

[PATCH V2 2/2] nvme-trace: Add support to trace fabrics command

[Minwoo Im]

This patch supports fabrics command decoded in trace.  Fabrics commands
are to be decoded with (opcode, fctype) pair.

Now the core driver traces command in a single place of a single place
nvme_setup_cmd() so that it might be easier if we checks whether the
given command is for fabrics or not in that place.

NVMe-oF spec 1.0a contains authentication-related commands also, but now
we don't have any implementation for them so that we can just skip them
in this patch.

Changes to V1:
  - fabrics commands should also be decoded, not just showing that it's
    a fabrics command. (Christoph)
  - do not make it within nvme admin commands (Chaitanya)

Cc: Keith Busch <keith.busch at intel.com>
Cc: Jens Axboe <axboe at fb.com>
Cc: Christoph Hellwig <hch at lst.de>
Cc: Sagi Grimberg <sagi at grimberg.me>
Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
---
 drivers/nvme/host/core.c  |  7 +++-
 drivers/nvme/host/trace.c | 67 +++++++++++++++++++++++++++++++++++++++
 drivers/nvme/host/trace.h | 37 +++++++++++++++++++++
 3 files changed, 110 insertions(+), 1 deletion(-)

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index cd16d98d1f1a..eeda3ce9afaa 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -733,7 +733,12 @@ blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req,
 	}
 
 	cmd->common.command_id = req->tag;
-	trace_nvme_setup_cmd(req, cmd);
+
+	if (likely(!nvme_is_fabrics(cmd)))
+		trace_nvme_setup_cmd(req, cmd);
+	else
+		trace_nvme_setup_fabrics_cmd(req, cmd);
+
 	return ret;
 }
 EXPORT_SYMBOL_GPL(nvme_setup_cmd);
diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
index 5f24ea7a28eb..0e027a49e3a1 100644
--- a/drivers/nvme/host/trace.c
+++ b/drivers/nvme/host/trace.c
@@ -135,6 +135,73 @@ const char *nvme_trace_parse_nvm_cmd(struct trace_seq *p,
 	}
 }
 
+static const char *nvme_trace_fabrics_property_set(struct trace_seq *p, u8 *spc)
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+	u8 attrib = spc[0];
+	u32 ofst = get_unaligned_le32(spc + 4);
+	u64 value = get_unaligned_le64(spc + 8);
+
+	trace_seq_printf(p, "attrib=%u, ofst=0x%x, value=0x%llx",
+				attrib, ofst, value);
+	trace_seq_putc(p, 0);
+
+	return ret;
+}
+
+static const char *nvme_trace_fabrics_connect(struct trace_seq *p, u8 *spc)
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+	u16 recfmt = get_unaligned_le16(spc);
+	u16 qid = get_unaligned_le16(spc + 2);
+	u16 sqsize = get_unaligned_le16(spc + 4);
+	u8 cattr = spc[6];
+	u32 kato = get_unaligned_le32(spc + 8);
+
+	trace_seq_printf(p, "recfmt=%u, qid=%u, sqsize=%u, cattr=%u, kato=%u",
+				recfmt, qid, sqsize, cattr, kato);
+	trace_seq_putc(p, 0);
+
+	return ret;
+}
+
+static const char *nvme_trace_fabrics_property_get(struct trace_seq *p, u8 *spc)
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+	u8 attrib = spc[0];
+	u32 ofst = get_unaligned_le32(spc + 4);
+
+	trace_seq_printf(p, "attrib=%u, ofst=0x%x", attrib, ofst);
+	trace_seq_putc(p, 0);
+
+	return ret;
+}
+
+static const char *nvme_trace_fabrics_common(struct trace_seq *p, u8 *spc)
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+
+	trace_seq_printf(p, "spcecific=%*ph", 24, spc);
+	trace_seq_putc(p, 0);
+
+	return ret;
+}
+
+const char *nvme_trace_parse_fabrics_cmd(struct trace_seq *p,
+					 u8 fctype, u8 *spc)
+{
+	switch (fctype) {
+	case nvme_fabrics_type_property_set:
+		return nvme_trace_fabrics_property_set(p, spc);
+	case nvme_fabrics_type_connect:
+		return nvme_trace_fabrics_connect(p, spc);
+	case nvme_fabrics_type_property_get:
+		return nvme_trace_fabrics_property_get(p, spc);
+	default:
+		return nvme_trace_fabrics_common(p, spc);
+	}
+}
+
 const char *nvme_trace_disk_name(struct trace_seq *p, char *name)
 {
 	const char *ret = trace_seq_buffer_ptr(p);
diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
index 97d3c77365b8..fa3a20007b56 100644
--- a/drivers/nvme/host/trace.h
+++ b/drivers/nvme/host/trace.h
@@ -57,8 +57,17 @@
 		nvme_opcode_name(nvme_cmd_resv_acquire),	\
 		nvme_opcode_name(nvme_cmd_resv_release))
 
+#define nvme_fabrics_type_name(type)	{ type, #type }
+#define show_nvm_fabrics_type_name(val)					\
+	__print_symbolic(val,						\
+		nvme_fabrics_type_name(nvme_fabrics_type_property_set),	\
+		nvme_fabrics_type_name(nvme_fabrics_type_connect),	\
+		nvme_fabrics_type_name(nvme_fabrics_type_property_get))
+
 #define show_opcode_name(qid, opcode)					\
 	(qid ? show_nvm_opcode_name(opcode) : show_admin_opcode_name(opcode))
+#define show_fabric_type_name(type)					\
+	show_nvm_fabrics_type_name(type)
 
 const char *nvme_trace_parse_admin_cmd(struct trace_seq *p, u8 opcode,
 		u8 *cdw10);
@@ -70,6 +79,12 @@ const char *nvme_trace_parse_nvm_cmd(struct trace_seq *p, u8 opcode,
 	 nvme_trace_parse_nvm_cmd(p, opcode, cdw10) : 		\
 	 nvme_trace_parse_admin_cmd(p, opcode, cdw10))
 
+const char *nvme_trace_parse_fabrics_cmd(struct trace_seq *p,
+		u8 fctype, u8 *spc);
+
+#define parse_nvmf_cmd(fctype, spc)				\
+	nvme_trace_parse_fabrics_cmd(p, fctype, spc)
+
 const char *nvme_trace_disk_name(struct trace_seq *p, char *name);
 #define __print_disk_name(name)				\
 	nvme_trace_disk_name(p, name)
@@ -118,6 +133,28 @@ TRACE_EVENT(nvme_setup_cmd,
 		      parse_nvme_cmd(__entry->qid, __entry->opcode, __entry->cdw10))
 );
 
+TRACE_EVENT(nvme_setup_fabrics_cmd,
+	    TP_PROTO(struct request *req, struct nvme_command *cmd),
+	    TP_ARGS(req, cmd),
+	    TP_STRUCT__entry(
+		__field(int, ctrl_id)
+		__field(u16, cid)
+		__field(u8, fctype)
+		__array(u8, spc, 24)
+	    ),
+	    TP_fast_assign(
+		__entry->ctrl_id = nvme_req(req)->ctrl->instance;
+		__entry->cid = cmd->fabrics.command_id;
+		__entry->fctype = cmd->fabrics.fctype;
+		memcpy(__entry->spc, &cmd->fabrics.ts,
+			sizeof(__entry->spc));
+	    ),
+	    TP_printk("nvme%d: cmdid=%u, cmd=(%s %s)",
+		      __entry->ctrl_id, __entry->cid,
+		      show_fabric_type_name(__entry->fctype),
+		      parse_nvmf_cmd(__entry->fctype, __entry->spc))
+);
+
 TRACE_EVENT(nvme_complete_rq,
 	    TP_PROTO(struct request *req),
 	    TP_ARGS(req),
-- 
2.17.1

[PATCH V2 2/2] nvme-trace: Add support to trace fabrics command

[Chaitanya Kulkarni]

This look okay me with some minor comments, but it does not cover the 
target side tracing. In fact on the target side we have many components
such as discovery/multipath/port/async events etc.

On 05/06/2019 12:47 PM, Minwoo Im wrote:
> This patch supports fabrics command decoded in trace.  Fabrics commands
> are to be decoded with (opcode, fctype) pair.
>
> Now the core driver traces command in a single place of a single place
> nvme_setup_cmd() so that it might be easier if we checks whether the
> given command is for fabrics or not in that place.
>
> NVMe-oF spec 1.0a contains authentication-related commands also, but now
> we don't have any implementation for them so that we can just skip them
> in this patch.
>
> Changes to V1:
>    - fabrics commands should also be decoded, not just showing that it's
>      a fabrics command. (Christoph)
>    - do not make it within nvme admin commands (Chaitanya)
>
> Cc: Keith Busch <keith.busch at intel.com>
> Cc: Jens Axboe <axboe at fb.com>
> Cc: Christoph Hellwig <hch at lst.de>
> Cc: Sagi Grimberg <sagi at grimberg.me>
> Signed-off-by: Minwoo Im <minwoo.im.dev at gmail.com>
> ---
>   drivers/nvme/host/core.c  |  7 +++-
>   drivers/nvme/host/trace.c | 67 +++++++++++++++++++++++++++++++++++++++
>   drivers/nvme/host/trace.h | 37 +++++++++++++++++++++
>   3 files changed, 110 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> index cd16d98d1f1a..eeda3ce9afaa 100644
> --- a/drivers/nvme/host/core.c
> +++ b/drivers/nvme/host/core.c
> @@ -733,7 +733,12 @@ blk_status_t nvme_setup_cmd(struct nvme_ns *ns, struct request *req,
>   	}
>
>   	cmd->common.command_id = req->tag;
> -	trace_nvme_setup_cmd(req, cmd);
> +
> +	if (likely(!nvme_is_fabrics(cmd)))
Do we really need likely here ? Isn't this branch should be guarded by 
trace config ? what if trace is not configured ?
> +		trace_nvme_setup_cmd(req, cmd);
> +	else
> +		trace_nvme_setup_fabrics_cmd(req, cmd);
> +
>   	return ret;
>   }
>   EXPORT_SYMBOL_GPL(nvme_setup_cmd);
> diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
> index 5f24ea7a28eb..0e027a49e3a1 100644
> --- a/drivers/nvme/host/trace.c
> +++ b/drivers/nvme/host/trace.c
> @@ -135,6 +135,73 @@ const char *nvme_trace_parse_nvm_cmd(struct trace_seq *p,
>   	}
>   }
>
> +static const char *nvme_trace_fabrics_property_set(struct trace_seq *p, u8 *spc)
> +{
> +	const char *ret = trace_seq_buffer_ptr(p);
> +	u8 attrib = spc[0];
> +	u32 ofst = get_unaligned_le32(spc + 4);
please use name "offset" here.
> +	u64 value = get_unaligned_le64(spc + 8);
> +
> +	trace_seq_printf(p, "attrib=%u, ofst=0x%x, value=0x%llx",
> +				attrib, ofst, value);
> +	trace_seq_putc(p, 0);
> +
> +	return ret;
> +}
> +
> +static const char *nvme_trace_fabrics_connect(struct trace_seq *p, u8 *spc)
> +{
> +	const char *ret = trace_seq_buffer_ptr(p);
> +	u16 recfmt = get_unaligned_le16(spc);
> +	u16 qid = get_unaligned_le16(spc + 2);
> +	u16 sqsize = get_unaligned_le16(spc + 4);
> +	u8 cattr = spc[6];
> +	u32 kato = get_unaligned_le32(spc + 8);
> +
> +	trace_seq_printf(p, "recfmt=%u, qid=%u, sqsize=%u, cattr=%u, kato=%u",
> +				recfmt, qid, sqsize, cattr, kato);
> +	trace_seq_putc(p, 0);
> +
> +	return ret;
> +}
> +
> +static const char *nvme_trace_fabrics_property_get(struct trace_seq *p, u8 *spc)
> +{
> +	const char *ret = trace_seq_buffer_ptr(p);
> +	u8 attrib = spc[0];
> +	u32 ofst = get_unaligned_le32(spc + 4);
Same here offset.
> +
> +	trace_seq_printf(p, "attrib=%u, ofst=0x%x", attrib, ofst);
> +	trace_seq_putc(p, 0);
> +
> +	return ret;
> +}
> +
> +static const char *nvme_trace_fabrics_common(struct trace_seq *p, u8 *spc)
> +{
> +	const char *ret = trace_seq_buffer_ptr(p);
> +
> +	trace_seq_printf(p, "spcecific=%*ph", 24, spc);
Please avoid magic numbers "24", see following comment.
> +	trace_seq_putc(p, 0);
> +
> +	return ret;
> +}
> +
> +const char *nvme_trace_parse_fabrics_cmd(struct trace_seq *p,
> +					 u8 fctype, u8 *spc)
> +{
> +	switch (fctype) {
> +	case nvme_fabrics_type_property_set:
> +		return nvme_trace_fabrics_property_set(p, spc);
> +	case nvme_fabrics_type_connect:
> +		return nvme_trace_fabrics_connect(p, spc);
> +	case nvme_fabrics_type_property_get:
> +		return nvme_trace_fabrics_property_get(p, spc);
> +	default:
> +		return nvme_trace_fabrics_common(p, spc);
> +	}
> +}
> +
>   const char *nvme_trace_disk_name(struct trace_seq *p, char *name)
>   {
>   	const char *ret = trace_seq_buffer_ptr(p);
> diff --git a/drivers/nvme/host/trace.h b/drivers/nvme/host/trace.h
> index 97d3c77365b8..fa3a20007b56 100644
> --- a/drivers/nvme/host/trace.h
> +++ b/drivers/nvme/host/trace.h
> @@ -57,8 +57,17 @@
>   		nvme_opcode_name(nvme_cmd_resv_acquire),	\
>   		nvme_opcode_name(nvme_cmd_resv_release))
>
> +#define nvme_fabrics_type_name(type)	{ type, #type }
> +#define show_nvm_fabrics_type_name(val)					\
> +	__print_symbolic(val,						\
> +		nvme_fabrics_type_name(nvme_fabrics_type_property_set),	\
> +		nvme_fabrics_type_name(nvme_fabrics_type_connect),	\
> +		nvme_fabrics_type_name(nvme_fabrics_type_property_get))
> +
>   #define show_opcode_name(qid, opcode)					\
>   	(qid ? show_nvm_opcode_name(opcode) : show_admin_opcode_name(opcode))
> +#define show_fabric_type_name(type)					\
> +	show_nvm_fabrics_type_name(type)
>
>   const char *nvme_trace_parse_admin_cmd(struct trace_seq *p, u8 opcode,
>   		u8 *cdw10);
> @@ -70,6 +79,12 @@ const char *nvme_trace_parse_nvm_cmd(struct trace_seq *p, u8 opcode,
>   	 nvme_trace_parse_nvm_cmd(p, opcode, cdw10) : 		\
>   	 nvme_trace_parse_admin_cmd(p, opcode, cdw10))
>
> +const char *nvme_trace_parse_fabrics_cmd(struct trace_seq *p,
> +		u8 fctype, u8 *spc);
> +
> +#define parse_nvmf_cmd(fctype, spc)				\
> +	nvme_trace_parse_fabrics_cmd(p, fctype, spc)
> +
>   const char *nvme_trace_disk_name(struct trace_seq *p, char *name);
>   #define __print_disk_name(name)				\
>   	nvme_trace_disk_name(p, name)
> @@ -118,6 +133,28 @@ TRACE_EVENT(nvme_setup_cmd,
>   		      parse_nvme_cmd(__entry->qid, __entry->opcode, __entry->cdw10))
>   );
>
> +TRACE_EVENT(nvme_setup_fabrics_cmd,
> +	    TP_PROTO(struct request *req, struct nvme_command *cmd),
> +	    TP_ARGS(req, cmd),
> +	    TP_STRUCT__entry(
> +		__field(int, ctrl_id)
> +		__field(u16, cid)
> +		__field(u8, fctype)
> +		__array(u8, spc, 24)
Can you please just declare a macro and replace 24 ? (I think we need to 
replace in the nvme_setup_cmd() also, please double check.
> +	    ),
> +	    TP_fast_assign(
> +		__entry->ctrl_id = nvme_req(req)->ctrl->instance;
> +		__entry->cid = cmd->fabrics.command_id;
> +		__entry->fctype = cmd->fabrics.fctype;
> +		memcpy(__entry->spc, &cmd->fabrics.ts,
> +			sizeof(__entry->spc));
> +	    ),
> +	    TP_printk("nvme%d: cmdid=%u, cmd=(%s %s)",
Do we need to differentiate between fabrics and local NVMe ctrl when
reporting ?
> +		      __entry->ctrl_id, __entry->cid,
> +		      show_fabric_type_name(__entry->fctype),
> +		      parse_nvmf_cmd(__entry->fctype, __entry->spc))
> +);
> +
>   TRACE_EVENT(nvme_complete_rq,
>   	    TP_PROTO(struct request *req),
>   	    TP_ARGS(req),
>

[PATCH V2 2/2] nvme-trace: Add support to trace fabrics command

[Minwoo Im]

> This look okay me with some minor comments, but it does not cover the
> target side tracing. In fact on the target side we have many components
> such as discovery/multipath/port/async events etc.

You're right, but this patch is just focused on nvme-host side command
tracing.  If host-side patch is accepted, then I think I can prepare the target
side tracing also with the experience of it.

What do you say?

> > diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
> > index cd16d98d1f1a..eeda3ce9afaa 100644
> > --- a/drivers/nvme/host/core.c
> > +++ b/drivers/nvme/host/core.c
> > @@ -733,7 +733,12 @@ blk_status_t nvme_setup_cmd(struct nvme_ns
> *ns, struct request *req,
> >   	}
> >
> >   	cmd->common.command_id = req->tag;
> > -	trace_nvme_setup_cmd(req, cmd);
> > +
> > +	if (likely(!nvme_is_fabrics(cmd)))
> Do we really need likely here ? Isn't this branch should be guarded by
> trace config ? what if trace is not configured ?

The reason why I put likely here is that nvme_is_write() in linux/nvme.h
is doing the same kind of thing because, I think, it's going to be invoked
every simgle time in a performance critical path.

It's also be able to be built without trace configuration.  Can I ask why
do you concern the configuration of trace? (Actually I have build tested)

> > +		trace_nvme_setup_cmd(req, cmd);
> > +	else
> > +		trace_nvme_setup_fabrics_cmd(req, cmd);
> > +
> >   	return ret;
> >   }
> >   EXPORT_SYMBOL_GPL(nvme_setup_cmd);
> > diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
> > index 5f24ea7a28eb..0e027a49e3a1 100644
> > --- a/drivers/nvme/host/trace.c
> > +++ b/drivers/nvme/host/trace.c
> > @@ -135,6 +135,73 @@ const char *nvme_trace_parse_nvm_cmd(struct
> trace_seq *p,
> >   	}
> >   }
> >
> > +static const char *nvme_trace_fabrics_property_set(struct trace_seq *p,
> u8 *spc)
> > +{
> > +	const char *ret = trace_seq_buffer_ptr(p);
> > +	u8 attrib = spc[0];
> > +	u32 ofst = get_unaligned_le32(spc + 4);
> please use name "offset" here.

I have named these local variables to the same with names on spec.

> >
> > +TRACE_EVENT(nvme_setup_fabrics_cmd,
> > +	    TP_PROTO(struct request *req, struct nvme_command *cmd),
> > +	    TP_ARGS(req, cmd),
> > +	    TP_STRUCT__entry(
> > +		__field(int, ctrl_id)
> > +		__field(u16, cid)
> > +		__field(u8, fctype)
> > +		__array(u8, spc, 24)
> Can you please just declare a macro and replace 24 ? (I think we need to
> replace in the nvme_setup_cmd() also, please double check.

If we need to replace them to a macro, maybe it needs to be prepared with
a new commit to update them.  Anyway, I think size of 24 looks readable.
Do we really need to replace them with a macro ?

> > +	    ),
> > +	    TP_fast_assign(
> > +		__entry->ctrl_id = nvme_req(req)->ctrl->instance;
> > +		__entry->cid = cmd->fabrics.command_id;
> > +		__entry->fctype = cmd->fabrics.fctype;
> > +		memcpy(__entry->spc, &cmd->fabrics.ts,
> > +			sizeof(__entry->spc));
> > +	    ),
> > +	    TP_printk("nvme%d: cmdid=%u, cmd=(%s %s)",
> Do we need to differentiate between fabrics and local NVMe ctrl when
> reporting ?

I think we do.  The former one shows few fields that fabrics SQE does not
have.

Thank you, Chaitanya.

[PATCH V2 2/2] nvme-trace: Add support to trace fabrics command

[Chaitanya Kulkarni]

On 5/6/19 3:56 PM, Minwoo Im wrote:
>> This look okay me with some minor comments, but it does not cover the
>> target side tracing. In fact on the target side we have many components
>> such as discovery/multipath/port/async events etc.
> You're right, but this patch is just focused on nvme-host side command
> tracing.  If host-side patch is accepted, then I think I can prepare the target
> side tracing also with the experience of it.
>
> What do you say?

I really like the idea of tracing for NVMeOF. However I think we need to
design the tracing code

so that common code for host and target will get share. That is the main
reason I want both the

host and target side tracing to be done in the one patch-series. Even if
it is taking longer we are

fine with that.

So ideally patch series should look like this :-

1. NVMe-Core tracing change for fabrics commands and common code
preparation patches.

2. NVMe Host tracing changes.

3. NVMe Target Tracing changes.

I'm fine with the interchanging order of 2 & 3.

any thoughts ?

>
>>> diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
>>> index cd16d98d1f1a..eeda3ce9afaa 100644
>>> --- a/drivers/nvme/host/core.c
>>> +++ b/drivers/nvme/host/core.c
>>> @@ -733,7 +733,12 @@ blk_status_t nvme_setup_cmd(struct nvme_ns
>> *ns, struct request *req,
>>>   	}
>>>
>>>   	cmd->common.command_id = req->tag;
>>> -	trace_nvme_setup_cmd(req, cmd);
>>> +
>>> +	if (likely(!nvme_is_fabrics(cmd)))
>> Do we really need likely here ? Isn't this branch should be guarded by
>> trace config ? what if trace is not configured ?
> The reason why I put likely here is that nvme_is_write() in linux/nvme.h
> is doing the same kind of thing because, I think, it's going to be invoked
> every simgle time in a performance critical path.

May be this makes sense because they want to improve the performance of the

write commands since write commands are slower than the read commands?

May be this is good questions for maintainers we we would know.

>
> It's also be able to be built without trace configuration.  Can I ask why
> do you concern the configuration of trace? (Actually I have build tested)

I'm not sure having additional branches for tracing is a good idea in
the kernel

where tracing is disabled, that is objective, if maintainers are okay
with that let's

keep it that way.

>
>>> +		trace_nvme_setup_cmd(req, cmd);
>>> +	else
>>> +		trace_nvme_setup_fabrics_cmd(req, cmd);
>>> +
>>>   	return ret;
>>>   }
>>>   EXPORT_SYMBOL_GPL(nvme_setup_cmd);
>>> diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
>>> index 5f24ea7a28eb..0e027a49e3a1 100644
>>> --- a/drivers/nvme/host/trace.c
>>> +++ b/drivers/nvme/host/trace.c
>>> @@ -135,6 +135,73 @@ const char *nvme_trace_parse_nvm_cmd(struct
>> trace_seq *p,
>>>   	}
>>>   }
>>>
>>> +static const char *nvme_trace_fabrics_property_set(struct trace_seq *p,
>> u8 *spc)
>>> +{
>>> +	const char *ret = trace_seq_buffer_ptr(p);
>>> +	u8 attrib = spc[0];
>>> +	u32 ofst = get_unaligned_le32(spc + 4);
>> please use name "offset" here.
> I have named these local variables to the same with names on spec.
>
>>> +TRACE_EVENT(nvme_setup_fabrics_cmd,
>>> +	    TP_PROTO(struct request *req, struct nvme_command *cmd),
>>> +	    TP_ARGS(req, cmd),
>>> +	    TP_STRUCT__entry(
>>> +		__field(int, ctrl_id)
>>> +		__field(u16, cid)
>>> +		__field(u8, fctype)
>>> +		__array(u8, spc, 24)
>> Can you please just declare a macro and replace 24 ? (I think we need to
>> replace in the nvme_setup_cmd() also, please double check.
> If we need to replace them to a macro, maybe it needs to be prepared with
> a new commit to update them.  Anyway, I think size of 24 looks readable.
> Do we really need to replace them with a macro ?

You are absolutely right. Since it is been used several places now this
is good time

to have a preparation patch, and I don't think we should delay it more.

Also for host and target NVMeOF tracing it will be useful if we
encounter same

scenario.

>
>>> +	    ),
>>> +	    TP_fast_assign(
>>> +		__entry->ctrl_id = nvme_req(req)->ctrl->instance;
>>> +		__entry->cid = cmd->fabrics.command_id;
>>> +		__entry->fctype = cmd->fabrics.fctype;
>>> +		memcpy(__entry->spc, &cmd->fabrics.ts,
>>> +			sizeof(__entry->spc));
>>> +	    ),
>>> +	    TP_printk("nvme%d: cmdid=%u, cmd=(%s %s)",
>> Do we need to differentiate between fabrics and local NVMe ctrl when
>> reporting ?
> I think we do.  The former one shows few fields that fabrics SQE does not
> have.
>
> Thank you, Chaitanya.
>

[PATCH V2 2/2] nvme-trace: Add support to trace fabrics command

[Minwoo Im]

> I really like the idea of tracing for NVMeOF. However I think we need to
> design the tracing code
> 
> so that common code for host and target will get share. That is the main
> reason I want both the
> 
> host and target side tracing to be done in the one patch-series. Even if
> it is taking longer we are
> 
> fine with that.
> 
> So ideally patch series should look like this :-
> 
> 1. NVMe-Core tracing change for fabrics commands and common code
> preparation patches.
> 
> 2. NVMe Host tracing changes.
> 
> 3. NVMe Target Tracing changes.
> 
> I'm fine with the interchanging order of 2 & 3.
> 
> any thoughts ?

I'm fine if it takes time.  will prepare V3 patch with what you have 
proposed above.

Thanks.

> I'm not sure having additional branches for tracing is a good idea in
> the kernel
> 
> where tracing is disabled, that is objective, if maintainers are okay
> with that let's
> 
> keep it that way.

I don't like conditional branches here neither.  Let me have a time to 
catch some idea about it.

> You are absolutely right. Since it is been used several places now this
> is good time
> 
> to have a preparation patch, and I don't think we should delay it more.
> 
> Also for host and target NVMeOF tracing it will be useful if we
> encounter same
> 
> scenario.

If other people agree on this, will prepare right away.

Thanks for your kindly review, Chaitanya.

[PATCH V2 2/2] nvme-trace: Add support to trace fabrics command

[Christoph Hellwig]

> +	if (likely(!nvme_is_fabrics(cmd)))
> +		trace_nvme_setup_cmd(req, cmd);
> +	else
> +		trace_nvme_setup_fabrics_cmd(req, cmd);
> +

I wonder if we really need this.  My idea would have been to just
branch out to the fabrics command parser from parse_nvme_cmd().  Or did
I miss something why that might not work?

The benefit is that we don't have an additional branch in the
non-tracing fast path (and also make it look a little nicer).

> +#define show_fabric_type_name(type)					\
> +	show_nvm_fabrics_type_name(type)

Why do we need this #define to the same name/signature?

[PATCH V2 2/2] nvme-trace: Add support to trace fabrics command

[Minwoo Im]

On 5/8/19 4:25 PM, Christoph Hellwig wrote:
>> +	if (likely(!nvme_is_fabrics(cmd)))
>> +		trace_nvme_setup_cmd(req, cmd);
>> +	else
>> +		trace_nvme_setup_fabrics_cmd(req, cmd);
>> +
> 
> I wonder if we really need this.  My idea would have been to just
> branch out to the fabrics command parser from parse_nvme_cmd().  Or did
> I miss something why that might not work?

You didn't miss anything.  I was trying to separate fabrics commands out 
of it because they have a little bit different "common" SQE fields than 
the existing one.  That's why I made it separated.

If you want parse_nvme_cmd() branch out to something like 
nvme_parse_fabrics_cmd(), it would be easier to go.  Actually that was 
my second try, but I thought people wanted to show the exact fields for 
fabrics commands in traces.

Anyway, If it's okay to go with common fields in common, and from the 
cdw10 position, I'l prepare a V3 patch.

> The benefit is that we don't have an additional branch in the
> non-tracing fast path (and also make it look a little nicer).

100% agreed.  As Chaitanya was worried about this part, I'm now also.

>> +#define show_fabric_type_name(type)					\
>> +	show_nvm_fabrics_type_name(type)
> 
> Why do we need this #define to the same name/signature?

Oh, I was just trying to form it like existing one.  I was referring 
show_opcode_name(), but, That just needs to branch out by a given "qid".

This #define has no need to define like this. Thanks for your review.