RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
Got it. You did mention about typedef in email chain, but I understood as
different way to achieve same.
I reviewed my code and found that most of the fields between driver-adapter
doesn't need attribute.
So far (a) removing packed and (b) BUILD_BUG_ON looks sufficient for current
set of structures. Initial test suffice the need.
Thanks.
Parav
> -Original Message-
> From: Jason Gunthorpe [mailto:jguntho...@obsidianresearch.com]
> Sent: Friday, March 23, 2012 10:24 PM
> To: Pandit, Parav
> Cc: david.lai...@aculab.com; rol...@purestorage.com; linux-
> r...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> On Fri, Mar 23, 2012 at 07:03:37AM -0700, parav.pan...@emulex.com wrote:
>
> > > David is saying you will get a 12 byte struct and fieldb will be
> > > unaligned. Since
> > > 12 is aligned to 4 no padding is added.
> >
> > So I decided to experiment above example before implementing in
> > driver. However I find structure of 16 bytes (instead of 12) with
> > padding after fielda in below example. Am I missing some compiler
> > option or syntax error in attribute? Sorry to ask this silly question.
> > I tried __attribute__((__aligned__(4))); too based on usage in other
> > kernel code.
>
> I got the syntax wrong for that specific case (it is a little unintuitive..
> IMHO,
> capping the alignment of a container should cap the alignment of all
> members, otherwise it is nonsense!):
>
> typedef uint64_t u64_unaligned_8 __attribute__((__aligned__(4)));
>
> struct foo {
> uint32_t fielda;
> u64_unaligned_8 fieldb;
> };
>
> struct foo2 {
> uint32_t fielda;
> uint64_t fieldb;
> };
>
> int main(int argc,const char *argv[])
> {
> printf("sizeof(foo) = %zu, fieldb = %zu\n",sizeof(struct foo),
> offsetof(struct foo,fieldb));
> printf("sizeof(foo2) = %zu, fieldb = %zu\n",sizeof(struct foo2),
> offsetof(struct foo2,fieldb));
> return 0;
> }
>
> sizeof(foo) = 12, fieldb = 4
> sizeof(foo2) = 16, fieldb = 8
>
> gcc version 4.6.1 (Ubuntu/Linaro 4.6.1-9ubuntu3)
>
> Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Fri, Mar 23, 2012 at 07:03:37AM -0700, parav.pan...@emulex.com wrote:
> > David is saying you will get a 12 byte struct and fieldb will be unaligned.
> > Since
> > 12 is aligned to 4 no padding is added.
>
> So I decided to experiment above example before implementing in
> driver. However I find structure of 16 bytes (instead of 12) with
> padding after fielda in below example. Am I missing some compiler
> option or syntax error in attribute? Sorry to ask this silly
> question. I tried __attribute__((__aligned__(4))); too based on
> usage in other kernel code.
I got the syntax wrong for that specific case (it is a little
unintuitive.. IMHO, capping the alignment of a container should cap
the alignment of all members, otherwise it is nonsense!):
typedef uint64_t u64_unaligned_8 __attribute__((__aligned__(4)));
struct foo {
uint32_t fielda;
u64_unaligned_8 fieldb;
};
struct foo2 {
uint32_t fielda;
uint64_t fieldb;
};
int main(int argc,const char *argv[])
{
printf("sizeof(foo) = %zu, fieldb = %zu\n",sizeof(struct foo),
offsetof(struct foo,fieldb));
printf("sizeof(foo2) = %zu, fieldb = %zu\n",sizeof(struct foo2),
offsetof(struct foo2,fieldb));
return 0;
}
sizeof(foo) = 12, fieldb = 4
sizeof(foo2) = 16, fieldb = 8
gcc version 4.6.1 (Ubuntu/Linaro 4.6.1-9ubuntu3)
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> -Original Message-
> From: Jason Gunthorpe [mailto:jguntho...@obsidianresearch.com]
> Sent: Friday, March 23, 2012 4:14 AM
> To: Pandit, Parav
> Cc: david.lai...@aculab.com; rol...@purestorage.com; linux-
> r...@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> On Thu, Mar 22, 2012 at 02:20:28PM -0700, parav.pan...@emulex.com
> wrote:
> > I got a question here lately.
> >
> > aligned directive will ensure that it will fall on boundary. Say
> > aligned(4) ensures that structure is aligned to 4 byte boundary.
> > Compiler can (at least theoretically) still have 4 byte structure
> > aligned to 8 byte boundary on 64-bit platform (which is 4 byte aligned
> > too).
>
> There are very specific rules defined in the platform's ABI for how C
> structures are layed out in memory, each ABI (ie CPU) has its own specific
> quirks, but broadly in Linux land you can boil it down to:
>
> 1) The alignment of a structure is the greatest alignment of all the
>members
> 2) Each member is aligned to its alignment.
>
> The alignment of the structure drives the total size of the structure, and
> specifically the padding added at the end to reach that alignment.
>
> So, no, a compiler that increased the alignment of a struct with one
> u32 to 8 would violate the various ABIs and not be usuable for Linux. It is
> important to bear in mind that Linux targets a particular set of ABI
> conventions, and it is not 'anything goes'.
>
> > struct {
> > u32 field;
> > };
>
> So in this case: the u32 is aligned to 4, the structure is aligned to
> 4 and the total size of the structure is 4 on everything linux supports.
>
> > struct {
> > u64 fielda
> > u32 field;
> > };
>
> In this case: On 64 bit: the u64 is aligned to 8 and the u32 is aligned to 4.
> So
> the structure is aligned to 8. A pad is inserted at the end of the struct to
> bring
> it out. On 32 bit, the u64 is aligned to 4, so the struct is aligned to 4, so
> no pad
> is added.
>
> > struct {
> > __float128 fielda
> > u32 field;
> > };
>
> In this case the float128 is aligned to 16 and thus the structure is aligned
> to 16
> and 12 pad bytes are added.
>
> > However requirement is to have this structure only 4 byte size(
> > because adapter excepts it to be 4B sise) and therefor packed is used.
> > I don't know the way to ensure size of 4 byte and alignment too. Or I
> > am misunderstanding?
>
> Yes, you are mis-understanding the rules for padding.. Structures are only
> padded out to their alignment, which depends on their constituent types.
> This is so arrays of structures have each array element starting on its
> natural
> alignment.
>
> The aligned attribute overrides the automatic determination of the
> alignment based on the contents and just forces it.
>
> So, as an example, if you have this hardware layout:
>
> struct {
> u32 fielda;
> u64 fieldb;
> } attribute ((aligned(4));
>
> David is saying you will get a 12 byte struct and fieldb will be unaligned.
> Since
> 12 is aligned to 4 no padding is added.
So I decided to experiment above example before implementing in driver. However
I find structure of 16 bytes (instead of 12) with padding after fielda in below
example.
Am I missing some compiler option or syntax error in attribute? Sorry to ask
this silly question.
I tried __attribute__((__aligned__(4))); too based on usage in other kernel
code.
#include
struct foo {
u32 a;
u64 b;
} __attribute__((aligned(4)));
static int __init main_init(void)
{
printk("<1> sizeof foo=%ld\n", sizeof(struct foo));
printk("<1> offset of a=%ld\n", offsetof(struct foo, a));
printk("<1> offset of b=%ld\n", offsetof(struct foo, b));
return 0;
}
static void __exit main_exit(void)
{
}
module_init(main_init);
module_exit(main_exit);
Output:
Mar 24 05:44:10 parav-sles11-sp1-64 kernel: [2006356.094931] sizeof foo=16
Mar 24 05:44:10 parav-sles11-sp1-64 kernel: [2006356.094934] offset of a=0
Mar 24 05:44:10 parav-sles11-sp1-64 kernel: [2006356.094936] offset of b=8
Gcc version is below.
Using built-in specs.
Target: x86_64-suse-linux
Configured with: ../configure --prefix=/usr --infodir=/usr/share/info
--mandir=/usr/share/man --libdir=/usr/lib64 --libexecdir=/usr/lib64
--enable-languages=c,c++,objc,fortran,obj-c++,java,ada
--enable-checking=release --with-gxx-include-dir=/usr/include/c++/4.3
--enable-ssp --disable-libssp --with-bugurl=http://bugs.opensuse.org/
--with-pkgversion='SUSE Linux'
RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> > struct {
> > u64 fielda
> > u32 field;
> > };
>
> In this case: On 64 bit: the u64 is aligned to 8 and the u32
> is aligned to 4. So
> the structure is aligned to 8. A pad is inserted at the end of the
> struct to bring it out. On 32 bit, the u64 is aligned to 4, so the
> struct is aligned to 4, so no pad is added.
That is true for 32bit i386, some 32bit ABI may require 64bit
alignment for 64bit items.
...
> The aligned attribute overrides the automatic determination of the
> alignment based on the contents and just forces it.
>
> So, as an example, if you have this hardware layout:
>
> struct {
> u32 fielda;
> u64 fieldb;
> } attribute ((aligned(4));
>
> David is saying you will get a 12 byte struct and fieldb will be
> unaligned. Since 12 is aligned to 4 no padding is added.
I was actually suggesting defining (modulo syntax errors):
typedef u64 u64_aligned_8 attribute ((aligned(8));
Then you can define:
struct foo {
u32 fielda;
u64_aligned_8 fieldb;
};
and know that the aligment should be the same on all systems.
(But I'd still add the explicit pad in the above.)
This is actually more useful when you need to process structures
that have misaligned 64bit items - as happens when doing 'compat 32'
code in a 64bit kernel. Or trying to map the x86 disk mbr.
> For hardware facing structures I'd combine this with a static assert
> to verify structure size at compile time.
Also useful if you need to ensure a structure doesn't accidentally
change size when you need to preserve a userspace interface.
> So..
>
> 1) Avoid using attributes unless the structure has unaligned members.
> 2) Avoid creating structures with unaligned members (eg for userspace
>communication)
> 3) Frown at hardware/firmware developers who make communication
>structures with unaligned members :)
> 4) Be explicit about padding in your layout for 64/32
>compatibility.
Agreed.
David
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Thu, Mar 22, 2012 at 3:44 PM, Jason Gunthorpe wrote: > For hardware facing structures I'd combine this with a static assert > to verify structure size at compile time. > > So.. > > 1) Avoid using attributes unless the structure has unaligned members. > 2) Avoid creating structures with unaligned members (eg for userspace > communication) > 3) Frown at hardware/firmware developers who make communication > structures with unaligned members :) > 4) Be explicit about padding in your layout for 64/32 > compatibility. Excellent summary, thanks Jason. - R. -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Thu, Mar 22, 2012 at 02:20:28PM -0700, parav.pan...@emulex.com wrote:
> I got a question here lately.
>
> aligned directive will ensure that it will fall on boundary. Say
> aligned(4) ensures that structure is aligned to 4 byte boundary.
> Compiler can (at least theoretically) still have 4 byte structure
> aligned to 8 byte boundary on 64-bit platform (which is 4 byte
> aligned too).
There are very specific rules defined in the platform's ABI for how C
structures are layed out in memory, each ABI (ie CPU) has its own
specific quirks, but broadly in Linux land you can boil it down to:
1) The alignment of a structure is the greatest alignment of all the
members
2) Each member is aligned to its alignment.
The alignment of the structure drives the total size of the structure,
and specifically the padding added at the end to reach that alignment.
So, no, a compiler that increased the alignment of a struct with one
u32 to 8 would violate the various ABIs and not be usuable for
Linux. It is important to bear in mind that Linux targets a particular
set of ABI conventions, and it is not 'anything goes'.
> struct {
> u32 field;
> };
So in this case: the u32 is aligned to 4, the structure is aligned to
4 and the total size of the structure is 4 on everything linux
supports.
> struct {
> u64 fielda
> u32 field;
> };
In this case: On 64 bit: the u64 is aligned to 8 and the u32 is aligned to 4. So
the structure is aligned to 8. A pad is inserted at the end of the
struct to bring it out. On 32 bit, the u64 is aligned to 4, so the
struct is aligned to 4, so no pad is added.
> struct {
> __float128 fielda
> u32 field;
> };
In this case the float128 is aligned to 16 and thus the structure is
aligned to 16 and 12 pad bytes are added.
> However requirement is to have this structure only 4 byte size(
> because adapter excepts it to be 4B sise) and therefor packed is
> used. I don't know the way to ensure size of 4 byte and alignment
> too. Or I am misunderstanding?
Yes, you are mis-understanding the rules for padding.. Structures are
only padded out to their alignment, which depends on their constituent
types. This is so arrays of structures have each array element
starting on its natural alignment.
The aligned attribute overrides the automatic determination of the
alignment based on the contents and just forces it.
So, as an example, if you have this hardware layout:
struct {
u32 fielda;
u64 fieldb;
} attribute ((aligned(4));
David is saying you will get a 12 byte struct and fieldb will be
unaligned. Since 12 is aligned to 4 no padding is added.
For hardware facing structures I'd combine this with a static assert
to verify structure size at compile time.
So..
1) Avoid using attributes unless the structure has unaligned members.
2) Avoid creating structures with unaligned members (eg for userspace
communication)
3) Frown at hardware/firmware developers who make communication
structures with unaligned members :)
4) Be explicit about padding in your layout for 64/32
compatibility.
Jason
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
I got a question here lately.
aligned directive will ensure that it will fall on boundary.
Say aligned(4) ensures that structure is aligned to 4 byte boundary.
Compiler can (at least theoretically) still have 4 byte structure aligned to 8
byte boundary on 64-bit platform (which is 4 byte aligned too).
struct {
u32 field;
} attribute ((aligned(4));
However requirement is to have this structure only 4 byte size( because adapter
excepts it to be 4B sise) and therefor packed is used.
I don't know the way to ensure size of 4 byte and alignment too.
Or I am misunderstanding?
Parav
> -Original Message-
> From: linux-rdma-ow...@vger.kernel.org [mailto:linux-rdma-
> ow...@vger.kernel.org] On Behalf Of parav.pan...@emulex.com
> Sent: Friday, March 23, 2012 2:41 AM
> To: jguntho...@obsidianresearch.com
> Cc: david.lai...@aculab.com; rol...@purestorage.com; linux-
> r...@vger.kernel.org; net...@vger.kernel.org
> Subject: RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
>
>
> > -Original Message-
> > From: Jason Gunthorpe [mailto:jguntho...@obsidianresearch.com]
> > Sent: Friday, March 23, 2012 2:28 AM
> > To: Pandit, Parav
> > Cc: david.lai...@aculab.com; rol...@purestorage.com; linux-
> > r...@vger.kernel.org; net...@vger.kernel.org
> > Subject: Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> > adapter
> >
> > On Thu, Mar 22, 2012 at 01:52:30PM -0700, parav.pan...@emulex.com
> > wrote:
> >
> > > > This can be used to force 32bit alignment in amd64 code in order
> > > > to match definitions in 32bit userspace.
> > > > For new things it would make sense to force 64bit alignment of
> > > > 64bit fields for 32bit code.
> > >
> > > o.k. so I'll use aligned attribute to align user-kernel interface
> > > data structure to 8 byte boundary. That should work for 32-bit and
> > > 64-bit user and kernel space and does't hurt performance either?
> >
> > If the structure is only for user/kernel interfacing then it is much
> > better to add explicit padding fields to naturally place 64 bit
> > quantities on an 8 byte alignment than to mess with gcc specific
> > attributes (user space has a much wide choice of compilers).
> >
> > This was David's second suggestion. Better to do this now before the
> > driver is accepted :)
> >
> o.k. I'll align them to naturally 8 byte boundary.
>
> > Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the
> body of a message to majord...@vger.kernel.org More majordomo info at
> http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> -Original Message- > From: Jason Gunthorpe [mailto:jguntho...@obsidianresearch.com] > Sent: Friday, March 23, 2012 2:28 AM > To: Pandit, Parav > Cc: david.lai...@aculab.com; rol...@purestorage.com; linux- > r...@vger.kernel.org; net...@vger.kernel.org > Subject: Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA > adapter > > On Thu, Mar 22, 2012 at 01:52:30PM -0700, parav.pan...@emulex.com > wrote: > > > > This can be used to force 32bit alignment in amd64 code in order to > > > match definitions in 32bit userspace. > > > For new things it would make sense to force 64bit alignment of 64bit > > > fields for 32bit code. > > > > o.k. so I'll use aligned attribute to align user-kernel interface data > > structure to 8 byte boundary. That should work for 32-bit and 64-bit > > user and kernel space and does't hurt performance either? > > If the structure is only for user/kernel interfacing then it is much better to > add explicit padding fields to naturally place 64 bit quantities on an 8 byte > alignment than to mess with gcc specific attributes (user space has a much > wide choice of compilers). > > This was David's second suggestion. Better to do this now before the driver is > accepted :) > o.k. I'll align them to naturally 8 byte boundary. > Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Thu, Mar 22, 2012 at 01:52:30PM -0700, parav.pan...@emulex.com wrote: > > This can be used to force 32bit alignment in amd64 code in order to match > > definitions in 32bit userspace. > > For new things it would make sense to force 64bit alignment of 64bit fields > > for 32bit code. > > o.k. so I'll use aligned attribute to align user-kernel interface > data structure to 8 byte boundary. That should work for 32-bit and > 64-bit user and kernel space and does't hurt performance either? If the structure is only for user/kernel interfacing then it is much better to add explicit padding fields to naturally place 64 bit quantities on an 8 byte alignment than to mess with gcc specific attributes (user space has a much wide choice of compilers). This was David's second suggestion. Better to do this now before the driver is accepted :) Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> -Original Message-
> From: David Laight [mailto:david.lai...@aculab.com]
> Sent: Wednesday, March 21, 2012 10:02 PM
> To: Roland Dreier; Pandit, Parav
> Cc: linux-rdma@vger.kernel.org; net...@vger.kernel.org
> Subject: RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
>
> > > - Header file for userspace library and kernel driver interface.
> >
> > > +struct ocrdma_alloc_ucontext_resp {
> > > + u32 dev_id;
> > > + u32 wqe_size;
> > > + u32 max_inline_data;
> > > + u32 dpp_wqe_size;
> > > + u64 ah_tbl_page;
> > > + u32 ah_tbl_len;
> > > + u32 rsvd;
> > > + u8 fw_ver[32];
> > > + u32 rqe_size;
> > > + u64 rsvd1;
> > > +} __packed;
> >
> > If I'm reading this correctly, you have the 8-byte rsvd1 member at an
> > offset only aligned to 4 bytes, because of the __packed directive. It
> > would be much better to have these structures laid out so they are
> > naturally the same on both 32-bit and 64-bit ABIs, and get rid of the
> > __packed directive, which wrecks gcc code generation in some cases.
> >
>
> gcc also supports defining types that have non-standard alignment
> constraints that can be used to force the same alignment for 64bit fields
> between i386 and amd64.
> Probably __attribute__((aligned,n)) or similar.
>
> This can be used to force 32bit alignment in amd64 code in order to match
> definitions in 32bit userspace.
> For new things it would make sense to force 64bit alignment of 64bit fields
> for 32bit code.
o.k. so I'll use aligned attribute to align user-kernel interface data
structure to 8 byte boundary.
That should work for 32-bit and 64-bit user and kernel space and does't hurt
performance either?
Driver-adapter structures will be aligned to 4 byte boundary using aligned
attribute instead of packed.
>
> Adding __packed (rather than 32bit alignment) forces the compiler to
> generate byte by byte accesses for all the fields on systems that can't do
> misaligned accesses in hardware (eg sparc).
>
> David
>
>
> David
>
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
I see couple of comments on rsvd words.
They were primarily not introduced for alignment. But there are other new
features that we will be adding with new set of hardware and firmware updates.
I don't want to change the user-kernel interface at such stage by modifying the
size of the structure.
For some features its under testing stage internally.
So once its ready rsvd will be replaced with actual element.
This will avoid abi compatibility issues between library and driver.
I'll consider alignment macro too so that compiler related byte alignment
access issue also gets resolved.
Parav
> -Original Message-
> From: Roland Dreier [mailto:rol...@purestorage.com]
> Sent: Wednesday, March 21, 2012 9:50 PM
> To: Pandit, Parav
> Cc: linux-rdma@vger.kernel.org; net...@vger.kernel.org
> Subject: Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA
> adapter
>
> On Tue, Mar 20, 2012 at 3:39 PM, wrote:
> > From: Parav Pandit
> >
> > - Header file for userspace library and kernel driver interface.
>
> > +struct ocrdma_alloc_ucontext_resp {
> > + u32 dev_id;
> > + u32 wqe_size;
> > + u32 max_inline_data;
> > + u32 dpp_wqe_size;
> > + u64 ah_tbl_page;
> > + u32 ah_tbl_len;
> > + u32 rsvd;
> > + u8 fw_ver[32];
> > + u32 rqe_size;
> > + u64 rsvd1;
> > +} __packed;
>
> If I'm reading this correctly, you have the 8-byte rsvd1 member at an offset
> only aligned to 4 bytes, because of the __packed directive. It would be much
> better to have these structures laid out so they are naturally the same on
> both 32-bit and 64-bit ABIs, and get rid of the __packed directive, which
> wrecks gcc code generation in some cases.
>
> In this particular case, it seems you could just move rqe_size into the slot
> where rsvd is, and get rid of rsvd1?
>
> > +/* user kernel communication data structures. */ struct
> > +ocrdma_alloc_pd_ureq {
> > + u64 rsvd1;
> > +} __packed;
>
> Similar comment -- __packed is silly for a structure with one reserved
> member (and which you don't seem to use anywhere)... why not just delete
> this struct?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> +struct ocrdma_alloc_ucontext_resp {
> + u32 dev_id;
> + u32 wqe_size;
> + u32 max_inline_data;
> + u32 dpp_wqe_size;
> + u64 ah_tbl_page;
> + u32 ah_tbl_len;
> + u32 rsvd;
> + u8 fw_ver[32];
> + u32 rqe_size;
> + u64 rsvd1;
> +} __packed;
Is there some reason to pack the structures in this file, rather than just
defining things to align on a 64-bit boundary? The structure above actually
ends up sized to a 4-byte boundary, rather than 8-byte, with rsvd1 field
misaligned.
> +/* user kernel communication data structures. */
> +struct ocrdma_alloc_pd_ureq {
> + u64 rsvd1;
> +} __packed;
> +
> +struct ocrdma_alloc_pd_uresp {
> + u32 id;
> + u32 dpp_enabled;
> + u32 dpp_page_addr_hi;
> + u32 dpp_page_addr_lo;
> + u64 rsvd1;
> +} __packed;
> +
> +struct ocrdma_create_cq_ureq {
> + u32 dpp_cq;
> + u32 rsvd;
> +} __packed;
> +
> +#define MAX_CQ_PAGES 8
> +struct ocrdma_create_cq_uresp {
> + u32 cq_id;
> + u32 page_size;
> + u32 num_pages;
> + u32 max_hw_cqe;
> + u64 page_addr[MAX_CQ_PAGES];
> + u64 db_page_addr;
> + u32 db_page_size;
> + u32 phase_change;
> + u64 rsvd1;
> + u64 rsvd2;
> +} __packed;
Why the extra reserved space?
> +
> +#define MAX_QP_PAGES 8
> +#define MAX_UD_AV_PAGES 8
> +
> +struct ocrdma_create_qp_ureq {
> + u8 enable_dpp_cq;
> + u8 rsvd;
> + u16 dpp_cq_id;
> + u32 rsvd1;
> +};
> +
> +struct ocrdma_create_qp_uresp {
> + u16 qp_id;
> + u16 sq_dbid;
> + u16 rq_dbid;
> + u16 resv0;
> + u32 sq_page_size;
> + u32 rq_page_size;
> + u32 num_sq_pages;
> + u32 num_rq_pages;
> + u64 sq_page_addr[MAX_QP_PAGES];
> + u64 rq_page_addr[MAX_QP_PAGES];
> + u64 db_page_addr;
> + u32 db_page_size;
> + u32 dpp_credit;
> + u32 dpp_offset;
> + u32 rsvd1;
> + u32 num_wqe_allocated;
> + u32 num_rqe_allocated;
> + u32 free_wqe_delta;
> + u32 free_rqe_delta;
> + u32 db_sq_offset;
> + u32 db_rq_offset;
> + u32 db_shift;
> + u64 rsvd2;
> + u64 rsvd3;
> +} __packed;
Why the extra reserved space? rsvd2 & 3 are also misaligned.
> +
> +struct ocrdma_create_srq_uresp {
> + u16 rq_dbid;
> + u16 resv0;
> + u32 resv1;
> +
> + u32 rq_page_size;
> + u32 num_rq_pages;
> +
> + u64 rq_page_addr[MAX_QP_PAGES];
> + u64 db_page_addr;
> +
> + u32 db_page_size;
> + u32 num_rqe_allocated;
> + u32 db_rq_offset;
> + u32 db_shift;
> +
> + u32 free_rqe_delta;
> + u32 rsvd2;
> + u64 rsvd3;
> +} __packed;
Why the extra reserved space?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
RE: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
> > - Header file for userspace library and kernel driver interface.
>
> > +struct ocrdma_alloc_ucontext_resp {
> > + u32 dev_id;
> > + u32 wqe_size;
> > + u32 max_inline_data;
> > + u32 dpp_wqe_size;
> > + u64 ah_tbl_page;
> > + u32 ah_tbl_len;
> > + u32 rsvd;
> > + u8 fw_ver[32];
> > + u32 rqe_size;
> > + u64 rsvd1;
> > +} __packed;
>
> If I'm reading this correctly, you have the 8-byte rsvd1 member at an
> offset only aligned to 4 bytes, because of the __packed directive. It
> would be much better to have these structures laid out so they are
> naturally the same on both 32-bit and 64-bit ABIs, and get rid of the
> __packed directive, which wrecks gcc code generation in some cases.
>
gcc also supports defining types that have non-standard alignment
constraints that can be used to force the same alignment for
64bit fields between i386 and amd64.
Probably __attribute__((aligned,n)) or similar.
This can be used to force 32bit alignment in amd64 code in order
to match definitions in 32bit userspace.
For new things it would make sense to force 64bit alignment
of 64bit fields for 32bit code.
Adding __packed (rather than 32bit alignment) forces the compiler
to generate byte by byte accesses for all the fields on systems
that can't do misaligned accesses in hardware (eg sparc).
David
David
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
On Tue, Mar 20, 2012 at 3:39 PM, wrote:
> From: Parav Pandit
>
> - Header file for userspace library and kernel driver interface.
> +struct ocrdma_alloc_ucontext_resp {
> + u32 dev_id;
> + u32 wqe_size;
> + u32 max_inline_data;
> + u32 dpp_wqe_size;
> + u64 ah_tbl_page;
> + u32 ah_tbl_len;
> + u32 rsvd;
> + u8 fw_ver[32];
> + u32 rqe_size;
> + u64 rsvd1;
> +} __packed;
If I'm reading this correctly, you have the 8-byte rsvd1 member at an
offset only aligned to 4 bytes, because of the __packed directive. It
would be much better to have these structures laid out so they are
naturally the same on both 32-bit and 64-bit ABIs, and get rid of the
__packed directive, which wrecks gcc code generation in some cases.
In this particular case, it seems you could just move rqe_size into the
slot where rsvd is, and get rid of rsvd1?
> +/* user kernel communication data structures. */
> +struct ocrdma_alloc_pd_ureq {
> + u64 rsvd1;
> +} __packed;
Similar comment -- __packed is silly for a structure with one reserved
member (and which you don't seem to use anywhere)... why not just
delete this struct?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/9] ocrdma: Driver for Emulex OneConnect RDMA adapter
From: Parav Pandit
- Header file for userspace library and kernel driver interface.
Signed-off-by: Parav Pandit
---
drivers/infiniband/hw/ocrdma/ocrdma_abi.h | 134 +
1 files changed, 134 insertions(+), 0 deletions(-)
create mode 100644 drivers/infiniband/hw/ocrdma/ocrdma_abi.h
diff --git a/drivers/infiniband/hw/ocrdma/ocrdma_abi.h
b/drivers/infiniband/hw/ocrdma/ocrdma_abi.h
new file mode 100644
index 000..a411a4e
--- /dev/null
+++ b/drivers/infiniband/hw/ocrdma/ocrdma_abi.h
@@ -0,0 +1,134 @@
+/***
+ * This file is part of the Emulex RoCE Device Driver for *
+ * RoCE (RDMA over Converged Ethernet) adapters. *
+ * Copyright (C) 2008-2012 Emulex. All rights reserved.*
+ * EMULEX and SLI are trademarks of Emulex.*
+ * www.emulex.com *
+ * *
+ * This program is free software; you can redistribute it and/or *
+ * modify it under the terms of version 2 of the GNU General *
+ * Public License as published by the Free Software Foundation.*
+ * This program is distributed in the hope that it will be useful. *
+ * ALL EXPRESS OR IMPLIED CONDITIONS, REPRESENTATIONS AND *
+ * WARRANTIES, INCLUDING ANY IMPLIED WARRANTY OF MERCHANTABILITY, *
+ * FITNESS FOR A PARTICULAR PURPOSE, OR NON-INFRINGEMENT, ARE *
+ * DISCLAIMED, EXCEPT TO THE EXTENT THAT SUCH DISCLAIMERS ARE HELD *
+ * TO BE LEGALLY INVALID. See the GNU General Public License for *
+ * more details, a copy of which can be found in the file COPYING *
+ * included with this package. *
+ *
+ * Contact Information:
+ * linux-driv...@emulex.com
+ *
+ * Emulex
+ * Susan Street
+ * Costa Mesa, CA 92626
+ ***/
+
+#ifndef __OCRDMA_ABI_H__
+#define __OCRDMA_ABI_H__
+
+struct ocrdma_alloc_ucontext_resp {
+ u32 dev_id;
+ u32 wqe_size;
+ u32 max_inline_data;
+ u32 dpp_wqe_size;
+ u64 ah_tbl_page;
+ u32 ah_tbl_len;
+ u32 rsvd;
+ u8 fw_ver[32];
+ u32 rqe_size;
+ u64 rsvd1;
+} __packed;
+
+/* user kernel communication data structures. */
+struct ocrdma_alloc_pd_ureq {
+ u64 rsvd1;
+} __packed;
+
+struct ocrdma_alloc_pd_uresp {
+ u32 id;
+ u32 dpp_enabled;
+ u32 dpp_page_addr_hi;
+ u32 dpp_page_addr_lo;
+ u64 rsvd1;
+} __packed;
+
+struct ocrdma_create_cq_ureq {
+ u32 dpp_cq;
+ u32 rsvd;
+} __packed;
+
+#define MAX_CQ_PAGES 8
+struct ocrdma_create_cq_uresp {
+ u32 cq_id;
+ u32 page_size;
+ u32 num_pages;
+ u32 max_hw_cqe;
+ u64 page_addr[MAX_CQ_PAGES];
+ u64 db_page_addr;
+ u32 db_page_size;
+ u32 phase_change;
+ u64 rsvd1;
+ u64 rsvd2;
+} __packed;
+
+#define MAX_QP_PAGES 8
+#define MAX_UD_AV_PAGES 8
+
+struct ocrdma_create_qp_ureq {
+ u8 enable_dpp_cq;
+ u8 rsvd;
+ u16 dpp_cq_id;
+ u32 rsvd1;
+};
+
+struct ocrdma_create_qp_uresp {
+ u16 qp_id;
+ u16 sq_dbid;
+ u16 rq_dbid;
+ u16 resv0;
+ u32 sq_page_size;
+ u32 rq_page_size;
+ u32 num_sq_pages;
+ u32 num_rq_pages;
+ u64 sq_page_addr[MAX_QP_PAGES];
+ u64 rq_page_addr[MAX_QP_PAGES];
+ u64 db_page_addr;
+ u32 db_page_size;
+ u32 dpp_credit;
+ u32 dpp_offset;
+ u32 rsvd1;
+ u32 num_wqe_allocated;
+ u32 num_rqe_allocated;
+ u32 free_wqe_delta;
+ u32 free_rqe_delta;
+ u32 db_sq_offset;
+ u32 db_rq_offset;
+ u32 db_shift;
+ u64 rsvd2;
+ u64 rsvd3;
+} __packed;
+
+struct ocrdma_create_srq_uresp {
+ u16 rq_dbid;
+ u16 resv0;
+ u32 resv1;
+
+ u32 rq_page_size;
+ u32 num_rq_pages;
+
+ u64 rq_page_addr[MAX_QP_PAGES];
+ u64 db_page_addr;
+
+ u32 db_page_size;
+ u32 num_rqe_allocated;
+ u32 db_rq_offset;
+ u32 db_shift;
+
+ u32 free_rqe_delta;
+ u32 rsvd2;
+ u64 rsvd3;
+} __packed;
+
+#endif /* __OCRDMA_ABI_H__ */
--
1.6.0.2
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
