Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751768AbeAPQt5 (ORCPT + 1 other); Tue, 16 Jan 2018 11:49:57 -0500 Received: from verein.lst.de ([213.95.11.211]:49012 "EHLO newverein.lst.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751740AbeAPQt4 (ORCPT ); Tue, 16 Jan 2018 11:49:56 -0500 Date: Tue, 16 Jan 2018 17:49:54 +0100 From: Christoph Hellwig To: Johannes Thumshirn Cc: Christoph Hellwig , Sagi Grimberg , Keith Busch , Linux Kernel Mailinglist , Hannes Reinecke , Linux NVMe Mailinglist Subject: Re: [PATCH v2 1/2] nvme: add tracepoint for nvme_setup_cmd Message-ID: <20180116164954.GA9812@lst.de> References: <20180116142821.11693-1-jthumshirn@suse.de> <20180116142821.11693-2-jthumshirn@suse.de> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20180116142821.11693-2-jthumshirn@suse.de> User-Agent: Mutt/1.5.17 (2007-11-01) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Return-Path: > + trace_seq_printf(p, "slba=%llu, length=%u, control=0x%x, dsmgmt=%u, reftag=%u", Overly long line. > + (unsigned long long)le64_to_cpu(rw.slba), u64 now always is an unsigned long long, no need for the cast. > + le16_to_cpu(rw.length), le16_to_cpu(rw.control), > + le32_to_cpu(rw.dsmgmt), le32_to_cpu(rw.reftag)); > + trace_seq_putc(p, 0); This look weird, is there a a good reason for putting a 0 character? Also I'd be tempted to add a new trace.c file for these and make them dependent on whatever kernel option enables the tracing. > --- /dev/null > +++ b/drivers/nvme/host/trace.h > @@ -0,0 +1,69 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ Please add a real copyright header including the author. const char *nvme_trace_parse_cmd(struct trace_seq *p, struct nvme_command cmnd); > +#define __parse_nvme_cmd(cmd) nvme_trace_parse_cmd(p, cmd) This looks weird. > +TRACE_EVENT(nvme_setup_cmd, > + > + TP_PROTO(struct nvme_command *cmd), > + > + TP_ARGS(cmd), > + Please remove these empty lines. > + TP_STRUCT__entry( > + __field( __u8, opcode ) > + __field( __u8, flags ) > + __field( __u16, cid ) > + __field( __le32, nsid ) > + __field( __le64, metadata ) > + __field_struct( struct nvme_command, cmnd ) And the additional whitespaces before and after the braces > + TP_fast_assign( > + __entry->opcode = cmd->common.opcode; > + __entry->flags = cmd->common.flags; > + __entry->cid = cmd->common.command_id; > + __entry->nsid = cmd->common.nsid; > + __entry->metadata = cmd->common.metadata; > + memcpy(&__entry->cmnd, cmd, sizeof(__entry->cmnd)); Copying the whole SQE in tracing seems rather excessive. > + TP_printk("nsid=%u, command_id=%u, flags=0x%x, metadata=0x%llx, cmd=(%s %s)", Too long line. > + le32_to_cpu(__entry->nsid), __entry->cid, __entry->flags, > + (unsigned long long) le64_to_cpu(__entry->metadata), no need for the cast. > +#undef TRACE_INCLUDE_PATH > +#define TRACE_INCLUDE_PATH ../../drivers/nvme/host This seems wrong. Instead you should make sure we include the srcdir in the makefile, take a look at fs/xfs/Makefile for example.