Re: [PATCH 13/22] scsi: use local buffer for printing the opcode
On 09/01/2014 12:19 AM, Christoph Hellwig wrote: On Thu, Aug 28, 2014 at 07:33:27PM +0200, Hannes Reinecke wrote: SCSI opcode printing is tricky and needs to take into account several different corner cases. So instead of trying to come up with an elaborate printk() statement we should be printing it into a local buffer. scsi_print_command callers are usually deep down the call chain., so I'd rather avoid using up even more stack here. What are the exact reasons that we need the local buffer? I know, and I'm not happy with this patch, either. However, we really, _really_, want to print the decode command name and the CDB in one line ie with one printk() statement. If we don't we'll end up with individual logging messages with one byte per line under high load. Making it impossible to figure out to which CDB these individual bytes are related to. It would be possible to unroll the CDB decoding loop, as we're typically have only 3 formats. But it's near impossible to find some common ground when decoding the command; I've ended up having really convoluted #defines calling into each other, making debugging that code really horrible. You're more than welcome to try... But for now I fear we have to bite the bullet, and ensure the additional stack space it only used when logging is enabled. Cheers, Hannes -- Dr. Hannes Reinecke zSeries Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- 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: [PATCH 13/22] scsi: use local buffer for printing the opcode
On 09/01/2014 10:57 AM, Hannes Reinecke wrote: On 09/01/2014 12:19 AM, Christoph Hellwig wrote: On Thu, Aug 28, 2014 at 07:33:27PM +0200, Hannes Reinecke wrote: SCSI opcode printing is tricky and needs to take into account several different corner cases. So instead of trying to come up with an elaborate printk() statement we should be printing it into a local buffer. scsi_print_command callers are usually deep down the call chain., so I'd rather avoid using up even more stack here. What are the exact reasons that we need the local buffer? I know, and I'm not happy with this patch, either. However, we really, _really_, want to print the decode command name and the CDB in one line ie with one printk() statement. If we don't we'll end up with individual logging messages with one byte per line under high load. Making it impossible to figure out to which CDB these individual bytes are related to. It would be possible to unroll the CDB decoding loop, as we're typically have only 3 formats. But it's near impossible to find some common ground when decoding the command; I've ended up having really convoluted #defines calling into each other, making debugging that code really horrible. Actually, I'm not sure we _can_ avoid increase stack usage when printing the CDB. At the end of the day, we want to print a CDB with one printk() statement. So we'll be having at least 9 arguments to printk per CDB (format, device name, opcode name, and CDB bytes). And as we only have a limited register set available we'll be ending up putting most things on the stack _anyway_, especially for larger CDBs. Hiding that behind an elaborate set of preprocessor definitions and va_format thingies just serves to obfuscate the matter. Hence I'll be voting to make this explicit and use a local buffer and only two or three function arguments. We can optimize it by calling printk() directly for smaller CDB lengths, but with 16-byte CDBs we'll be using probably the stack size as with a local buffer. Cheers, Hannes -- Dr. Hannes Reinecke zSeries Storage h...@suse.de +49 911 74053 688 SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg GF: J. Hawn, J. Guild, F. Imendörffer, HRB 16746 (AG Nürnberg) -- 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: [PATCH 13/22] scsi: use local buffer for printing the opcode
On Thu, Aug 28, 2014 at 07:33:27PM +0200, Hannes Reinecke wrote: SCSI opcode printing is tricky and needs to take into account several different corner cases. So instead of trying to come up with an elaborate printk() statement we should be printing it into a local buffer. scsi_print_command callers are usually deep down the call chain., so I'd rather avoid using up even more stack here. What are the exact reasons that we need the local buffer? -- 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
[PATCH 13/22] scsi: use local buffer for printing the opcode
SCSI opcode printing is tricky and needs to take into account
several different corner cases. So instead of trying to come
up with an elaborate printk() statement we should be printing
it into a local buffer.
Signed-off-by: Hannes Reinecke h...@suse.de
---
drivers/scsi/constants.c | 72 ++--
1 file changed, 45 insertions(+), 27 deletions(-)
diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 813c482..e9c099d 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -295,18 +295,20 @@ static int scsi_opcode_sa_name(int cmd, int
service_action,
}
/* attempt to guess cdb length if cdb_len==0 . No trailing linefeed. */
-static void print_opcode_name(unsigned char * cdbp, int cdb_len)
+static int print_opcode_name(unsigned char * cdbp, int cdb_len,
+char *buf, int buf_len)
{
- int sa, len, cdb0;
- const char * name;
+ int sa, len, cdb0, off;
+ const char * name = NULL;
cdb0 = cdbp[0];
if (cdb0 == VARIABLE_LENGTH_CMD) {
len = scsi_varlen_cdb_length(cdbp);
if (len 10) {
- printk(short variable length command,
- len=%d ext_len=%d, len, cdb_len);
- return;
+ off = scnprintf(buf, buf_len,
+ short variable length command,
+ len=%d ext_len=%d, len, cdb_len);
+ return off;
}
sa = (cdbp[8] 8) + cdbp[9];
} else {
@@ -318,41 +320,51 @@ static void print_opcode_name(unsigned char * cdbp, int
cdb_len)
if (cdb0 0xc0) {
name = cdb_byte0_names[cdb0];
if (name)
- printk(%s, name);
+ off = scnprintf(buf, buf_len, %s, name);
else
- printk(cdb[0]=0x%x (reserved), cdb0);
+ off = scnprintf(buf, buf_len,
+ cdb[0]=0x%x (reserved), cdb0);
} else
- printk(cdb[0]=0x%x (vendor), cdb0);
+ off = scnprintf(buf, buf_len,
+ cdb[0]=0x%x (vendor), cdb0);
} else {
if (name)
- printk(%s, name);
+ off = scnprintf(buf, buf_len, %s, name);
else
- printk(cdb[0]=0x%x, sa=0x%x, cdb0, sa);
+ off = scnprintf(buf, buf_len,
+ cdb[0]=0x%x, sa=0x%x, cdb0, sa);
if ((cdb_len 0) (len != cdb_len))
- printk(, in_cdb_len=%d, ext_len=%d, len, cdb_len);
+ off += scnprintf(buf + off, buf_len - off,
+, in_cdb_len=%d, ext_len=%d,
+len, cdb_len);
}
+ return off;
}
#else /* ifndef CONFIG_SCSI_CONSTANTS */
-static void print_opcode_name(unsigned char * cdbp, int cdb_len)
+static int print_opcode_name(unsigned char * cdbp, int cdb_len,
+char *buf, int buf_len)
{
- int sa, len, cdb0;
+ int sa, len, cdb0, off;
cdb0 = cdbp[0];
switch(cdb0) {
case VARIABLE_LENGTH_CMD:
len = scsi_varlen_cdb_length(cdbp);
if (len 10) {
- printk(short opcode=0x%x command, len=%d
- ext_len=%d, cdb0, len, cdb_len);
+ off = scnprintf(buf, buf_len,
+ short opcode=0x%x command, len=%d
+ ext_len=%d, cdb0, len, cdb_len);
break;
}
sa = (cdbp[8] 8) + cdbp[9];
- printk(cdb[0]=0x%x, sa=0x%x, cdb0, sa);
+ off = scnprintf(buf, buf_len, cdb[0]=0x%x, sa=0x%x, cdb0, sa);
if (len != cdb_len)
- printk(, in_cdb_len=%d, ext_len=%d, len, cdb_len);
+ off += scnprintf(buf + off, buflen - off,
+, in_cdb_len=%d, ext_len=%d,
+len, cdb_len);
break;
case MAINTENANCE_IN:
case MAINTENANCE_OUT:
@@ -366,23 +378,29 @@ static void print_opcode_name(unsigned char * cdbp, int
cdb_len)
case THIRD_PARTY_COPY_IN:
case THIRD_PARTY_COPY_OUT:
sa = cdbp[1] 0x1f;
- printk(cdb[0]=0x%x, sa=0x%x, cdb0, sa);
+ off = scnprintf(buf + off, buflen - off,
+ cdb[0]=0x%x, sa=0x%x, cdb0, sa);
break;
default:
if (cdb0 0xc0)
-
