Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1515379 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH-v2] ARC: syscall for userspace cmpxchg assist Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-04 21:30 +0100
    [PATCH] ARC: tweak semantics of userspace cmpxchg assist Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2016-11-07 20:00 +0100

#1515379 — Re: [PATCH-v2] ARC: syscall for userspace cmpxchg assist

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@canonical.com>
> Signed-off-by: Vineet Gupta <vgupta@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

[toc] | [next] | [standalone]

#1516474 — [PATCH] ARC: tweak semantics of userspace cmpxchg assist

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@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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web