From owner-svn-src-user@FreeBSD.ORG Thu Oct 25 03:14:27 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 B109E8F2; Thu, 25 Oct 2012 03:14:27 +0000 (UTC) (envelope-from asmrookie@gmail.com) Received: from mail-la0-f54.google.com (mail-la0-f54.google.com [209.85.215.54]) by mx1.freebsd.org (Postfix) with ESMTP id 40C4B8FC14; Thu, 25 Oct 2012 03:14:25 +0000 (UTC) Received: by mail-la0-f54.google.com with SMTP id e12so1103126lag.13 for ; Wed, 24 Oct 2012 20:14:19 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=mime-version:reply-to:sender:in-reply-to:references:date :x-google-sender-auth:message-id:subject:from:to:cc:content-type; bh=9KgevtZKm9pwLFGk8J5heidf/sDFFrwqM3cBbrVMos4=; b=jR2B8EKQM6hbnLv+7oidz0IOc6D7epQvzeSkRHD0VddMyVFjM3d62G98VKA5zmeXtb ENSn8yt3VTeNN3ruIgUNS/KjUHS4ddYS5ngc+aoPdBvVQsSogk9bgNtH01PjxKWS4t8P 4PfILhe//SEmVGJy2HkyKegJKmVaBMV/6kgAeZIf9ES1O4HX7XDoJMy42e3e93SSLMbs l4SOuzb87R6rSh/a6WfsyQXWIf+ZcoJqKQKZSQW1ZMvnjBZef6jsk5XmVuK3UCljaNPy GzMKKeYQ/IiFV4SR20Q7DCqltyiYtN6vNBo21LffdEAVexOCqd5t3hulfOs2tvybJdLM isHQ== MIME-Version: 1.0 Received: by 10.112.82.103 with SMTP id h7mr7066652lby.50.1351134859041; Wed, 24 Oct 2012 20:14:19 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.30.37 with HTTP; Wed, 24 Oct 2012 20:14:18 -0700 (PDT) In-Reply-To: References: <201210221418.q9MEINkr026751@svn.freebsd.org> <201210241136.06154.jhb@freebsd.org> <201210241414.30723.jhb@freebsd.org> Date: Thu, 25 Oct 2012 04:14:18 +0100 X-Google-Sender-Auth: MKc7XSKvIUT1Y2epnltvUSJxoHw Message-ID: 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/... From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 Cc: mdf@freebsd.org, src-committers@freebsd.org, Andre Oppermann , svn-src-user@freebsd.org, Jeff Roberson , Bruce Evans X-BeenThere: svn-src-user@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: attilio@FreeBSD.org 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 03:14:27 -0000 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: >>> >> > On Wed, Oct 24, 2012 at 3:45 PM, John Baldwin wrote: >>> >> >> On Wednesday, October 24, 2012 10:34:34 am Attilio Rao wrote: >>> >> >>> On Wed, Oct 24, 2012 at 3:05 PM, John Baldwin wrote: >>> >> >>> > On Tuesday, October 23, 2012 7:20:04 pm Andre Oppermann wrote: >>> >> >>> >> On 24.10.2012 00:15, mdf@FreeBSD.org wrote: >>> >> >>> >> > On Tue, Oct 23, 2012 at 7:41 AM, Andre Oppermann >>> >> >> wrote: >>> >> >>> >> >> Struct mtx and MTX_SYSINIT always occur as pair next to each other. >>> >> >>> >> > >>> >> >>> >> > That doesn't matter. Language basics like variable definitions should >>> >> >>> >> > not be obscured by macros. It either takes longer to figure out what >>> >> >>> >> > a variable is (because one needs to look up the definition of the >>> >> >>> >> > macro) or makes it almost impossible (because now e.g. cscope doesn't >>> >> >>> >> > know this is a variable definition. >>> >> >>> >> >>> >> >>> >> Sigh, cscope doesn't expand macros? >>> >> >>> >> >>> >> >>> >> Is there a way to do the cache line alignment in a sane way without >>> >> >>> >> littering __aligned(CACHE_LINE_SIZE) all over the place? >>> >> >>> > >>> >> >>> > I was hoping to do something with an anonymous union or some such like: >>> >> >>> > >>> >> >>> > union mtx_aligned { >>> >> >>> > struct mtx; >>> >> >>> > char[roundup2(sizeof(struct mtx), CACHE_LINE_SIZE)]; >>> >> >>> > } >>> >> >>> > >>> >> >>> > I don't know if there is a useful way to define an 'aligned mutex' type >>> >> >>> > that will transparently map to a 'struct mtx', e.g.: >>> >> >>> > >>> >> >>> > typedef struct mtx __aligned(CACHE_LINE_SIZE) aligned_mtx_t; >>> >> >>> >>> >> >>> Unfortunately that doesn't work as I've verified with alc@ few months ago. >>> >> >>> The __aligned() attribute only works with structures definition, not >>> >> >>> objects declaration. >>> >> >> >>> >> >> Are you saying that the typedef doesn't (I expect it doesn't), or that this >>> >> >> doesn't: >>> >> >> >>> >> >> struct mtx foo __aligned(CACHE_LINE_SIZE); >>> >> > >>> >> > I meant to say that such notation won't address the padding issue >>> >> > which is as import as the alignment. Infact, for sensitive locks, >>> >> > having just an aligned object is not really useful if the cacheline >>> >> > gets shared. >>> >> > In the end you will need to use explicit padding or use __aligned in >>> >> > the struct definition, which cannot be used as a general pattern. >>> >> >>> >> The quickest way I see this can be made general is to have a specific >>> >> struct defined in sys/_mutex.h like that >>> >> >>> >> struct mtx_unshare { >>> >> struct mtx lock; >>> >> char _pad[CACHE_LINE_SIZE - sizeof(struct mtx)]; >>> >> } __aligned(CACHE_LINE_SIZE); >>> > >>> > I think instead you want my union above that uses roundup2 in case a lock >>> > eats up multiple cache lines: >>> >>> Do you think locks can eat more than one cacheline? This would be >>> absolutely killer for performance. >> >> Not the lock cookie, but 'struct lock_object', etc. aren't entirely trivial. >> If you had a 32-bit platform with a 16-byte cache line size I wouldn't be >> surprised if the entire structure spilled over a cacheline. > > Cache line usually contains 8 words. > struct mtx is madeup only by 4 or 5 (depending if you are on 64 or 32 bits). > I think this is a no-concern and we should not encourage adding more > words to it anyway. > >>> > 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. 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. Thanks, Attilio -- Peace can only be achieved by understanding - A. Einstein