2.5 fast poll on ppc64
Quoted 17 lines · Anton Blanchard
I was unable to boot 2.5-BK on ppc64 and narrowed it down to the fast poll patch. I found:
offsetof(struct poll_list, entries)== 12 butsizeof(struct poll_list)== 16This means pp+1 did not match up with
pp->entries. Im not sure what the alignment requirements are for a zero length struct (ie is this a compiler bug) but the following patch fixes the problem and also changes ->len to a long to ensure 8 byte alignment of ->entries on 64bit archs.Anton
=====
fs/select.c1.15 vs edited =====
fs/select.c line 367
Quoted 7 lines
fs/select.c +1
-1@@ -362,7 +362,7 @@struct poll_list {struct poll_list *next; -int len; +long len;struct pollfd entries[0 ]; };
To me (I'm a compiler writer) it looks like your compiler did NOT mess up. Assuming struct pollfd has 32-bit alignment, the compiler is doing the right thing by starting entries[] at offset 12. The 16-byte size for struct poll_list is because the 'next' field has 64-bit alignment, which forces the compiler to pad struct poll_lists's size to a multiple of 8 bytes, i.e. 16 bytes in this case. So the compiler is not broken.
fs/select.c line 475
Quoted 8 lines
fs/select.c +1
-1@@ -471,7 +471,7 @@ walk->next = pp; walk = pp; -if (copy_from_user (pp+1 , ufds + nfds-i, +if (copy_from_user (pp->entries, ufds + nfds-i,sizeof (struct pollfd )*pp->len)) {
But the old code which assumed pp+1 == pp->entries is so horribly broken I can't find words for it. s/pp+1/pp->entries/ is the correct fix.
/Mikael
Quoted 19 lines · Mikael Pettersson
Quoted 18 lines · Anton Blanchard
I was unable to boot 2.5-BK on ppc64 and narrowed it down to the fast poll patch. I found:
offsetof(struct poll_list, entries)== 12 butsizeof(struct poll_list)== 16This means pp+1 did not match up with
pp->entries. Im not sure what the alignment requirements are for a zero length struct (ie is this a compiler bug) but the following patch fixes the problem and also changes ->len to a long to ensure 8 byte alignment of ->entries on 64bit archs.Anton
=====
fs/select.c1.15 vs edited =====fs/select.c +0
-0@@ -362,7 +362,7 @@
Quoted 16 lines
fs/select.c line 367
Quoted 6 lines
fs/select.c +1
-1struct poll_list {struct poll_list *next; -int len; +long len;struct pollfd entries[0 ]; };To me (I'm a compiler writer) it looks like your compiler did NOT mess up. Assuming struct pollfd has 32-bit alignment, the compiler is doing the right thing by starting entries[] at offset 12. The 16-byte size for struct
poll_listis because the 'next' field has 64-bit alignment, which forces the compiler to pad struct poll_lists's size to a multiple of 8 bytes, i.e. 16 bytes in this case. So the compiler is not broken.
I would agree, but there is a special restriction on offsetof() and sizeof() of structures with flexible array members: section 6.7.2.1, clause 16:
First, the size of the structure shall be equal to the offset of the last element of an otherwise identical structure that replaces the flexible array member with an array of unspecified length.
The simplest test case I've found is
struct a1 { int a; char b; short c[];};
struct a2 { int a; char b; short c[1];};
sizeof(struct a{1,2}) is 8.
offsetof(struct a{1,2}, c) is 6.
--> sizeof(struct a1) != offsetof(struct a2, c)
Quoted 4 lines
Quoted 4 lines
fs/select.c +0
-0@@ -471,7 +471,7 @@ walk->next = pp;
Quoted 9 lines
fs/select.c line 475
Quoted 4 lines
fs/select.c +1
-1walk = pp; -if (copy_from_user (pp+1 , ufds + nfds-i, +if (copy_from_user (pp->entries, ufds + nfds-i,sizeof (struct pollfd )*pp->len)) {But the old code which assumed pp+1 ==
pp->entriesis so horribly broken I can't find words for it. s/pp+1/pp->entries/ is the correct fix.
I agree. Should we fix the kmalloc allocations, too?
- pp = kmalloc(sizeof(struct poll_list)+
+ pp = kmalloc(offsetof(struct poll_list,entries)+
sizeof(struct pollfd)* (i>POLLFD_PER_PAGE?POLLFD_PER_PAGE:i), GFP_KERNEL);
--
Manfred