From owner-cvs-src@FreeBSD.ORG Sat Nov 18 15:50:23 2006 Return-Path: X-Original-To: cvs-src@freebsd.org Delivered-To: cvs-src@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 9E32B16A4FB; Sat, 18 Nov 2006 15:50:23 +0000 (UTC) (envelope-from bde@zeta.org.au) Received: from mailout2.pacific.net.au (mailout2-3.pacific.net.au [61.8.2.226]) by mx1.FreeBSD.org (Postfix) with ESMTP id 1DDC643D83; Sat, 18 Nov 2006 15:50:16 +0000 (GMT) (envelope-from bde@zeta.org.au) Received: from mailproxy1.pacific.net.au (mailproxy1.pacific.net.au [61.8.2.162]) by mailout2.pacific.net.au (Postfix) with ESMTP id 94FDE10A3BD; Sun, 19 Nov 2006 02:50:17 +1100 (EST) Received: from katana.zip.com.au (katana.zip.com.au [61.8.7.246]) by mailproxy1.pacific.net.au (Postfix) with ESMTP id 75A178C03; Sun, 19 Nov 2006 02:50:16 +1100 (EST) Date: Sun, 19 Nov 2006 02:50:15 +1100 (EST) From: Bruce Evans X-X-Sender: bde@delplex.bde.org To: "M. Warner Losh" In-Reply-To: <20061118.071625.-432825382.imp@bsdimp.com> Message-ID: <20061119014447.N16088@delplex.bde.org> References: <20061117201432.X11101@delplex.bde.org> <20061117.105304.1769993002.imp@bsdimp.com> <20061118214618.U15111@delplex.bde.org> <20061118.071625.-432825382.imp@bsdimp.com> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed Cc: src-committers@freebsd.org, yar@comp.chem.msu.su, cvs-all@freebsd.org, phk@phk.freebsd.dk, cvs-src@freebsd.org, jkoshy@freebsd.org Subject: Re: cvs commit: src/include ar.h X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 18 Nov 2006 15:50:23 -0000 On Sat, 18 Nov 2006, M. Warner Losh wrote: > In message: <20061118214618.U15111@delplex.bde.org> > Bruce Evans writes: > : On Fri, 17 Nov 2006, M. Warner Losh wrote: > : > : > In message: <20061117201432.X11101@delplex.bde.org> > : > Bruce Evans 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 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