Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed
Hi James, On 11/10/12 15:10, James Hogan wrote: > On 11/10/12 13:58, James Bottomley wrote: >> On Thu, 2012-10-11 at 12:32 +0100, James Hogan wrote: >>> On 11/10/12 11:24, James Bottomley wrote: On Thu, 2012-10-11 at 10:15 +0100, James Hogan wrote: > The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when > used in struct osd_cdb_head, but it isn't marked as packed. Some > architectures will round the struct size up which triggers BUILD_BUG_ON > compile errors in osd_initiator.c when the outer structs are unexpected > sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed. What actual problem have you encountered? The structure is {u8[8], u16} which is naturally packed on every architecture I know about. I've even built osd_initiator without problem on parisc, which has some of the most rigid alignment rules I've seen. >>> >>> Hi James, >>> >>> The alignment is fine (the offset of the u16 is 8 bytes), but >>> unfortunately with the metag port of gcc, sizeof(struct >>> scsi_varlen_cdb_hdr) is rounded up to a 4 byte boundary (even though the >>> largest data member alignment is only 2 bytes), which is 12 bytes >>> instead of 10. >> >> That sounds to be a bug in your compiler ... it shouldn't be rounding up >> structure sizes if the structure can fit in 10 bytes. This isn't >> happening in any other architecture that I know of (otherwise we'd have >> had a reported build break). > > This was my initial thought, and I share your feeling that this isn't > ideal compiler behaviour, however it's pretty much set in stone as part > of our ABI now. Having talked to one of our compiler folk about it > here's what he had to say: >> In GCC 4.2 the following backends have STRUCTURE_SIZE_BOUNDARY set to >=32 >> which will trigger this behaviour: >> >> Alpha+unicos >> Arm prior to AAPCS >> Mt >> >> This is within standards to my knowledge. What are the chances of getting this patch accepted? I believe it is correct and the compiler behaviour is within standards, and hopefully arch/metag will be merged for v3.9 so without this patch the build of osd_initiator.c will be broken on an in-tree architecture. Thanks James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed
On 11/10/12 13:58, James Bottomley wrote: > On Thu, 2012-10-11 at 12:32 +0100, James Hogan wrote: >> On 11/10/12 11:24, James Bottomley wrote: >>> On Thu, 2012-10-11 at 10:15 +0100, James Hogan wrote: The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when used in struct osd_cdb_head, but it isn't marked as packed. Some architectures will round the struct size up which triggers BUILD_BUG_ON compile errors in osd_initiator.c when the outer structs are unexpected sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed. >>> >>> What actual problem have you encountered? The structure is {u8[8], u16} >>> which is naturally packed on every architecture I know about. I've even >>> built osd_initiator without problem on parisc, which has some of the >>> most rigid alignment rules I've seen. >> >> Hi James, >> >> The alignment is fine (the offset of the u16 is 8 bytes), but >> unfortunately with the metag port of gcc, sizeof(struct >> scsi_varlen_cdb_hdr) is rounded up to a 4 byte boundary (even though the >> largest data member alignment is only 2 bytes), which is 12 bytes >> instead of 10. > > That sounds to be a bug in your compiler ... it shouldn't be rounding up > structure sizes if the structure can fit in 10 bytes. This isn't > happening in any other architecture that I know of (otherwise we'd have > had a reported build break). This was my initial thought, and I share your feeling that this isn't ideal compiler behaviour, however it's pretty much set in stone as part of our ABI now. Having talked to one of our compiler folk about it here's what he had to say: > In GCC 4.2 the following backends have STRUCTURE_SIZE_BOUNDARY set to >=32 > which will trigger this behaviour: > > Alpha+unicos > Arm prior to AAPCS > Mt > > This is within standards to my knowledge. Cheers James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed
> > The alignment is fine (the offset of the u16 is 8 bytes), but > > unfortunately with the metag port of gcc, sizeof(struct > > scsi_varlen_cdb_hdr) is rounded up to a 4 byte boundary (even though the > > largest data member alignment is only 2 bytes), which is 12 bytes > > instead of 10. > > That sounds to be a bug in your compiler ... it shouldn't be rounding up > structure sizes if the structure can fit in 10 bytes. This isn't > happening in any other architecture that I know of (otherwise we'd have > had a reported build break). It's not a bug for the alignment rules of the processor as far as I can see. The architectural definition is perfectly entitled to have tail padding in this case. The x86 equivalent would be struct foo { double x; int a; }; which is *NOT* 12 bytes long. It is indeed a portability bug in the scsi layer. Alan -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed
On Thu, 2012-10-11 at 12:32 +0100, James Hogan wrote: > On 11/10/12 11:24, James Bottomley wrote: > > On Thu, 2012-10-11 at 10:15 +0100, James Hogan wrote: > >> The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when > >> used in struct osd_cdb_head, but it isn't marked as packed. Some > >> architectures will round the struct size up which triggers BUILD_BUG_ON > >> compile errors in osd_initiator.c when the outer structs are unexpected > >> sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed. > > > > What actual problem have you encountered? The structure is {u8[8], u16} > > which is naturally packed on every architecture I know about. I've even > > built osd_initiator without problem on parisc, which has some of the > > most rigid alignment rules I've seen. > > Hi James, > > The alignment is fine (the offset of the u16 is 8 bytes), but > unfortunately with the metag port of gcc, sizeof(struct > scsi_varlen_cdb_hdr) is rounded up to a 4 byte boundary (even though the > largest data member alignment is only 2 bytes), which is 12 bytes > instead of 10. That sounds to be a bug in your compiler ... it shouldn't be rounding up structure sizes if the structure can fit in 10 bytes. This isn't happening in any other architecture that I know of (otherwise we'd have had a reported build break). James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed
On 11/10/12 11:24, James Bottomley wrote: > On Thu, 2012-10-11 at 10:15 +0100, James Hogan wrote: >> The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when >> used in struct osd_cdb_head, but it isn't marked as packed. Some >> architectures will round the struct size up which triggers BUILD_BUG_ON >> compile errors in osd_initiator.c when the outer structs are unexpected >> sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed. > > What actual problem have you encountered? The structure is {u8[8], u16} > which is naturally packed on every architecture I know about. I've even > built osd_initiator without problem on parisc, which has some of the > most rigid alignment rules I've seen. Hi James, The alignment is fine (the offset of the u16 is 8 bytes), but unfortunately with the metag port of gcc, sizeof(struct scsi_varlen_cdb_hdr) is rounded up to a 4 byte boundary (even though the largest data member alignment is only 2 bytes), which is 12 bytes instead of 10. Cheers James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed
On 11/10/12 11:01, Bart Van Assche wrote: > On 10/11/12 11:15, James Hogan wrote: >> The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when >> used in struct osd_cdb_head, but it isn't marked as packed. Some >> architectures will round the struct size up which triggers BUILD_BUG_ON >> compile errors in osd_initiator.c when the outer structs are unexpected >> sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed. >> >> Signed-off-by: James Hogan >> --- >> include/scsi/scsi.h |2 +- >> 1 files changed, 1 insertions(+), 1 deletions(-) >> >> diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h >> index 66216c1..3beaef3 100644 >> --- a/include/scsi/scsi.h >> +++ b/include/scsi/scsi.h >> @@ -198,7 +198,7 @@ struct scsi_varlen_cdb_hdr { >> __u8 additional_cdb_length; /* total cdb length - 8 */ >> __be16 service_action; >> /* service specific data follows */ >> -}; >> +} __packed; >> >> static inline unsigned >> scsi_varlen_cdb_length(const void *hdr) > > Hello James, > > Are you aware that __packed can also be used on individual struct > members and that doing so has a lower performance penalty than using the > __packed attribute on an entire struct ? See e.g. for > an example. Hi Bart, Thanks, I wasn't aware of that, however the problem is the size of the struct as a whole rather than the alignment or offsets of any individual data members. E.g. adding __attribute__((packed,aligned(2))) to the 16bit member doesn't reduce the struct to 10 bytes, or change the offset from 8 bytes. Cheers James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed
On Thu, 2012-10-11 at 10:15 +0100, James Hogan wrote: > The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when > used in struct osd_cdb_head, but it isn't marked as packed. Some > architectures will round the struct size up which triggers BUILD_BUG_ON > compile errors in osd_initiator.c when the outer structs are unexpected > sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed. What actual problem have you encountered? The structure is {u8[8], u16} which is naturally packed on every architecture I know about. I've even built osd_initiator without problem on parisc, which has some of the most rigid alignment rules I've seen. James -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed
On 10/11/12 11:15, James Hogan wrote: The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when used in struct osd_cdb_head, but it isn't marked as packed. Some architectures will round the struct size up which triggers BUILD_BUG_ON compile errors in osd_initiator.c when the outer structs are unexpected sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed. Signed-off-by: James Hogan --- include/scsi/scsi.h |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h index 66216c1..3beaef3 100644 --- a/include/scsi/scsi.h +++ b/include/scsi/scsi.h @@ -198,7 +198,7 @@ struct scsi_varlen_cdb_hdr { __u8 additional_cdb_length; /* total cdb length - 8 */ __be16 service_action; /* service specific data follows */ -}; +} __packed; static inline unsigned scsi_varlen_cdb_length(const void *hdr) Hello James, Are you aware that __packed can also be used on individual struct members and that doing so has a lower performance penalty than using the __packed attribute on an entire struct ? See e.g. for an example. Bart. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[RESEND PATCH] scsi: make struct scsi_varlen_cdb_hdr packed
The struct scsi_varlen_cdb_hdr is expected to be exactly 10 bytes when used in struct osd_cdb_head, but it isn't marked as packed. Some architectures will round the struct size up which triggers BUILD_BUG_ON compile errors in osd_initiator.c when the outer structs are unexpected sizes. This is fixed by marking struct scsi_varlen_cdb_hdr as __packed. Signed-off-by: James Hogan --- include/scsi/scsi.h |2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/include/scsi/scsi.h b/include/scsi/scsi.h index 66216c1..3beaef3 100644 --- a/include/scsi/scsi.h +++ b/include/scsi/scsi.h @@ -198,7 +198,7 @@ struct scsi_varlen_cdb_hdr { __u8 additional_cdb_length; /* total cdb length - 8 */ __be16 service_action; /* service specific data follows */ -}; +} __packed; static inline unsigned scsi_varlen_cdb_length(const void *hdr) -- 1.7.7.6 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html