Skip to contentSkip to conversation

Save some bytes in scsi_cmnd by rearranging members

PATCH In review Matthew Wilcox 3 participants 5 messages Oct 8, 2007 – Oct 11, 2007 SCSI

lore
Share & tools
lore

Matthew Wilcox

author
#0 lore raw

PATCH 1/1 Save some bytes in scsi_cmnd by rearranging members

Thanks to acme's pahole utility, I found some places where we can save a lot of bytes in scsi_cmnd, just by rearranging struct elements and reducing the size of some elements. We go from 272 to 260 bytes on x86 and from 368 to 344 bytes on x86-64.

- eh_eflags had a 4-byte hole after it on 64-bit. In fact, this has value 0 or 1, so reduce it to an unsigned char, and put it with the other chars in scsi_cmnd.
- sc_data_direction has a value from 0-3, so make it an unsigned char rather than an enum. Saves at least 4 bytes.
- Putting 'tag' with the other char elements saves another 4 bytes
- Moving 'result' up from the end saves another 4 bytes on 64-bit

Signed-off-by Matthew Wilcox <[email protected]>
include/scsi/scsi_cmnd.h +6 -10
index 65ab514..4a8c15f 100644
@@ -15,12 +15,10 @@ struct scsi_device;
 /* embedded in scsi_cmnd */
 struct scsi_pointer {
 	char *ptr;		/* data pointer */
-	int this_residual;	/* left in this buffer */
 	struct scatterlist *buffer;	/* which buffer */
+	dma_addr_t dma_handle;
+	int this_residual;	/* left in this buffer */
 	int buffers_residual;	/* how many buffers left */
-
-        dma_addr_t dma_handle;
-
 	volatile int Status;
 	volatile int Message;
 	volatile int have_data_in;

@@ -32,7 +30,6 @@ struct scsi_cmnd {
 	struct scsi_device *device;
 	struct list_head list;  /* scsi_cmnd participates in queue lists */
 	struct list_head eh_entry; /* entry for the host eh_cmd_q */
-	int eh_eflags;		/* Used by error handlr */
 
 	/*
 	 * A SCSI Command is assigned a nonzero serial_number before passed

@@ -54,9 +51,12 @@ struct scsi_cmnd {
 	int retries;
 	int allowed;
 	int timeout_per_command;
+	int result;		/* Status code from lower level driver */
 
 	unsigned char cmd_len;
-	enum dma_data_direction sc_data_direction;
+	unsigned char eh_eflags;		/* Used by error handler */
+	unsigned char sc_data_direction;
+	unsigned char tag;			/* SCSI-II queued command tag */
 
 	/* These elements define the operation we are about to perform */
 #define MAX_COMMAND_SIZE	16

@@ -109,10 +109,6 @@ struct scsi_cmnd {
 					 * to release this memory.  (The memory
 					 * obtained by scsi_malloc is guaranteed
 					 * to be at an address < 16Mb). */
-
-	int result;		/* Status code from lower level driver */
-
-	unsigned char tag;	/* SCSI-II queued command tag */
 };
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);
Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
#1 lore raw
Quoted 58 lines · Matthew Wilcox

Thanks to acme's pahole utility, I found some places where we can save a lot of bytes in scsi_cmnd, just by rearranging struct elements and reducing the size of some elements. We go from 272 to 260 bytes on x86 and from 368 to 344 bytes on x86-64.

- eh_eflags had a 4-byte hole after it on 64-bit. In fact, this has value 0 or 1, so reduce it to an unsigned char, and put it with the other chars in scsi_cmnd.
- sc_data_direction has a value from 0-3, so make it an unsigned char rather than an enum. Saves at least 4 bytes.
- Putting 'tag' with the other char elements saves another 4 bytes
- Moving 'result' up from the end saves another 4 bytes on 64-bit

Signed-off-by Matthew Wilcox <[email protected]>
include/scsi/scsi_cmnd.h +6 -6
index 65ab514..4a8c15f 100644
@@ -15,12 +15,10 @@ struct scsi_device;
 /* embedded in scsi_cmnd */
 struct scsi_pointer {
 	char *ptr;		/* data pointer */
-	int this_residual;	/* left in this buffer */
 	struct scatterlist *buffer;	/* which buffer */
+	dma_addr_t dma_handle;
+	int this_residual;	/* left in this buffer */
 	int buffers_residual;	/* how many buffers left */
-
-        dma_addr_t dma_handle;
-
 	volatile int Status;
 	volatile int Message;
 	volatile int have_data_in;

@@ -32,7 +30,6 @@ struct scsi_cmnd {
 	struct scsi_device *device;
 	struct list_head list;  /* scsi_cmnd participates in queue lists */
 	struct list_head eh_entry; /* entry for the host eh_cmd_q */
-	int eh_eflags;		/* Used by error handlr */
 
 	/*
 	 * A SCSI Command is assigned a nonzero serial_number before passed

@@ -54,9 +51,12 @@ struct scsi_cmnd {
 	int retries;
 	int allowed;
 	int timeout_per_command;
+	int result;		/* Status code from lower level driver */
 
 	unsigned char cmd_len;
-	enum dma_data_direction sc_data_direction;
+	unsigned char eh_eflags;		/* Used by error handler */
+	unsigned char sc_data_direction;
+	unsigned char tag;			/* SCSI-II queued command tag */
 
 	/* These elements define the operation we are about to perform */
 #define MAX_COMMAND_SIZE	16

include/scsi/scsi_cmnd.h struct scsi_cmnd { · line 114

Quoted 10 lines
include/scsi/scsi_cmnd.h +0 -4
@@ -109,10 +109,6 @@ struct scsi_cmnd {
 					 * to release this memory.  (The memory
 					 * obtained by scsi_malloc is guaranteed
 					 * to be at an address < 16Mb). */
-
-	int result;		/* Status code from lower level driver */
-
-	unsigned char tag;	/* SCSI-II queued command tag */
 };
 
 extern struct scsi_cmnd *scsi_get_command(struct scsi_device *, gfp_t);

Please don't do this patch for now. This is because of the
soon to come scsi_data_buffer patch that rearages most of the members above and puts them in a substructure. Maybe after the scsi_data_buffer patch you can try to do this again.

Thanks
Boaz

author
#2 lore raw
Quoted 4 lines · Boaz Harrosh

Please don't do this patch for now. This is because of the
soon to come scsi_data_buffer patch that rearages most of the members above and puts them in a substructure. Maybe after the scsi_data_buffer patch you can try to do this again.

Need to be careful with sub-structures. They can cause more waste; for example scsi_pointer (on 64-bit) has 4 bytes wasted at the end that can't be filled when it's embedded into the scsi_cmnd.

Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
author
#3 lore raw
In reply to Boaz Harrosh
Quoted 4 lines · Boaz Harrosh

Please don't do this patch for now. This is because of the
soon to come scsi_data_buffer patch that rearages most of the members above and puts them in a substructure. Maybe after the scsi_data_buffer patch you can try to do this again.

BTW, how soon will this patch be coming? If it'll be after 2.6.24, let's at least save some memory for people now.

Signature · 4 lines
Intel are signing my paycheques ... these opinions are still mine
"Bill, look, we understand that you're interested in selling us this
operating system, but compare it to ours.  We can't possibly take such
a retrograde step."
#4 lore raw
Quoted 8 lines · On Tue, 9 Oct 2007 12:06:01 -0600 Matthew Wilcox <[email protected]> wrote:
Quoted 4 lines · Boaz Harrosh

Please don't do this patch for now. This is because of the
soon to come scsi_data_buffer patch that rearages most of the members above and puts them in a substructure. Maybe after the scsi_data_buffer patch you can try to do this again.

BTW, how soon will this patch be coming? If it'll be after 2.6.24, let's at least save some memory for people now.

I wait for Boaz's "Lots of the Accessors patches and !use_sg cleanup" patchset to be merged. After that, I'll update and resend the scsi_data_buffer patchset:

http://marc.info/?l=linux-scsi&m=118911556915876&w=2