[RFC] perf: fix building for ARCv1

[RFC] perf: fix building for ARCv1

[Vineet Gupta]

On Saturday 17 October 2015 07:06 PM, Alexey Brodkin wrote:
> Perf uses atomic options and so it is required to have atomics enabled
> in toolchain.
>
> In case of ARC atomics are enabled by default for ARCv2 but disabled for
> ARCv1. Now we explicitly enable atomics for either ARC achitecture
> version so perf could be successfully built.
>
> Currently on attempt to build perf for ARCv1 you'll see tons of:
> ----------------->8-----------------
> undefined reference to `__sync_add_and_fetch_4'
> ----------------->8-----------------
>
> Still note if ARCv1 CPU is configured without LL/SC perf will crash on
> execution once "llock" instruction is attempted to be executed.
>
> Cc: Vineet Gupta <vgupta at synopsys.com>
> Cc: Peter Zijlstra <peterz at infradead.org>
> Cc: Arnaldo Carvalho de Melo <acme at kernel.org>
> Signed-off-by: Alexey Brodkin <abrodkin at synopsys.com>

Acked-by: Vineet Gupta <vgupta at synopsys.com>


> ---
>  tools/perf/config/Makefile | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/tools/perf/config/Makefile b/tools/perf/config/Makefile
> index 38a0853..dc7c0a8 100644
> --- a/tools/perf/config/Makefile
> +++ b/tools/perf/config/Makefile
> @@ -47,6 +47,11 @@ ifeq ($(ARCH),arm64)
>    LIBUNWIND_LIBS = -lunwind -lunwind-aarch64
>  endif
>  
> +# Additional ARCH settings for ARC
> +ifeq ($(ARCH),arc)
> +  CFLAGS += -matomic
> +endif
> +
>  ifeq ($(NO_PERF_REGS),0)
>    $(call detected,CONFIG_PERF_REGS)
>  endif

[RFC] perf: fix building for ARCv1

[Alexey Brodkin]

Hi Vineet,

On Tue, 2015-10-20@10:45 +0000, Vineet Gupta wrote:
> On Tuesday 20 October 2015 03:41 PM, Peter Zijlstra wrote:
> > > > Can we use existing syscall(s) - again this is what our good old pthread library
> > > > code did.
> > > > 
> > > > static void __pthread_acquire(int * spinlock)
> > > > {
> > > >   int cnt = 0;
> > > >   struct timespec tm;
> > > > 
> > > >   READ_MEMORY_BARRIER();
> > > > 
> > > >   while (testandset(spinlock)) {   <---- atomic EXchange
> > > >     if (cnt < 50) {
> > > >       sched_yield();
> > > >       cnt++;
> > > >     } else {
> > > >       tm.tv_sec = 0;
> > > >       tm.tv_nsec = 2000001;
> > > >       nanosleep(&tm, ((void *)0));
> > > >       cnt = 0;
> > > >     }
> > > >   }
> > *shudder* that is quite horrible.
> > 
> > This means all your 'atomics' are broken for anything SCHED_FIFO and the
> > like. You simply _cannot_ run a realtime system.
> 
> The code above is from uClibc old threading library which we don't use anymore.
> The NPTL version doesn't have all of this song-n-dance and relies on futexes. The
> change we are talking about is only for the atomics in perf itself. I do
> understand your POV though.
> 
> > (also, for ACQUIRE you want the READ_MEMORY_BARRIER() _after_ the
> > test-and-set control dependency.)
> 
> Absolutely and in this case it will have to be added both inside the loop and one
> at the end to cover both the scenarios !
> 

I'm wondering what are our plans for now?
Are we going to accept proposed fix just for ARC in 4.4 (and to all stables then)
or we'll try to come up with more general solution?

-Alexey

[RFC] perf: fix building for ARCv1

[Vineet Gupta]

On Thursday 29 October 2015 09:28 PM, Alexey Brodkin wrote:
> Hi Vineet,
>
> On Tue, 2015-10-20@10:45 +0000, Vineet Gupta wrote:
>> On Tuesday 20 October 2015 03:41 PM, Peter Zijlstra wrote:
>>>>> Can we use existing syscall(s) - again this is what our good old pthread library
>>>>> code did.
>>>>>
>>>>> static void __pthread_acquire(int * spinlock)
>>>>> {
>>>>>   int cnt = 0;
>>>>>   struct timespec tm;
>>>>>
>>>>>   READ_MEMORY_BARRIER();
>>>>>
>>>>>   while (testandset(spinlock)) {   <---- atomic EXchange
>>>>>     if (cnt < 50) {
>>>>>       sched_yield();
>>>>>       cnt++;
>>>>>     } else {
>>>>>       tm.tv_sec = 0;
>>>>>       tm.tv_nsec = 2000001;
>>>>>       nanosleep(&tm, ((void *)0));
>>>>>       cnt = 0;
>>>>>     }
>>>>>   }
>>> *shudder* that is quite horrible.
>>>
>>> This means all your 'atomics' are broken for anything SCHED_FIFO and the
>>> like. You simply _cannot_ run a realtime system.
>> The code above is from uClibc old threading library which we don't use anymore.
>> The NPTL version doesn't have all of this song-n-dance and relies on futexes. The
>> change we are talking about is only for the atomics in perf itself. I do
>> understand your POV though.
>>
>>> (also, for ACQUIRE you want the READ_MEMORY_BARRIER() _after_ the
>>> test-and-set control dependency.)
>> Absolutely and in this case it will have to be added both inside the loop and one
>> at the end to cover both the scenarios !
>>
> I'm wondering what are our plans for now?
> Are we going to accept proposed fix just for ARC in 4.4 (and to all stables then)
> or we'll try to come up with more general solution?

I agree with the current solution to add -atomic to for arc700 builds.
Although making that default for arc700 tools will be better but that will not fix
things before next release of tools etc.

But we *do* need to improve generic solution
1. Add atomics detection in perf to add fall back arch stubs
2. ARC needs to add syscall for facilitating atomic r-m-w !

-Vineet

[RFC] perf: fix building for ARCv1

[Alexey Brodkin]

Hi Vineet,

On Fri, 2015-10-30@06:19 +0000, Vineet Gupta wrote:
> On Thursday 29 October 2015 09:28 PM, Alexey Brodkin wrote:
> > Hi Vineet,
> > 
> > On Tue, 2015-10-20@10:45 +0000, Vineet Gupta wrote:
> > > On Tuesday 20 October 2015 03:41 PM, Peter Zijlstra wrote:
> > > > > > Can we use existing syscall(s) - again this is what our good old pthread library
> > > > > > code did.
> > > > > > 
> > > > > > static void __pthread_acquire(int * spinlock)
> > > > > > {
> > > > > >   int cnt = 0;
> > > > > >   struct timespec tm;
> > > > > > 
> > > > > >   READ_MEMORY_BARRIER();
> > > > > > 
> > > > > >   while (testandset(spinlock)) {   <---- atomic EXchange
> > > > > >     if (cnt < 50) {
> > > > > >       sched_yield();
> > > > > >       cnt++;
> > > > > >     } else {
> > > > > >       tm.tv_sec = 0;
> > > > > >       tm.tv_nsec = 2000001;
> > > > > >       nanosleep(&tm, ((void *)0));
> > > > > >       cnt = 0;
> > > > > >     }
> > > > > >   }
> > > > *shudder* that is quite horrible.
> > > > 
> > > > This means all your 'atomics' are broken for anything SCHED_FIFO and the
> > > > like. You simply _cannot_ run a realtime system.
> > > The code above is from uClibc old threading library which we don't use anymore.
> > > The NPTL version doesn't have all of this song-n-dance and relies on futexes. The
> > > change we are talking about is only for the atomics in perf itself. I do
> > > understand your POV though.
> > > 
> > > > (also, for ACQUIRE you want the READ_MEMORY_BARRIER() _after_ the
> > > > test-and-set control dependency.)
> > > Absolutely and in this case it will have to be added both inside the loop and one
> > > at the end to cover both the scenarios !
> > > 
> > I'm wondering what are our plans for now?
> > Are we going to accept proposed fix just for ARC in 4.4 (and to all stables then)
> > or we'll try to come up with more general solution?
> 
> I agree with the current solution to add -atomic to for arc700 builds.
> Although making that default for arc700 tools will be better but that will not fix
> things before next release of tools etc.
> 
> But we *do* need to improve generic solution
> 1. Add atomics detection in perf to add fall back arch stubs
> 2. ARC needs to add syscall for facilitating atomic r-m-w !

So the most recent ARC GNU tools (2015.12) were just released yesterday
and still atomics are disabled by default for ARCv1.
That means perf will continue to fail on building for now.

Do you think we may apply my initial fix to 4.5 while it is in development and
then to stable trees as well?

-Alexey

[RFC] perf: fix building for ARCv1

[Vineet Gupta]

+CC Noam

On Wednesday 03 February 2016 09:50 PM, Alexey Brodkin wrote:
>> I agree with the current solution to add -atomic to for arc700 builds.
>> > Although making that default for arc700 tools will be better but that will not fix
>> > things before next release of tools etc.
>> > 
>> > But we *do* need to improve generic solution
>> > 1. Add atomics detection in perf to add fall back arch stubs
>> > 2. ARC needs to add syscall for facilitating atomic r-m-w !
> So the most recent ARC GNU tools (2015.12) were just released yesterday
> and still atomics are disabled by default for ARCv1.
> That means perf will continue to fail on building for now.
> 
> Do you think we may apply my initial fix to 4.5 while it is in development and
> then to stable trees as well?

Noam, what's the atomic story for EZChip. Do you support such things for user
space in GNU tools. If -atomic is added to perf user space builds are you guys OK!

-Vineet

[RFC] perf: fix building for ARCv1

[Noam Camus]

________________________________________
>From: Vineet Gupta <Vineet.Gupta1 at synopsys.com>
>Sent: Thursday, February 4, 2016 6:13 AM
>Noam, what's the atomic story for EZChip. Do you support such things for user
space in GNU tools. If -atomic is added to perf user space builds are you guys OK!

Well here for EZchip I also see the:
undefined reference to `__sync_add_and_fetch_4'
undefined reference to `__sync_sub_and_fetch_4'

This is since at file tools/include/asm/atomic.h we use the generic implementation
If for ARC I could use just like x86 my own header file then functions like:
atomic_inc()
atomic_dec_and_test()
Are easy to implement and you may see an example for such atomic methods in my patch set for the new platform.

You however wants to use some GCC flag -matomic which I assume somehow will implement the above __sync*.
I can't find the implementation but if it uses LLSC then it won't work for me since I am not supporting LLSC.

So seem that either I have my own header at kernel or that I need to change the GCC implementation for __sync* to use my atomic instructions.
I am personally tend to the x86 solution and not the generic one since changing GCC will require to have new compiler dependency.
 
-Noam

[RFC] perf: fix building for ARCv1

[acme]

Em Fri, Feb 05, 2016 at 11:18:52AM +0000, Noam Camus escreveu:
> Well here for EZchip I also see the:
> undefined reference to `__sync_add_and_fetch_4'
> undefined reference to `__sync_sub_and_fetch_4'

Yeah, because there is no: tools/arch/arc/include/asm/atomic.h, can't
you guys adapt arch/arc/include/asm/atomic.h to use in userspace?

- Arnaldo
 
> This is since at file tools/include/asm/atomic.h we use the generic implementation
> If for ARC I could use just like x86 my own header file then functions like:
> atomic_inc()
> atomic_dec_and_test()
> Are easy to implement and you may see an example for such atomic methods in my patch set for the new platform.
> 
> You however wants to use some GCC flag -matomic which I assume somehow will implement the above __sync*.
> I can't find the implementation but if it uses LLSC then it won't work for me since I am not supporting LLSC.

> So seem that either I have my own header at kernel or that I need to
> change the GCC implementation for __sync* to use my atomic
> instructions.  I am personally tend to the x86 solution and not the
> generic one since changing GCC will require to have new compiler
> dependency.

[RFC] perf: fix building for ARCv1

[Vineet Gupta]

On Friday 05 February 2016 09:40 PM, acme@redhat.com wrote:
> Em Fri, Feb 05, 2016 at 11:18:52AM +0000, Noam Camus escreveu:
>> Well here for EZchip I also see the:
>> undefined reference to `__sync_add_and_fetch_4'
>> undefined reference to `__sync_sub_and_fetch_4'
> 
> Yeah, because there is no: tools/arch/arc/include/asm/atomic.h, can't
> you guys adapt arch/arc/include/asm/atomic.h to use in userspace?

Sure - however we need to support 3 variants: LLSC, !LLSC, EZCHIP

If needed, latter 2 could be done using a new atomic assist syscall

I presume kernel Kconfig items are no go in this header so this diversity
management needs to use toolchain defined macros e.g. __ezchip__


> 
> - Arnaldo
>  
>> This is since at file tools/include/asm/atomic.h we use the generic implementation
>> If for ARC I could use just like x86 my own header file then functions like:
>> atomic_inc()
>> atomic_dec_and_test()
>> Are easy to implement and you may see an example for such atomic methods in my patch set for the new platform.
>>
>> You however wants to use some GCC flag -matomic which I assume somehow will implement the above __sync*.
>> I can't find the implementation but if it uses LLSC then it won't work for me since I am not supporting LLSC.
> 
>> So seem that either I have my own header at kernel or that I need to
>> change the GCC implementation for __sync* to use my atomic
>> instructions.  I am personally tend to the x86 solution and not the
>> generic one since changing GCC will require to have new compiler
>> dependency.

[RFC] perf: fix building for ARCv1

[Vineet Gupta]

On 10/19/2015 02:35 AM, Peter Zijlstra wrote:
> On Mon, Oct 19, 2015@09:28:43AM +0000, Vineet Gupta wrote:
>> On Monday 19 October 2015 11:20 AM, Andi Kleen wrote:
>>> Vineet Gupta <Vineet.Gupta1 at synopsys.com> writes:
>>>> But this user space - so IMHO UP/SMP doesn't matter and we can't simulate them in
>>>> C just by itself.
>>> It matters when you access the perf ring buffer which is updated by kernel.
>> That's part of the problem. The issue is with atomic_* APIs proliferation in perf
>> user space code which assumes native atomix r-m-w support which is not always
>> true. So I think we still need a feature detection mechanism and if absent leave
>> the ball in arch court by calling arch_atomic_* which can use creative or half
>> working measures so perf will work to some extent atleast and not bomb outright.
>>
>> Also can u please elaborate a bit on "simulate them in C" - u mean just simple
>> unprotected LD, OP, ST or do u fancy usage of futex etc?
> Doesn't ARMv5 have a cmpxchg syscall to deal with this? It does an
> IRQ-disabled load-op-store sequence.

So I got around to addressing this - now that someone actually is trying to use
NPTL (which uses llock/scond) on ARC700 lacking those instructions. However given
that we are going this route, FWIW ARM kernel got rid of this syscall with
db695c0509d6ec ("ARM: remove user cmpxchg syscall") citing some security hole.
Even of we were to disregard, the code at the time had some open code MM trickery,
which I'd rather not replicate. My use case is simple - I only need to support UP
config - and a simple {get,put}_user would suffice - given that that can
potentially take a TLB refill Miss or worse still a full page fault. I'm going to
cook that patch to add that syscall, but wanted to get some thoughts ahead of that.

-Vineet

[PATCH-v2] ARC: syscall for userspace cmpxchg assist

[Vineet Gupta]

Older ARC700 cores (ARC750 specifically) lack instructions to implement
atomic r-w-w. This is problematic for userspace libraries such as NPTL
which need atomic primitives. So enable them by providing kernel assist.
This is costly but really the only sane soluton (othern than tight
spinning using the otherwise avaialble atomic exchange EX instruciton).

Good thing is there are only a few of these cores running Linux out in
the wild.

This only works on UP systems.

Reviewed-by: Colin Ian King <colin.king at canonical.com>
Signed-off-by: Vineet Gupta <vgupta at synopsys.com>
---
Changes since v1
 - errno not returned for access_ok() failing  [Colin]
 - Beefed up change log
 - WARN_ON_ONCE() for CONFIG_SMP since this is only UP safe
---
 arch/arc/include/asm/syscalls.h    |  1 +
 arch/arc/include/uapi/asm/unistd.h |  9 +++++----
 arch/arc/kernel/process.c          | 33 +++++++++++++++++++++++++++++++++
 3 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/arch/arc/include/asm/syscalls.h b/arch/arc/include/asm/syscalls.h
index e56f9fcc5581..772b67ca56e7 100644
--- a/arch/arc/include/asm/syscalls.h
+++ b/arch/arc/include/asm/syscalls.h
@@ -17,6 +17,7 @@ int sys_clone_wrapper(int, int, int, int, int);
 int sys_cacheflush(uint32_t, uint32_t uint32_t);
 int sys_arc_settls(void *);
 int sys_arc_gettls(void);
+int sys_arc_usr_cmpxchg(int *, int, int);
 
 #include <asm-generic/syscalls.h>
 
diff --git a/arch/arc/include/uapi/asm/unistd.h b/arch/arc/include/uapi/asm/unistd.h
index 41fa2ec9e02c..9a34136d84b2 100644
--- a/arch/arc/include/uapi/asm/unistd.h
+++ b/arch/arc/include/uapi/asm/unistd.h
@@ -27,18 +27,19 @@
 
 #define NR_syscalls	__NR_syscalls
 
+/* Generic syscall (fs/filesystems.c - lost in asm-generic/unistd.h */
+#define __NR_sysfs		(__NR_arch_specific_syscall + 3)
+
 /* ARC specific syscall */
 #define __NR_cacheflush		(__NR_arch_specific_syscall + 0)
 #define __NR_arc_settls		(__NR_arch_specific_syscall + 1)
 #define __NR_arc_gettls		(__NR_arch_specific_syscall + 2)
+#define __NR_arc_usr_cmpxchg	(__NR_arch_specific_syscall + 4)
 
 __SYSCALL(__NR_cacheflush, sys_cacheflush)
 __SYSCALL(__NR_arc_settls, sys_arc_settls)
 __SYSCALL(__NR_arc_gettls, sys_arc_gettls)
-
-
-/* Generic syscall (fs/filesystems.c - lost in asm-generic/unistd.h */
-#define __NR_sysfs		(__NR_arch_specific_syscall + 3)
+__SYSCALL(__NR_arc_usr_cmpxchg, sys_arc_usr_cmpxchg)
 __SYSCALL(__NR_sysfs, sys_sysfs)
 
 #undef __SYSCALL
diff --git a/arch/arc/kernel/process.c b/arch/arc/kernel/process.c
index be1972bd2729..59aa43cb146e 100644
--- a/arch/arc/kernel/process.c
+++ b/arch/arc/kernel/process.c
@@ -41,6 +41,39 @@ SYSCALL_DEFINE0(arc_gettls)
 	return task_thread_info(current)->thr_ptr;
 }
 
+SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new)
+{
+	int uval;
+	int ret;
+
+	/*
+	 * This is only for old cores lacking LLOCK/SCOND, which by defintion
+	 * can't possibly be SMP. Thus doesn't need to be SMP safe.
+	 * And this also helps reduce the overhead for serializing in
+	 * the UP case
+	 */
+	WARN_ON_ONCE(IS_ENABLED(CONFIG_SMP));
+
+	if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
+		return -EFAULT;
+
+	preempt_disable();
+
+	ret = __get_user(uval, uaddr);
+	if (ret)
+		goto done;
+
+	if (uval != expected)
+		ret = -EAGAIN;
+	else
+		ret = __put_user(new, uaddr);
+
+done:
+	preempt_enable();
+
+	return ret;
+}
+
 void arch_cpu_idle(void)
 {
 	/* sleep, but enable all interrupts before committing */
-- 
2.7.4

[PATCH-v2] ARC: syscall for userspace cmpxchg assist

[Vineet Gupta]

On 10/24/2016 09:17 AM, Vineet Gupta wrote:
> Older ARC700 cores (ARC750 specifically) lack instructions to implement
> atomic r-w-w. This is problematic for userspace libraries such as NPTL
> which need atomic primitives. So enable them by providing kernel assist.
> This is costly but really the only sane soluton (othern than tight
> spinning using the otherwise avaialble atomic exchange EX instruciton).
> 
> Good thing is there are only a few of these cores running Linux out in
> the wild.
> 
> This only works on UP systems.
> 
> Reviewed-by: Colin Ian King <colin.king at canonical.com>
> Signed-off-by: Vineet Gupta <vgupta at synopsys.com>
> ---
> Changes since v1
>  - errno not returned for access_ok() failing  [Colin]
>  - Beefed up change log
>  - WARN_ON_ONCE() for CONFIG_SMP since this is only UP safe
> ---
>  arch/arc/include/asm/syscalls.h    |  1 +
>  arch/arc/include/uapi/asm/unistd.h |  9 +++++----
>  arch/arc/kernel/process.c          | 33 +++++++++++++++++++++++++++++++++
>  3 files changed, 39 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arc/include/asm/syscalls.h b/arch/arc/include/asm/syscalls.h
> index e56f9fcc5581..772b67ca56e7 100644
> --- a/arch/arc/include/asm/syscalls.h
> +++ b/arch/arc/include/asm/syscalls.h
> @@ -17,6 +17,7 @@ int sys_clone_wrapper(int, int, int, int, int);
>  int sys_cacheflush(uint32_t, uint32_t uint32_t);
>  int sys_arc_settls(void *);
>  int sys_arc_gettls(void);
> +int sys_arc_usr_cmpxchg(int *, int, int);
>  
>  #include <asm-generic/syscalls.h>
>  
> diff --git a/arch/arc/include/uapi/asm/unistd.h b/arch/arc/include/uapi/asm/unistd.h
> index 41fa2ec9e02c..9a34136d84b2 100644
> --- a/arch/arc/include/uapi/asm/unistd.h
> +++ b/arch/arc/include/uapi/asm/unistd.h
> @@ -27,18 +27,19 @@
>  
>  #define NR_syscalls	__NR_syscalls
>  
> +/* Generic syscall (fs/filesystems.c - lost in asm-generic/unistd.h */
> +#define __NR_sysfs		(__NR_arch_specific_syscall + 3)
> +
>  /* ARC specific syscall */
>  #define __NR_cacheflush		(__NR_arch_specific_syscall + 0)
>  #define __NR_arc_settls		(__NR_arch_specific_syscall + 1)
>  #define __NR_arc_gettls		(__NR_arch_specific_syscall + 2)
> +#define __NR_arc_usr_cmpxchg	(__NR_arch_specific_syscall + 4)
>  
>  __SYSCALL(__NR_cacheflush, sys_cacheflush)
>  __SYSCALL(__NR_arc_settls, sys_arc_settls)
>  __SYSCALL(__NR_arc_gettls, sys_arc_gettls)
> -
> -
> -/* Generic syscall (fs/filesystems.c - lost in asm-generic/unistd.h */
> -#define __NR_sysfs		(__NR_arch_specific_syscall + 3)
> +__SYSCALL(__NR_arc_usr_cmpxchg, sys_arc_usr_cmpxchg)
>  __SYSCALL(__NR_sysfs, sys_sysfs)
>  
>  #undef __SYSCALL
> diff --git a/arch/arc/kernel/process.c b/arch/arc/kernel/process.c
> index be1972bd2729..59aa43cb146e 100644
> --- a/arch/arc/kernel/process.c
> +++ b/arch/arc/kernel/process.c
> @@ -41,6 +41,39 @@ SYSCALL_DEFINE0(arc_gettls)
>  	return task_thread_info(current)->thr_ptr;
>  }
>  
> +SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new)
> +{
> +	int uval;
> +	int ret;
> +
> +	/*
> +	 * This is only for old cores lacking LLOCK/SCOND, which by defintion
> +	 * can't possibly be SMP. Thus doesn't need to be SMP safe.
> +	 * And this also helps reduce the overhead for serializing in
> +	 * the UP case
> +	 */
> +	WARN_ON_ONCE(IS_ENABLED(CONFIG_SMP));
> +
> +	if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
> +		return -EFAULT;
> +
> +	preempt_disable();
> +
> +	ret = __get_user(uval, uaddr);
> +	if (ret)
> +		goto done;
> +
> +	if (uval != expected)
> +		ret = -EAGAIN;
> +	else
> +		ret = __put_user(new, uaddr);
> +
> +done:
> +	preempt_enable();
> +
> +	return ret;
> +}

It seems there is a subtle issue with this interface. Userspace cares more about
"prev" value to be able to build it's own state machine(s) - my existing uclibc
code was flawed as it was tight looping on the errno result.

We can add a return param, by passing a pointer, but I think it would be better
(and slightly cheaper) to just ditch the errno and simply return the prev value
which and current value could be checked for success/fail decision etc.

-Vineet

[PATCH] ARC: tweak semantics of userspace cmpxchg assist

[Vineet Gupta]

The original code only used to return errno to indicate if cmpxchg
succeeded. It was not returning the "previous" value which typical cmpxhg
callers are interested in to build their slowpaths or retry loops.
Given user preemption in syscall return path etc, it is not wise to
check this in userspace afterwards, but should what kernel actually
observed in the syscall.

So change the syscall interface to always return the previous value and
additionally set Z flag to indicate whether operation succeeded or not
(just like ARM implementation when they used ot have this syscall)
The flag approach avoids having to put_user errno and specially when
the use case for this syscall cares mostly about the "previous" value.

Signed-off-by: Vineet Gupta <vgupta at synopsys.com>
---
 arch/arc/include/asm/arcregs.h |  2 ++
 arch/arc/kernel/process.c      | 20 +++++++++++---------
 2 files changed, 13 insertions(+), 9 deletions(-)

diff --git a/arch/arc/include/asm/arcregs.h b/arch/arc/include/asm/arcregs.h
index 7f3f9f63708c..1bd24ec3e350 100644
--- a/arch/arc/include/asm/arcregs.h
+++ b/arch/arc/include/asm/arcregs.h
@@ -43,12 +43,14 @@
 #define STATUS_AE_BIT		5	/* Exception active */
 #define STATUS_DE_BIT		6	/* PC is in delay slot */
 #define STATUS_U_BIT		7	/* User/Kernel mode */
+#define STATUS_Z_BIT            11
 #define STATUS_L_BIT		12	/* Loop inhibit */
 
 /* These masks correspond to the status word(STATUS_32) bits */
 #define STATUS_AE_MASK		(1<<STATUS_AE_BIT)
 #define STATUS_DE_MASK		(1<<STATUS_DE_BIT)
 #define STATUS_U_MASK		(1<<STATUS_U_BIT)
+#define STATUS_Z_MASK		(1<<STATUS_Z_BIT)
 #define STATUS_L_MASK		(1<<STATUS_L_BIT)
 
 /*
diff --git a/arch/arc/kernel/process.c b/arch/arc/kernel/process.c
index 59aa43cb146e..a41a79a4f4fe 100644
--- a/arch/arc/kernel/process.c
+++ b/arch/arc/kernel/process.c
@@ -43,8 +43,8 @@ SYSCALL_DEFINE0(arc_gettls)
 
 SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new)
 {
-	int uval;
-	int ret;
+	struct pt_regs *regs = current_pt_regs();
+	int uval = -EFAULT;
 
 	/*
 	 * This is only for old cores lacking LLOCK/SCOND, which by defintion
@@ -54,24 +54,26 @@ SYSCALL_DEFINE3(arc_usr_cmpxchg, int *, uaddr, int, expected, int, new)
 	 */
 	WARN_ON_ONCE(IS_ENABLED(CONFIG_SMP));
 
+	/* Z indicates to userspace if operation succeded */
+	regs->status32 &= ~STATUS_Z_MASK;
+
 	if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
 		return -EFAULT;
 
 	preempt_disable();
 
-	ret = __get_user(uval, uaddr);
-	if (ret)
+	if (__get_user(uval, uaddr))
 		goto done;
 
-	if (uval != expected)
-		ret = -EAGAIN;
-	else
-		ret = __put_user(new, uaddr);
+	if (uval == expected) {
+		if (!__put_user(new, uaddr))
+			regs->status32 |= STATUS_Z_MASK;
+	}
 
 done:
 	preempt_enable();
 
-	return ret;
+	return uval;
 }
 
 void arch_cpu_idle(void)
-- 
2.7.4