Date: Thu, 25 Oct 2012 18:15:47 +0200 From: Andre Oppermann <andre@freebsd.org> To: attilio@FreeBSD.org Cc: mdf@freebsd.org, src-committers@freebsd.org, John Baldwin <jhb@freebsd.org>, svn-src-user@freebsd.org, Jeff Roberson <jroberson@jroberson.net>, Bruce Evans <brde@optusnet.com.au> 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/... Message-ID: <508965B3.2020705@freebsd.org> In-Reply-To: <CAJ-FndBqRpkBhCntd2aqwVYPu%2B2EHGeuXr5srLtrNNDK-ButxA@mail.gmail.com> References: <201210221418.q9MEINkr026751@svn.freebsd.org> <201210241136.06154.jhb@freebsd.org> <CAJ-FndAG-Qp%2B1aQvoL7YRj=R151Qe9_wNrUeOAaDsdYao_-zCQ@mail.gmail.com> <201210241414.30723.jhb@freebsd.org> <CAJ-FndAu6BGeMMbtFTLaSqy82mbhM9CVEyJ3Lb1WhAogJr59yA@mail.gmail.com> <CAJ-FndBqRpkBhCntd2aqwVYPu%2B2EHGeuXr5srLtrNNDK-ButxA@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On 25.10.2012 05:14, Attilio Rao wrote: > On Wed, Oct 24, 2012 at 9:13 PM, Attilio Rao <attilio@freebsd.org> wrote: >> On Wed, Oct 24, 2012 at 7:14 PM, John Baldwin <jhb@freebsd.org> wrote: >>> On Wednesday, October 24, 2012 11:41:24 am Attilio Rao wrote: >>>> On Wed, Oct 24, 2012 at 4:36 PM, John Baldwin <jhb@freebsd.org> wrote: >>>>> On Wednesday, October 24, 2012 11:24:22 am Attilio Rao wrote: >>>>>> On Wed, Oct 24, 2012 at 4:09 PM, Attilio Rao <attilio@freebsd.org> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?508965B3.2020705>