From owner-svn-src-all@FreeBSD.ORG Mon Feb 9 06:00:59 2015 Return-Path: Delivered-To: svn-src-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [8.8.178.115]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by hub.freebsd.org (Postfix) with ESMTPS id 325864D2; Mon, 9 Feb 2015 06:00:59 +0000 (UTC) Received: from mail110.syd.optusnet.com.au (mail110.syd.optusnet.com.au [211.29.132.97]) by mx1.freebsd.org (Postfix) with ESMTP id E9ACAAD2; Mon, 9 Feb 2015 06:00:58 +0000 (UTC) Received: from c211-30-166-197.carlnfd1.nsw.optusnet.com.au (c211-30-166-197.carlnfd1.nsw.optusnet.com.au [211.30.166.197]) by mail110.syd.optusnet.com.au (Postfix) with ESMTPS id 13BDA784423; Mon, 9 Feb 2015 17:00:50 +1100 (AEDT) Date: Mon, 9 Feb 2015 17:00:49 +1100 (EST) From: Bruce Evans X-X-Sender: bde@besplex.bde.org To: Oleksandr Tymoshenko Subject: Re: svn commit: r278431 - head/sys/contrib/vchiq/interface/vchiq_arm In-Reply-To: <201502090231.t192VS6C060751@svn.freebsd.org> Message-ID: <20150209170045.S1037@besplex.bde.org> References: <201502090231.t192VS6C060751@svn.freebsd.org> MIME-Version: 1.0 Content-Type: TEXT/PLAIN; charset=US-ASCII; format=flowed X-Optus-CM-Score: 0 X-Optus-CM-Analysis: v=2.1 cv=A5NVYcmG c=1 sm=1 tr=0 a=KA6XNC2GZCFrdESI5ZmdjQ==:117 a=PO7r1zJSAAAA:8 a=kj9zAlcOel0A:10 a=JzwRw_2MAAAA:8 a=j7Id98u7Vm7jTOBlUM4A:9 a=CjuIK1q_8ugA:10 Cc: svn-src-head@freebsd.org, svn-src-all@freebsd.org, src-committers@freebsd.org X-BeenThere: svn-src-all@freebsd.org X-Mailman-Version: 2.1.18-1 Precedence: list List-Id: "SVN commit messages for the entire src tree \(except for " user" and " projects" \)" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 09 Feb 2015 06:00:59 -0000 On Mon, 9 Feb 2015, Oleksandr Tymoshenko wrote: > Log: > Do not mark shared structures as __packed, it leads to race condition > > If structure packed as __packed clang (and probably gcc) generates > code that loads word fields (e.g. tx_pos) byte-by-byte and if it's > modified by VideoCore in the same time as ARM loads the value result > is going to be mixed combination of bytes from previous value and > new one. Most uses of __packed are bugs. It gives pessimizations as well as non-atomic accesses for sub-object accesses. I think the full bugs only occur when arch has strict alignment requirements and the alignment of the __packed objects is not known. This means that only lesser bugs occur on x86 (unless you enable alignment checking, but this arguably breaks the ABI). The compiler just generates possibly-misaligned full-width accesses if the arch doesn't have strict alignment requirements. Often the acceses turn out to be aligned at runtime. Otherwise, the hardware does them atomically, with a smaller efficiency penalty than split accesses. Many packed structs should also be declared as __aligned(N). This is rarely done. Old networking code in uses __packed just once, and this also uses __aligned(4) (for struct ip) Newer networking code in (for ipv6) is massively pessimized, with __packed used extensively and __aligned(N) never used with __packed. sctp is worse. It uses its own macro that defeats grepping for __packed. It has 82 instances of this in where ipv6 has only 34. Newer networking should need __packed less than old ones, since no one would misdesign a data struct so that it needs packing now. gcc documents the -Wpacked flag for finding some cases of bogus packing. It is rarely used. Bruce