cvs commit: src/include ar.h

[Bruce Evans]

On Sat, 18 Nov 2006, M. Warner Losh wrote:

> In message: <20061118214618.U15111 at delplex.bde.org>
>            Bruce Evans <bde at zeta.org.au> writes:
> : On Fri, 17 Nov 2006, M. Warner Losh wrote:
> :
> : > In message: <20061117201432.X11101 at delplex.bde.org>
> : >            Bruce Evans <bde at zeta.org.au> writes:
> : > : For that the comment should be something like:
> : > :
> : > :  	__packed;	/* Align (sic) to work around bugs on arm (*). */
> : > :
> : > : but I doubt that arm is that broken.
> : > :
> : > : (*) See this thread for more details.
> : >
> : > But they aren't bugs.
> :
> : Er, this thread gived the details of why they are bugs.
>
> I read the thread.  They gave reasons why this broke the assumptions
> we've made til now.  These assumptions weren't allowed by the
> standard.  Hence my contention that they aren't bugs.

You seem to have missed the main details.  No assumptions about alignment
were made for struct ip.  It is just required to be aligned.  Quoting
<net/ethernet.h> again:

%%%
/*
   * Mbuf adjust factor to force 32-bit alignment of IP header.
   * Drivers should do m_adj(m, ETHER_ALIGN) when setting up a
   * receive so the upper layers get the IP header properly aligned
   * past the 14-byte Ethernet header.
   */
#define	ETHER_ALIGN		2	/* driver adjust for IP hdr alignment */
%%%

This needs to assume things about packing and alignment to work, but
these assumptions are satisfied by all supported arches, and are made
for much more that struct ip.

Some assumptions about packing were made for struct ip.  These are satisfied
for all supported arches.

Declaring struct ip as __packed without also declaring it as __aligned(4):

    o bogotifies the above documenation.

    o punishes all non-broken drivers that do the adjustment.

    o rewards all broken drivers that don't do the adjustment.

    o inhibit detection of regressions from non-broken to broken.

    o gives slow accesses to struct ip on all arches with strict alignment
      requirements.  On other arches, the accesses are only pessimized for
      cases where an instance of struct ip is actually misaligned, since the
      compiler doesn't generate extra instructions to access words a byte at
      a time.  Whether a struct ip is aligned depends on how it was generated.
      If it was generated by a non-broken driver then it is sure to be aligned;
      if it was generated by a broken driver then it is sure to be misaligned;
      if it is a local variable then its alignment depends on the compiler's
      strategy for packing objects that don't need alignment.

      Misaligned struct ip's are especially bogus for arm, since arm wants
      extra alignment for chars so as to reduce the number of 1-byte accesses,
      even for chars, but using __packed increases the number of 1-byte
      accesses even for non-chars.

    o breaks all multi-byte accesses to struct ip of the for foo(&ip->mem).
      This is a compiler bug.  When this is fixed, the __packed kludge
      will become less expedient since either foo()'s API will have to be
      changed to accept packed objects or ip->mem will have to be laboriously
      copied to and from a non-packed object just like it should have been
      all along to be correct and inefficient.

Declaring struct ip as both __packed and __aligned(4) would mainly fix
all of the above (until m_adj()'s packing and alignment assumptions
break) and expose broken drivers and/or ethernet infrastructure.  I
doubt that there are many such drivers, but note that my favourite
etherernet driver (fxp) doesn't have a single reference to either of
m_adj() or ETHER_ALIGN.  em and bge have several such references, and
doing the alignment is apparently a large performance penalty so they
have ifdefs on __NO_STRICT_ALIGNMENT to give the pessimized case for
arches with strict alignment requirements.  __NO_STRICT_ALIGNMENT is
only defined for amd64 and i386 and only used in the following drivers
in /sys/dev: bge, dc, em, stge.

Bruce