[PATCH 1/1] nvme: add get-feature to admin cmds tracer

[PATCH 1/1] nvme: add get-feature to admin cmds tracer

[Max Gurtovoy]

This will print get-feature cmd in more informative way. For example,
run "nvme get-feature /dev/nvme0 -n 1 -f 0x9 -c 10" will trace:

 nvme-3907  [008] ....  1763.635054: nvme_setup_cmd: nvme0: qid=0, cmdid=6, nsid=1, flags=0x0, meta=0x0, cmd=(nvme_admin_get_features fid=0x9 sel=0x0 cdw11=0xa)
<idle>-0     [001] d.h.  1763.635112: nvme_sq: nvme0: qid=0, head=27, tail=27
<idle>-0     [008] ..s.  1763.635121: nvme_complete_rq: nvme0: qid=0, cmdid=6, res=10, retries=0, flags=0x2, status=0

Signed-off-by: Max Gurtovoy <maxg at mellanox.com>
---
 drivers/nvme/host/trace.c | 14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/drivers/nvme/host/trace.c b/drivers/nvme/host/trace.c
index 5566dda..caa18bb 100644
--- a/drivers/nvme/host/trace.c
+++ b/drivers/nvme/host/trace.c
@@ -58,7 +58,19 @@ static const char *nvme_trace_admin_identify(struct trace_seq *p, u8 *cdw10)
 	return ret;
 }
 
+static const char *nvme_trace_admin_get_features(struct trace_seq *p,
+						 u8 *cdw10)
+{
+	const char *ret = trace_seq_buffer_ptr(p);
+	u8 fid = cdw10[0];
+	u8 sel = cdw10[1];
+	u32 cdw11 = get_unaligned_le32(cdw10 + 4);
+
+	trace_seq_printf(p, "fid=0x%x sel=0x%x cdw11=0x%x", fid, sel, cdw11);
+	trace_seq_putc(p, 0);
 
+	return ret;
+}
 
 static const char *nvme_trace_read_write(struct trace_seq *p, u8 *cdw10)
 {
@@ -109,6 +121,8 @@ const char *nvme_trace_parse_admin_cmd(struct trace_seq *p,
 		return nvme_trace_create_cq(p, cdw10);
 	case nvme_admin_identify:
 		return nvme_trace_admin_identify(p, cdw10);
+	case nvme_admin_get_features:
+		return nvme_trace_admin_get_features(p, cdw10);
 	default:
 		return nvme_trace_common(p, cdw10);
 	}
-- 
1.8.3.1

[PATCH 1/1] nvme: add get-feature to admin cmds tracer

[Keith Busch]

On Thu, Jan 31, 2019@05:13:30PM +0200, Max Gurtovoy wrote:
> +static const char *nvme_trace_admin_get_features(struct trace_seq *p,
> +						 u8 *cdw10)
> +{
> +	const char *ret = trace_seq_buffer_ptr(p);
> +	u8 fid = cdw10[0];
> +	u8 sel = cdw10[1];
> +	u32 cdw11 = get_unaligned_le32(cdw10 + 4);
> +
> +	trace_seq_printf(p, "fid=0x%x sel=0x%x cdw11=0x%x", fid, sel, cdw11);
> +	trace_seq_putc(p, 0);

The 'sel' field is technically 3 bits, so this decoding may have a
forward compatibility problem if the commitee defines the upper bits
to a different field. Of course we'd have to update any trace event if
the spec changes in the future anyway.

So then maybe it would make more sense to *not* decode these things in
the kernel and just print out raw dwords that can be piped to a user
space utility for a more human friendly interpretation. Something like
what blkparse does for block trace events. Does that sound okay, or do
people really prefer having the kernel do this?

[PATCH 1/1] nvme: add get-feature to admin cmds tracer

[Max Gurtovoy]

On 1/31/2019 6:11 PM, Keith Busch wrote:
> On Thu, Jan 31, 2019@05:13:30PM +0200, Max Gurtovoy wrote:
>> +static const char *nvme_trace_admin_get_features(struct trace_seq *p,
>> +						 u8 *cdw10)
>> +{
>> +	const char *ret = trace_seq_buffer_ptr(p);
>> +	u8 fid = cdw10[0];
>> +	u8 sel = cdw10[1];

u8 sel = cdw10[1] & 0x7;


>> +	u32 cdw11 = get_unaligned_le32(cdw10 + 4);
>> +
>> +	trace_seq_printf(p, "fid=0x%x sel=0x%x cdw11=0x%x", fid, sel, cdw11);
>> +	trace_seq_putc(p, 0);
> The 'sel' field is technically 3 bits, so this decoding may have a
> forward compatibility problem if the commitee defines the upper bits
> to a different field. Of course we'd have to update any trace event if
> the spec changes in the future anyway.

see above.


>
> So then maybe it would make more sense to *not* decode these things in
> the kernel and just print out raw dwords that can be piped to a user
> space utility for a more human friendly interpretation. Something like
> what blkparse does for block trace events. Does that sound okay, or do
> people really prefer having the kernel do this?

Well, I guess that nvme-cli is the only common tool used by the majority 
of the community.

we can add something like "nvme trace-parse <raw-cmd>".

but this will open a various places to go wrong (and it will happen for 
sure).

Let's see what other guys think..

[PATCH 1/1] nvme: add get-feature to admin cmds tracer

[Johannes Thumshirn]

On 31/01/2019 18:01, Max Gurtovoy wrote:
> 
> On 1/31/2019 6:11 PM, Keith Busch wrote:
>> On Thu, Jan 31, 2019@05:13:30PM +0200, Max Gurtovoy wrote:
>>> +static const char *nvme_trace_admin_get_features(struct trace_seq *p,
>>> +???????????????????????? u8 *cdw10)
>>> +{
>>> +??? const char *ret = trace_seq_buffer_ptr(p);
>>> +??? u8 fid = cdw10[0];
>>> +??? u8 sel = cdw10[1];
> 
> u8 sel = cdw10[1] & 0x7;

Yup

[...]

> Well, I guess that nvme-cli is the only common tool used by the majority
> of the community.
> 
> we can add something like "nvme trace-parse <raw-cmd>".
> 
> but this will open a various places to go wrong (and it will happen for
> sure).
> 
> Let's see what other guys think..

I'm open for both sides, but we're already decoding a bunch of commands
in the kernel and so does the SCSI tracing, so I don't see a compelling
reason to stop it now.

-- 
Johannes Thumshirn                            SUSE Labs Filesystems
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 1/1] nvme: add get-feature to admin cmds tracer

[Max Gurtovoy]

On 2/4/2019 10:23 AM, Johannes Thumshirn wrote:
> On 31/01/2019 18:01, Max Gurtovoy wrote:
>> On 1/31/2019 6:11 PM, Keith Busch wrote:
>>> On Thu, Jan 31, 2019@05:13:30PM +0200, Max Gurtovoy wrote:
>>>> +static const char *nvme_trace_admin_get_features(struct trace_seq *p,
>>>> +???????????????????????? u8 *cdw10)
>>>> +{
>>>> +??? const char *ret = trace_seq_buffer_ptr(p);
>>>> +??? u8 fid = cdw10[0];
>>>> +??? u8 sel = cdw10[1];
>> u8 sel = cdw10[1] & 0x7;
> Yup
>
> [...]
>
>> Well, I guess that nvme-cli is the only common tool used by the majority
>> of the community.
>>
>> we can add something like "nvme trace-parse <raw-cmd>".
>>
>> but this will open a various places to go wrong (and it will happen for
>> sure).
>>
>> Let's see what other guys think..
> I'm open for both sides, but we're already decoding a bunch of commands
> in the kernel and so does the SCSI tracing, so I don't see a compelling
> reason to stop it now.

Yup, I also think we shouldn't throw all cmd decoding from kernel and we 
can add more decoding incrementally as I did in this commit.

Sagi/Christoph,

what is your standing in this topic ? should I send V2 ?

[PATCH 1/1] nvme: add get-feature to admin cmds tracer

[Christoph Hellwig]

On Thu, Feb 07, 2019@12:28:07PM +0200, Max Gurtovoy wrote:
>> I'm open for both sides, but we're already decoding a bunch of commands
>> in the kernel and so does the SCSI tracing, so I don't see a compelling
>> reason to stop it now.
>
> Yup, I also think we shouldn't throw all cmd decoding from kernel and we 
> can add more decoding incrementally as I did in this commit.
>
> Sagi/Christoph,
>
> what is your standing in this topic ? should I send V2 ?

I don't really care.  We clearly have command decoding in the trace
points, so adding a little more won't hurt, but I'm also not sure
we really need it.

[PATCH 1/1] nvme: add get-feature to admin cmds tracer

[Johannes Thumshirn]

On 08/02/2019 09:42, Christoph Hellwig wrote:
> I don't really care.  We clearly have command decoding in the trace
> points, so adding a little more won't hurt, but I'm also not sure
> we really need it.

Hmm one thing, *iff* we want to decode the commands in user-space we
have to agree on one input format for the decoder which then becomes an
ABI (see for example blktrace). If we decode in the kernel we can easily
extent it (like we already did with the tracepoints).

IMHO maintaining an ABI like format is a major pain in the lower body
parts and the benefits from it aren't worth it.

Just my .2$.

Byte,
	Johannes
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
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 1/1] nvme: add get-feature to admin cmds tracer

[Keith Busch]

On Fri, Feb 08, 2019@12:49:47AM -0800, Johannes Thumshirn wrote:
> On 08/02/2019 09:42, Christoph Hellwig wrote:
> > I don't really care.  We clearly have command decoding in the trace
> > points, so adding a little more won't hurt, but I'm also not sure
> > we really need it.
> 
> Hmm one thing, *iff* we want to decode the commands in user-space we
> have to agree on one input format for the decoder which then becomes an
> ABI (see for example blktrace). If we decode in the kernel we can easily
> extent it (like we already did with the tracepoints).
> 
> IMHO maintaining an ABI like format is a major pain in the lower body
> parts and the benefits from it aren't worth it.

That's fine, I certainly won't nak this and am okay to see this staged for
the next merge window. I just wanted to confirm preference on maintaining
the kernel vs tooling as the spec changes.

[PATCH 1/1] nvme: add get-feature to admin cmds tracer

[Johannes Thumshirn]

On Fri, Feb 08, 2019@08:30:40AM -0700, Keith Busch wrote:
> That's fine, I certainly won't nak this and am okay to see this staged for
> the next merge window. I just wanted to confirm preference on maintaining
> the kernel vs tooling as the spec changes.

Sure and I'm not totally sure doing the decoding in the kernel as is should be
carved in stone either.

I guess we have to revisit this decision some day in the future, possibly due
to spec changes and so on.

Byte,
	Johannes
-- 
Johannes Thumshirn                            SUSE Labs Filesystems
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