2.5 fast poll on ppc64

2.5 fast poll on ppc64

Post by Anton Blanchar » Fri, 27 Dec 2002 09:00:09



Hi,

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 but
sizeof(struct poll_list) == 16

This 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.c 1.15 vs edited =====
--- 1.15/fs/select.c    Sat Dec 21 20:42:41 2002

 struct poll_list {
        struct poll_list *next;
-       int len;
+       long len;
        struct pollfd entries[0];
 };

                        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)) {
                        err = -EFAULT;
                        goto out_fds;
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

2.5 fast poll on ppc64

Post by Manfred Sprau » Fri, 27 Dec 2002 14:20:04



>Hi,

>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 but
>sizeof(struct poll_list) == 16

>This 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.

That seems to be the root of the problem: ->entries requires 4 byte
alignment, while struct poll_list requires 8 byte alignment. Thus the
compiler aligns entries to a 4-byte boundary, and rounds up
sizeof(struct poll_list) to 16.

Attached is a small test case that shows the problem on both 64-bit and
32-bit platforms. I'm not sure if this is a gcc bug: the Compaq C
compiler on Tru64 and MSVC on Windows generate the same structure layout.

--
    Manfred

[ test.c 1K ]
/*
 * Testcase for a possible compiler bug:
 * structure with flexible array member.
 * offsetof(struct,flexible_member) != sizeof(struct).
 *
 * That seems to be a violation of 6.7.2.1, constraint 16 of the C99
 * standard:
 * "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.
 */
#include <stdio.h>

/*
 * structure: sizeof() = 4.
 * alignment: 2, due to the short.
 */
struct pollfd {
        short fd;
        char events;
        char revents;

Quote:};

/*
 * sizeof: 12
 * alignment: 4, due to the int.
 */
struct n2 {
        /* offset 0 */
        int m;
        /* offset 4 */
        char n;
        /* one byte padding, 'struct pollfd' requires 2 byte alignment */
        /* offset 6 */
        struct pollfd a[1];
        /* offset 10: two bytes padding for 4 byte alignment,
         * required for the int in this structure */

Quote:};

/*
 * sizeof: 8.
 * XXX
 * Is this correct?
 * offsetof(struct n1, a) is 6
 * --> offsetof(struct n1, a) != sizeof(struct n1)
 * XXX
 */
struct n1 {
        /* offset 0 */
        int m;
        /* offset 4 */
        char n;
        /* one byte padding, 'struct pollfd' required 2 byte alignment */
        /* offset 6 */
        struct pollfd a[];

Quote:};

#define offsetof(a,b) \
                (int)&((a*)NULL)->b

int main(void)
{
        printf("pollfd: sizeof: %d.\n", sizeof(struct pollfd));
        printf("fix: sizeof: %d, offsetof: %d.\n", sizeof(struct n2), offsetof(struct n2, a));
        printf("var: sizeof: %d, offsetof: %d.\n", sizeof(struct n1), offsetof(struct n1, a));
        return 0;

Quote:}


 
 
 

2.5 fast poll on ppc64

Post by Mikael Pettersso » Fri, 27 Dec 2002 15:10:06


Quote:Anton Blanchard writes:

 > 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 but
 > sizeof(struct poll_list) == 16
 >
 > This 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.c 1.15 vs edited =====
 > --- 1.15/fs/select.c      Sat Dec 21 20:42:41 2002
 > +++ edited/fs/select.c    Thu Dec 26 17:31:16 2002

 >  
 >  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.

 >  

 >                   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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

2.5 fast poll on ppc64

Post by Manfred Sprau » Fri, 27 Dec 2002 16:00:12



>Anton Blanchard writes:
> > 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 but
> > sizeof(struct poll_list) == 16

> > This 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.c 1.15 vs edited =====
> > --- 1.15/fs/select.c  Sat Dec 21 20:42:41 2002
> > +++ edited/fs/select.c        Thu Dec 26 17:31:16 2002

> >  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.

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)


> >                       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.

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

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

2.5 fast poll on ppc64

Post by Mikael Pettersso » Fri, 27 Dec 2002 23:00:09




>>Anton Blanchard writes:
>> > 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 but
>> > sizeof(struct poll_list) == 16
...
>>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.

>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)

Oh dear. I checked my C9x draft copy and you seem to be right.
The standard states that sizeof(struct a1) == offsetof(struct a1, c),
but both gcc (2.95.3 and 3.2) and Intel's icc (7.0) get it wrong on x86:
they make sizeof(struct a1) == 8 but offsetof(struct a1, c) == 6.

Technically speaking, the kernel code which uses 'entries[0]' is
non-compliant since the proper syntax is 'entries[]', but the empty
array size syntax isn't implemented in gcc 2.95.3.

Quote:>>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.

My mistake. The old code is Ok for C99, but broken for ANSI-C.

Quote:>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);

Yes, this should be changed as you suggest. The old code only
works in C99-compliant implementations, but we now know that both
gcc and icc get this wrong, so it seems prudent to revert to
the classical formulation, using the 'entries[0]' declaration
syntax and offsetof() instead of sizeof().

/Mikael
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

2.5 fast poll on ppc64

Post by Linus Torvald » Fri, 27 Dec 2002 23:40:07



> Technically speaking, the kernel code which uses 'entries[0]' is
> non-compliant since the proper syntax is 'entries[]', but the empty
> array size syntax isn't implemented in gcc 2.95.3.

The two things have two totally different semantics:

 - array[0] is a zero-sized array, and is a long-time gcc extension that
   has nothing to do with the modern "flexible array". The kernel uses
   zero-sized arrays, because flexible arrays simply aren't historically
   supported by gcc at all.

 - array[] is the standard "flexible array" thing, and has different rules
   than array[0]. For example, sizeof() is undefined on flexible arrays,
   but is well-defined on zero-sized ones (at zero). Also, the alignment
   constraints are potentially quite different.

The gcc people want to make the two behave fairly similarly to ease
implementation issues, but they are definitely not the same.

Quote:> My mistake. The old code is Ok for C99, but broken for ANSI-C.

The old code is ugly and arguably always broken, I agree 100% with making
the usage code use "pp->entries". Whether the allocators use "sizeof" or
"offsetof" is secondary, at worst it ends up being conservative.

                Linus

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

2.5 fast poll on ppc64

Post by Manfred Sprau » Fri, 27 Dec 2002 23:50:06



>>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)

>Oh dear. I checked my C9x draft copy and you seem to be right.
>The standard states that sizeof(struct a1) == offsetof(struct a1, c),
>but both gcc (2.95.3 and 3.2) and Intel's icc (7.0) get it wrong on x86:
>they make sizeof(struct a1) == 8 but offsetof(struct a1, c) == 6.

I've filed a gcc bug, no 9058: As the reply, I got a mail from Joseph
Myers with links to TC to the C99 standard:
http://std.dkuug.dk/JTC1/SC22/WG14/www/docs/n983.htm
http://std.dkuug.dk/JTC1/SC22/WG14/www/docs/n987.htm

- Show quoted text -

Quote:

>>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);

>Yes, this should be changed as you suggest. The old code only
>works in C99-compliant implementations, but we now know that both
>gcc and icc get this wrong, so it seems prudent to revert to
>the classical formulation, using the 'entries[0]' declaration
>syntax and offsetof() instead of sizeof().

I found an even simpler formula:

    offsetof(struct poll_list,entries[i>POLLFD_PER_PAGE?POLLFD_PER_PAGE:i];

I'll ask the gcc guys if that formula is a portable solution.

--
    Manfred

-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

 
 
 

2.5 fast poll on ppc64

Post by Richard Henderso » Sat, 28 Dec 2002 02:20:08



> >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.
[...]
> Oh dear. I checked my C9x draft copy and you seem to be right.
> The standard states that sizeof(struct a1) == offsetof(struct a1, c),
> but both gcc (2.95.3 and 3.2) and Intel's icc (7.0) get it wrong on x86:
> they make sizeof(struct a1) == 8 but offsetof(struct a1, c) == 6.

Indeed, *every* compiler that supports flexible array members
does it this way.  On the gcc lists we've conjectured that this
will wind up being a Defect Report, and so have elected not to
change anything for the nonce.

That said, you should also note that "struct S foo[0]" is *not*
a flexible array member; "struct S foo[]" is.  The former is
gcc's zero-length array extension.  There are subtle differences
between the two features.  See the gcc docs for details.

r~
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in

More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/