Date: Wed, 10 Oct 2012 17:49:13 +0100 From: Attilio Rao <attilio@freebsd.org> To: John Baldwin <jhb@freebsd.org> Cc: Davide Italiano <davide@freebsd.org>, src-committers@freebsd.org, Jeff Roberson <jeff@freebsd.org>, Dimitry Andric <dim@freebsd.org>, svn-src-projects@freebsd.org, Konstantin Belousov <kostikbel@gmail.com> Subject: Re: svn commit: r238907 - projects/calloutng/sys/kern Message-ID: <CAJ-FndBqV2uD8Th9ePtxyJwhMAPzY3AXA5cQ7HszLp=%2BfSpHTA@mail.gmail.com> In-Reply-To: <CAJ-FndCRg0UCThFkatp=tw7rUWWCvhsApLE=iztLpxpGBC1F9w@mail.gmail.com> References: <201207301350.q6UDobCI099069@svn.freebsd.org> <CAJ-FndBj8tpC_BJXs_RH8sG2TBG8yA=Lxu3-GTVT9Ap_zOCuVQ@mail.gmail.com> <CAJ-FndDnO7wjnWPV0tTu%2BUGHjsxa3YDarMxmyei3ZmjLAFvRkQ@mail.gmail.com> <201207301732.33474.jhb@freebsd.org> <CAJ-FndD5EO12xsWOAe6u0EvX00q33wxO4OivnGjzj0=T2Oe8uA@mail.gmail.com> <CAJ-FndCRg0UCThFkatp=tw7rUWWCvhsApLE=iztLpxpGBC1F9w@mail.gmail.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Tue, Sep 18, 2012 at 1:13 AM, Attilio Rao <attilio@freebsd.org> wrote:
> On Thu, Aug 2, 2012 at 9:56 PM, Attilio Rao <attilio@freebsd.org> wrote:
>> On 7/30/12, John Baldwin <jhb@freebsd.org> 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--;
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?CAJ-FndBqV2uD8Th9ePtxyJwhMAPzY3AXA5cQ7HszLp=%2BfSpHTA>