: Save some bytes in struct scsi_target
PATCH 1/1 : Save some bytes in struct scsi_target
Before:
[acme@newtoy kpahole-2.6]$ pahole --cacheline 32 /tmp/scsi.o.before scsi_target /* include/scsi/scsi_device.h:86 */
struct scsi_target {
struct scsi_device * starget_sdev_user; /* 0 4 */
struct list_head siblings; /* 4 8 */
struct list_head devices; /* 12 8 */
struct device dev; /* 20 300 */ /* --- cacheline 10 boundary (320 bytes) --- */
unsigned int reap_ref; /* 320 4 */
unsigned int channel; /* 324 4 */
unsigned int id; /* 328 4 */
unsigned int create:1; /* 332 4 */
/* XXX 31 bits hole, try to pack */
unsigned int pdt_1f_for_no_lun; /* 336 4 */
char scsi_level; /* 340 1 */
/* XXX 3 bytes hole, try to pack */
struct execute_work ew; /* 344 16 */
/* --- cacheline 11 boundary (352 bytes) was 8 bytes ago --- */
enum scsi_target_state state; /* 360 4 */
void * hostdata; /* 364 4 */
long unsigned int starget_data[0]; /* 368 0 */
}; /* size: 368, cachelines: 12 */
/* sum members: 365, holes: 1, sum holes: 3 */
/* bit holes: 1, sum bit holes: 31 bits */
/* last cacheline: 16 bytes */
After:
[acme@newtoy kpahole-2.6]$ pahole --cacheline 32 drivers/scsi/scsi.o scsi_target /* include/scsi/scsi_device.h:86 */
struct scsi_target {
struct scsi_device * starget_sdev_user; /* 0 4 */
struct list_head siblings; /* 4 8 */
struct list_head devices; /* 12 8 */
struct device dev; /* 20 300 */ /* --- cacheline 10 boundary (320 bytes) --- */
unsigned int reap_ref; /* 320 4 */
unsigned int channel; /* 324 4 */
unsigned int id; /* 328 4 */
char scsi_level; /* 332 1 */
unsigned char create:1; /* 333 1 */
/* XXX 7 bits hole, try to pack */
/* XXX 2 bytes hole, try to pack */
unsigned int pdt_1f_for_no_lun; /* 336 4 */
struct execute_work ew; /* 340 16 */
/* --- cacheline 11 boundary (352 bytes) was 4 bytes ago --- */
enum scsi_target_state state; /* 356 4 */
void * hostdata; /* 360 4 */
long unsigned int starget_data[0]; /* 364 0 */
}; /* size: 364, cachelines: 12 */
/* sum members: 362, holes: 1, sum holes: 2 */
/* bit holes: 1, sum bit holes: 7 bits */
/* last cacheline: 12 bytes */
[acme@newtoy kpahole-2.6]$ codiff -V /tmp/scsi.o.before drivers/scsi/scsi.o drivers/scsi/scsi.c:
struct scsi_target | -4
create:1;
from: unsigned int /* 332(31) 4(1) */
to: unsigned char /* 333(7) 1(1) */
scsi_level;
from: char /* 340(0) 1(0) */
to: char /* 332(0) 1(0) */<SNIP offset changes>
1 struct changed
---
scsi_device.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)
include/scsi/scsi_device.h +2 -2
@@ -175,11 +175,11 @@ struct scsi_target {unsigned int channel;unsigned int id;/* target id ... replace * scsi_device.id eventually */ - unsigned int create:1; /* signal that it needs to be added */ +char scsi_level; + unsigned char create:1; /* signal that it needs to be added */unsigned int pdt_1f_for_no_lun;/* PDT = 0x1f */ /* means no lun present */ -char scsi_level;struct execute_work ew;enum scsi_target_state state;void *hostdata;/* available to low-level driver */
Quoted 2 lines · Arnaldo Carvalho de Melo
}; /* size: 368, cachelines: 12 */
}; /* size: 364, cachelines: 12 */
Saving space is always good ;-)
include/scsi/scsi_device.h struct scsi_target { · line 182
Quoted 7 lines
include/scsi/scsi_device.h +2
-2- unsigned int create:1; /* signal that it needs to be added */ +char scsi_level; + unsigned char create:1; /* signal that it needs to be added */unsigned int pdt_1f_for_no_lun;/* PDT = 0x1f */ /* means no lun present */ -char scsi_level;
However, pdt_1f_for_no_lun is really only one bit, saving another 4 bytes.
include/scsi/scsi_device.h struct scsi_target { · line 184
Quoted 2 lines
include/scsi/scsi_device.h +0
-0struct execute_work ew;enum scsi_target_state state;
enums are a bit of a pain. Even though scsi_target_state uses only two values, it's represented as an int. Unless you're on arm-eabi, when it'll use less. And even then, it won't use less than a byte, as it has to be addressable. I wonder if we can turn scsi_target_state into a bit. That'll save another 8 bytes total.
Quoted 16 lines · Matthew Wilcox
Quoted 2 lines · Arnaldo Carvalho de Melo
}; /* size: 368, cachelines: 12 */
}; /* size: 364, cachelines: 12 */Saving space is always good ;-)
Review
Quoted 7 lines
(quoted diff) +2
-2- unsigned int create:1; /* signal that it needs to be added */ + char scsi_level; + unsigned char create:1; /* signal that it needs to be added */ unsigned int pdt_1f_for_no_lun; /* PDT = 0x1f */ /* means no lun present */ - char scsi_level;However,
pdt_1f_for_no_lunis really only one bit, saving another 4 bytes.
Quoted 8 lines
Review
Quoted 2 lines
(quoted diff) +0
-0struct execute_work ew; enum scsi_target_state state;enums are a bit of a pain. Even though
scsi_target_stateuses only two values, it's represented as an int. Unless you're on arm-eabi, when it'll use less. And even then, it won't use less than a byte, as it has to be addressable. I wonder if we can turnscsi_target_stateinto a bit. That'll save another 8 bytes total.
I guess we could use:
enum scsi_target_state state:1;
And make the enum entries start with 0 and not 1 as is today, no? With that we get down to:
}; /* size: 356, cachelines: 12 */
/* last cacheline: 4 bytes */
Anything else to save these 4 bytes and get down to 11 cachelines per scsi_target instance? Following patch is on top of the previous one.
include/scsi/scsi_device.h +3 -3
@@ -157,7 +157,7 @@ #define scmd_printk(prefix, scmd, fmt, a dev_printk(prefix, &(scmd)->device->sdev_gendev, fmt, ##a) enum scsi_target_state { - STARGET_RUNNING = 1, + STARGET_RUNNING = 0, STARGET_DEL, };
@@ -176,12 +176,12 @@ struct scsi_target {unsigned int id;/* target id ... replace * scsi_device.id eventually */ char scsi_level; + enum scsi_target_state state:1; unsigned char create:1; /* signal that it needs to be added */ -unsigned int pdt_1f_for_no_lun;/* PDT = 0x1f */ + unsigned char pdt_1f_for_no_lun:1; /* PDT = 0x1f *//* means no lun present */ struct execute_work ew; -enum scsi_target_state state;void *hostdata;/* available to low-level driver */ unsigned long starget_data[0 ];/* for the transport */ /* starget_data must be the last element!!!! */