[PATCH] nvme: trace: parse format nvm command in detail

[PATCH] nvme: trace: parse format nvm command in detail

[Michal Krakowiak]

FTrace does not parse NVMe commands consistently. There are commands
that are being parsed in detail and commands for which the trace log
contains only raw bytes of command dwords. Add detailed parsing of
format nvm admin command to make the trace log more consistent and
human-readable.

Signed-off-by: Michal Krakowiak <michal.krakowiak@intel.com>
Acked-by: Dan Williams <dan.j.williams@intel.com>
---
 drivers/nvme/host/trace.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
index 5c3cb6928f3c..e0400de713b5 100644
--- a/drivers/nvme/host/trace.c
+++ b/drivers/nvme/host/trace.c
@@ -102,6 +102,23 @@ static const char *nvme_trace_get_lba_status(struct trace_seq *p,
 	return ret;
 }

+static const char *nvme_trace_admin_format_nvm(struct trace_seq *p, u8 *cdw10)
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+	u8 lbaf = cdw10[0] & 0xF;
+	u8 mset = (cdw10[0] >> 4) & 0x1;
+	u8 pi = (cdw10[0] >> 5) & 0x7;
+	u8 pil = cdw10[1] & 0x1;
+	u8 ses = (cdw10[1] >> 1) & 0x7;
+
+	trace_seq_printf(p, "lbaf=%u, mset=%u, pi=%u, pil=%u, ses=%u",
+			lbaf, mset, pi, pil, ses);
+
+	trace_seq_putc(p, 0);
+
+	return ret;
+}
+
 static const char *nvme_trace_read_write(struct trace_seq *p, u8 *cdw10)
 {
 	const char *ret = trace_seq_buffer_ptr(p);
@@ -159,6 +176,8 @@ const char *nvme_trace_parse_admin_cmd(struct trace_seq *p,
 		return nvme_trace_admin_get_features(p, cdw10);
 	case nvme_admin_get_lba_status:
 		return nvme_trace_get_lba_status(p, cdw10);
+	case nvme_admin_format_nvm:
+		return nvme_trace_admin_format_nvm(p, cdw10);
 	default:
 		return nvme_trace_common(p, cdw10);
 	}
--
2.20.1


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Keith Busch]

On Mon, Jan 04, 2021 at 04:53:43PM +0100, Michal Krakowiak wrote:
> FTrace does not parse NVMe commands consistently. There are commands
> that are being parsed in detail and commands for which the trace log
> contains only raw bytes of command dwords. Add detailed parsing of
> format nvm admin command to make the trace log more consistent and
> human-readable.

Perhaps it's too late since we've already gone done this path in the
driver, but IMO, we should have only traced raw dwords without any
additional decoding. This would have allowed consistent output for all
kernel versions and resilient to future spec changes. Then that could
have made possible for a user space utility to do the decoding to a
human friendly format, kind of like what blkparse does for block layer
trace events.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Christoph Hellwig]

On Mon, Jan 04, 2021 at 08:02:20AM -0800, Keith Busch wrote:
> Perhaps it's too late since we've already gone done this path in the
> driver, but IMO, we should have only traced raw dwords without any
> additional decoding. This would have allowed consistent output for all
> kernel versions and resilient to future spec changes. Then that could
> have made possible for a user space utility to do the decoding to a
> human friendly format, kind of like what blkparse does for block layer
> trace events.

the block tracepoints actually contain pretty detailed decoding as well.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Keith Busch]

On Wed, Jan 06, 2021 at 10:00:53AM +0100, Christoph Hellwig wrote:
> On Mon, Jan 04, 2021 at 08:02:20AM -0800, Keith Busch wrote:
> > Perhaps it's too late since we've already gone done this path in the
> > driver, but IMO, we should have only traced raw dwords without any
> > additional decoding. This would have allowed consistent output for all
> > kernel versions and resilient to future spec changes. Then that could
> > have made possible for a user space utility to do the decoding to a
> > human friendly format, kind of like what blkparse does for block layer
> > trace events.
> 
> the block tracepoints actually contain pretty detailed decoding as well.

Sure, but what I'm trying to say is that the current nvme decoding isn't
forward compatible with spec changes that tools will be able to use, and
will not be possible to see currently reserved fields. It will be a
never ending maintenance burden to chase those changes, and there will
be no consistency in nvme trace events for different kernel versions.

If it would help at all to move in a more future proof direction, I
wrote a little script handling all the decoding and posted it here:

  https://github.com/linux-nvme/nvme-trace/blob/main/nvme-trace.pl

If that's okay, we can greatly simplify the nvme tracing so that it
will always be consistent and able to obtain new command fields as they
show up.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Dan Williams]

On Fri, Jan 8, 2021 at 2:34 PM Keith Busch <kbusch@kernel.org> wrote:
>
> On Wed, Jan 06, 2021 at 10:00:53AM +0100, Christoph Hellwig wrote:
> > On Mon, Jan 04, 2021 at 08:02:20AM -0800, Keith Busch wrote:
> > > Perhaps it's too late since we've already gone done this path in the
> > > driver, but IMO, we should have only traced raw dwords without any
> > > additional decoding. This would have allowed consistent output for all
> > > kernel versions and resilient to future spec changes. Then that could
> > > have made possible for a user space utility to do the decoding to a
> > > human friendly format, kind of like what blkparse does for block layer
> > > trace events.
> >
> > the block tracepoints actually contain pretty detailed decoding as well.
>
> Sure, but what I'm trying to say is that the current nvme decoding isn't
> forward compatible with spec changes that tools will be able to use, and
> will not be possible to see currently reserved fields. It will be a
> never ending maintenance burden to chase those changes, and there will
> be no consistency in nvme trace events for different kernel versions.
>
> If it would help at all to move in a more future proof direction, I
> wrote a little script handling all the decoding and posted it here:
>
>   https://github.com/linux-nvme/nvme-trace/blob/main/nvme-trace.pl
>
> If that's okay, we can greatly simplify the nvme tracing so that it
> will always be consistent and able to obtain new command fields as they
> show up.

Is there a more centralized place to land such a helper? trace-cmd,
perf? The nice thing about kernel parsing is I don't need to have a
separate pile of disparate tools across subsystems.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Keith Busch]

On Fri, Jan 08, 2021 at 04:12:31PM -0800, Dan Williams wrote:
> Is there a more centralized place to land such a helper? trace-cmd,
> perf? The nice thing about kernel parsing is I don't need to have a
> separate pile of disparate tools across subsystems.

Yes, I just placed it in a temporary location to see if the other nvme
stakeholders are okay with the idea. If so, I can make a real patch
series to remove the in-kernel decoding, and add the parsing script
somewhere in tools/.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Dan Williams]

On Mon, Jan 11, 2021 at 9:01 AM Keith Busch <kbusch@kernel.org> wrote:
>
> On Fri, Jan 08, 2021 at 04:12:31PM -0800, Dan Williams wrote:
> > Is there a more centralized place to land such a helper? trace-cmd,
> > perf? The nice thing about kernel parsing is I don't need to have a
> > separate pile of disparate tools across subsystems.
>
> Yes, I just placed it in a temporary location to see if the other nvme
> stakeholders are okay with the idea. If so, I can make a real patch
> series to remove the in-kernel decoding, and add the parsing script
> somewhere in tools/.

"Somewhere in tools/" is still not quite what I'm asking for, I want
something nearly as automatic and seamless as the kernel ftrace pipe.
If I need nvme tools for nvme events and mce tools for mce events etc,
I'd rather just have decode in the kernel. Perhaps this is a use case
for a usermode helper that accompanies a new kernel tracing file?. If
the helper exists the events supported events are decoded if the
helper is missing the interface is equivalent to the raw formatted
events. Decoded events "just work".

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Keith Busch]

On Mon, Jan 11, 2021 at 09:10:34AM -0800, Dan Williams wrote:
> On Mon, Jan 11, 2021 at 9:01 AM Keith Busch <kbusch@kernel.org> wrote:
> >
> > On Fri, Jan 08, 2021 at 04:12:31PM -0800, Dan Williams wrote:
> > > Is there a more centralized place to land such a helper? trace-cmd,
> > > perf? The nice thing about kernel parsing is I don't need to have a
> > > separate pile of disparate tools across subsystems.
> >
> > Yes, I just placed it in a temporary location to see if the other nvme
> > stakeholders are okay with the idea. If so, I can make a real patch
> > series to remove the in-kernel decoding, and add the parsing script
> > somewhere in tools/.
> 
> "Somewhere in tools/" is still not quite what I'm asking for, I want
> something nearly as automatic and seamless as the kernel ftrace pipe.
> If I need nvme tools for nvme events and mce tools for mce events etc,
> I'd rather just have decode in the kernel. Perhaps this is a use case
> for a usermode helper that accompanies a new kernel tracing file?. If
> the helper exists the events supported events are decoded if the
> helper is missing the interface is equivalent to the raw formatted
> events. Decoded events "just work".

My only concern is that decoding omits fields, so you have to ugprade
your kernel in order to see the missing ones when the current kernel's
decoding doesn't "just work". If that's how people want to maintain
them, then I won't object any longer.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Michal Krakowiak]

On Mon, Jan 11, 2021 at 09:50:18AM -0800, Keith Busch wrote:
>On Mon, Jan 11, 2021 at 09:10:34AM -0800, Dan Williams wrote:
>> On Mon, Jan 11, 2021 at 9:01 AM Keith Busch <kbusch@kernel.org> wrote:
>> >
>> > On Fri, Jan 08, 2021 at 04:12:31PM -0800, Dan Williams wrote:
>> > > Is there a more centralized place to land such a helper? trace-cmd,
>> > > perf? The nice thing about kernel parsing is I don't need to have a
>> > > separate pile of disparate tools across subsystems.
>> >
>> > Yes, I just placed it in a temporary location to see if the other nvme
>> > stakeholders are okay with the idea. If so, I can make a real patch
>> > series to remove the in-kernel decoding, and add the parsing script
>> > somewhere in tools/.
>>
>> "Somewhere in tools/" is still not quite what I'm asking for, I want
>> something nearly as automatic and seamless as the kernel ftrace pipe.
>> If I need nvme tools for nvme events and mce tools for mce events etc,
>> I'd rather just have decode in the kernel. Perhaps this is a use case
>> for a usermode helper that accompanies a new kernel tracing file?. If
>> the helper exists the events supported events are decoded if the
>> helper is missing the interface is equivalent to the raw formatted
>> events. Decoded events "just work".
>
>My only concern is that decoding omits fields, so you have to ugprade
>your kernel in order to see the missing ones when the current kernel's
>decoding doesn't "just work". If that's how people want to maintain
>them, then I won't object any longer.

Like Dan, I really appreciate having the trace parsed in the kernel. I 
understand the concerns. What about having both: detailed parsing and 
raw bytes? It does not seem to me like a big deal to append a field with 
the raw data even if detailed parsing is implemented. Let the parsing 
extends the raw data instead of replacing them. This will result in the 
detailed human-friendly output (which is nice to have) while not losing 
data in the future (in the case that a new unsupported by parser field 
appears).

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Christoph Hellwig]

On Mon, Jan 11, 2021 at 09:15:52PM +0100, Michal Krakowiak wrote:
> Like Dan, I really appreciate having the trace parsed in the kernel. I 
> understand the concerns. What about having both: detailed parsing and raw 
> bytes? It does not seem to me like a big deal to append a field with the 
> raw data even if detailed parsing is implemented. Let the parsing extends 
> the raw data instead of replacing them. This will result in the detailed 
> human-friendly output (which is nice to have) while not losing data in the 
> future (in the case that a new unsupported by parser field appears).

That is how the kernel trace events work, including for the block layer.
blktrace is a separate piece of software that attaches to the raw
tracepoints and adds its own handling, so I don't think it is the right
comparism here.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Minwoo Im]

Hello,

On 21-01-04 16:53:43, Michal Krakowiak wrote:
> FTrace does not parse NVMe commands consistently. There are commands
> that are being parsed in detail and commands for which the trace log
> contains only raw bytes of command dwords. Add detailed parsing of
> format nvm admin command to make the trace log more consistent and
> human-readable.
> 
> Signed-off-by: Michal Krakowiak <michal.krakowiak@intel.com>
> Acked-by: Dan Williams <dan.j.williams@intel.com>

Looks good to me unless driver decides to go to raw trace with
user-space application like blkparse according to Keith's opinion.

Actually Format command has several meaningful fields related
to LBA format and protection information slo so that I would be happy
for format command to be parsed for now.

Reviewed-by: Minwoo Im <minwoo.im.dev@gmail.com>

Thanks,

> ---
>  drivers/nvme/host/trace.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
> index 5c3cb6928f3c..e0400de713b5 100644
> --- a/drivers/nvme/host/trace.c
> +++ b/drivers/nvme/host/trace.c
> @@ -102,6 +102,23 @@ static const char *nvme_trace_get_lba_status(struct trace_seq *p,
>  	return ret;
>  }
> 
> +static const char *nvme_trace_admin_format_nvm(struct trace_seq *p, u8 *cdw10)
> +{
> +	const char *ret = trace_seq_buffer_ptr(p);
> +	u8 lbaf = cdw10[0] & 0xF;
> +	u8 mset = (cdw10[0] >> 4) & 0x1;
> +	u8 pi = (cdw10[0] >> 5) & 0x7;
> +	u8 pil = cdw10[1] & 0x1;
> +	u8 ses = (cdw10[1] >> 1) & 0x7;
> +
> +	trace_seq_printf(p, "lbaf=%u, mset=%u, pi=%u, pil=%u, ses=%u",
> +			lbaf, mset, pi, pil, ses);
> +
> +	trace_seq_putc(p, 0);
> +
> +	return ret;
> +}
> +
>  static const char *nvme_trace_read_write(struct trace_seq *p, u8 *cdw10)
>  {
>  	const char *ret = trace_seq_buffer_ptr(p);
> @@ -159,6 +176,8 @@ const char *nvme_trace_parse_admin_cmd(struct trace_seq *p,
>  		return nvme_trace_admin_get_features(p, cdw10);
>  	case nvme_admin_get_lba_status:
>  		return nvme_trace_get_lba_status(p, cdw10);
> +	case nvme_admin_format_nvm:
> +		return nvme_trace_admin_format_nvm(p, cdw10);
>  	default:
>  		return nvme_trace_common(p, cdw10);
>  	}
> --
> 2.20.1
> 
> 
> _______________________________________________
> Linux-nvme mailing list
> Linux-nvme@lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/linux-nvme

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Christoph Hellwig]

Thanks,

applied to nvme-5.12.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Michal Krakowiak]

Thanks,

I'd like to ask about future nvme tracing development: Will preparing 
similar patches (for more commands) be welcome? I'd like to do it, but I 
got some concerns after the discussion raised due to this patch.


On Wed, Jan 27, 2021 at 06:46:06PM +0100, Christoph Hellwig wrote:
>Thanks,
>
>applied to nvme-5.12.
>

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme: trace: parse format nvm command in detail

[Christoph Hellwig]

On Thu, Jan 28, 2021 at 01:25:01PM +0100, Michal Krakowiak wrote:
> Thanks,
>
> I'd like to ask about future nvme tracing development: Will preparing 
> similar patches (for more commands) be welcome? I'd like to do it, but I 
> got some concerns after the discussion raised due to this patch.

I'm a little worried to that we grow decoders for every obscure NVMe
command in the long run.  I think everything that is either issued by
the kernel driver or very commonly used by administrators (e.g. Format
NVM) is a fair game, but at some point we have to draw a line.


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme