Skip to contentSkip to conversation

srp.h: avoid padding of structs

PATCH In review Arne Redlich 3 participants 6 messages Apr 16, 2006 – Apr 28, 2006 1 message not in the archive

Subsystems (2)
SCSI SCSI RDMA protocol (SRP) initiator
lore
Share & tools
lore

Arne Redlich

author
#0 lore raw

PATCH 1/1 srp.h: avoid padding of structs

Several structs in srp.h are padded by the compiler on some architectures, causing problems e.g. in ib_srp:

				i386	x86_64	(SRP spec)

sizeof(struct indirect_buf) 20 24 (20)
sizeof(struct srp_login_rsp) 52 56 (52)
sizeof(struct srp_rsp) 36 40 (36)

The patch below addresses this issue by packing these three structs, as well as the remaining ones.

Signed-off-by Arne Redlich <[email protected]>
include/scsi/srp.h +12 -25
diff -prauN a/include/scsi/srp.h b/include/scsi/srp.h
@@ -91,18 +91,13 @@ struct srp_direct_buf {
 	__be64	va;
 	__be32	key;
 	__be32  len;
-};
+} __attribute__((packed));
 
-/*
- * We need the packed attribute because the SRP spec puts the list of
- * descriptors at an offset of 20, which is not aligned to the size
- * of struct srp_direct_buf.
- */
 struct srp_indirect_buf {
 	struct srp_direct_buf	table_desc;
 	__be32			len;
 	struct srp_direct_buf	desc_list[0] __attribute__((packed));
-};
+} __attribute__((packed));
 
 enum {
 	SRP_MULTICHAN_SINGLE = 0,

@@ -120,7 +115,7 @@ struct srp_login_req {
 	u8	reserved3[5];
 	u8	initiator_port_id[16];
 	u8	target_port_id[16];
-};
+} __attribute__((packed));
 
 struct srp_login_rsp {
 	u8	opcode;

@@ -132,7 +127,7 @@ struct srp_login_rsp {
 	__be16	buf_fmt;
 	u8	rsp_flags;
 	u8	reserved2[25];
-};
+} __attribute__((packed));
 
 struct srp_login_rej {
 	u8	opcode;

@@ -142,13 +137,13 @@ struct srp_login_rej {
 	u8	reserved2[8];
 	__be16	buf_fmt;
 	u8	reserved3[6];
-};
+} __attribute__((packed));
 
 struct srp_i_logout {
 	u8	opcode;
 	u8	reserved[7];
 	u64	tag;
-};
+} __attribute__((packed));
 
 struct srp_t_logout {
 	u8	opcode;

@@ -156,30 +151,22 @@ struct srp_t_logout {
 	u8	reserved[2];
 	__be32	reason;
 	u64	tag;
-};
+} __attribute__((packed));
 
-/*
- * We need the packed attribute because the SRP spec only aligns the
- * 8-byte LUN field to 4 bytes.
- */
 struct srp_tsk_mgmt {
 	u8	opcode;
 	u8	sol_not;
 	u8	reserved1[6];
 	u64	tag;
 	u8	reserved2[4];
-	__be64	lun __attribute__((packed));
+	__be64	lun;
 	u8	reserved3[2];
 	u8	tsk_mgmt_func;
 	u8	reserved4;
 	u64	task_tag;
 	u8	reserved5[8];
-};
+} __attribute__((packed));
 
-/*
- * We need the packed attribute because the SRP spec only aligns the
- * 8-byte LUN field to 4 bytes.
- */
 struct srp_cmd {
 	u8	opcode;
 	u8	sol_not;

@@ -189,14 +176,14 @@ struct srp_cmd {
 	u8	data_in_desc_cnt;
 	u64	tag;
 	u8	reserved2[4];
-	__be64	lun __attribute__((packed));
+	__be64	lun;
 	u8	reserved3;
 	u8	task_attr;
 	u8	reserved4;
 	u8	add_cdb_len;
 	u8	cdb[16];
 	u8	add_data[0];
-};
+} __attribute__((packed));
 
 enum {
 	SRP_RSP_FLAG_RSPVALID = 1 << 0,

@@ -221,6 +208,6 @@ struct srp_rsp {
 	__be32	sense_data_len;
 	__be32	resp_data_len;
 	u8	data[0];
-};
+} __attribute__((packed));
 
 #endif /* SCSI_SRP_H */
#1 lore raw
Quoted 10 lines · Arne Redlich

Several structs in srp.h are padded by the compiler on some architectures, causing problems e.g. in ib_srp:

				i386	x86_64	(SRP spec)

sizeof(struct indirect_buf) 20 24 (20)
sizeof(struct srp_login_rsp) 52 56 (52)
sizeof(struct srp_rsp) 36 40 (36)

The patch below addresses this issue by packing these three structs, as well as the remaining ones.

Scratch the previous version - I've mistakenly left an old and broken "__attribute__((packed))" in struct srp_indirect_buf. Corrected below.

Signed-off-by Arne Redlich <[email protected]>
include/scsi/srp.h +13 -26
diff -prauN a/include/scsi/srp.h b/include/scsi/srp.h
@@ -91,18 +91,13 @@ struct srp_direct_buf {
 	__be64	va;
 	__be32	key;
 	__be32  len;
-};
+} __attribute__((packed));
 
-/*
- * We need the packed attribute because the SRP spec puts the list of
- * descriptors at an offset of 20, which is not aligned to the size
- * of struct srp_direct_buf.
- */
 struct srp_indirect_buf {
 	struct srp_direct_buf	table_desc;
 	__be32			len;
-	struct srp_direct_buf	desc_list[0] __attribute__((packed));
-};
+	struct srp_direct_buf	desc_list[0];
+} __attribute__((packed));
 
 enum {
 	SRP_MULTICHAN_SINGLE = 0,

@@ -120,7 +115,7 @@ struct srp_login_req {
 	u8	reserved3[5];
 	u8	initiator_port_id[16];
 	u8	target_port_id[16];
-};
+} __attribute__((packed));
 
 struct srp_login_rsp {
 	u8	opcode;

@@ -132,7 +127,7 @@ struct srp_login_rsp {
 	__be16	buf_fmt;
 	u8	rsp_flags;
 	u8	reserved2[25];
-};
+} __attribute__((packed));
 
 struct srp_login_rej {
 	u8	opcode;

@@ -142,13 +137,13 @@ struct srp_login_rej {
 	u8	reserved2[8];
 	__be16	buf_fmt;
 	u8	reserved3[6];
-};
+} __attribute__((packed));
 
 struct srp_i_logout {
 	u8	opcode;
 	u8	reserved[7];
 	u64	tag;
-};
+} __attribute__((packed));
 
 struct srp_t_logout {
 	u8	opcode;

@@ -156,30 +151,22 @@ struct srp_t_logout {
 	u8	reserved[2];
 	__be32	reason;
 	u64	tag;
-};
+} __attribute__((packed));
 
-/*
- * We need the packed attribute because the SRP spec only aligns the
- * 8-byte LUN field to 4 bytes.
- */
 struct srp_tsk_mgmt {
 	u8	opcode;
 	u8	sol_not;
 	u8	reserved1[6];
 	u64	tag;
 	u8	reserved2[4];
-	__be64	lun __attribute__((packed));
+	__be64	lun;
 	u8	reserved3[2];
 	u8	tsk_mgmt_func;
 	u8	reserved4;
 	u64	task_tag;
 	u8	reserved5[8];
-};
+} __attribute__((packed));
 
-/*
- * We need the packed attribute because the SRP spec only aligns the
- * 8-byte LUN field to 4 bytes.
- */
 struct srp_cmd {
 	u8	opcode;
 	u8	sol_not;

@@ -189,14 +176,14 @@ struct srp_cmd {
 	u8	data_in_desc_cnt;
 	u64	tag;
 	u8	reserved2[4];
-	__be64	lun __attribute__((packed));
+	__be64	lun;
 	u8	reserved3;
 	u8	task_attr;
 	u8	reserved4;
 	u8	add_cdb_len;
 	u8	cdb[16];
 	u8	add_data[0];
-};
+} __attribute__((packed));
 
 enum {
 	SRP_RSP_FLAG_RSPVALID = 1 << 0,

@@ -221,6 +208,6 @@ struct srp_rsp {
 	__be32	sense_data_len;
 	__be32	resp_data_len;
 	u8	data[0];
-};
+} __attribute__((packed));
 
 #endif /* SCSI_SRP_H */

A message that is not in the archive (sent off-list or not yet received): 20050823221723.GE31949@krispykreme

#3 lore raw

PATCH 1/1 srp.h: avoid padding of structs

Thanks for pointing out this problem. However I don't think we should pack every structure in the file, because packing structures that don't need it leads to gcc generating horrible code on architectures like ia64. I'd prefer something like the following.

James, can you queue this up? I'd be happy to merge it through my tree but it seems more appropriate for it to go through the SCSI tree. It's probably 2.6.17 material at this point in the cycle.

Thanks,
Roland

Several structs in <scsi/srp.h> get padded to a multiple of 8 bytes on 64-bit architectures and end up with a size that does not match the definition in the SRP spec:

SRP spec 64-bit
sizeof (struct indirect_buf) 20 24
sizeof (struct srp_login_rsp) 52 56
sizeof (struct srp_rsp) 36 40

Fix this by adding __attribute__((packed)) to the offending structs.

Problem pointed out by Arne Redlich <[email protected]>.

Signed-off-by Roland Dreier <[email protected]>
include/scsi/srp.h +17 -6
index 6c2681d..637f77e 100644
@@ -95,14 +95,15 @@ struct srp_direct_buf {
 
 /*
  * We need the packed attribute because the SRP spec puts the list of
- * descriptors at an offset of 20, which is not aligned to the size
- * of struct srp_direct_buf.
+ * descriptors at an offset of 20, which is not aligned to the size of
+ * struct srp_direct_buf.  The whole structure must be packed to avoid
+ * having the 20-byte structure padded to 24 bytes on 64-bit architectures.
  */
 struct srp_indirect_buf {
 	struct srp_direct_buf	table_desc;
 	__be32			len;
-	struct srp_direct_buf	desc_list[0] __attribute__((packed));
-};
+	struct srp_direct_buf	desc_list[0];
+} __attribute__((packed));
 
 enum {
 	SRP_MULTICHAN_SINGLE = 0,

@@ -122,6 +123,11 @@ struct srp_login_req {
 	u8	target_port_id[16];
 };
 
+/*
+ * The SRP spec defines the size of the LOGIN_RSP structure to be 52
+ * bytes, so it needs to be packed to avoid having it padded to 56
+ * bytes on 64-bit architectures.
+ */
 struct srp_login_rsp {
 	u8	opcode;
 	u8	reserved1[3];

@@ -132,7 +138,7 @@ struct srp_login_rsp {
 	__be16	buf_fmt;
 	u8	rsp_flags;
 	u8	reserved2[25];
-};
+} __attribute__((packed));
 
 struct srp_login_rej {
 	u8	opcode;

@@ -207,6 +213,11 @@ enum {
 	SRP_RSP_FLAG_DIUNDER  = 1 << 5
 };
 
+/*
+ * The SRP spec defines the size of the RSP structure to be 36 bytes,
+ * so it needs to be packed to avoid having it padded to 40 bytes on
+ * 64-bit architectures.
+ */
 struct srp_rsp {
 	u8	opcode;
 	u8	sol_not;

@@ -221,6 +232,6 @@ struct srp_rsp {
 	__be32	sense_data_len;
 	__be32	resp_data_len;
 	u8	data[0];
-};
+} __attribute__((packed));
 
 #endif /* SCSI_SRP_H */
#4 lore raw

Am Dienstag, den 18.04.2006, 09:05 -0700 schrieb Roland Dreier:

Quoted 4 lines

Thanks for pointing out this problem. However I don't think we should pack every structure in the file, because packing structures that don't need it leads to gcc generating horrible code on architectures like ia64. I'd prefer something like the following.

Sure - I wasn't aware that gcc will produce different code even if it doesn't have to insert padding bytes.

Thanks,
Arne

Signature · 2 lines
Arne Redlich
Xiranet Communications GmbH
#5 lore raw

Arne> Sure - I wasn't aware that gcc will produce different code Arne> even if it doesn't have to insert padding bytes.

Yes, it gets paranoid about alignment. For example, if you build the following on ia64:

	struct foo { int a; };
	struct bar { int b; } __attribute__((packed));

	int c(struct foo *x) { return x->a; }
	int d(struct bar *x) { return x->b; }

Then c() compiles to:

      0:       13 40 00 40 10 10       [MBB]       ld4 r8=[r32]
      6:       00 00 00 00 10 80                   nop.b 0x0
      c:       08 00 84 00                         br.ret.sptk.many b0;;

and d() compiles to:

     10:       09 70 00 40 00 21       [MMI]       mov r14=r32
     16:       f0 10 80 00 42 00                   adds r15=2,r32
     1c:       34 00 01 84                         adds r32=3,r32;;
     20:       19 80 04 1c 00 14       [MMB]       ld1 r16=[r14],1
     26:       f0 00 3c 00 20 00                   ld1 r15=[r15]
     2c:       00 00 00 20                         nop.b 0x0;;
     30:       09 70 00 1c 00 10       [MMI]       ld1 r14=[r14]
     36:       80 00 80 00 20 e0                   ld1 r8=[r32]
     3c:       f1 78 bd 53                         shl r15=r15,16;;
     40:       01 00 00 00 01 00       [MII]       nop.m 0x0
     46:       e0 70 dc ee 29 00                   shl r14=r14,8
     4c:       81 38 9d 53                         shl r8=r8,24;;
     50:       0b 70 40 1c 0e 20       [MMI]       or r14=r16,r14;;
     56:       f0 70 3c 1c 40 00                   or r15=r14,r15
     5c:       00 00 04 00                         nop.i 0x0;;
     60:       11 00 00 00 01 00       [MIB]       nop.m 0x0
     66:       80 78 20 1c 40 80                   or r8=r15,r8
     6c:       08 00 84 00                         br.ret.sptk.many b0;;

We should use __attribute__((packed)) only when it's really needed.

- R.

#6 lore raw
In reply to Roland Dreier

Am Dienstag, den 18.04.2006, 09:05 -0700 schrieb Roland Dreier:

Quoted 23 lines

Thanks for pointing out this problem. However I don't think we should pack every structure in the file, because packing structures that don't need it leads to gcc generating horrible code on architectures like ia64. I'd prefer something like the following.

James, can you queue this up? I'd be happy to merge it through my tree but it seems more appropriate for it to go through the SCSI tree. It's probably 2.6.17 material at this point in the cycle.

Thanks,
Roland

Several structs in <scsi/srp.h> get padded to a multiple of 8 bytes on 64-bit architectures and end up with a size that does not match the definition in the SRP spec:

SRP spec 64-bit
sizeof (struct indirect_buf) 20 24
sizeof (struct srp_login_rsp) 52 56
sizeof (struct srp_rsp) 36 40

Quoted 5 lines

Fix this by adding __attribute__((packed)) to the offending structs.

Problem pointed out by Arne Redlich <[email protected]>.

Signed-off-by Roland Dreier <[email protected]>

James,

since this one isn't in scsi-rc-fixes-2.6 yet and you didn't comment on it, either, I assume you missed this one:

http://marc.theaimsgroup.com/?l=linux-scsi&m=114537635902712&w=2

Arne

Signature · 2 lines
Arne Redlich
Xiranet Communications GmbH