Re: 2.6.9 PageAnon bug

Re: 2.6.9 PageAnon bug

[Andi Kleen]

Hugh Dickins <hugh@veritas.com> writes:
 
> But what does "aligned(2)" or "aligned(4)" do on 64-bit machines -
> any danger of it aligning stupidly?  I think not, but know little.

It will align stupidly. 

This means on x86-64 i don't care too much because the misalignment
penalty on K8 is only 1 cycle and not that much worse on P4, but 
others may care more.

-Andi
 

Re: 2.6.9 PageAnon bug

[Hugh Dickins]

On 20 Oct 2004, Andi Kleen wrote:
> Hugh Dickins <hugh@veritas.com> writes:
>  
> > But what does "aligned(2)" or "aligned(4)" do on 64-bit machines -
> > any danger of it aligning stupidly?  I think not, but know little.
> 
> It will align stupidly. 

I was imagining not in this case, where the i_data follows immediately
after the struct address_space *i_mapping.  But let's not rely on that.

> This means on x86-64 i don't care too much because the misalignment
> penalty on K8 is only 1 cycle and not that much worse on P4, but 
> others may care more.

I would care: not so much the penalty, as the danger of surprise.

Would __attribute__((aligned((sizeof(long))))) seem better to you?

Do you think it would be better on the declaration of struct
address_space itself, than on the struct address_space i_data?

Thanks for the feedback,
Hugh

RE: 2.6.9 PageAnon bug

[Mikael Starvik]

>Ah, sorry for messing CRIS up, I was unaware of that.

Well, it's kind of odd nowadays to have the freedom of arbitrary alignment. 

>I don't think that's ugly, and the comment is good.
>It only actually needs "aligned(2)", would that be better?

Yes, aligned(2) is enough.

>But what does "aligned(2)" or "aligned(4)" do on 64-bit machines -
>any danger of it aligning stupidly?  I think not, but know little.

Same here, we need input from the 64-bit world (or make it aligned(8)).

>>Another possible patch would be to move i_data above i_bytes and i_sock.
>Really?  Precarious, I think you'd still need to insist on alignment.

I agree that there may be compilers out there that actually pads the
structure to make the members unaligned. So you are correct, aligned()
should be used to be safe (until memory allocation routines start to return
unaligned addresses).

Will you send this upstream to Andrew?

Thanks for the quick response!
/Mikael

RE: 2.6.9 PageAnon bug

[Hugh Dickins]

On Wed, 20 Oct 2004, Mikael Starvik wrote:

> >Ah, sorry for messing CRIS up, I was unaware of that.
> 
> Well, it's kind of odd nowadays to have the freedom of arbitrary alignment. 
> 
> >I don't think that's ugly, and the comment is good.
> >It only actually needs "aligned(2)", would that be better?
> 
> Yes, aligned(2) is enough.
> 
> >But what does "aligned(2)" or "aligned(4)" do on 64-bit machines -
> >any danger of it aligning stupidly?  I think not, but know little.
> 
> Same here, we need input from the 64-bit world (or make it aligned(8)).
> 
> >>Another possible patch would be to move i_data above i_bytes and i_sock.
> >Really?  Precarious, I think you'd still need to insist on alignment.
> 
> I agree that there may be compilers out there that actually pads the
> structure to make the members unaligned. So you are correct, aligned()
> should be used to be safe (until memory allocation routines start to return
> unaligned addresses).
> 
> Will you send this upstream to Andrew?

Not without confirmation from people who know more about
__attribute__((aligned(N))) than we do.  I notice init.h has an
		__attribute__((aligned((sizeof(long)))))
which perhaps would be better (though going the other way than from 4 to 2).

And would it be better on the declaration of struct address_space itself,
than on the struct address_space i_data?

If nobody chimes in to help us, I'll ping a few people in a day or two:
for now use what works for you.

Hugh

2.6.9 PageAnon bug

[Mikael Starvik]

There is at least one architecture supported by 2.6.9 that has no alignment
restrictions what so ever and no struct padding added by the compiler. 

The patch named "rmaplock: PageAnon in mapping" in 2.6.9 doesn't work for
this architecture because it assumes that the address of a member in a
struct can't be odd. 

One possible but ugly patch below. Another possible patch would be to move
i_data above i_bytes and i_sock. I would really like a cleaner patch but I
guess its a bad idea to add a new field to struct page?

Index: fs.h
===================================================================
RCS file: /usr/local/cvs/linux/os/lx25/include/linux/fs.h,v
retrieving revision 1.20
retrieving revision 1.21
diff -r1.20 -r1.21
449c449,453
< 	struct address_space	i_data;
---
> 	/* The LSB in i_data below is used for the PAGE_MAPPING_ANON flag. 
> 	 * This assumes that the address of this member isn't odd which
> 	 * is not true for all architectures. Force the compiler to align
it.
> 	 */
> 	struct address_space	i_data __attribute__ ((aligned(4)));

Anyone who knows about similar usage of bit 0 and/or 1 in pointers
anywhere?

/Mikael 

PS. The architecture I'm referring to is CRIS but there may be more with the
same sloppyness regarding alignment. DS

Re: 2.6.9 PageAnon bug

[Hugh Dickins]

On Wed, 20 Oct 2004, Mikael Starvik wrote:

> There is at least one architecture supported by 2.6.9 that has no alignment
> restrictions what so ever and no struct padding added by the compiler. 

Ah, sorry for messing CRIS up, I was unaware of that.

> The patch named "rmaplock: PageAnon in mapping" in 2.6.9 doesn't work for
> this architecture because it assumes that the address of a member in a
> struct can't be odd. 

Yes.

> One possible but ugly patch below.

I don't think that's ugly, and the comment is good.
It only actually needs "aligned(2)", would that be better?

But what does "aligned(2)" or "aligned(4)" do on 64-bit machines -
any danger of it aligning stupidly?  I think not, but know little.

> Another possible patch would be to move i_data above i_bytes and i_sock.

Really?  Precarious, I think you'd still need to insist on alignment.

> I would really like a cleaner patch but I
> guess its a bad idea to add a new field to struct page?

You guessed right!

Hugh

> Index: fs.h
> ===================================================================
> RCS file: /usr/local/cvs/linux/os/lx25/include/linux/fs.h,v
> retrieving revision 1.20
> retrieving revision 1.21
> diff -r1.20 -r1.21
> 449c449,453
> < 	struct address_space	i_data;
> ---
> > 	/* The LSB in i_data below is used for the PAGE_MAPPING_ANON flag. 
> > 	 * This assumes that the address of this member isn't odd which
> > 	 * is not true for all architectures. Force the compiler to align
> it.
> > 	 */
> > 	struct address_space	i_data __attribute__ ((aligned(4)));
> 
> Anyone who knows about similar usage of bit 0 and/or 1 in pointers
> anywhere?
> 
> /Mikael 
> 
> PS. The architecture I'm referring to is CRIS but there may be more with the
> same sloppyness regarding alignment. DS