From owner-svn-src-user@FreeBSD.ORG Thu Oct 25 16:16:01 2012 Return-Path: Delivered-To: svn-src-user@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id B22D0C90 for ; Thu, 25 Oct 2012 16:16:01 +0000 (UTC) (envelope-from andre@freebsd.org) Received: from c00l3r.networx.ch (c00l3r.networx.ch [62.48.2.2]) by mx1.freebsd.org (Postfix) with ESMTP id 0D0F78FC0C for ; Thu, 25 Oct 2012 16:16:00 +0000 (UTC) Received: (qmail 42159 invoked from network); 25 Oct 2012 17:53:37 -0000 Received: from c00l3r.networx.ch (HELO [127.0.0.1]) ([62.48.2.2]) (envelope-sender ) by c00l3r.networx.ch (qmail-ldap-1.03) with SMTP for ; 25 Oct 2012 17:53:37 -0000 Message-ID: <508965B3.2020705@freebsd.org> Date: Thu, 25 Oct 2012 18:15:47 +0200 From: Andre Oppermann User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:16.0) Gecko/20121010 Thunderbird/16.0.1 MIME-Version: 1.0 To: attilio@FreeBSD.org Subject: Re: svn commit: r241889 - in user/andre/tcp_workqueue/sys: arm/arm cddl/compat/opensolaris/kern cddl/contrib/opensolaris/uts/common/dtrace cddl/contrib/opensolaris/uts/common/fs/zfs ddb dev/acpica dev/... References: <201210221418.q9MEINkr026751@svn.freebsd.org> <201210241136.06154.jhb@freebsd.org> <201210241414.30723.jhb@freebsd.org> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Cc: mdf@freebsd.org, src-committers@freebsd.org, John Baldwin , svn-src-user@freebsd.org, Jeff Roberson , Bruce Evans X-BeenThere: svn-src-user@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list List-Id: "SVN commit messages for the experimental " user" src tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Thu, 25 Oct 2012 16:16:01 -0000 On 25.10.2012 05:14, Attilio Rao wrote: > On Wed, Oct 24, 2012 at 9:13 PM, Attilio Rao wrote: >> On Wed, Oct 24, 2012 at 7:14 PM, John Baldwin wrote: >>> On Wednesday, October 24, 2012 11:41:24 am Attilio Rao wrote: >>>> On Wed, Oct 24, 2012 at 4:36 PM, John Baldwin wrote: >>>>> On Wednesday, October 24, 2012 11:24:22 am Attilio Rao wrote: >>>>>> On Wed, Oct 24, 2012 at 4:09 PM, Attilio Rao wrote: >>>>> union mtx_foo { >>>>> struct mtx lock; >>>>> char junk[roundup2(sizeof(struct mtx), CACHE_LINE_SIZE)]; >>>>> } __aligned_CACHE_LINE_SIZE; >>>>> >>>>>> then let mtx_* functions to accept void ptrs and cast them to struct >>>>>> mtx as long as the functions enter. >>>>> >>>>> Eh, that removes all compile time type checks. That seems very dubious to me. >>>> >>>> Well right now fast path already has a fair amount of macros wrapping >>>> the operations, which don't really enforce any type checks. >>> >>> Sure they do. They still call a function that takes a 'struct mtx *' even >>> if it isn't called in the fast path. If you pass a 'struct sx *' to >>> mtx_lock() it will fail to compile. That needs to stay that way. >> >> I think that with some trickery using CTASSERT() and typeof() we may >> be able to enforce sanity even with void * arguments. > > I think I've had a better idea for this. > In our locking scheme we already rely on the fact that lock_object > will always be present in all our locking primitives and that it will > be the first object of every locking primitives. This is an assumption > we must live with in order to correctly implement lock classes. I > think we can use the same concept in order to use the same KPI for the > 2 different structures (struct mtx and struct mtx_unshare) and keep > the compile time ability to find stupid bugs. I think we're completely overdoing it. On amd64 the size difference of 64B cache line aligning and padding all mutex, sx and rw_lock structures adds the tiny amount of 16K on a GENERIC kernel of 19MB. That is a 0.009% increase in size. Of course dynamically allocated memory that includes a mutex grows a tiny bit at well. Hence I propose to unconditionally slap __aligned(CACHE_LINE_SIZE) into all locking structures and be done with it. As an added benefit we don't have to worry about individual micro-optimizations on a case by case basis. > What I propose is that we assume mtx_lock remains always the second > member of the struct mtx/mtx_unshare and no other lock is allowed to > use such member name. This happens natually nowadays so there is no > problem in having such a rule. What does that allows to do is to pass > the address of the mtx_lock member to the underlying functions and > from there we can get back the address of the mutex (because we assume > that mtx_lock will be just after the first mandatory member > lock_object). > > Here is a patch that implements mtx_init() in the way I think about: > http://people.freebsd.org/~attilio/mtx_unshare_poc.patch > > this should give us all the desired effects. In this patch I've used > volatile uintptr_t * but it can certainly be void * too, if you prefer > less verbose. > If you agree with this idea I can hack a patch right away. -- Andre