Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]
Groups > linux.kernel > #1240106 > unrolled thread
| Started by | Julian Calaby <julian.calaby@gmail.com> |
|---|---|
| First post | 2015-10-06 03:40 +0200 |
| Last post | 2015-10-07 01:10 +0200 |
| Articles | 3 — 2 participants |
Back to article view | Back to linux.kernel
This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by
below is the oldest one visible, not the original post.
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
| From | Julian Calaby <julian.calaby@gmail.com> |
|---|---|
| Date | 2015-10-06 03:40 +0200 |
| Subject | Re: RFC: reduce CONFIG_SCSI_CONSTANTS impact by 4k |
| Message-ID | <qgpou-8an-3@gated-at.bofh.it> |
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] | [next] | [standalone]
| From | Rasmus Villemoes <linux@rasmusvillemoes.dk> |
|---|---|
| Date | 2015-10-06 17:40 +0200 |
| Message-ID | <qgCvn-1OO-5@gated-at.bofh.it> |
| In reply to | #1240106 |
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]
| From | Julian Calaby <julian.calaby@gmail.com> |
|---|---|
| Date | 2015-10-07 01:10 +0200 |
| Message-ID | <qgJwS-3HK-21@gated-at.bofh.it> |
| In reply to | #1240573 |
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