Re: Re: [RFC PATCH 07/10] scsi/trace: Use scsi_show_result trace point instead of printk
Hi Christoph, Sorry for the late reply. (2014/08/29 9:50), Christoph Hellwig wrote: I'm not sure this is the correct way. Currently we have quite some code duplication in scsi_trace.c and constants.c, correct. So I definitely would like to see them both merged. But constants.c is influenced by CONFIG_SCSI_CONSTANTS, whereas scsi_trace isn't, and the functions in constants.c are used throughout the scsi stack. So I'd rather see to have scsi_trace to be updated to use the functions from constants.c, and remove the duplicate code in scsi_trace. The tracepoints need to use the magic print_flags & co helpers so that output works properly if using the binary tracebuffer and user space tools that decoded it (e.g. trace-cmd or perf), so using a kernel function for decoding is not an option. Ah, I see. The "format" files in SCSI traceevents output decoders, so we don't need to implement own decoders in userland. I think the current problem is duplicated decoders, so we'll consolidate those. If we use decoders in constants.c, the decoders are not output in format file, so user tools cannot decode the binary. On the other hand, if we use decoders in scsi_trace.c, the output format of command names is changed and there are unsupported command names. We would use decoders in scsi_trace.c and add new command names to decoders in scsi_trace.c, I think. How do you think about this? Hannes? As another topic, we found that we cannot decode SCSI traceevents by using current decoder in format files. For example, format file of scsi_dispatch_cmd_timeout outputs prot_op as follows: __print_symbolic(REC->prot_op, { SCSI_PROT_NORMAL, "SCSI_PROT_NORMAL" }, { SCSI_PROT_READ_INSERT, "SCSI_PROT_READ_INSERT" }, { SCSI_PROT_WRITE_STRIP, "SCSI_PROT_WRITE_STRIP" }, { SCSI_PROT_READ_STRIP, "SCSI_PROT_READ_STRIP" }, { SCSI_PROT_WRITE_INSERT, "SCSI_PROT_WRITE_INSERT" }, { SCSI_PROT_READ_PASS, "SCSI_PROT_READ_PASS" }, { SCSI_PROT_WRITE_PASS, "SCSI_PROT_WRITE_PASS" }) Decoding will fail to do macro expansion here, so we need to fix this. (We don't use enum for traceevents.) Thanks, Yoshihiro YUNOMAE But we can make these tracepoints dependent on CONFIG_SCSI_CONSTANTS to still allow building lighter kernels if we really care about it. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html -- Yoshihiro YUNOMAE Software Platform Research Dept. Linux Technology Center Hitachi, Ltd., Yokohama Research Laboratory E-mail: yoshihiro.yunomae...@hitachi.com -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC PATCH 07/10] scsi/trace: Use scsi_show_result trace point instead of printk
> I'm not sure this is the correct way. > Currently we have quite some code duplication in scsi_trace.c and > constants.c, correct. > So I definitely would like to see them both merged. > > But constants.c is influenced by CONFIG_SCSI_CONSTANTS, whereas > scsi_trace isn't, and the functions in constants.c are used throughout the > scsi stack. > So I'd rather see to have scsi_trace to be updated to use the functions > from constants.c, and remove the duplicate code in > scsi_trace. The tracepoints need to use the magic print_flags & co helpers so that output works properly if using the binary tracebuffer and user space tools that decoded it (e.g. trace-cmd or perf), so using a kernel function for decoding is not an option. But we can make these tracepoints dependent on CONFIG_SCSI_CONSTANTS to still allow building lighter kernels if we really care about it. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Re: [RFC PATCH 07/10] scsi/trace: Use scsi_show_result trace point instead of printk
(2014/08/27 23:12), Hannes Reinecke wrote: On 08/08/2014 01:50 PM, Yoshihiro YUNOMAE wrote: Current SCSI trace has hostbyte table and driverbyte table, so we don't need to have the same table in scsi/constants.c. - Result examples (printk) sd 2:0:0:0: [sda] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE (ftrace) scsi_show_result: host_no=2 channel=0 id=0 lun=0 [sda] result=(driver=DRIVER_SENSE host=DID_OK) Signed-off-by: Yoshihiro YUNOMAE Cc: Hannes Reinecke Cc: Doug Gilbert Cc: Martin K. Petersen Cc: Christoph Hellwig Cc: "James E.J. Bottomley" Cc: Hidehiro Kawai Cc: Masami Hiramatsu --- drivers/scsi/constants.c| 52 --- drivers/scsi/scsi_trace.c | 16 + include/trace/events/scsi.h | 38 +++ 3 files changed, 53 insertions(+), 53 deletions(-) diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c index 6fad6b4..f7b7f32 100644 --- a/drivers/scsi/constants.c +++ b/drivers/scsi/constants.c @@ -1488,55 +1488,3 @@ void scsi_print_sense(struct scsi_cmnd *cmd) SCSI_SENSE_BUFFERSIZE); } EXPORT_SYMBOL(scsi_print_sense); - -#ifdef CONFIG_SCSI_CONSTANTS - -static const char * const hostbyte_table[]={ -"DID_OK", "DID_NO_CONNECT", "DID_BUS_BUSY", "DID_TIME_OUT", "DID_BAD_TARGET", -"DID_ABORT", "DID_PARITY", "DID_ERROR", "DID_RESET", "DID_BAD_INTR", -"DID_PASSTHROUGH", "DID_SOFT_ERROR", "DID_IMM_RETRY", "DID_REQUEUE", -"DID_TRANSPORT_DISRUPTED", "DID_TRANSPORT_FAILFAST", "DID_TARGET_FAILURE", -"DID_NEXUS_FAILURE" }; -#define NUM_HOSTBYTE_STRS ARRAY_SIZE(hostbyte_table) - -static const char * const driverbyte_table[]={ -"DRIVER_OK", "DRIVER_BUSY", "DRIVER_SOFT", "DRIVER_MEDIA", "DRIVER_ERROR", -"DRIVER_INVALID", "DRIVER_TIMEOUT", "DRIVER_HARD", "DRIVER_SENSE"}; -#define NUM_DRIVERBYTE_STRS ARRAY_SIZE(driverbyte_table) - -void scsi_show_result(struct scsi_device *sdev, const char *name, int result) -{ -int hb = host_byte(result); -int db = driver_byte(result); -const char *hb_string; -const char *db_string; - -hb_string = (hb < NUM_HOSTBYTE_STRS) ? hostbyte_table[hb] : "invalid"; -db_string = (db < NUM_DRIVERBYTE_STRS) ? -driverbyte_table[db] : "invalid"; - - -sdev_printk(KERN_INFO, sdev, -"[%s] Result: hostbyte=%s driverbyte=%s\n", -name, hb_string, db_string); -} - -#else - -void scsi_show_result(struct scsi_device *sdev, const char *name, int result) -{ -sdev_printk(KERN_INFO, sdev, -"[%s] Result: hostbyte=0x%02x driverbyte=0x%02x\n", -name, host_byte(result), driver_byte(result)); -} - -#endif -EXPORT_SYMBOL(scsi_show_result); - -void scsi_print_result(struct scsi_cmnd *cmd) -{ -const char *devname = cmd->request->rq_disk ? -cmd->request->rq_disk->disk_name : "scsi"; -scsi_show_result(cmd->device, devname, cmd->result); -} -EXPORT_SYMBOL(scsi_print_result); diff --git a/drivers/scsi/scsi_trace.c b/drivers/scsi/scsi_trace.c index 2bea4f0..6ffbc40 100644 --- a/drivers/scsi/scsi_trace.c +++ b/drivers/scsi/scsi_trace.c @@ -19,6 +19,8 @@ #include #include +#include + #define SERVICE_ACTION16(cdb) (cdb[1] & 0x1f) #define SERVICE_ACTION32(cdb) ((cdb[8] << 8) | cdb[9]) @@ -286,3 +288,17 @@ scsi_trace_parse_cdb(struct trace_seq *p, unsigned char *cdb, int len) return scsi_trace_misc(p, cdb, len); } } + +void scsi_show_result(struct scsi_device *sdev, const char *name, int result) +{ +trace_scsi_show_result(sdev, name, result); +} +EXPORT_SYMBOL(scsi_show_result); + +void scsi_print_result(struct scsi_cmnd *cmd) +{ +const char *devname = cmd->request->rq_disk ? +cmd->request->rq_disk->disk_name : "scsi"; +scsi_show_result(cmd->device, devname, cmd->result); +} +EXPORT_SYMBOL(scsi_print_result); diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h index 8aecdc2..0675195 100644 --- a/include/trace/events/scsi.h +++ b/include/trace/events/scsi.h @@ -123,7 +123,11 @@ scsi_hostbyte_name(DID_IMM_RETRY),\ scsi_hostbyte_name(DID_REQUEUE),\ scsi_hostbyte_name(DID_TRANSPORT_DISRUPTED),\ -scsi_hostbyte_name(DID_TRANSPORT_FAILFAST)) +scsi_hostbyte_name(DID_TRANSPORT_FAILFAST),\ +scsi_hostbyte_name(DID_TARGET_FAILURE),\ +scsi_hostbyte_name(DID_NEXUS_FAILURE),\ +scsi_hostbyte_name(DID_ALLOC_FAILURE),\ +scsi_hostbyte_name(DID_MEDIUM_ERROR)) #define scsi_driverbyte_name(result){ result, #result } #define show_driverbyte_name(val)\ @@ -359,6 +363,38 @@ TRACE_EVENT(scsi_eh_wakeup, TP_printk("host_no=%u", __entry->host_no) ); +TRACE_EVENT(scsi_show_result, + +TP_PROTO(struct scsi_device *sdev, const char *devname, int result), + +TP_ARGS(sdev, devname, result), + +TP_STRUCT__entry( +__field( unsigned int,host_no) +__field( unsigned int,
Re: [RFC PATCH 07/10] scsi/trace: Use scsi_show_result trace point instead of printk
On 08/08/2014 01:50 PM, Yoshihiro YUNOMAE wrote: Current SCSI trace has hostbyte table and driverbyte table, so we don't need to have the same table in scsi/constants.c. - Result examples (printk) sd 2:0:0:0: [sda] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE (ftrace) scsi_show_result: host_no=2 channel=0 id=0 lun=0 [sda] result=(driver=DRIVER_SENSE host=DID_OK) Signed-off-by: Yoshihiro YUNOMAE Cc: Hannes Reinecke Cc: Doug Gilbert Cc: Martin K. Petersen Cc: Christoph Hellwig Cc: "James E.J. Bottomley" Cc: Hidehiro Kawai Cc: Masami Hiramatsu --- drivers/scsi/constants.c| 52 --- drivers/scsi/scsi_trace.c | 16 + include/trace/events/scsi.h | 38 +++ 3 files changed, 53 insertions(+), 53 deletions(-) diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c index 6fad6b4..f7b7f32 100644 --- a/drivers/scsi/constants.c +++ b/drivers/scsi/constants.c @@ -1488,55 +1488,3 @@ void scsi_print_sense(struct scsi_cmnd *cmd) SCSI_SENSE_BUFFERSIZE); } EXPORT_SYMBOL(scsi_print_sense); - -#ifdef CONFIG_SCSI_CONSTANTS - -static const char * const hostbyte_table[]={ -"DID_OK", "DID_NO_CONNECT", "DID_BUS_BUSY", "DID_TIME_OUT", "DID_BAD_TARGET", -"DID_ABORT", "DID_PARITY", "DID_ERROR", "DID_RESET", "DID_BAD_INTR", -"DID_PASSTHROUGH", "DID_SOFT_ERROR", "DID_IMM_RETRY", "DID_REQUEUE", -"DID_TRANSPORT_DISRUPTED", "DID_TRANSPORT_FAILFAST", "DID_TARGET_FAILURE", -"DID_NEXUS_FAILURE" }; -#define NUM_HOSTBYTE_STRS ARRAY_SIZE(hostbyte_table) - -static const char * const driverbyte_table[]={ -"DRIVER_OK", "DRIVER_BUSY", "DRIVER_SOFT", "DRIVER_MEDIA", "DRIVER_ERROR", -"DRIVER_INVALID", "DRIVER_TIMEOUT", "DRIVER_HARD", "DRIVER_SENSE"}; -#define NUM_DRIVERBYTE_STRS ARRAY_SIZE(driverbyte_table) - -void scsi_show_result(struct scsi_device *sdev, const char *name, int result) -{ - int hb = host_byte(result); - int db = driver_byte(result); - const char *hb_string; - const char *db_string; - - hb_string = (hb < NUM_HOSTBYTE_STRS) ? hostbyte_table[hb] : "invalid"; - db_string = (db < NUM_DRIVERBYTE_STRS) ? - driverbyte_table[db] : "invalid"; - - - sdev_printk(KERN_INFO, sdev, - "[%s] Result: hostbyte=%s driverbyte=%s\n", - name, hb_string, db_string); -} - -#else - -void scsi_show_result(struct scsi_device *sdev, const char *name, int result) -{ - sdev_printk(KERN_INFO, sdev, - "[%s] Result: hostbyte=0x%02x driverbyte=0x%02x\n", - name, host_byte(result), driver_byte(result)); -} - -#endif -EXPORT_SYMBOL(scsi_show_result); - -void scsi_print_result(struct scsi_cmnd *cmd) -{ - const char *devname = cmd->request->rq_disk ? - cmd->request->rq_disk->disk_name : "scsi"; - scsi_show_result(cmd->device, devname, cmd->result); -} -EXPORT_SYMBOL(scsi_print_result); diff --git a/drivers/scsi/scsi_trace.c b/drivers/scsi/scsi_trace.c index 2bea4f0..6ffbc40 100644 --- a/drivers/scsi/scsi_trace.c +++ b/drivers/scsi/scsi_trace.c @@ -19,6 +19,8 @@ #include #include +#include + #define SERVICE_ACTION16(cdb) (cdb[1] & 0x1f) #define SERVICE_ACTION32(cdb) ((cdb[8] << 8) | cdb[9]) @@ -286,3 +288,17 @@ scsi_trace_parse_cdb(struct trace_seq *p, unsigned char *cdb, int len) return scsi_trace_misc(p, cdb, len); } } + +void scsi_show_result(struct scsi_device *sdev, const char *name, int result) +{ + trace_scsi_show_result(sdev, name, result); +} +EXPORT_SYMBOL(scsi_show_result); + +void scsi_print_result(struct scsi_cmnd *cmd) +{ + const char *devname = cmd->request->rq_disk ? + cmd->request->rq_disk->disk_name : "scsi"; + scsi_show_result(cmd->device, devname, cmd->result); +} +EXPORT_SYMBOL(scsi_print_result); diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h index 8aecdc2..0675195 100644 --- a/include/trace/events/scsi.h +++ b/include/trace/events/scsi.h @@ -123,7 +123,11 @@ scsi_hostbyte_name(DID_IMM_RETRY), \ scsi_hostbyte_name(DID_REQUEUE),\ scsi_hostbyte_name(DID_TRANSPORT_DISRUPTED),\ - scsi_hostbyte_name(DID_TRANSPORT_FAILFAST)) + scsi_hostbyte_name(DID_TRANSPORT_FAILFAST), \ + scsi_hostbyte_name(DID_TARGET_FAILURE), \ + scsi_hostbyte_name(DID_NEXUS_FAILURE), \ + scsi_hostbyte_name(DID_ALLOC_FAILURE), \ + scsi_hostbyte_name(DID_MEDIUM_ERROR)) #define scsi_driverbyte_name(result) { result, #result } #define show_driverbyte_name(val) \ @@ -359,6 +363,38 @@ TRACE_EVENT(scsi_eh_wakeup, TP_printk("host_no=%u", __entry->host_no) ); +TRACE_EVENT(scsi_show_result, + + TP_PROTO(struct scsi_device *sdev, const char *devname,
[RFC PATCH 07/10] scsi/trace: Use scsi_show_result trace point instead of printk
Current SCSI trace has hostbyte table and driverbyte table, so we don't need to have the same table in scsi/constants.c. - Result examples (printk) sd 2:0:0:0: [sda] Result: hostbyte=DID_OK driverbyte=DRIVER_SENSE (ftrace) scsi_show_result: host_no=2 channel=0 id=0 lun=0 [sda] result=(driver=DRIVER_SENSE host=DID_OK) Signed-off-by: Yoshihiro YUNOMAE Cc: Hannes Reinecke Cc: Doug Gilbert Cc: Martin K. Petersen Cc: Christoph Hellwig Cc: "James E.J. Bottomley" Cc: Hidehiro Kawai Cc: Masami Hiramatsu --- drivers/scsi/constants.c| 52 --- drivers/scsi/scsi_trace.c | 16 + include/trace/events/scsi.h | 38 +++ 3 files changed, 53 insertions(+), 53 deletions(-) diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c index 6fad6b4..f7b7f32 100644 --- a/drivers/scsi/constants.c +++ b/drivers/scsi/constants.c @@ -1488,55 +1488,3 @@ void scsi_print_sense(struct scsi_cmnd *cmd) SCSI_SENSE_BUFFERSIZE); } EXPORT_SYMBOL(scsi_print_sense); - -#ifdef CONFIG_SCSI_CONSTANTS - -static const char * const hostbyte_table[]={ -"DID_OK", "DID_NO_CONNECT", "DID_BUS_BUSY", "DID_TIME_OUT", "DID_BAD_TARGET", -"DID_ABORT", "DID_PARITY", "DID_ERROR", "DID_RESET", "DID_BAD_INTR", -"DID_PASSTHROUGH", "DID_SOFT_ERROR", "DID_IMM_RETRY", "DID_REQUEUE", -"DID_TRANSPORT_DISRUPTED", "DID_TRANSPORT_FAILFAST", "DID_TARGET_FAILURE", -"DID_NEXUS_FAILURE" }; -#define NUM_HOSTBYTE_STRS ARRAY_SIZE(hostbyte_table) - -static const char * const driverbyte_table[]={ -"DRIVER_OK", "DRIVER_BUSY", "DRIVER_SOFT", "DRIVER_MEDIA", "DRIVER_ERROR", -"DRIVER_INVALID", "DRIVER_TIMEOUT", "DRIVER_HARD", "DRIVER_SENSE"}; -#define NUM_DRIVERBYTE_STRS ARRAY_SIZE(driverbyte_table) - -void scsi_show_result(struct scsi_device *sdev, const char *name, int result) -{ - int hb = host_byte(result); - int db = driver_byte(result); - const char *hb_string; - const char *db_string; - - hb_string = (hb < NUM_HOSTBYTE_STRS) ? hostbyte_table[hb] : "invalid"; - db_string = (db < NUM_DRIVERBYTE_STRS) ? - driverbyte_table[db] : "invalid"; - - - sdev_printk(KERN_INFO, sdev, - "[%s] Result: hostbyte=%s driverbyte=%s\n", - name, hb_string, db_string); -} - -#else - -void scsi_show_result(struct scsi_device *sdev, const char *name, int result) -{ - sdev_printk(KERN_INFO, sdev, - "[%s] Result: hostbyte=0x%02x driverbyte=0x%02x\n", - name, host_byte(result), driver_byte(result)); -} - -#endif -EXPORT_SYMBOL(scsi_show_result); - -void scsi_print_result(struct scsi_cmnd *cmd) -{ - const char *devname = cmd->request->rq_disk ? - cmd->request->rq_disk->disk_name : "scsi"; - scsi_show_result(cmd->device, devname, cmd->result); -} -EXPORT_SYMBOL(scsi_print_result); diff --git a/drivers/scsi/scsi_trace.c b/drivers/scsi/scsi_trace.c index 2bea4f0..6ffbc40 100644 --- a/drivers/scsi/scsi_trace.c +++ b/drivers/scsi/scsi_trace.c @@ -19,6 +19,8 @@ #include #include +#include + #define SERVICE_ACTION16(cdb) (cdb[1] & 0x1f) #define SERVICE_ACTION32(cdb) ((cdb[8] << 8) | cdb[9]) @@ -286,3 +288,17 @@ scsi_trace_parse_cdb(struct trace_seq *p, unsigned char *cdb, int len) return scsi_trace_misc(p, cdb, len); } } + +void scsi_show_result(struct scsi_device *sdev, const char *name, int result) +{ + trace_scsi_show_result(sdev, name, result); +} +EXPORT_SYMBOL(scsi_show_result); + +void scsi_print_result(struct scsi_cmnd *cmd) +{ + const char *devname = cmd->request->rq_disk ? + cmd->request->rq_disk->disk_name : "scsi"; + scsi_show_result(cmd->device, devname, cmd->result); +} +EXPORT_SYMBOL(scsi_print_result); diff --git a/include/trace/events/scsi.h b/include/trace/events/scsi.h index 8aecdc2..0675195 100644 --- a/include/trace/events/scsi.h +++ b/include/trace/events/scsi.h @@ -123,7 +123,11 @@ scsi_hostbyte_name(DID_IMM_RETRY), \ scsi_hostbyte_name(DID_REQUEUE),\ scsi_hostbyte_name(DID_TRANSPORT_DISRUPTED),\ - scsi_hostbyte_name(DID_TRANSPORT_FAILFAST)) + scsi_hostbyte_name(DID_TRANSPORT_FAILFAST), \ + scsi_hostbyte_name(DID_TARGET_FAILURE), \ + scsi_hostbyte_name(DID_NEXUS_FAILURE), \ + scsi_hostbyte_name(DID_ALLOC_FAILURE), \ + scsi_hostbyte_name(DID_MEDIUM_ERROR)) #define scsi_driverbyte_name(result) { result, #result } #define show_driverbyte_name(val) \ @@ -359,6 +363,38 @@ TRACE_EVENT(scsi_eh_wakeup, TP_printk("host_no=%u", __entry->host_no) ); +TRACE_EVENT(scsi_show_result, + + TP_PROTO(struct scsi_device *sdev, const char *devname, int result), + + TP_ARGS(sdev, devname, result), + +