[ltt-dev] [PATCH] fix the "unknown" case

[ltt-dev] [PATCH] fix the "unknown" case

[Ulrich Weigand]

"Paul E. McKenney" <paulmck at linux.vnet.ibm.com> wrote on 06/15/2010
12:06:00 AM:

> When I use __sync_synchronize(), I get failures from the
rcutorture_urcu_mb
> stress test.  When I use a "dmb" instruction, I don't get any failures.
>
> No idea why, to be honest.

If you get the code as below for __sync_synchronize, that's no wonder: this
code is a plain compiler optimization barrier, it does not have any
hardware
memory barrier at all ...

> int
> f()
> {
>    __sync_synchronize();
> }
[snip]
>    str   fp, [sp, #-4]!
>    add   fp, sp, #0
>    add   sp, fp, #0
>    ldmfd   sp!, {fp}
>    bx   lr

No hardware barrier instruction ...

> (insn 5 4 14 3 sync_sync.c:4 (parallel [
>             (asm_operands/v ("") ("") 0 []
>                  [] 477)
>             (clobber (mem:BLK (scratch) [0 A8]))
>         ]) -1 (nil))

... as this is just a optimization barrier here.

Looking at the compiler sources, the __sync_synchronize call gets
translated into:

a. a hardware memory barrier instruction if provided by the platform
b. otherwise, a call to a __sync_synchronize libgcc routine if provided
   by the platform
c. otherwise, just a compiler optimization barrier

On ARM (at least on GCC mainline), there is no memory barrier hardware
instruction defined in the backend, so you'll never get a.

There *is* support for b., i.e. a call to a library func, but only if
you're using a quite recent compiler.  Support was added to mainline
on 2009-08-12; the first official GCC release to contain the change
is 4.5.0; the change was later backported to 4.4.3.  (As you seem to
be using 4.4.1, this feature is probably just not there yet.)

See also the discussion in this bugzilla and the mailing list threads
linked from it:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42263


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
  IBM Deutschland Research & Development GmbH
  Vorsitzender des Aufsichtsrats: Martin Jetter | Gesch?ftsf?hrung: Dirk
Wittkopp
  Sitz der Gesellschaft: B?blingen | Registergericht: Amtsgericht
Stuttgart, HRB 243294

[ltt-dev] [PATCH] fix the "unknown" case

[Paul E. McKenney]

On Tue, Jun 15, 2010 at 02:35:16PM +0200, Ulrich Weigand wrote:
> "Paul E. McKenney" <paulmck at linux.vnet.ibm.com> wrote on 06/15/2010
> 12:06:00 AM:
> 
> > When I use __sync_synchronize(), I get failures from the
> rcutorture_urcu_mb
> > stress test.  When I use a "dmb" instruction, I don't get any failures.
> >
> > No idea why, to be honest.
> 
> If you get the code as below for __sync_synchronize, that's no wonder: this
> code is a plain compiler optimization barrier, it does not have any
> hardware
> memory barrier at all ...

That would explain it!  ;-)

> > int
> > f()
> > {
> >    __sync_synchronize();
> > }
> [snip]
> >    str   fp, [sp, #-4]!
> >    add   fp, sp, #0
> >    add   sp, fp, #0
> >    ldmfd   sp!, {fp}
> >    bx   lr
> 
> No hardware barrier instruction ...
> 
> > (insn 5 4 14 3 sync_sync.c:4 (parallel [
> >             (asm_operands/v ("") ("") 0 []
> >                  [] 477)
> >             (clobber (mem:BLK (scratch) [0 A8]))
> >         ]) -1 (nil))
> 
> ... as this is just a optimization barrier here.
> 
> Looking at the compiler sources, the __sync_synchronize call gets
> translated into:
> 
> a. a hardware memory barrier instruction if provided by the platform
> b. otherwise, a call to a __sync_synchronize libgcc routine if provided
>    by the platform
> c. otherwise, just a compiler optimization barrier
> 
> On ARM (at least on GCC mainline), there is no memory barrier hardware
> instruction defined in the backend, so you'll never get a.
> 
> There *is* support for b., i.e. a call to a library func, but only if
> you're using a quite recent compiler.  Support was added to mainline
> on 2009-08-12; the first official GCC release to contain the change
> is 4.5.0; the change was later backported to 4.4.3.  (As you seem to
> be using 4.4.1, this feature is probably just not there yet.)

Thank you very much for looking into this and for the information!

> See also the discussion in this bugzilla and the mailing list threads
> linked from it:
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42263

So I cannot expect the other __sync_ primitives to be generate
memory barriers, either, correct?  Hmmm...  From looking at
http://gcc.gnu.org/ml/gcc-patches/2009-12/msg00198.html, I wonder
whether I can rely on them to be using atomic instructions -- though
I do admit that __sync_lock_release() often does not need an atomic
instruction.

So, should I just bite the bullet and write the usual set of asms
myself?  People are going to want to build liburcu on old compilers,
sad to say...

							Thanx, Paul

> Mit freundlichen Gruessen / Best Regards
> 
> Ulrich Weigand
> 
> --
>   Dr. Ulrich Weigand | Phone: +49-7031/16-3727
>   STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
>   IBM Deutschland Research & Development GmbH
>   Vorsitzender des Aufsichtsrats: Martin Jetter | Gesch?ftsf?hrung: Dirk
> Wittkopp
>   Sitz der Gesellschaft: B?blingen | Registergericht: Amtsgericht
> Stuttgart, HRB 243294
> 

[ltt-dev] [PATCH] fix the "unknown" case

[Paolo Bonzini]

On 06/15/2010 05:07 PM, Paul E. McKenney wrote:
> So, should I just bite the bullet and write the usual set of asms
> myself?  People are going to want to build liburcu on old compilers,
> sad to say...

If you care about old/buggy compilers you'll always have to anyway.

Paolo

[ltt-dev] [PATCH] fix the "unknown" case

[Ulrich Weigand]

"Paul E. McKenney" <paulmck at linux.vnet.ibm.com> wrote on 06/15/2010
05:07:27 PM:

> > See also the discussion in this bugzilla and the mailing list threads
> > linked from it:
> > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42263
>
> So I cannot expect the other __sync_ primitives to be generate
> memory barriers, either, correct?  Hmmm...  From looking at
> http://gcc.gnu.org/ml/gcc-patches/2009-12/msg00198.html, I wonder
> whether I can rely on them to be using atomic instructions -- though
> I do admit that __sync_lock_release() often does not need an atomic
> instruction.

As mentioned in the bugzilla, there were bugs in __sync_synchronize
(which was just a compiler optimization barrier), and in
__sync_lock_release,
which did use a memory barrier, but at the wrong place.

With a compiler where these two are fixes (GCC 4.4.3 and above), all the
__sync_ primitives will generate memory barriers, but not directly: the
compiler will call the libgcc library function, which will call a special
ARM kernel entry point, which will then use an appropriate instruction
depending whether the kernel is compiled for SMP or UP, and depending on
the target instruction set level.  For example, the __kernel_dmb call
uses this piece of code in the kernel:

        .macro  smp_dmb
#ifdef CONFIG_SMP
#if __LINUX_ARM_ARCH__ >= 7
        dmb
#elif __LINUX_ARM_ARCH__ == 6
        mcr     p15, 0, r0, c7, c10, 5  @ dmb
#endif
#endif
        .endm

> So, should I just bite the bullet and write the usual set of asms
> myself?  People are going to want to build liburcu on old compilers,
> sad to say...

As Paolo already said, you probably do want to support older compilers.
On the other hand, if you hard-code the dmb instruction, it seems you're
tied to the ARM 7 architecture level ...   Maybe you want to directly
use the __kernel_dmb etc. calls in liburcu?  They seem to be official
kernel ABI, so should be OK to use.


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
  IBM Deutschland Research & Development GmbH
  Vorsitzender des Aufsichtsrats: Martin Jetter | Gesch?ftsf?hrung: Dirk
Wittkopp
  Sitz der Gesellschaft: B?blingen | Registergericht: Amtsgericht
Stuttgart, HRB 243294

[ltt-dev] [PATCH] fix the "unknown" case

[Paul E. McKenney]

On Tue, Jun 15, 2010 at 05:18:13PM +0200, Paolo Bonzini wrote:
> On 06/15/2010 05:07 PM, Paul E. McKenney wrote:
> >So, should I just bite the bullet and write the usual set of asms
> >myself?  People are going to want to build liburcu on old compilers,
> >sad to say...
> 
> If you care about old/buggy compilers you'll always have to anyway.

Well, maybe I can at least have a #ifdef that checks for compiler
version or something.

							Thanx, Paul

[ltt-dev] [PATCH] fix the "unknown" case

[Paul E. McKenney]

On Tue, Jun 15, 2010 at 06:07:20PM +0200, Ulrich Weigand wrote:
> "Paul E. McKenney" <paulmck at linux.vnet.ibm.com> wrote on 06/15/2010
> 05:07:27 PM:
> 
> > > See also the discussion in this bugzilla and the mailing list threads
> > > linked from it:
> > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42263
> >
> > So I cannot expect the other __sync_ primitives to be generate
> > memory barriers, either, correct?  Hmmm...  From looking at
> > http://gcc.gnu.org/ml/gcc-patches/2009-12/msg00198.html, I wonder
> > whether I can rely on them to be using atomic instructions -- though
> > I do admit that __sync_lock_release() often does not need an atomic
> > instruction.
> 
> As mentioned in the bugzilla, there were bugs in __sync_synchronize
> (which was just a compiler optimization barrier), and in
> __sync_lock_release,
> which did use a memory barrier, but at the wrong place.
> 
> With a compiler where these two are fixes (GCC 4.4.3 and above), all the
> __sync_ primitives will generate memory barriers, but not directly: the
> compiler will call the libgcc library function, which will call a special
> ARM kernel entry point, which will then use an appropriate instruction
> depending whether the kernel is compiled for SMP or UP, and depending on
> the target instruction set level.  For example, the __kernel_dmb call
> uses this piece of code in the kernel:
> 
>         .macro  smp_dmb
> #ifdef CONFIG_SMP
> #if __LINUX_ARM_ARCH__ >= 7
>         dmb
> #elif __LINUX_ARM_ARCH__ == 6
>         mcr     p15, 0, r0, c7, c10, 5  @ dmb
> #endif
> #endif
>         .endm

OK, so GCC 4.4.3 and above can be trusted.

> > So, should I just bite the bullet and write the usual set of asms
> > myself?  People are going to want to build liburcu on old compilers,
> > sad to say...
> 
> As Paolo already said, you probably do want to support older compilers.
> On the other hand, if you hard-code the dmb instruction, it seems you're
> tied to the ARM 7 architecture level ...   Maybe you want to directly
> use the __kernel_dmb etc. calls in liburcu?  They seem to be official
> kernel ABI, so should be OK to use.

My current autoconf code checks for "armv7l", so I am OK being ARMv7
specific.  But it would be really good to cover the rest of the ARM
family, and __kernel_dmb and friends might be a way to do that!

							Thanx, Paul

[ltt-dev] [PATCH] fix the "unknown" case

[Mathieu Desnoyers]

* Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> On Tue, Jun 15, 2010 at 06:07:20PM +0200, Ulrich Weigand wrote:
> > "Paul E. McKenney" <paulmck at linux.vnet.ibm.com> wrote on 06/15/2010
> > 05:07:27 PM:
> > 
> > > > See also the discussion in this bugzilla and the mailing list threads
> > > > linked from it:
> > > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=42263
> > >
> > > So I cannot expect the other __sync_ primitives to be generate
> > > memory barriers, either, correct?  Hmmm...  From looking at
> > > http://gcc.gnu.org/ml/gcc-patches/2009-12/msg00198.html, I wonder
> > > whether I can rely on them to be using atomic instructions -- though
> > > I do admit that __sync_lock_release() often does not need an atomic
> > > instruction.
> > 
> > As mentioned in the bugzilla, there were bugs in __sync_synchronize
> > (which was just a compiler optimization barrier), and in
> > __sync_lock_release,
> > which did use a memory barrier, but at the wrong place.
> > 
> > With a compiler where these two are fixes (GCC 4.4.3 and above), all the
> > __sync_ primitives will generate memory barriers, but not directly: the
> > compiler will call the libgcc library function, which will call a special
> > ARM kernel entry point, which will then use an appropriate instruction
> > depending whether the kernel is compiled for SMP or UP, and depending on
> > the target instruction set level.  For example, the __kernel_dmb call
> > uses this piece of code in the kernel:
> > 
> >         .macro  smp_dmb
> > #ifdef CONFIG_SMP
> > #if __LINUX_ARM_ARCH__ >= 7
> >         dmb
> > #elif __LINUX_ARM_ARCH__ == 6
> >         mcr     p15, 0, r0, c7, c10, 5  @ dmb
> > #endif
> > #endif
> >         .endm
> 
> OK, so GCC 4.4.3 and above can be trusted.
> 
> > > So, should I just bite the bullet and write the usual set of asms
> > > myself?  People are going to want to build liburcu on old compilers,
> > > sad to say...
> > 
> > As Paolo already said, you probably do want to support older compilers.
> > On the other hand, if you hard-code the dmb instruction, it seems you're
> > tied to the ARM 7 architecture level ...   Maybe you want to directly
> > use the __kernel_dmb etc. calls in liburcu?  They seem to be official
> > kernel ABI, so should be OK to use.
> 
> My current autoconf code checks for "armv7l", so I am OK being ARMv7
> specific.  But it would be really good to cover the rest of the ARM
> family, and __kernel_dmb and friends might be a way to do that!

I wonder starting with which Linux kernel version __kernel_dmb appeared.
Tying ourself directly to a Linux kernel ABI might complicate things.

Is this ABI presented in a vDSO or userland have to go through a system call ?
Is there any way to probe for its availability ?

Thanks,

Mathieu

> 
> 							Thanx, Paul

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH] fix the "unknown" case

[Ulrich Weigand]

Mathieu Desnoyers <mathieu.desnoyers at efficios.com> wrote on 06/15/2010
07:03:15 PM:

> I wonder starting with which Linux kernel version __kernel_dmb appeared.
> Tying ourself directly to a Linux kernel ABI might complicate things.
>
> Is this ABI presented in a vDSO or userland have to go through a system
call ?
> Is there any way to probe for its availability ?

This looks sort-of like a vDSO, except without the DSO part :-)

The kernel simply makes the code available at a fixed address that is
directly callable by user space.  See the comments in
linux/arch/arm/kernel/entry-armv.S:

/*
 * User helpers.
 *
 * These are segment of kernel provided user code reachable from user space
 * at a fixed address in kernel memory.  This is used to provide user space
 * with some operations which require kernel help because of unimplemented
 * native feature and/or instructions in many ARM CPUs. The idea is for
 * this code to be executed directly in user mode for best efficiency but
 * which is too intimate with the kernel counter part to be left to user
 * libraries.  In fact this code might even differ from one CPU to another
 * depending on the available  instruction set and restrictions like on
 * SMP systems.  In other words, the kernel reserves the right to change
 * this code as needed without warning. Only the entry points and their
 * results are guaranteed to be stable.
 *
 * Each segment is 32-byte aligned and will be moved to the top of the high
 * vector page.  New segments (if ever needed) must be added in front of
 * existing ones.  This mechanism should be used only for things that are
 * really small and justified, and not be abused freely.
 *
 * User space is expected to implement those things inline when optimizing
 * for a processor that has the necessary native support, but only if such
 * resulting binaries are already to be incompatible with earlier ARM
 * processors due to the use of unsupported instructions other than what
 * is provided here.  In other words don't make binaries unable to run on
 * earlier processors just for the sake of not using these kernel helpers
 * if your compiled code is not going to use the new instructions for other
 * purpose.
 */


/*
 * Reference prototype:
 *
 *           void __kernel_memory_barrier(void)
 *
 * Input:
 *
 *           lr = return address
 *
 * Output:
 *
 *           none
 *
 * Clobbered:
 *
 *           none
 *
 * Definition and user space usage example:
 *
 *           typedef void (__kernel_dmb_t)(void);
 *           #define __kernel_dmb (*(__kernel_dmb_t *)0xffff0fa0)
 *
 * Apply any needed memory barrier to preserve consistency with data
modified
 * manually and __kuser_cmpxchg usage.
 *
 * This could be used as follows:
 *
 * #define __kernel_dmb() \
 *         asm volatile ( "mov r0, #0xffff0fff; mov lr, pc; sub pc, r0,
#95" \
 *		         : : : "r0", "lr","cc" )
 */


As far as I can see, the only provision to check whether a feature is
available
is this one:

/*
 * Reference declaration:
 *
 *           extern unsigned int __kernel_helper_version;
 *
 * Definition and user space usage example:
 *
 *           #define __kernel_helper_version (*(unsigned int *)0xffff0ffc)
 *
 * User space may read this to determine the curent number of helpers
 * available.
 */

However, note that libgcc code does not perform this check, it simply
assumes
the above routine to be present.

The __kernel_dmb (which is the most recently added helper available in
current
mainline) seems to have been available since kernel 2.6.15, so presumably
code
using any of the GCC sync primitives would simply fail on any older kernel,
unless I'm missing something here ...


Mit freundlichen Gruessen / Best Regards

Ulrich Weigand

--
  Dr. Ulrich Weigand | Phone: +49-7031/16-3727
  STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
  IBM Deutschland Research & Development GmbH
  Vorsitzender des Aufsichtsrats: Martin Jetter | Gesch?ftsf?hrung: Dirk
Wittkopp
  Sitz der Gesellschaft: B?blingen | Registergericht: Amtsgericht
Stuttgart, HRB 243294

[ltt-dev] Userspace helpers at static addresses on ARM [was: Re: [PATCH] fix the "unknown" case]

[Mathieu Desnoyers]

* Ulrich Weigand (Ulrich.Weigand at de.ibm.com) wrote:
> Mathieu Desnoyers <mathieu.desnoyers at efficios.com> wrote on 06/15/2010
> 07:03:15 PM:
> 
> > I wonder starting with which Linux kernel version __kernel_dmb appeared.
> > Tying ourself directly to a Linux kernel ABI might complicate things.
> >
> > Is this ABI presented in a vDSO or userland have to go through a system
> call ?
> > Is there any way to probe for its availability ?
> 
> This looks sort-of like a vDSO, except without the DSO part :-)
> 
> The kernel simply makes the code available at a fixed address that is
> directly callable by user space.  See the comments in
> linux/arch/arm/kernel/entry-armv.S:
> 

Hrm, statically addressed shared objects. The security guys should be freaking
out here. This can sadly make stack overflow exploitation much, much, easier
because of lack of randomization of addresses where the code is located. :-/

About the original topic of our discussion:
Thanks for the explanation below. I think making urcu test for the kernel
feature at library load seems like the best portable solution so far. We can
directly use the specific memory barriers when armv7+ is specified, and check
at runtime if the kernel feature is there for "generic" arm build. For generic
ARM build where we discover that the kernel lacks the proper features, we could
rely on Paul's double-fake-mutex scheme (assuming we audit glibc pthreads to
ensure the proper memory barriers are there). If we find out that even pthreads
mutexes got the barriers wrong there, then we should refuse to load the library
altogether.

Thanks,

Mathieu

> /*
>  * User helpers.
>  *
>  * These are segment of kernel provided user code reachable from user space
>  * at a fixed address in kernel memory.  This is used to provide user space
>  * with some operations which require kernel help because of unimplemented
>  * native feature and/or instructions in many ARM CPUs. The idea is for
>  * this code to be executed directly in user mode for best efficiency but
>  * which is too intimate with the kernel counter part to be left to user
>  * libraries.  In fact this code might even differ from one CPU to another
>  * depending on the available  instruction set and restrictions like on
>  * SMP systems.  In other words, the kernel reserves the right to change
>  * this code as needed without warning. Only the entry points and their
>  * results are guaranteed to be stable.
>  *
>  * Each segment is 32-byte aligned and will be moved to the top of the high
>  * vector page.  New segments (if ever needed) must be added in front of
>  * existing ones.  This mechanism should be used only for things that are
>  * really small and justified, and not be abused freely.
>  *
>  * User space is expected to implement those things inline when optimizing
>  * for a processor that has the necessary native support, but only if such
>  * resulting binaries are already to be incompatible with earlier ARM
>  * processors due to the use of unsupported instructions other than what
>  * is provided here.  In other words don't make binaries unable to run on
>  * earlier processors just for the sake of not using these kernel helpers
>  * if your compiled code is not going to use the new instructions for other
>  * purpose.
>  */
> 
> 
> /*
>  * Reference prototype:
>  *
>  *           void __kernel_memory_barrier(void)
>  *
>  * Input:
>  *
>  *           lr = return address
>  *
>  * Output:
>  *
>  *           none
>  *
>  * Clobbered:
>  *
>  *           none
>  *
>  * Definition and user space usage example:
>  *
>  *           typedef void (__kernel_dmb_t)(void);
>  *           #define __kernel_dmb (*(__kernel_dmb_t *)0xffff0fa0)
>  *
>  * Apply any needed memory barrier to preserve consistency with data
> modified
>  * manually and __kuser_cmpxchg usage.
>  *
>  * This could be used as follows:
>  *
>  * #define __kernel_dmb() \
>  *         asm volatile ( "mov r0, #0xffff0fff; mov lr, pc; sub pc, r0,
> #95" \
>  *		         : : : "r0", "lr","cc" )
>  */
> 
> 
> As far as I can see, the only provision to check whether a feature is
> available
> is this one:
> 
> /*
>  * Reference declaration:
>  *
>  *           extern unsigned int __kernel_helper_version;
>  *
>  * Definition and user space usage example:
>  *
>  *           #define __kernel_helper_version (*(unsigned int *)0xffff0ffc)
>  *
>  * User space may read this to determine the curent number of helpers
>  * available.
>  */
> 
> However, note that libgcc code does not perform this check, it simply
> assumes
> the above routine to be present.
> 
> The __kernel_dmb (which is the most recently added helper available in
> current
> mainline) seems to have been available since kernel 2.6.15, so presumably
> code
> using any of the GCC sync primitives would simply fail on any older kernel,
> unless I'm missing something here ...
> 
> 
> Mit freundlichen Gruessen / Best Regards
> 
> Ulrich Weigand
> 
> --
>   Dr. Ulrich Weigand | Phone: +49-7031/16-3727
>   STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
>   IBM Deutschland Research & Development GmbH
>   Vorsitzender des Aufsichtsrats: Martin Jetter | Gesch?ftsf?hrung: Dirk
> Wittkopp
>   Sitz der Gesellschaft: B?blingen | Registergericht: Amtsgericht
> Stuttgart, HRB 243294
> 

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] Userspace helpers at static addresses on ARM [was: Re: [PATCH] fix the "unknown" case]

[Paul E. McKenney]

On Tue, Jun 15, 2010 at 02:29:19PM -0400, Mathieu Desnoyers wrote:
> * Ulrich Weigand (Ulrich.Weigand at de.ibm.com) wrote:
> > Mathieu Desnoyers <mathieu.desnoyers at efficios.com> wrote on 06/15/2010
> > 07:03:15 PM:
> > 
> > > I wonder starting with which Linux kernel version __kernel_dmb appeared.
> > > Tying ourself directly to a Linux kernel ABI might complicate things.
> > >
> > > Is this ABI presented in a vDSO or userland have to go through a system
> > call ?
> > > Is there any way to probe for its availability ?
> > 
> > This looks sort-of like a vDSO, except without the DSO part :-)
> > 
> > The kernel simply makes the code available at a fixed address that is
> > directly callable by user space.  See the comments in
> > linux/arch/arm/kernel/entry-armv.S:
> > 
> 
> Hrm, statically addressed shared objects. The security guys should be freaking
> out here. This can sadly make stack overflow exploitation much, much, easier
> because of lack of randomization of addresses where the code is located. :-/
> 
> About the original topic of our discussion:
> Thanks for the explanation below. I think making urcu test for the kernel
> feature at library load seems like the best portable solution so far. We can
> directly use the specific memory barriers when armv7+ is specified, and check
> at runtime if the kernel feature is there for "generic" arm build. For generic
> ARM build where we discover that the kernel lacks the proper features, we could
> rely on Paul's double-fake-mutex scheme (assuming we audit glibc pthreads to
> ensure the proper memory barriers are there). If we find out that even pthreads
> mutexes got the barriers wrong there, then we should refuse to load the library
> altogether.

OK.  The gcc patches were for __sync_sychronize(), which I have replaced
with a "dmb" asm, and for __sync_lock_release(), which I do not use.
If I understand Paolo and Uli correctly (a dubious assumption, to be
sure), then the memory barriers and atomicity should be supplied by
the libraries and/or kernel for the other __sync_ primitives.

So for ARMv7, my prior patch should suffice.  (Or am I still missing
something?)

Additional patches are no doubt required for other ARM flavors, and
perhaps also for older compilers and kernels.

							Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> > /*
> >  * User helpers.
> >  *
> >  * These are segment of kernel provided user code reachable from user space
> >  * at a fixed address in kernel memory.  This is used to provide user space
> >  * with some operations which require kernel help because of unimplemented
> >  * native feature and/or instructions in many ARM CPUs. The idea is for
> >  * this code to be executed directly in user mode for best efficiency but
> >  * which is too intimate with the kernel counter part to be left to user
> >  * libraries.  In fact this code might even differ from one CPU to another
> >  * depending on the available  instruction set and restrictions like on
> >  * SMP systems.  In other words, the kernel reserves the right to change
> >  * this code as needed without warning. Only the entry points and their
> >  * results are guaranteed to be stable.
> >  *
> >  * Each segment is 32-byte aligned and will be moved to the top of the high
> >  * vector page.  New segments (if ever needed) must be added in front of
> >  * existing ones.  This mechanism should be used only for things that are
> >  * really small and justified, and not be abused freely.
> >  *
> >  * User space is expected to implement those things inline when optimizing
> >  * for a processor that has the necessary native support, but only if such
> >  * resulting binaries are already to be incompatible with earlier ARM
> >  * processors due to the use of unsupported instructions other than what
> >  * is provided here.  In other words don't make binaries unable to run on
> >  * earlier processors just for the sake of not using these kernel helpers
> >  * if your compiled code is not going to use the new instructions for other
> >  * purpose.
> >  */
> > 
> > 
> > /*
> >  * Reference prototype:
> >  *
> >  *           void __kernel_memory_barrier(void)
> >  *
> >  * Input:
> >  *
> >  *           lr = return address
> >  *
> >  * Output:
> >  *
> >  *           none
> >  *
> >  * Clobbered:
> >  *
> >  *           none
> >  *
> >  * Definition and user space usage example:
> >  *
> >  *           typedef void (__kernel_dmb_t)(void);
> >  *           #define __kernel_dmb (*(__kernel_dmb_t *)0xffff0fa0)
> >  *
> >  * Apply any needed memory barrier to preserve consistency with data
> > modified
> >  * manually and __kuser_cmpxchg usage.
> >  *
> >  * This could be used as follows:
> >  *
> >  * #define __kernel_dmb() \
> >  *         asm volatile ( "mov r0, #0xffff0fff; mov lr, pc; sub pc, r0,
> > #95" \
> >  *		         : : : "r0", "lr","cc" )
> >  */
> > 
> > 
> > As far as I can see, the only provision to check whether a feature is
> > available
> > is this one:
> > 
> > /*
> >  * Reference declaration:
> >  *
> >  *           extern unsigned int __kernel_helper_version;
> >  *
> >  * Definition and user space usage example:
> >  *
> >  *           #define __kernel_helper_version (*(unsigned int *)0xffff0ffc)
> >  *
> >  * User space may read this to determine the curent number of helpers
> >  * available.
> >  */
> > 
> > However, note that libgcc code does not perform this check, it simply
> > assumes
> > the above routine to be present.
> > 
> > The __kernel_dmb (which is the most recently added helper available in
> > current
> > mainline) seems to have been available since kernel 2.6.15, so presumably
> > code
> > using any of the GCC sync primitives would simply fail on any older kernel,
> > unless I'm missing something here ...
> > 
> > 
> > Mit freundlichen Gruessen / Best Regards
> > 
> > Ulrich Weigand
> > 
> > --
> >   Dr. Ulrich Weigand | Phone: +49-7031/16-3727
> >   STSM, GNU compiler and toolchain for Linux on System z and Cell/B.E.
> >   IBM Deutschland Research & Development GmbH
> >   Vorsitzender des Aufsichtsrats: Martin Jetter | Gesch?ftsf?hrung: Dirk
> > Wittkopp
> >   Sitz der Gesellschaft: B?blingen | Registergericht: Amtsgericht
> > Stuttgart, HRB 243294
> > 
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com

[ltt-dev] [PATCH] fix the "unknown" case

[Paul E. McKenney]

On Tue, Jun 15, 2010 at 10:52:23AM +0200, Paolo Bonzini wrote:
> On 06/14/2010 08:25 PM, Paul E. McKenney wrote:
> >>Anyway, a simple configure test is to compile this with -fdump-rtl-expand:
> >>
> >>	int
> >>	f()
> >>	{
> >>	  __sync_synchronize();
> >>	}
> >>
> >>If the assembly output includes "__sync_synchronize", or the dump
> >>file includes the text "unspec:BLK", it should be fine.  In
> >>particular, ia64, mips, and Alpha are ok.  Else you can use the
> >>pthreads trick.  I can try to make a patch if you're interested.
> >>Or, more simply, it's possible to hardcode the above three platforms
> >>since it's unlikely that others will be added soon.
> >
> >And I attached the input file, the .expand file, and the .s file.
> >
> >I see neither __sync_synchronize in the .s file nor "unspec" in the
> >.expand file.  Or was I confused about what to look for?
> 
> No, the __sync_synchronize instruction is optimized out.  I think
> I'll make GCC 4.6+ give a warning.

Thank you for looking into this!

Hmmm...  One mistake I made last time was to forget the "-mcpu=cortex-a9
-mtune=cortex-a9" -- but adding this does not change the result.

If I have other code in the function, I do get the following in
the .s file:

	@ 10 "gcc_sync.c" 1
		#__sync_synchronize

But still no meaningful code.

So for the moment I am using my own asm to generate a dmb instruction.

Which raises the question as to whether I can really trust the other
__sync_ instructions.  They -seem- to be working for me, but one cannot
prove them correct by testing.  :-(

							Thanx, Paul

[ltt-dev] [PATCH] fix the "unknown" case

[Paolo Bonzini]

On 06/14/2010 08:25 PM, Paul E. McKenney wrote:
>> Anyway, a simple configure test is to compile this with -fdump-rtl-expand:
>>
>> 	int
>> 	f()
>> 	{
>> 	  __sync_synchronize();
>> 	}
>>
>> If the assembly output includes "__sync_synchronize", or the dump
>> file includes the text "unspec:BLK", it should be fine.  In
>> particular, ia64, mips, and Alpha are ok.  Else you can use the
>> pthreads trick.  I can try to make a patch if you're interested.
>> Or, more simply, it's possible to hardcode the above three platforms
>> since it's unlikely that others will be added soon.
>
> And I attached the input file, the .expand file, and the .s file.
>
> I see neither __sync_synchronize in the .s file nor "unspec" in the
> .expand file.  Or was I confused about what to look for?

No, the __sync_synchronize instruction is optimized out.  I think I'll 
make GCC 4.6+ give a warning.

Paolo

[ltt-dev] [PATCH] fix the "unknown" case

[Paul E. McKenney]

On Mon, Jun 14, 2010 at 07:04:43PM +0200, Paolo Bonzini wrote:
> On 06/13/2010 11:28 PM, Paul E. McKenney wrote:
> >On Sun, Jun 13, 2010 at 05:20:34PM -0400, Mathieu Desnoyers wrote:
> >>* Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> >>>On Thu, Jun 10, 2010 at 09:46:31PM -0400, Mathieu Desnoyers wrote:
> >>
> >>[...]
> >>
> >>(will reply to the rest in the individual patches)
> >>
> >>>>Can we trust __sync_lock_test_and_set/__sync_add_and_fetch given that
> >>>>__sync_synchronize is broken ?
> >>>
> >>>I don't know yet.  If it turns out that we cannot, then I will use some
> >>>form of global locking.  But the __sync_lock_test_and_set() do at least
> >>>generate instructions, unlike __sync_synchronize().  ;-)
> >>
> >>I'm concerned about the fact that their synchronization primitives might have
> >>the assembly all with, except for the memory barriers.
> 
> The default implementation of these __sync_* builtins is based on
> cmpxchg, and will cause a link error unless cmpxchg is also
> available (either in libgcc or with a compiler-provided inline
> implementation).
> 
> Instead, the default implementation of __sync_synchronize is to just
> do a compiler barrier.  ARM implements __sync_synchronize only for
> Linux, so at least there it is not needed.  Strange that Paul needs
> it too.

When I use __sync_synchronize(), I get failures from the rcutorture_urcu_mb
stress test.  When I use a "dmb" instruction, I don't get any failures.

No idea why, to be honest.

> Anyway, a simple configure test is to compile this with -fdump-rtl-expand:
> 
> 	int
> 	f()
> 	{
> 	  __sync_synchronize();
> 	}
> 
> If the assembly output includes "__sync_synchronize", or the dump
> file includes the text "unspec:BLK", it should be fine.  In
> particular, ia64, mips, and Alpha are ok.  Else you can use the
> pthreads trick.  I can try to make a patch if you're interested.
> Or, more simply, it's possible to hardcode the above three platforms
> since it's unlikely that others will be added soon.

And I attached the input file, the .expand file, and the .s file.

I see neither __sync_synchronize in the .s file nor "unspec" in the
.expand file.  Or was I confused about what to look for?

							Thanx, Paul
-------------- next part --------------
A non-text attachment was scrubbed...
Name: sync_sync.c
Type: text/x-csrc
Size: 35 bytes
Desc: sync_sync.c
URL: <http://lists.casi.polymtl.ca/pipermail/lttng-dev/attachments/20100614/20bd70cd/attachment-0003.c>
-------------- next part --------------
	.arch armv6
	.eabi_attribute 27, 3
	.fpu vfp
	.eabi_attribute 20, 1
	.eabi_attribute 21, 1
	.eabi_attribute 23, 3
	.eabi_attribute 24, 1
	.eabi_attribute 25, 1
	.eabi_attribute 26, 2
	.eabi_attribute 30, 6
	.eabi_attribute 18, 4
	.file	"sync_sync.c"
	.text
	.align	2
	.global	f
	.type	f, %function
f:
	@ args = 0, pretend = 0, frame = 0
	@ frame_needed = 1, uses_anonymous_args = 0
	@ link register save eliminated.
	str	fp, [sp, #-4]!
	add	fp, sp, #0
	add	sp, fp, #0
	ldmfd	sp!, {fp}
	bx	lr
	.size	f, .-f
	.ident	"GCC: (Ubuntu 4.4.1-4ubuntu9) 4.4.1"
	.section	.note.GNU-stack,"",%progbits
-------------- next part --------------

;; Function f (f)


;; Generating RTL for gimple basic block 2


;;
;; Full RTL generated for this function:
;;
(note 1 0 3 NOTE_INSN_DELETED)

(note 3 1 2 2 [bb 2] NOTE_INSN_BASIC_BLOCK)

(note 2 3 4 2 NOTE_INSN_FUNCTION_BEG)

(note 4 2 5 3 [bb 3] NOTE_INSN_BASIC_BLOCK)

(insn 5 4 14 3 sync_sync.c:4 (parallel [
            (asm_operands/v ("") ("") 0 []
                 [] 477)
            (clobber (mem:BLK (scratch) [0 A8]))
        ]) -1 (nil))

(note 14 5 8 4 [bb 4] NOTE_INSN_BASIC_BLOCK)

(insn 8 14 9 4 sync_sync.c:5 (clobber (reg/i:SI 0 r0)) -1 (nil))

(insn 9 8 10 4 sync_sync.c:5 (clobber (reg:SI 133 [ <result> ])) -1 (nil))

(jump_insn 10 9 11 4 sync_sync.c:5 (set (pc)
        (label_ref 12)) -1 (nil))

(barrier 11 10 6)

(code_label 6 11 15 5 1 "" [0 uses])

(note 15 6 7 5 [bb 5] NOTE_INSN_BASIC_BLOCK)

(insn 7 15 12 5 sync_sync.c:5 (set (reg/i:SI 0 r0)
        (reg:SI 133 [ <result> ])) -1 (nil))

(code_label 12 7 16 6 2 "" [1 uses])

(note 16 12 13 6 [bb 6] NOTE_INSN_BASIC_BLOCK)

(insn 13 16 0 6 sync_sync.c:5 (use (reg/i:SI 0 r0)) -1 (nil))

[ltt-dev] [PATCH] fix the "unknown" case

[Paolo Bonzini]

On 06/13/2010 11:28 PM, Paul E. McKenney wrote:
> On Sun, Jun 13, 2010 at 05:20:34PM -0400, Mathieu Desnoyers wrote:
>> * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
>>> On Thu, Jun 10, 2010 at 09:46:31PM -0400, Mathieu Desnoyers wrote:
>>
>> [...]
>>
>> (will reply to the rest in the individual patches)
>>
>>>> Can we trust __sync_lock_test_and_set/__sync_add_and_fetch given that
>>>> __sync_synchronize is broken ?
>>>
>>> I don't know yet.  If it turns out that we cannot, then I will use some
>>> form of global locking.  But the __sync_lock_test_and_set() do at least
>>> generate instructions, unlike __sync_synchronize().  ;-)
>>
>> I'm concerned about the fact that their synchronization primitives might have
>> the assembly all with, except for the memory barriers.

The default implementation of these __sync_* builtins is based on 
cmpxchg, and will cause a link error unless cmpxchg is also available 
(either in libgcc or with a compiler-provided inline implementation).

Instead, the default implementation of __sync_synchronize is to just do 
a compiler barrier.  ARM implements __sync_synchronize only for Linux, 
so at least there it is not needed.  Strange that Paul needs it too.

Anyway, a simple configure test is to compile this with -fdump-rtl-expand:

	int
	f()
	{
	  __sync_synchronize();
	}

If the assembly output includes "__sync_synchronize", or the dump file 
includes the text "unspec:BLK", it should be fine.  In particular, ia64, 
mips, and Alpha are ok.  Else you can use the pthreads trick.  I can try 
to make a patch if you're interested.  Or, more simply, it's possible to 
hardcode the above three platforms since it's unlikely that others will 
be added soon.

Paolo

[ltt-dev] [PATCH] fix the "unknown" case

[Paul E. McKenney]

On Sun, Jun 13, 2010 at 10:46:35PM -0400, Mathieu Desnoyers wrote:
> * Mathieu Desnoyers (mathieu.desnoyers at efficios.com) wrote:
> > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > On Sun, Jun 13, 2010 at 05:36:05PM -0400, Mathieu Desnoyers wrote:
> > > > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > > > On Sun, Jun 13, 2010 at 05:20:34PM -0400, Mathieu Desnoyers wrote:
> > > > > > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > > > > > On Thu, Jun 10, 2010 at 09:46:31PM -0400, Mathieu Desnoyers wrote:
> > > > > > 
> > > > > > [...]
> > > > > > 
> > > > > > (will reply to the rest in the individual patches)
> > > > > > 
> > > > > > > > Can we trust __sync_lock_test_and_set/__sync_add_and_fetch given that
> > > > > > > > __sync_synchronize is broken ?
> > > > > > > 
> > > > > > > I don't know yet.  If it turns out that we cannot, then I will use some
> > > > > > > form of global locking.  But the __sync_lock_test_and_set() do at least
> > > > > > > generate instructions, unlike __sync_synchronize().  ;-)
> > > > > > 
> > > > > > I'm concerned about the fact that their synchronization primitives might have
> > > > > > the assembly all with, except for the memory barriers.
> > > > > 
> > > > > OK.  How about if I used a hashed array of locks, indexed by a hash of
> > > > > the cacheline number of the access in question?  Then the "unknown" case
> > > > > doesn't depend at all on the __sync_ primitives.
> > > > 
> > > > I'm afraid this won't work with signal handlers. :-/ It would be a shame to have
> > > > to make these primitives non-signal-safe just for the sake of the "unknown"
> > > > arch. Or maybe it could make sense to disable signals around these, even though
> > > > it will be really slow ?
> > > 
> > > Hmmm...
> > > 
> > > Maybe the right thing to do is to make the build fail in the "unknown"
> > > case.  By the time we make this safe, the performance will be pretty
> > > bad!
> > > 
> > > My thought is to simply not have the two "unknown" include files, so
> > > that configuration dies when trying to copy them over.  Perhaps better
> > > yet, have the two "unknown" include files have nothing but a #error
> > > statement.  Seem like a reasonable approach?
> > 
> > Yep, I like the #error approach. It will provide a meaningful error message.
> 
> Hrm. OK, I think I got a compromise for you so we can have a working "unknown"
> architecture.
> 
> smp_mb(): double-fake-mutex (stack-local)
>           Note: we depend on having correct memory barriers in the pthread mutex
> implementation. Hopefully they got, at least, that right for ARM. ;)
> 
> uatomic_xchg():
> 
>   smp_mb();
>   ret = __sync_lock_test_and_set(addr, v);
>   smp_mb();
>   ret;
> 
> uatomic_cmpxchg():
> 
>   smp_mb();
>   ret = __sync_val_compare_and_swap(addr, old, _new)
>   smp_mb();
>   ret;
> 
> uatomic_add_return():
> 
>   smp_mb();
>   ret = __sync_add_and_fetch(addr, v);
>   smp_mb();
>   ret;
> 
> get_cycles(): use gettimeofday.
> 
> This is based on the assumption that all architecture's __sync_*() primitives
> are right, modulo possibly incorrect memory barriers (so we provide our own).
> This would be basically showing extreme distrust in gcc's __sync_*() memory
> barriers.
> 
> Thoughts ?

Interesting...  I just managed to convince myself that "unknown" should
refuse to build.  I will think some more about it and send a patch
tomorrow.  ;-)

							Thanx, Paul

[ltt-dev] [PATCH] fix the "unknown" case

[Paul E. McKenney]

On Sun, Jun 13, 2010 at 10:46:35PM -0400, Mathieu Desnoyers wrote:
> * Mathieu Desnoyers (mathieu.desnoyers at efficios.com) wrote:
> > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > On Sun, Jun 13, 2010 at 05:36:05PM -0400, Mathieu Desnoyers wrote:
> > > > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > > > On Sun, Jun 13, 2010 at 05:20:34PM -0400, Mathieu Desnoyers wrote:
> > > > > > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > > > > > On Thu, Jun 10, 2010 at 09:46:31PM -0400, Mathieu Desnoyers wrote:
> > > > > > 
> > > > > > [...]
> > > > > > 
> > > > > > (will reply to the rest in the individual patches)
> > > > > > 
> > > > > > > > Can we trust __sync_lock_test_and_set/__sync_add_and_fetch given that
> > > > > > > > __sync_synchronize is broken ?
> > > > > > > 
> > > > > > > I don't know yet.  If it turns out that we cannot, then I will use some
> > > > > > > form of global locking.  But the __sync_lock_test_and_set() do at least
> > > > > > > generate instructions, unlike __sync_synchronize().  ;-)
> > > > > > 
> > > > > > I'm concerned about the fact that their synchronization primitives might have
> > > > > > the assembly all with, except for the memory barriers.
> > > > > 
> > > > > OK.  How about if I used a hashed array of locks, indexed by a hash of
> > > > > the cacheline number of the access in question?  Then the "unknown" case
> > > > > doesn't depend at all on the __sync_ primitives.
> > > > 
> > > > I'm afraid this won't work with signal handlers. :-/ It would be a shame to have
> > > > to make these primitives non-signal-safe just for the sake of the "unknown"
> > > > arch. Or maybe it could make sense to disable signals around these, even though
> > > > it will be really slow ?
> > > 
> > > Hmmm...
> > > 
> > > Maybe the right thing to do is to make the build fail in the "unknown"
> > > case.  By the time we make this safe, the performance will be pretty
> > > bad!
> > > 
> > > My thought is to simply not have the two "unknown" include files, so
> > > that configuration dies when trying to copy them over.  Perhaps better
> > > yet, have the two "unknown" include files have nothing but a #error
> > > statement.  Seem like a reasonable approach?
> > 
> > Yep, I like the #error approach. It will provide a meaningful error message.
> 
> Hrm. OK, I think I got a compromise for you so we can have a working "unknown"
> architecture.
> 
> smp_mb(): double-fake-mutex (stack-local)
>           Note: we depend on having correct memory barriers in the pthread mutex
> implementation. Hopefully they got, at least, that right for ARM. ;)
> 
> uatomic_xchg():
> 
>   smp_mb();
>   ret = __sync_lock_test_and_set(addr, v);
>   smp_mb();
>   ret;
> 
> uatomic_cmpxchg():
> 
>   smp_mb();
>   ret = __sync_val_compare_and_swap(addr, old, _new)
>   smp_mb();
>   ret;
> 
> uatomic_add_return():
> 
>   smp_mb();
>   ret = __sync_add_and_fetch(addr, v);
>   smp_mb();
>   ret;
> 
> get_cycles(): use gettimeofday.
> 
> This is based on the assumption that all architecture's __sync_*() primitives
> are right, modulo possibly incorrect memory barriers (so we provide our own).
> This would be basically showing extreme distrust in gcc's __sync_*() memory
> barriers.
> 
> Thoughts ?

Worth a try.  If we have trouble with it (e.g., non-atomic implementations
of the primitives), then there is always #error.  ;-)

							Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> 
> > 
> > Thanks,
> > 
> > Mathieu
> > 
> > > 
> > > 							Thanx, Paul
> > > 
> > > > Thanks,
> > > > 
> > > > Mathieu
> > > > 
> > > > > 
> > > > > 							Thanx, Paul
> > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > Mathieu
> > > > > > 
> > > > > > > 
> > > > > > > I will send two patches, one that incorporates your suggestions, and
> > > > > > > another that removes sync_core().
> > > > > > > 
> > > > > > > 							Thanx, Paul
> > > > > > > 
> > > > > > > > Thanks,
> > > > > > > > 
> > > > > > > > Mathieu
> > > > > > > > 
> > > > > > > > > +
> > > > > > > > > +#ifdef __cplusplus 
> > > > > > > > > +}
> > > > > > > > > +#endif
> > > > > > > > > +
> > > > > > > > > +#include <urcu/uatomic_generic.h>
> > > > > > > > > +
> > > > > > > > > +#endif /* _URCU_ARCH_UATOMIC_ARMV7_H */
> > > > > > > > 
> > > > > > > > -- 
> > > > > > > > Mathieu Desnoyers
> > > > > > > > Operating System Efficiency R&D Consultant
> > > > > > > > EfficiOS Inc.
> > > > > > > > http://www.efficios.com
> > > > > > 
> > > > > > -- 
> > > > > > Mathieu Desnoyers
> > > > > > Operating System Efficiency R&D Consultant
> > > > > > EfficiOS Inc.
> > > > > > http://www.efficios.com
> > > > 
> > > > -- 
> > > > Mathieu Desnoyers
> > > > Operating System Efficiency R&D Consultant
> > > > EfficiOS Inc.
> > > > http://www.efficios.com
> > 
> > -- 
> > Mathieu Desnoyers
> > Operating System Efficiency R&D Consultant
> > EfficiOS Inc.
> > http://www.efficios.com
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com

[ltt-dev] [PATCH] fix the "unknown" case

[Mathieu Desnoyers]

* Mathieu Desnoyers (mathieu.desnoyers at efficios.com) wrote:
> * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > On Sun, Jun 13, 2010 at 05:36:05PM -0400, Mathieu Desnoyers wrote:
> > > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > > On Sun, Jun 13, 2010 at 05:20:34PM -0400, Mathieu Desnoyers wrote:
> > > > > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > > > > On Thu, Jun 10, 2010 at 09:46:31PM -0400, Mathieu Desnoyers wrote:
> > > > > 
> > > > > [...]
> > > > > 
> > > > > (will reply to the rest in the individual patches)
> > > > > 
> > > > > > > Can we trust __sync_lock_test_and_set/__sync_add_and_fetch given that
> > > > > > > __sync_synchronize is broken ?
> > > > > > 
> > > > > > I don't know yet.  If it turns out that we cannot, then I will use some
> > > > > > form of global locking.  But the __sync_lock_test_and_set() do at least
> > > > > > generate instructions, unlike __sync_synchronize().  ;-)
> > > > > 
> > > > > I'm concerned about the fact that their synchronization primitives might have
> > > > > the assembly all with, except for the memory barriers.
> > > > 
> > > > OK.  How about if I used a hashed array of locks, indexed by a hash of
> > > > the cacheline number of the access in question?  Then the "unknown" case
> > > > doesn't depend at all on the __sync_ primitives.
> > > 
> > > I'm afraid this won't work with signal handlers. :-/ It would be a shame to have
> > > to make these primitives non-signal-safe just for the sake of the "unknown"
> > > arch. Or maybe it could make sense to disable signals around these, even though
> > > it will be really slow ?
> > 
> > Hmmm...
> > 
> > Maybe the right thing to do is to make the build fail in the "unknown"
> > case.  By the time we make this safe, the performance will be pretty
> > bad!
> > 
> > My thought is to simply not have the two "unknown" include files, so
> > that configuration dies when trying to copy them over.  Perhaps better
> > yet, have the two "unknown" include files have nothing but a #error
> > statement.  Seem like a reasonable approach?
> 
> Yep, I like the #error approach. It will provide a meaningful error message.

Hrm. OK, I think I got a compromise for you so we can have a working "unknown"
architecture.

smp_mb(): double-fake-mutex (stack-local)
          Note: we depend on having correct memory barriers in the pthread mutex
implementation. Hopefully they got, at least, that right for ARM. ;)

uatomic_xchg():

  smp_mb();
  ret = __sync_lock_test_and_set(addr, v);
  smp_mb();
  ret;

uatomic_cmpxchg():

  smp_mb();
  ret = __sync_val_compare_and_swap(addr, old, _new)
  smp_mb();
  ret;

uatomic_add_return():

  smp_mb();
  ret = __sync_add_and_fetch(addr, v);
  smp_mb();
  ret;

get_cycles(): use gettimeofday.

This is based on the assumption that all architecture's __sync_*() primitives
are right, modulo possibly incorrect memory barriers (so we provide our own).
This would be basically showing extreme distrust in gcc's __sync_*() memory
barriers.

Thoughts ?

Thanks,

Mathieu


> 
> Thanks,
> 
> Mathieu
> 
> > 
> > 							Thanx, Paul
> > 
> > > Thanks,
> > > 
> > > Mathieu
> > > 
> > > > 
> > > > 							Thanx, Paul
> > > > 
> > > > > Thanks,
> > > > > 
> > > > > Mathieu
> > > > > 
> > > > > > 
> > > > > > I will send two patches, one that incorporates your suggestions, and
> > > > > > another that removes sync_core().
> > > > > > 
> > > > > > 							Thanx, Paul
> > > > > > 
> > > > > > > Thanks,
> > > > > > > 
> > > > > > > Mathieu
> > > > > > > 
> > > > > > > > +
> > > > > > > > +#ifdef __cplusplus 
> > > > > > > > +}
> > > > > > > > +#endif
> > > > > > > > +
> > > > > > > > +#include <urcu/uatomic_generic.h>
> > > > > > > > +
> > > > > > > > +#endif /* _URCU_ARCH_UATOMIC_ARMV7_H */
> > > > > > > 
> > > > > > > -- 
> > > > > > > Mathieu Desnoyers
> > > > > > > Operating System Efficiency R&D Consultant
> > > > > > > EfficiOS Inc.
> > > > > > > http://www.efficios.com
> > > > > 
> > > > > -- 
> > > > > Mathieu Desnoyers
> > > > > Operating System Efficiency R&D Consultant
> > > > > EfficiOS Inc.
> > > > > http://www.efficios.com
> > > 
> > > -- 
> > > Mathieu Desnoyers
> > > Operating System Efficiency R&D Consultant
> > > EfficiOS Inc.
> > > http://www.efficios.com
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH] fix the "unknown" case

[Mathieu Desnoyers]

* Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> On Sun, Jun 13, 2010 at 05:36:05PM -0400, Mathieu Desnoyers wrote:
> > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > On Sun, Jun 13, 2010 at 05:20:34PM -0400, Mathieu Desnoyers wrote:
> > > > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > > > On Thu, Jun 10, 2010 at 09:46:31PM -0400, Mathieu Desnoyers wrote:
> > > > 
> > > > [...]
> > > > 
> > > > (will reply to the rest in the individual patches)
> > > > 
> > > > > > Can we trust __sync_lock_test_and_set/__sync_add_and_fetch given that
> > > > > > __sync_synchronize is broken ?
> > > > > 
> > > > > I don't know yet.  If it turns out that we cannot, then I will use some
> > > > > form of global locking.  But the __sync_lock_test_and_set() do at least
> > > > > generate instructions, unlike __sync_synchronize().  ;-)
> > > > 
> > > > I'm concerned about the fact that their synchronization primitives might have
> > > > the assembly all with, except for the memory barriers.
> > > 
> > > OK.  How about if I used a hashed array of locks, indexed by a hash of
> > > the cacheline number of the access in question?  Then the "unknown" case
> > > doesn't depend at all on the __sync_ primitives.
> > 
> > I'm afraid this won't work with signal handlers. :-/ It would be a shame to have
> > to make these primitives non-signal-safe just for the sake of the "unknown"
> > arch. Or maybe it could make sense to disable signals around these, even though
> > it will be really slow ?
> 
> Hmmm...
> 
> Maybe the right thing to do is to make the build fail in the "unknown"
> case.  By the time we make this safe, the performance will be pretty
> bad!
> 
> My thought is to simply not have the two "unknown" include files, so
> that configuration dies when trying to copy them over.  Perhaps better
> yet, have the two "unknown" include files have nothing but a #error
> statement.  Seem like a reasonable approach?

Yep, I like the #error approach. It will provide a meaningful error message.

Thanks,

Mathieu

> 
> 							Thanx, Paul
> 
> > Thanks,
> > 
> > Mathieu
> > 
> > > 
> > > 							Thanx, Paul
> > > 
> > > > Thanks,
> > > > 
> > > > Mathieu
> > > > 
> > > > > 
> > > > > I will send two patches, one that incorporates your suggestions, and
> > > > > another that removes sync_core().
> > > > > 
> > > > > 							Thanx, Paul
> > > > > 
> > > > > > Thanks,
> > > > > > 
> > > > > > Mathieu
> > > > > > 
> > > > > > > +
> > > > > > > +#ifdef __cplusplus 
> > > > > > > +}
> > > > > > > +#endif
> > > > > > > +
> > > > > > > +#include <urcu/uatomic_generic.h>
> > > > > > > +
> > > > > > > +#endif /* _URCU_ARCH_UATOMIC_ARMV7_H */
> > > > > > 
> > > > > > -- 
> > > > > > Mathieu Desnoyers
> > > > > > Operating System Efficiency R&D Consultant
> > > > > > EfficiOS Inc.
> > > > > > http://www.efficios.com
> > > > 
> > > > -- 
> > > > Mathieu Desnoyers
> > > > Operating System Efficiency R&D Consultant
> > > > EfficiOS Inc.
> > > > http://www.efficios.com
> > 
> > -- 
> > Mathieu Desnoyers
> > Operating System Efficiency R&D Consultant
> > EfficiOS Inc.
> > http://www.efficios.com

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com

[ltt-dev] [PATCH] fix the "unknown" case

[Paul E. McKenney]

On Sun, Jun 13, 2010 at 05:36:05PM -0400, Mathieu Desnoyers wrote:
> * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > On Sun, Jun 13, 2010 at 05:20:34PM -0400, Mathieu Desnoyers wrote:
> > > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > > On Thu, Jun 10, 2010 at 09:46:31PM -0400, Mathieu Desnoyers wrote:
> > > 
> > > [...]
> > > 
> > > (will reply to the rest in the individual patches)
> > > 
> > > > > Can we trust __sync_lock_test_and_set/__sync_add_and_fetch given that
> > > > > __sync_synchronize is broken ?
> > > > 
> > > > I don't know yet.  If it turns out that we cannot, then I will use some
> > > > form of global locking.  But the __sync_lock_test_and_set() do at least
> > > > generate instructions, unlike __sync_synchronize().  ;-)
> > > 
> > > I'm concerned about the fact that their synchronization primitives might have
> > > the assembly all with, except for the memory barriers.
> > 
> > OK.  How about if I used a hashed array of locks, indexed by a hash of
> > the cacheline number of the access in question?  Then the "unknown" case
> > doesn't depend at all on the __sync_ primitives.
> 
> I'm afraid this won't work with signal handlers. :-/ It would be a shame to have
> to make these primitives non-signal-safe just for the sake of the "unknown"
> arch. Or maybe it could make sense to disable signals around these, even though
> it will be really slow ?

Hmmm...

Maybe the right thing to do is to make the build fail in the "unknown"
case.  By the time we make this safe, the performance will be pretty
bad!

My thought is to simply not have the two "unknown" include files, so
that configuration dies when trying to copy them over.  Perhaps better
yet, have the two "unknown" include files have nothing but a #error
statement.  Seem like a reasonable approach?

							Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> > 
> > 							Thanx, Paul
> > 
> > > Thanks,
> > > 
> > > Mathieu
> > > 
> > > > 
> > > > I will send two patches, one that incorporates your suggestions, and
> > > > another that removes sync_core().
> > > > 
> > > > 							Thanx, Paul
> > > > 
> > > > > Thanks,
> > > > > 
> > > > > Mathieu
> > > > > 
> > > > > > +
> > > > > > +#ifdef __cplusplus 
> > > > > > +}
> > > > > > +#endif
> > > > > > +
> > > > > > +#include <urcu/uatomic_generic.h>
> > > > > > +
> > > > > > +#endif /* _URCU_ARCH_UATOMIC_ARMV7_H */
> > > > > 
> > > > > -- 
> > > > > Mathieu Desnoyers
> > > > > Operating System Efficiency R&D Consultant
> > > > > EfficiOS Inc.
> > > > > http://www.efficios.com
> > > 
> > > -- 
> > > Mathieu Desnoyers
> > > Operating System Efficiency R&D Consultant
> > > EfficiOS Inc.
> > > http://www.efficios.com
> 
> -- 
> Mathieu Desnoyers
> Operating System Efficiency R&D Consultant
> EfficiOS Inc.
> http://www.efficios.com

[ltt-dev] [PATCH] fix the "unknown" case

[Mathieu Desnoyers]

* Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> On Sun, Jun 13, 2010 at 05:20:34PM -0400, Mathieu Desnoyers wrote:
> > * Paul E. McKenney (paulmck at linux.vnet.ibm.com) wrote:
> > > On Thu, Jun 10, 2010 at 09:46:31PM -0400, Mathieu Desnoyers wrote:
> > 
> > [...]
> > 
> > (will reply to the rest in the individual patches)
> > 
> > > > Can we trust __sync_lock_test_and_set/__sync_add_and_fetch given that
> > > > __sync_synchronize is broken ?
> > > 
> > > I don't know yet.  If it turns out that we cannot, then I will use some
> > > form of global locking.  But the __sync_lock_test_and_set() do at least
> > > generate instructions, unlike __sync_synchronize().  ;-)
> > 
> > I'm concerned about the fact that their synchronization primitives might have
> > the assembly all with, except for the memory barriers.
> 
> OK.  How about if I used a hashed array of locks, indexed by a hash of
> the cacheline number of the access in question?  Then the "unknown" case
> doesn't depend at all on the __sync_ primitives.

I'm afraid this won't work with signal handlers. :-/ It would be a shame to have
to make these primitives non-signal-safe just for the sake of the "unknown"
arch. Or maybe it could make sense to disable signals around these, even though
it will be really slow ?

Thanks,

Mathieu

> 
> 							Thanx, Paul
> 
> > Thanks,
> > 
> > Mathieu
> > 
> > > 
> > > I will send two patches, one that incorporates your suggestions, and
> > > another that removes sync_core().
> > > 
> > > 							Thanx, Paul
> > > 
> > > > Thanks,
> > > > 
> > > > Mathieu
> > > > 
> > > > > +
> > > > > +#ifdef __cplusplus 
> > > > > +}
> > > > > +#endif
> > > > > +
> > > > > +#include <urcu/uatomic_generic.h>
> > > > > +
> > > > > +#endif /* _URCU_ARCH_UATOMIC_ARMV7_H */
> > > > 
> > > > -- 
> > > > Mathieu Desnoyers
> > > > Operating System Efficiency R&D Consultant
> > > > EfficiOS Inc.
> > > > http://www.efficios.com
> > 
> > -- 
> > Mathieu Desnoyers
> > Operating System Efficiency R&D Consultant
> > EfficiOS Inc.
> > http://www.efficios.com

-- 
Mathieu Desnoyers
Operating System Efficiency R&D Consultant
EfficiOS Inc.
http://www.efficios.com