Re: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

Re: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

[Chen Gang]

On 01/21/2014 06:36 PM, James Hogan wrote:
> Hi Dan,
> 
> On 20/01/14 21:13, Dan Carpenter wrote:
>> I made a quick and dirty sparse patch to check for this.  I don't think
>> I will bother trying to send it to sparse upstream, but you can if you
>> want to.
>>
>> It found 289 unions which might need a __packed added.  The lustre
>> unions were not in my allmodconfig so they're not listed.
> 
> Thanks a lot for this, it seems to be useful. I'm adapting it to reduce
> false negatives (e.g. omitting the check if the struct/union is already
> packed), and I imagine it could be made to only warn about padded
> unpacked structs/unions which are used as nested members of packed
> structs/unions. It wouldn't catch everything but would probably catch a
> lot of cases that are most likely to be genuine since they would have
> been packed at the outer level for a reason.
> 
>> Perhaps there could be a command line option or a pragma so that unions
>> will work in the kernel.  We don't care about linking to outside
>> libraries.
> 
> We still interact with userland via structs and unions, so it would
> probably have to exclude anything in uapi/.
> 

Thank all of you firstly.

But excuse me, I am still not quit clear that: "what need we do enough
to solve this feature issue?"

So I guess our current result is:

 - It is not a good idea to only let kernel to fit with compiler.

 - It is not a good idea to only let compiler to fit with kernel.

 - Need let compiler and kernel to fit with each other:

   - compiler will print related warning, but not break compiling.
     so metag compiler need be improvement (check and warn for it).

   - if check alignment explicitly in kernel source code, it need be
     fixed within kernel: "apply related patches (pack each struct or
     union), but the related patch comments need be improved".

Is what I guess correct?

Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

Re: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

[Chen Gang]

On 01/25/2014 07:55 PM, Chen Gang wrote:
> On 01/21/2014 06:36 PM, James Hogan wrote:
>> Hi Dan,
>>
>> On 20/01/14 21:13, Dan Carpenter wrote:
>>> I made a quick and dirty sparse patch to check for this.  I don't think
>>> I will bother trying to send it to sparse upstream, but you can if you
>>> want to.
>>>
>>> It found 289 unions which might need a __packed added.  The lustre
>>> unions were not in my allmodconfig so they're not listed.
>>
>> Thanks a lot for this, it seems to be useful. I'm adapting it to reduce
>> false negatives (e.g. omitting the check if the struct/union is already
>> packed), and I imagine it could be made to only warn about padded
>> unpacked structs/unions which are used as nested members of packed
>> structs/unions. It wouldn't catch everything but would probably catch a
>> lot of cases that are most likely to be genuine since they would have
>> been packed at the outer level for a reason.
>>
>>> Perhaps there could be a command line option or a pragma so that unions
>>> will work in the kernel.  We don't care about linking to outside
>>> libraries.
>>
>> We still interact with userland via structs and unions, so it would
>> probably have to exclude anything in uapi/.
>>
> 

It seems, our kernel still stick to treate 'pack' region have effect
with both 'align' and 'sizeof'.

So for compiler, could we add one additional cflag parameter to tell
compiler to switch it (compatible with ABI, or satisfy upstream kernel).

And for kernel, it will be OK enough to only append this parameter to
KBUILD_CFLAGS in "arch/metag/Makefile".


Thanks.

> Thank all of you firstly.
> 
> But excuse me, I am still not quit clear that: "what need we do enough
> to solve this feature issue?"
> 
> So I guess our current result is:
> 
>  - It is not a good idea to only let kernel to fit with compiler.
> 
>  - It is not a good idea to only let compiler to fit with kernel.
> 
>  - Need let compiler and kernel to fit with each other:
> 
>    - compiler will print related warning, but not break compiling.
>      so metag compiler need be improvement (check and warn for it).
> 
>    - if check alignment explicitly in kernel source code, it need be
>      fixed within kernel: "apply related patches (pack each struct or
>      union), but the related patch comments need be improved".
> 
> Is what I guess correct?
> 
> Thanks.
> 


-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

Re: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

[Dan Carpenter]

On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
> It seems, our kernel still stick to treate 'pack' region have effect
> with both 'align' and 'sizeof'.
> 

It's not about packed regions.  It's about unions.  It's saying the
sizeof() a union is a multiple of 4 unless it's packed.

union foo {
	short x;
	short y;
};

The author intended the sizeof(union foo) to be 2 but on metag arch then
it is 4.

regards,
dan carpenter

Re: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

[Chen Gang]

On 02/03/2014 04:58 PM, Dan Carpenter wrote:
> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
>> It seems, our kernel still stick to treate 'pack' region have effect
>> with both 'align' and 'sizeof'.
>>
> 
> It's not about packed regions.  It's about unions.  It's saying the
> sizeof() a union is a multiple of 4 unless it's packed.
> 
> union foo {
> 	short x;
> 	short y;
> };
> 
> The author intended the sizeof(union foo) to be 2 but on metag arch then
> it is 4.
> 

Yeah, just like your original discussion.  :-)


Hmm... can we say: "for metag compiler, in a pack region, it considers
variables alignment, but does not consider about struct/union alignment
(except append packed to each related struct/union)".

For compatible (consider about its ABI), it has to keep this features,
but for kernel, it needs be changed.

So, I suggest to add one parameter to compiler to switch this feature,
and append this parameter to KBUILD_CFLAGS in "arch/metag/Makefile"
which can satisfy both ABI and kernel.


Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

Re: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

[Chen Gang]

On 02/03/2014 06:03 PM, Chen Gang wrote:
> On 02/03/2014 04:58 PM, Dan Carpenter wrote:
>> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
>>> It seems, our kernel still stick to treate 'pack' region have effect
>>> with both 'align' and 'sizeof'.
>>>
>>
>> It's not about packed regions.  It's about unions.  It's saying the
>> sizeof() a union is a multiple of 4 unless it's packed.
>>
>> union foo {
>> 	short x;
>> 	short y;
>> };
>>
>> The author intended the sizeof(union foo) to be 2 but on metag arch then
>> it is 4.
>>
> 
> Yeah, just like your original discussion.  :-)
> 
> 
> Hmm... can we say: "for metag compiler, in a pack region, it considers
> variables alignment, but does not consider about struct/union alignment
> (except append packed to each related struct/union)".
> 
> For compatible (consider about its ABI), it has to keep this features,
> but for kernel, it needs be changed.
> 
> So, I suggest to add one parameter to compiler to switch this feature,
> and append this parameter to KBUILD_CFLAGS in "arch/metag/Makefile"
> which can satisfy both ABI and kernel.
> 

After append the parameter to KBUILD_CFLAGS in "arch/metag/Makefile",

 - I guess/assume "include/uapi/*" should/will not need be modified.

 - but need check all files in "arch/metag/include/uapi/*".
   (add padding data for packed struct/union when __KERNEL__ defined)

 - maybe we have to process metag related ABI which not in "*/uapi/*"
   (add padding data for packed struct/union when __KERNEL__ defined)
   and when we find them, recommend to move all of them to "*/uapi/*".


Sorry, I don't know whether this way is the best way or not, but for me
it is an executable way to solve this feature issue and satisfy both
kernel and ABI.


Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed
--
To unsubscribe from this list: send the line "unsubscribe linux-metag" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

RE: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

[David Laight]

From: Dan Carpenter
> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
> > It seems, our kernel still stick to treate 'pack' region have effect
> > with both 'align' and 'sizeof'.
> 
> It's not about packed regions.  It's about unions.  It's saying the
> sizeof() a union is a multiple of 4 unless it's packed.
> 
> union foo {
> 	short x;
> 	short y;
> };
> 
> The author intended the sizeof(union foo) to be 2 but on metag arch then
> it is 4.

The same is probably be true of: struct foo { _u16 bar; };

Architectures that define such alignment rules are a right PITA.
You either need to get the size to 2 without using 'packed', or
just not define such structures.
It is worth seeing if adding aligned(2) will change the size - I'm
not sure.

	David

Re: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

[James Hogan]

On 03/02/14 10:05, David Laight wrote:
> From: Dan Carpenter
>> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
>>> It seems, our kernel still stick to treate 'pack' region have effect
>>> with both 'align' and 'sizeof'.
>>
>> It's not about packed regions.  It's about unions.  It's saying the
>> sizeof() a union is a multiple of 4 unless it's packed.
>>
>> union foo {
>> 	short x;
>> 	short y;
>> };
>>
>> The author intended the sizeof(union foo) to be 2 but on metag arch then
>> it is 4.
> 
> The same is probably be true of: struct foo { _u16 bar; };

Yes indeed.

> Architectures that define such alignment rules are a right PITA.
> You either need to get the size to 2 without using 'packed', or
> just not define such structures.
> It is worth seeing if adding aligned(2) will change the size - I'm
> not sure.

__aligned(2) alone doesn't seem to have any effect on sizeof() or
__alignof__() unless it is accompanied by __packed. x86_64 is similar in
that respect (it just packs sanely in the first place).

Combining __packed with __aligned(2) does the trick though (__packed
alone sets __aligned(1) which is obviously going to be suboptimal).

Cheers
James

Re: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

[Chen Gang]

On 02/03/2014 06:22 PM, James Hogan wrote:
> On 03/02/14 10:05, David Laight wrote:
>> From: Dan Carpenter
>>> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
>>>> It seems, our kernel still stick to treate 'pack' region have effect
>>>> with both 'align' and 'sizeof'.
>>>
>>> It's not about packed regions.  It's about unions.  It's saying the
>>> sizeof() a union is a multiple of 4 unless it's packed.
>>>
>>> union foo {
>>> 	short x;
>>> 	short y;
>>> };
>>>
>>> The author intended the sizeof(union foo) to be 2 but on metag arch then
>>> it is 4.
>>
>> The same is probably be true of: struct foo { _u16 bar; };
> 
> Yes indeed.
> 
>> Architectures that define such alignment rules are a right PITA.
>> You either need to get the size to 2 without using 'packed', or
>> just not define such structures.
>> It is worth seeing if adding aligned(2) will change the size - I'm
>> not sure.
> 
> __aligned(2) alone doesn't seem to have any effect on sizeof() or
> __alignof__() unless it is accompanied by __packed. x86_64 is similar in
> that respect (it just packs sanely in the first place).
> 
> Combining __packed with __aligned(2) does the trick though (__packed
> alone sets __aligned(1) which is obviously going to be suboptimal).
> 

Oh, thank you for your explanation.

And hope this feature issue can be fixed, and satisfy both kernel and
ABI. :-)


Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed

RE: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

[David Laight]

From: James Hogan 
> On 03/02/14 10:05, David Laight wrote:
> > From: Dan Carpenter
> >> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
> >>> It seems, our kernel still stick to treate 'pack' region have effect
> >>> with both 'align' and 'sizeof'.
> >>
> >> It's not about packed regions.  It's about unions.  It's saying the
> >> sizeof() a union is a multiple of 4 unless it's packed.
> >>
> >> union foo {
> >> 	short x;
> >> 	short y;
> >> };
> >>
> >> The author intended the sizeof(union foo) to be 2 but on metag arch then
> >> it is 4.
> >
> > The same is probably be true of: struct foo { _u16 bar; };
> 
> Yes indeed.
> 
> > Architectures that define such alignment rules are a right PITA.
> > You either need to get the size to 2 without using 'packed', or
> > just not define such structures.
> > It is worth seeing if adding aligned(2) will change the size - I'm
> > not sure.
> 
> __aligned(2) alone doesn't seem to have any effect on sizeof() or
> __alignof__() unless it is accompanied by __packed. x86_64 is similar in
> that respect (it just packs sanely in the first place).
> 
> Combining __packed with __aligned(2) does the trick though (__packed
> alone sets __aligned(1) which is obviously going to be suboptimal).

Compile some code for a cpu that doesn't support misaligned transfers
(probably one of sparc, arm, ppc) and see if the compiler generates a
single 16bit request or two 8 bits ones.
You don't want the compiler generating multiple byte-sized memory transfers.

	David



--
To unsubscribe from this list: send the line "unsubscribe linux-metag" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

[James Hogan]

On 03/02/14 10:35, David Laight wrote:
> From: James Hogan 
>> On 03/02/14 10:05, David Laight wrote:
>>> Architectures that define such alignment rules are a right PITA.
>>> You either need to get the size to 2 without using 'packed', or
>>> just not define such structures.
>>> It is worth seeing if adding aligned(2) will change the size - I'm
>>> not sure.
>>
>> __aligned(2) alone doesn't seem to have any effect on sizeof() or
>> __alignof__() unless it is accompanied by __packed. x86_64 is similar in
>> that respect (it just packs sanely in the first place).
>>
>> Combining __packed with __aligned(2) does the trick though (__packed
>> alone sets __aligned(1) which is obviously going to be suboptimal).
> 
> Compile some code for a cpu that doesn't support misaligned transfers
> (probably one of sparc, arm, ppc) and see if the compiler generates a
> single 16bit request or two 8 bits ones.
> You don't want the compiler generating multiple byte-sized memory transfers.

Meta is also one of those arches, and according to my quick tests,
__packed alone does correctly make it fall back to byte loads/stores,
but with __packed __aligned(2) it uses 16bit loads/stores. I've also
confirmed that with an ARM toolchain (see below for example).

Cheers
James

input:

#define __aligned(x)			__attribute__((aligned(x)))
#define __packed			__attribute__((packed))

union a {
	short x, y;
} __aligned(2) __packed;

struct b {
	short x;
} __aligned(2) __packed;

unsigned int soa = sizeof(union a);
unsigned int aoa = __alignof__(union a);
unsigned int sob = sizeof(struct b);
unsigned int aob = __alignof__(struct b);

void t(struct b *x, union a *y)
{
	++x->x;
	++y->x;
}

ARM output (-O2):

	.cpu arm10tdmi
	.fpu softvfp
	.eabi_attribute 20, 1
	.eabi_attribute 21, 1
	.eabi_attribute 23, 3
	.eabi_attribute 24, 1
	.eabi_attribute 25, 1
	.eabi_attribute 26, 2
	.eabi_attribute 30, 2
	.eabi_attribute 34, 0
	.eabi_attribute 18, 4
	.file	"alignment4.c"
	.text
	.align	2
	.global	t
	.type	t, %function
t:
	@ args = 0, pretend = 0, frame = 0
	@ frame_needed = 0, uses_anonymous_args = 0
	@ link register save eliminated.
	ldrh	r3, [r0, #0]
	add	r3, r3, #1
	strh	r3, [r0, #0]	@ movhi
	ldrh	r3, [r1, #0]
	add	r3, r3, #1
	strh	r3, [r1, #0]	@ movhi
	bx	lr
	.size	t, .-t
	.global	aob
	.global	sob
	.global	aoa
	.global	soa
	.data
	.align	2
	.type	aob, %object
	.size	aob, 4
aob:
	.word	2
	.type	sob, %object
	.size	sob, 4
sob:
	.word	2
	.type	aoa, %object
	.size	aoa, 4
aoa:
	.word	2
	.type	soa, %object
	.size	soa, 4
soa:
	.word	2
	.ident	"GCC: (GNU) 4.7.1 20120606 (Red Hat 4.7.1-0.1.20120606)"
	.section	.note.GNU-stack,"",%progbits

RE: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

[David Laight]

From: James Hogan
> On 03/02/14 10:35, David Laight wrote:
> > From: James Hogan
> >> Combining __packed with __aligned(2) does the trick though (__packed
> >> alone sets __aligned(1) which is obviously going to be suboptimal).
...
> 
> Meta is also one of those arches, and according to my quick tests,
> __packed alone does correctly make it fall back to byte loads/stores,
> but with __packed __aligned(2) it uses 16bit loads/stores. I've also
> confirmed that with an ARM toolchain (see below for example).

I would either:
1a) Add explicit padding to the relevant structures so that they are
   multiple of 4 bytes.
or:
1b) #define some token to "__packed __aligned(2)" before all the structures
   that require changing, and use that in there definitions.
   This lets you comment on WHY you are doing it.
and:
2) Add a compile-time assert that the structures are the correct size.

Clearly you don't want to mark anything that contains a 32bit value
with __packed __aligned(2).

I'm not at all clear whether you are sometimes using a different compiler.

	David

Re: [PATCH] drivers: staging: lustre: lustre: include: add "__attribute__((packed))" for the related union

[Chen Gang]

On 02/03/2014 06:05 PM, David Laight wrote:
> From: Dan Carpenter
>> On Sat, Feb 01, 2014 at 09:57:39PM +0800, Chen Gang wrote:
>>> It seems, our kernel still stick to treate 'pack' region have effect
>>> with both 'align' and 'sizeof'.
>>
>> It's not about packed regions.  It's about unions.  It's saying the
>> sizeof() a union is a multiple of 4 unless it's packed.
>>
>> union foo {
>> 	short x;
>> 	short y;
>> };
>>
>> The author intended the sizeof(union foo) to be 2 but on metag arch then
>> it is 4.
> 
> The same is probably be true of: struct foo { _u16 bar; };
> 

I guess so.


> Architectures that define such alignment rules are a right PITA.

Sorry, I do not know about PITA (after google or wiki, I can not get
more related information).

Could you provide more information about PITA, thanks?


> You either need to get the size to 2 without using 'packed', or
> just not define such structures.

Excuse me, I don't quite understand your meaning. I guess your meaning
is:

  "normally, we should not use a struct/union like that, no matter what it is (2 or 4)".

Is it correct.


> It is worth seeing if adding aligned(2) will change the size - I'm
> not sure.
> 

Yes, it will/should make sure that it must be 2.


Thanks.
-- 
Chen Gang

Open, share and attitude like air, water and life which God blessed
--
To unsubscribe from this list: send the line "unsubscribe linux-metag" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html