From owner-svn-src-projects@FreeBSD.ORG Wed Oct 10 16:49:16 2012 Return-Path: Delivered-To: svn-src-projects@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id C231D2C2; Wed, 10 Oct 2012 16:49:16 +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 230278FC08; Wed, 10 Oct 2012 16:49:14 +0000 (UTC) Received: by mail-la0-f54.google.com with SMTP id e12so668155lag.13 for ; Wed, 10 Oct 2012 09:49:14 -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=UuqEr05qFzD9lUOG2xIR9bGCDNaEP40Eo2mYzcDPzCE=; b=OhDO2rFrqtTW8VyrwoCRcx2CKgv6OlB/DAviK2F/O6A9IQNw1caXdZugLhL1+VyJez FIs19SDxcdOBvZ2GivZjDp7wukVSp6C9x/fbY9jHPj3pnY+asmtGRt5f2Dt1LdmO18/k QWw9FUNkh2YMleHtAVYLUP7AYqJ1JR+mLbq+VY7kHXKd4NPA+4NdzKHJJCVs9jMd/RQr A0EOzfahHmSdr9h+NuuQvqXY9Y4OXeaH7p9v3C3APRRlOnvQ3MnUsZXsbwTd1hr8UeAZ CPkKDJJFLkFIR5+mx6EvS0rOuz/wURijSKUNEoQrDPb+Hdmdy/ERPsOTd8oeylTltuKW pNUA== MIME-Version: 1.0 Received: by 10.112.51.205 with SMTP id m13mr9234722lbo.75.1349887753931; Wed, 10 Oct 2012 09:49:13 -0700 (PDT) Sender: asmrookie@gmail.com Received: by 10.112.101.234 with HTTP; Wed, 10 Oct 2012 09:49:13 -0700 (PDT) In-Reply-To: References: <201207301350.q6UDobCI099069@svn.freebsd.org> <201207301732.33474.jhb@freebsd.org> Date: Wed, 10 Oct 2012 17:49:13 +0100 X-Google-Sender-Auth: DjoyqutlGnwQbzo8QK1wtszGr2s Message-ID: Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern From: Attilio Rao To: John Baldwin Content-Type: text/plain; charset=UTF-8 Cc: Davide Italiano , src-committers@freebsd.org, Jeff Roberson , Dimitry Andric , svn-src-projects@freebsd.org, Konstantin Belousov X-BeenThere: svn-src-projects@freebsd.org X-Mailman-Version: 2.1.14 Precedence: list Reply-To: attilio@FreeBSD.org List-Id: "SVN commit messages for the src " projects" tree" List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 10 Oct 2012 16:49:16 -0000 On Tue, Sep 18, 2012 at 1:13 AM, Attilio Rao wrote: > On Thu, Aug 2, 2012 at 9:56 PM, Attilio Rao wrote: >> On 7/30/12, John Baldwin wrote: > > [ trimm ] > >>> --- //depot/projects/smpng/sys/kern/kern_rmlock.c 2012-03-25 >>> 18:45:29.000000000 0000 >>> +++ //depot/user/jhb/lock/kern/kern_rmlock.c 2012-06-18 21:20:58.000000000 >>> 0000 >>> @@ -70,6 +70,9 @@ >>> } >>> >>> static void assert_rm(const struct lock_object *lock, int what); >>> +#ifdef DDB >>> +static void db_show_rm(const struct lock_object *lock); >>> +#endif >>> static void lock_rm(struct lock_object *lock, int how); >>> #ifdef KDTRACE_HOOKS >>> static int owner_rm(const struct lock_object *lock, struct thread >>> **owner); >> >> While here, did you consider also: >> - Abstracting compiler_memory_barrier() into a MI, compiler dependent function? > > So what do you think about this patch? (Please double-check the GIT log). The real reason why I wanted an abstract __compiler_membar() was to cleanly fix a bug that I think affects sched_pin()/sched_unpin() right now. Infact, I think the compiler can reorder operations around them making their usage completely bogus. In addition, td_pinned field is not even marked as volatile, thus I think currently the compiler can decide to do stupid things like caching the value of td_pinned in CPU registers or other optimizations which invalidate scheduler pinning. I reviewed all the possible races involved here with Jeff and John and after some discussion I think that in order to prevent these side effects the usage of compiler memory barriers in the operations is both due and effective (see the attached patch). However, please note that the presence of the compiler membar should not really change the code, assuming current compilers don't screw something up. I verified this is not the case with this very simple environment: - amd64 CURRENT configuration, without the kernel debugging options (in order to increase chances of optimizations) - kernel compared are pre- and post- patch - gcc is default FreeBSD: gcc version 4.2.1 20070831 patched [FreeBSD] [root@netboot-amd64 ~]# ls -al kernel.nopatch.md5 kernel.patch-novolatile.md5 -rwxr-xr-x 1 root wheel 19807568 Oct 10 16:04 kernel.nopatch.md5 -rwxr-xr-x 1 root wheel 19807568 Oct 10 16:41 kernel.patch-novolatile.md5 [root@netboot-amd64 ~]# md5 kernel.nopatch.md5 MD5 (kernel.nopatch.md5) = 7c5c5e33f2547a5e5bc701a3b124f0d9 [root@netboot-amd64 ~]# md5 kernel.patch-novolatile.md5 MD5 (kernel.patch-novolatile.md5) = 91c51afb4cc4ad229cc28da2024d8f54 So as you can see the size of the kernel is not changed but the md5 CRC actually is. This should point in the direction that code actually might have changed (unless someone has a good explanation for this already) and then there could be some code reshuffling due to the use of compiler membars. I also tried a version using volatile for td_pinned. I don't see why this would be needed, but maybe someone would use this for consistency. The following kernel.patch.md5 contains basically the membars and the volatile marker for td_pinned: [root@netboot-amd64 ~]# ls -al kernel.nopatch.md5 kernel.patch.md5 -rwxr-xr-x 1 root wheel 19807568 Oct 10 16:04 kernel.nopatch.md5 -rwxr-xr-x 1 root wheel 19807824 Oct 10 16:09 kernel.patch.md5 As you can see the size of the kernel actually increased. I think that gcc should probabilly produce very unoptimized code for volatile operands, at least that is also what I understood by reading the arguments behind atomic_t type in Linux (not using volatile by default). Considering this, I think I should prevent to be using volatile for td_pinned and possibly we should think carefully when introducing new volatile members in the future. Thanks, Attilio Index: sys/sys/sched.h =================================================================== --- sys/sys/sched.h (revision 241395) +++ sys/sys/sched.h (working copy) @@ -146,11 +146,13 @@ static __inline void sched_pin(void) { curthread->td_pinned++; + __compiler_membar(); } static __inline void sched_unpin(void) { + __compiler_membar(); curthread->td_pinned--; }