Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1236582 > unrolled thread

RFC: reduce CONFIG_SCSI_CONSTANTS impact by 4k

Back to article view | Back to linux.kernel

Contents

  RFC: reduce CONFIG_SCSI_CONSTANTS impact by 4k Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-09-30 19:40 +0200
    Re: RFC: reduce CONFIG_SCSI_CONSTANTS impact by 4k Christoph Hellwig <hch@infradead.org> - 2015-10-03 19:20 +0200
      Re: RFC: reduce CONFIG_SCSI_CONSTANTS impact by 4k Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-04 00:20 +0200
        Re: RFC: reduce CONFIG_SCSI_CONSTANTS impact by 4k Christoph Hellwig <hch@infradead.org> - 2015-10-04 10:10 +0200
          [PATCH 0/2] scsi: reduce CONFIG_SCSI_CONSTANTS=y impact by 8k Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-05 11:30 +0200
            [PATCH 2/2] scsi: reduce CONFIG_SCSI_CONSTANTS=y impact by 8k Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-05 11:30 +0200
              Re: [PATCH 2/2] scsi: reduce CONFIG_SCSI_CONSTANTS=y impact by 8k Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-06 17:40 +0200
            Re: [PATCH 1/2] scsi: move Additional Sense Codes to separate file Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-06 17:30 +0200
              Re: [PATCH 1/2] scsi: move Additional Sense Codes to separate file Rasmus Villemoes <rv@rasmusvillemoes.dk> - 2015-10-18 00:20 +0200
        Re: RFC: reduce CONFIG_SCSI_CONSTANTS impact by 4k Julian Calaby <julian.calaby@gmail.com> - 2015-10-06 03:40 +0200
          Re: RFC: reduce CONFIG_SCSI_CONSTANTS impact by 4k Rasmus Villemoes <linux@rasmusvillemoes.dk> - 2015-10-06 17:40 +0200
            Re: RFC: reduce CONFIG_SCSI_CONSTANTS impact by 4k Julian Calaby <julian.calaby@gmail.com> - 2015-10-07 01:10 +0200

#1236582 — RFC: reduce CONFIG_SCSI_CONSTANTS impact by 4k

struct error_info has 6 bytes of padding on x86_64 (and I assume also
other 64 bit platforms). This currently amounts to about 4k of wasted
space (and presumably a third of that on 32 bit).

We can avoid that by keeping the codes and the strings in separate
arrays. Keeping those in sync should be easy enough if we use the
standard trick of including a table twice.

Patch 2/2 would be something like below; 1/2 is a 1600 line purely
mechanical thing which I won't spam the lists with unless someone else
thinks this is a good idea.

What do you think?

Subject: [PATCH 2/2] scsi: split additional[] array in two

struct error_info has 6 bytes of padding (on 64 bit platforms), which
amounts to over 4K of wasted space in the additional[]
array. Splitting it in two avoids that waste. A BUILD_BUG_ON and the
fact that the two arrays are generated from the same include file
should keep these two arrays in sync.

$ scripts/bloat-o-meter /tmp/vmlinux vmlinux
add/remove: 2/1 grow/shrink: 1/0 up/down: 7073/-11312 (-4239)

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/scsi/constants.c | 21 +++++++++++----------
 1 file changed, 11 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 14d5069ca3ff..03f28cdcbc31 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -290,19 +290,19 @@ bool scsi_opcode_sa_name(int opcode, int service_action,
 	return true;
 }
 
-struct error_info {
-	unsigned short code12;	/* 0x0302 looks better than 0x03,0x02 */
-	const char * text;
+static unsigned short additional_code12[] = {
+#define SENSE_CODE(c, s) c
+#include "sense_codes.h"
+#undef SENSE_CODE
 };
 
-
-static const struct error_info additional[] =
-{
-#define SENSE_CODE(c, s) {c, s}
+static const char *additional_text[] = {
+#define SENSE_CODE(c, s) s
 #include "sense_codes.h"
 #undef SENSE_CODE
 };
 
+
 struct error_info2 {
 	unsigned char code1, code2_min, code2_max;
 	const char * str;
@@ -364,11 +364,12 @@ scsi_extd_sense_format(unsigned char asc, unsigned char ascq, const char **fmt)
 {
 	int i;
 	unsigned short code = ((asc << 8) | ascq);
+	BUILD_BUG_ON(ARRAY_SIZE(additional_code12) != ARRAY_SIZE(additional_text));
 
 	*fmt = NULL;
-	for (i = 0; additional[i].text; i++)
-		if (additional[i].code12 == code)
-			return additional[i].text;
+	for (i = 0; additional_text[i]; i++)
+		if (additional_code12[i] == code)
+			return additional_text[i];
 	for (i = 0; additional2[i].fmt; i++) {
 		if (additional2[i].code1 == asc &&
 		    ascq >= additional2[i].code2_min &&
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1238963

Hi Rasmus,

I like this idea.  But maybe it's also time to just move the constants
to a plain text file and auto-generate C headers from them?  That way
the format in which they can be edited is decoupled from the
representation in the kernel image.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239009

On Sat, Oct 03 2015, Christoph Hellwig <hch@infradead.org> wrote:

> Hi Rasmus,
>
> I like this idea.  But maybe it's also time to just move the constants
> to a plain text file and auto-generate C headers from them?  That way
> the format in which they can be edited is decoupled from the
> representation in the kernel image.

Well, I don't really have an opinion on that part.

In the meantime, I got another idea for doubling the saving to 8k. It
requires a few more code changes and is perhaps also more hacky. 2/2
would be something like below. Please let me know which version you'd
prefer, and I'll send both patches properly.

Thanks,
Rasmus

Subject: [PATCH 2/2] scsi: reduce CONFIG_SCSI_CONSTANTS=y impact by 8k

On 64 bit, struct error_info has 6 bytes of padding, which amounts to
over 4k of wasted space in the additional[] array. We could easily get
rid of that by instead using separate arrays for the codes and the
pointers. However, we can do even better than that and save an
additional 6 bytes per entry: In the table, just store the sizeof()
the corresponding string literal. The cumulative sum of these is then
the appropriate offset into additional_text, which is built from the
concatenation (with '\0's inbetween) of the strings.

$ scripts/bloat-o-meter /tmp/vmlinux vmlinux
add/remove: 0/0 grow/shrink: 1/1 up/down: 24/-8488 (-8464)
function                                     old     new   delta
scsi_extd_sense_format                       136     160     +24
additional                                 11312    2824   -8488

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/scsi/constants.c   | 25 +++++++++++++++++++++----
 drivers/scsi/sense_codes.h |  2 --
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 47aaccd5e68e..ccd34b0481cd 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -292,17 +292,31 @@ bool scsi_opcode_sa_name(int opcode, int service_action,
 
 struct error_info {
 	unsigned short code12;	/* 0x0302 looks better than 0x03,0x02 */
-	const char * text;
+	unsigned short size;
 };
 
 
+/*
+ * There are 700+ entries in this table. To save space, we don't store
+ * (code, pointer) pairs, which would make sizeof(struct
+ * error_info)==16 on 64 bits. Rather, the second element just stores
+ * the size (including \0) of the corresponding string, and we use the
+ * sum of these to get the appropriate offset into additional_text
+ * defined below. This approach saves 12 bytes per entry.
+ */
 static const struct error_info additional[] =
 {
-#define SENSE_CODE(c, s) {c, s},
+#define SENSE_CODE(c, s) {c, sizeof(s)},
 #include "sense_codes.h"
 #undef SENSE_CODE
 };
 
+static const char *additional_text =
+#define SENSE_CODE(c, s) s "\0"
+#include "sense_codes.h"
+#undef SENSE_CODE
+	;
+
 struct error_info2 {
 	unsigned char code1, code2_min, code2_max;
 	const char * str;
@@ -364,11 +378,14 @@ scsi_extd_sense_format(unsigned char asc, unsigned char ascq, const char **fmt)
 {
 	int i;
 	unsigned short code = ((asc << 8) | ascq);
+	unsigned offset = 0;
 
 	*fmt = NULL;
-	for (i = 0; additional[i].text; i++)
+	for (i = 0; i < ARRAY_SIZE(additional); i++) {
 		if (additional[i].code12 == code)
-			return additional[i].text;
+			return additional_text + offset;
+		offset += additional[i].size;
+	}
 	for (i = 0; additional2[i].fmt; i++) {
 		if (additional2[i].code1 == asc &&
 		    ascq >= additional2[i].code2_min &&
diff --git a/drivers/scsi/sense_codes.h b/drivers/scsi/sense_codes.h
index 54b3939d6309..da84d53b3379 100644
--- a/drivers/scsi/sense_codes.h
+++ b/drivers/scsi/sense_codes.h
@@ -833,5 +833,3 @@ SENSE_CODE(0x746E, "External data encryption control timeout")
 SENSE_CODE(0x746F, "External data encryption control error")
 SENSE_CODE(0x7471, "Logical unit access not authorized")
 SENSE_CODE(0x7479, "Security conflict in translated device")
-
-SENSE_CODE(0, NULL)
-- 
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239058

On Sun, Oct 04, 2015 at 12:09:58AM +0200, Rasmus Villemoes wrote:
> On Sat, Oct 03 2015, Christoph Hellwig <hch@infradead.org> wrote:
> 
> > Hi Rasmus,
> >
> > I like this idea.  But maybe it's also time to just move the constants
> > to a plain text file and auto-generate C headers from them?  That way
> > the format in which they can be edited is decoupled from the
> > representation in the kernel image.
> 
> Well, I don't really have an opinion on that part.
> 
> In the meantime, I got another idea for doubling the saving to 8k. It
> requires a few more code changes and is perhaps also more hacky. 2/2
> would be something like below. Please let me know which version you'd
> prefer, and I'll send both patches properly.

I don't think the new version is too bad, and the saving is impressive.
So I'd opt for the new one.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239384 — [PATCH 0/2] scsi: reduce CONFIG_SCSI_CONSTANTS=y impact by 8k

This reduces the impact of choosing CONFIG_SCSI_CONSTANTS by about 8KB.

2dd951ecd511 ("scsi: Conditionally compile in constants.c") updated
the Kconfig help text from 12KB to 75KB. The 12K predated git so was
certainly outdated. But I'm not sure where the 75K comes from; using
size(1) on a defconfig (with/without this config option) vmlinux shows
a difference of about 47K, and 39K after these patches are applied. In
any case, I've left the Kconfig text alone, since I'm not sure I'm
counting the same way the 75K was computed (I'm fairly certain of the
8K delta, however).

Tested with a trivial module calling scsi_extd_sense_format with a few
random known codes and comparing the result to the expected value.

Rasmus Villemoes (2):
  scsi: move Additional Sense Codes to separate file
  scsi: reduce CONFIG_SCSI_CONSTANTS=y impact by 8k

 drivers/scsi/constants.c   | 860 ++-------------------------------------------
 drivers/scsi/sense_codes.h | 835 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 857 insertions(+), 838 deletions(-)
 create mode 100644 drivers/scsi/sense_codes.h

-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1239385 — [PATCH 2/2] scsi: reduce CONFIG_SCSI_CONSTANTS=y impact by 8k

On 64 bit, struct error_info has 6 bytes of padding, which amounts to
over 4k of wasted space in the additional[] array. We could easily get
rid of that by instead using separate arrays for the codes and the
pointers. However, we can do even better than that and save an
additional 6 bytes per entry: In the table, just store the sizeof()
the corresponding string literal. The cumulative sum of these is then
the appropriate offset into additional_text, which is built from the
concatenation (with '\0's inbetween) of the strings.

$ scripts/bloat-o-meter /tmp/vmlinux vmlinux
add/remove: 0/0 grow/shrink: 1/1 up/down: 24/-8488 (-8464)
function                                     old     new   delta
scsi_extd_sense_format                       136     160     +24
additional                                 11312    2824   -8488

Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
---
 drivers/scsi/constants.c   | 25 +++++++++++++++++++++----
 drivers/scsi/sense_codes.h |  2 --
 2 files changed, 21 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
index 47aaccd5e68e..ccd34b0481cd 100644
--- a/drivers/scsi/constants.c
+++ b/drivers/scsi/constants.c
@@ -292,17 +292,31 @@ bool scsi_opcode_sa_name(int opcode, int service_action,
 
 struct error_info {
 	unsigned short code12;	/* 0x0302 looks better than 0x03,0x02 */
-	const char * text;
+	unsigned short size;
 };
 
 
+/*
+ * There are 700+ entries in this table. To save space, we don't store
+ * (code, pointer) pairs, which would make sizeof(struct
+ * error_info)==16 on 64 bits. Rather, the second element just stores
+ * the size (including \0) of the corresponding string, and we use the
+ * sum of these to get the appropriate offset into additional_text
+ * defined below. This approach saves 12 bytes per entry.
+ */
 static const struct error_info additional[] =
 {
-#define SENSE_CODE(c, s) {c, s},
+#define SENSE_CODE(c, s) {c, sizeof(s)},
 #include "sense_codes.h"
 #undef SENSE_CODE
 };
 
+static const char *additional_text =
+#define SENSE_CODE(c, s) s "\0"
+#include "sense_codes.h"
+#undef SENSE_CODE
+	;
+
 struct error_info2 {
 	unsigned char code1, code2_min, code2_max;
 	const char * str;
@@ -364,11 +378,14 @@ scsi_extd_sense_format(unsigned char asc, unsigned char ascq, const char **fmt)
 {
 	int i;
 	unsigned short code = ((asc << 8) | ascq);
+	unsigned offset = 0;
 
 	*fmt = NULL;
-	for (i = 0; additional[i].text; i++)
+	for (i = 0; i < ARRAY_SIZE(additional); i++) {
 		if (additional[i].code12 == code)
-			return additional[i].text;
+			return additional_text + offset;
+		offset += additional[i].size;
+	}
 	for (i = 0; additional2[i].fmt; i++) {
 		if (additional2[i].code1 == asc &&
 		    ascq >= additional2[i].code2_min &&
diff --git a/drivers/scsi/sense_codes.h b/drivers/scsi/sense_codes.h
index 54b3939d6309..da84d53b3379 100644
--- a/drivers/scsi/sense_codes.h
+++ b/drivers/scsi/sense_codes.h
@@ -833,5 +833,3 @@ SENSE_CODE(0x746E, "External data encryption control timeout")
 SENSE_CODE(0x746F, "External data encryption control error")
 SENSE_CODE(0x7471, "Logical unit access not authorized")
 SENSE_CODE(0x7479, "Security conflict in translated device")
-
-SENSE_CODE(0, NULL)
-- 
2.1.3

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240603 — Re: [PATCH 2/2] scsi: reduce CONFIG_SCSI_CONSTANTS=y impact by 8k

On Mon, Oct 05 2015, Bart Van Assche <bart.vanassche@sandisk.com> wrote:

> On 10/05/15 02:26, Rasmus Villemoes wrote:
>>   struct error_info {
>>   	unsigned short code12;	/* 0x0302 looks better than 0x03,0x02 */
>> -	const char * text;
>> +	unsigned short size;
>>   };
>
> Had you considered to use the type uint16_t instead of unsigned short ?
>

Yes, but I thought I'd keep it consistent with the other member. AFAIK,
they're one and the same on all relevant arches. I actually think
spelling it u16 for both members would make sense (for the code because
it explicitly is meant to hold two bytes), but again I think that's
better done as a trivial follow-up patch, if we really want to change this.

Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240561 — Re: [PATCH 1/2] scsi: move Additional Sense Codes to separate file

On Mon, Oct 05 2015, Bart Van Assche <bart.vanassche@sandisk.com> wrote:

> On 10/05/15 02:26, Rasmus Villemoes wrote:
>> -	{0x041A, "Logical unit not ready, start stop unit command in "
>> -	 "progress"},
>> -	{0x041B, "Logical unit not ready, sanitize in progress"},
>> -	{0x041C, "Logical unit not ready, additional power use not yet "
>> -	 "granted"},
>
> Please convert these multi-line strings into single line string
> constants such that users can look up these easily with grep.

I can certainly do that, but I'd prefer to do it in a separate patch. I
really want to keep this as mechanical as possible.

>> +
>> +SENSE_CODE(0, NULL)
>
> The above looks confusing to me. Please leave this out and add { 0,
> NULL } at the end of the additional[] array instead.

OK, I agree that that would have been cleaner. However, the sentinel
entry is removed in 2/2 (since we loop using ARRAY_SIZE instead of
checking for the sentinel). Let me know if you want me to resend the
1600 line monster anyway with this addressed.

Thanks,
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1249477 — Re: [PATCH 1/2] scsi: move Additional Sense Codes to separate file

On Tue, Oct 06 2015, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:

> On Mon, Oct 05 2015, Bart Van Assche <bart.vanassche@sandisk.com> wrote:
>
>> On 10/05/15 02:26, Rasmus Villemoes wrote:
>>> -	{0x041A, "Logical unit not ready, start stop unit command in "
>>> -	 "progress"},
>>> -	{0x041B, "Logical unit not ready, sanitize in progress"},
>>> -	{0x041C, "Logical unit not ready, additional power use not yet "
>>> -	 "granted"},
>>
>> Please convert these multi-line strings into single line string
>> constants such that users can look up these easily with grep.
>
> I can certainly do that, but I'd prefer to do it in a separate patch. I
> really want to keep this as mechanical as possible.
>
>>> +
>>> +SENSE_CODE(0, NULL)
>>
>> The above looks confusing to me. Please leave this out and add { 0,
>> NULL } at the end of the additional[] array instead.
>
> OK, I agree that that would have been cleaner. However, the sentinel
> entry is removed in 2/2 (since we loop using ARRAY_SIZE instead of
> checking for the sentinel). Let me know if you want me to resend the
> 1600 line monster anyway with this addressed.

Should I resend, or can these be picked up as-is? I can easily send a
3/2 making the strings more greppable.

Thanks,
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240106

Hi Rasmus,

On Sun, Oct 4, 2015 at 9:09 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> Subject: [PATCH 2/2] scsi: reduce CONFIG_SCSI_CONSTANTS=y impact by 8k
>
> On 64 bit, struct error_info has 6 bytes of padding, which amounts to
> over 4k of wasted space in the additional[] array. We could easily get
> rid of that by instead using separate arrays for the codes and the
> pointers. However, we can do even better than that and save an
> additional 6 bytes per entry: In the table, just store the sizeof()
> the corresponding string literal. The cumulative sum of these is then
> the appropriate offset into additional_text, which is built from the
> concatenation (with '\0's inbetween) of the strings.
>
> $ scripts/bloat-o-meter /tmp/vmlinux vmlinux
> add/remove: 0/0 grow/shrink: 1/1 up/down: 24/-8488 (-8464)
> function                                     old     new   delta
> scsi_extd_sense_format                       136     160     +24
> additional                                 11312    2824   -8488

Quick question:

> Signed-off-by: Rasmus Villemoes <linux@rasmusvillemoes.dk>
> ---
>  drivers/scsi/constants.c   | 25 +++++++++++++++++++++----
>  drivers/scsi/sense_codes.h |  2 --
>  2 files changed, 21 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
> index 47aaccd5e68e..ccd34b0481cd 100644
> --- a/drivers/scsi/constants.c
> +++ b/drivers/scsi/constants.c
> @@ -292,17 +292,31 @@ bool scsi_opcode_sa_name(int opcode, int service_action,
>
>  struct error_info {
>         unsigned short code12;  /* 0x0302 looks better than 0x03,0x02 */
> -       const char * text;
> +       unsigned short size;
>  };
>
>
> +/*
> + * There are 700+ entries in this table. To save space, we don't store
> + * (code, pointer) pairs, which would make sizeof(struct
> + * error_info)==16 on 64 bits. Rather, the second element just stores
> + * the size (including \0) of the corresponding string, and we use the
> + * sum of these to get the appropriate offset into additional_text
> + * defined below. This approach saves 12 bytes per entry.
> + */
>  static const struct error_info additional[] =
>  {
> -#define SENSE_CODE(c, s) {c, s},
> +#define SENSE_CODE(c, s) {c, sizeof(s)},
>  #include "sense_codes.h"
>  #undef SENSE_CODE
>  };
>
> +static const char *additional_text =
> +#define SENSE_CODE(c, s) s "\0"
> +#include "sense_codes.h"
> +#undef SENSE_CODE
> +       ;
> +
>  struct error_info2 {
>         unsigned char code1, code2_min, code2_max;
>         const char * str;
> @@ -364,11 +378,14 @@ scsi_extd_sense_format(unsigned char asc, unsigned char ascq, const char **fmt)
>  {
>         int i;
>         unsigned short code = ((asc << 8) | ascq);
> +       unsigned offset = 0;
>
>         *fmt = NULL;
> -       for (i = 0; additional[i].text; i++)
> +       for (i = 0; i < ARRAY_SIZE(additional); i++) {
>                 if (additional[i].code12 == code)
> -                       return additional[i].text;
> +                       return additional_text + offset;
> +               offset += additional[i].size;

You don't seem to be accounting for the null bytes here.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1240573

On Tue, Oct 06 2015, Julian Calaby <julian.calaby@gmail.com> wrote:

> Hi Rasmus,
>
>>
>> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
>> index 47aaccd5e68e..ccd34b0481cd 100644
>> --- a/drivers/scsi/constants.c
>> +++ b/drivers/scsi/constants.c
>> @@ -292,17 +292,31 @@ bool scsi_opcode_sa_name(int opcode, int service_action,
>>
>>  struct error_info {
>>         unsigned short code12;  /* 0x0302 looks better than 0x03,0x02 */
>> -       const char * text;
>> +       unsigned short size;
>>  };
>>
>>
>> +/*
>> + * There are 700+ entries in this table. To save space, we don't store
>> + * (code, pointer) pairs, which would make sizeof(struct
>> + * error_info)==16 on 64 bits. Rather, the second element just stores
>> + * the size (including \0) of the corresponding string, and we use the
>> + * sum of these to get the appropriate offset into additional_text
>> + * defined below. This approach saves 12 bytes per entry.
>> + */
>>  static const struct error_info additional[] =
>>  {
>> -#define SENSE_CODE(c, s) {c, s},
>> +#define SENSE_CODE(c, s) {c, sizeof(s)},
>>  #include "sense_codes.h"
>>  #undef SENSE_CODE
>>  };
>>
>> +static const char *additional_text =
>> +#define SENSE_CODE(c, s) s "\0"
>> +#include "sense_codes.h"
>> +#undef SENSE_CODE
>> +       ;
>> +
>>  struct error_info2 {
>>         unsigned char code1, code2_min, code2_max;
>>         const char * str;
>> @@ -364,11 +378,14 @@ scsi_extd_sense_format(unsigned char asc, unsigned char ascq, const char **fmt)
>>  {
>>         int i;
>>         unsigned short code = ((asc << 8) | ascq);
>> +       unsigned offset = 0;
>>
>>         *fmt = NULL;
>> -       for (i = 0; additional[i].text; i++)
>> +       for (i = 0; i < ARRAY_SIZE(additional); i++) {
>>                 if (additional[i].code12 == code)
>> -                       return additional[i].text;
>> +                       return additional_text + offset;
>> +               offset += additional[i].size;
>
> You don't seem to be accounting for the null bytes here.

Well, no, I account for the nul bytes where I define the table (the
comment actually says as much). sizeof("foo") is 4. Since
additional_text ends up pointing to a string containing

"foo" "\0" "xyzzy" "\0" "..." "\0"

aka

"foo\0xyzzy\0...\0"

this is the right amount to skip. As I said in the cover letter, I did
test this (so that I'd at least catch silly off-by-ones), and I do get
the right strings out.

Thanks,
Rasmus
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1241073

Hi Rasmus,

On Wed, Oct 7, 2015 at 2:39 AM, Rasmus Villemoes
<linux@rasmusvillemoes.dk> wrote:
> On Tue, Oct 06 2015, Julian Calaby <julian.calaby@gmail.com> wrote:
>
>> Hi Rasmus,
>>
>>>
>>> diff --git a/drivers/scsi/constants.c b/drivers/scsi/constants.c
>>> index 47aaccd5e68e..ccd34b0481cd 100644
>>> --- a/drivers/scsi/constants.c
>>> +++ b/drivers/scsi/constants.c
>>> @@ -292,17 +292,31 @@ bool scsi_opcode_sa_name(int opcode, int service_action,
>>>
>>>  struct error_info {
>>>         unsigned short code12;  /* 0x0302 looks better than 0x03,0x02 */
>>> -       const char * text;
>>> +       unsigned short size;
>>>  };
>>>
>>>
>>> +/*
>>> + * There are 700+ entries in this table. To save space, we don't store
>>> + * (code, pointer) pairs, which would make sizeof(struct
>>> + * error_info)==16 on 64 bits. Rather, the second element just stores
>>> + * the size (including \0) of the corresponding string, and we use the
>>> + * sum of these to get the appropriate offset into additional_text
>>> + * defined below. This approach saves 12 bytes per entry.
>>> + */
>>>  static const struct error_info additional[] =
>>>  {
>>> -#define SENSE_CODE(c, s) {c, s},
>>> +#define SENSE_CODE(c, s) {c, sizeof(s)},
>>>  #include "sense_codes.h"
>>>  #undef SENSE_CODE
>>>  };
>>>
>>> +static const char *additional_text =
>>> +#define SENSE_CODE(c, s) s "\0"
>>> +#include "sense_codes.h"
>>> +#undef SENSE_CODE
>>> +       ;
>>> +
>>>  struct error_info2 {
>>>         unsigned char code1, code2_min, code2_max;
>>>         const char * str;
>>> @@ -364,11 +378,14 @@ scsi_extd_sense_format(unsigned char asc, unsigned char ascq, const char **fmt)
>>>  {
>>>         int i;
>>>         unsigned short code = ((asc << 8) | ascq);
>>> +       unsigned offset = 0;
>>>
>>>         *fmt = NULL;
>>> -       for (i = 0; additional[i].text; i++)
>>> +       for (i = 0; i < ARRAY_SIZE(additional); i++) {
>>>                 if (additional[i].code12 == code)
>>> -                       return additional[i].text;
>>> +                       return additional_text + offset;
>>> +               offset += additional[i].size;
>>
>> You don't seem to be accounting for the null bytes here.
>
> Well, no, I account for the nul bytes where I define the table (the
> comment actually says as much). sizeof("foo") is 4. Since
> additional_text ends up pointing to a string containing
>
> "foo" "\0" "xyzzy" "\0" "..." "\0"
>
> aka
>
> "foo\0xyzzy\0...\0"
>
> this is the right amount to skip. As I said in the cover letter, I did
> test this (so that I'd at least catch silly off-by-ones), and I do get
> the right strings out.

Ah, that makes sense. It just didn't look right.

Thanks,

-- 
Julian Calaby

Email: julian.calaby@gmail.com
Profile: http://www.google.com/profiles/julian.calaby/
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web