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

Started byRasmus Villemoes <linux@rasmusvillemoes.dk>
First post2015-09-30 19:40 +0200
Last post2015-10-05 11:30 +0200
Articles 6 — 2 participants

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

#1236582 — RFC: reduce CONFIG_SCSI_CONSTANTS impact by 4k

FromRasmus Villemoes <linux@rasmusvillemoes.dk>
Date2015-09-30 19:40 +0200
SubjectRFC: reduce CONFIG_SCSI_CONSTANTS impact by 4k
Message-ID<qetwe-3ox-29@gated-at.bofh.it>
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

FromChristoph Hellwig <hch@infradead.org>
Date2015-10-03 19:20 +0200
Message-ID<qfyDw-8hJ-13@gated-at.bofh.it>
In reply to#1236582
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

FromRasmus Villemoes <linux@rasmusvillemoes.dk>
Date2015-10-04 00:20 +0200
Message-ID<qfDjP-6BW-1@gated-at.bofh.it>
In reply to#1238963
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

FromChristoph Hellwig <hch@infradead.org>
Date2015-10-04 10:10 +0200
Message-ID<qfMwN-2Yz-3@gated-at.bofh.it>
In reply to#1239009
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

FromRasmus Villemoes <linux@rasmusvillemoes.dk>
Date2015-10-05 11:30 +0200
Subject[PATCH 0/2] scsi: reduce CONFIG_SCSI_CONSTANTS=y impact by 8k
Message-ID<qgafM-3de-25@gated-at.bofh.it>
In reply to#1239058
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

FromRasmus Villemoes <linux@rasmusvillemoes.dk>
Date2015-10-05 11:30 +0200
Subject[PATCH 2/2] scsi: reduce CONFIG_SCSI_CONSTANTS=y impact by 8k
Message-ID<qgafN-3de-37@gated-at.bofh.it>
In reply to#1239384
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] | [standalone]

Back to top | Article view | linux.kernel

csiph-web