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

Groups > linux.kernel > #1201704 > unrolled thread

[PATCH 1/4] ARC: add barriers to futex code

Started byVineet Gupta <Vineet.Gupta1@synopsys.com>
First post2015-08-06 14:40 +0200
Last post2015-08-07 13:50 +0200
Articles 6 — 4 participants

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

  [PATCH 1/4] ARC: add barriers to futex code Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2015-08-06 14:40 +0200
    Re: [PATCH 1/4] ARC: add barriers to futex code David Hildenbrand <dahi@linux.vnet.ibm.com> - 2015-08-06 15:20 +0200
      Re: [PATCH 1/4] ARC: add barriers to futex code Vineet Gupta <Vineet.Gupta1@synopsys.com> - 2015-08-06 15:30 +0200
    Re: [PATCH 1/4] ARC: add barriers to futex code Peter Zijlstra <peterz@infradead.org> - 2015-08-06 15:50 +0200
      Re: [PATCH 1/4] ARC: add barriers to futex code Will Deacon <will.deacon@arm.com> - 2015-08-06 16:20 +0200
      Re: [PATCH 1/4] ARC: add barriers to futex code Peter Zijlstra <peterz@infradead.org> - 2015-08-07 13:50 +0200

#1201704 — [PATCH 1/4] ARC: add barriers to futex code

FromVineet Gupta <Vineet.Gupta1@synopsys.com>
Date2015-08-06 14:40 +0200
Subject[PATCH 1/4] ARC: add barriers to futex code
Message-ID<pUsCK-zY-17@gated-at.bofh.it>
The atomic ops on futex need to provide the full barrier just like
regular atomics in kernel.

Also remove pagefault_enable/disable in futex_atomic_cmpxchg_inatomic()
as core code already does that

Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
Cc: Thomas Gleixner <tglx@linutronix.de>
Cc: Michel Lespinasse <walken@google.com>
Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
---
 arch/arc/include/asm/futex.h | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arc/include/asm/futex.h b/arch/arc/include/asm/futex.h
index 70cfe16b742d..160656d0a15a 100644
--- a/arch/arc/include/asm/futex.h
+++ b/arch/arc/include/asm/futex.h
@@ -20,6 +20,7 @@
 
 #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\
 							\
+	smp_mb();					\
 	__asm__ __volatile__(				\
 	"1:	llock	%1, [%2]		\n"	\
 		insn				"\n"	\
@@ -40,12 +41,14 @@
 							\
 	: "=&r" (ret), "=&r" (oldval)			\
 	: "r" (uaddr), "r" (oparg), "ir" (-EFAULT)	\
-	: "cc", "memory")
+	: "cc", "memory");				\
+	smp_mb();					\
 
 #else	/* !CONFIG_ARC_HAS_LLSC */
 
 #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\
 							\
+	smp_mb();					\
 	__asm__ __volatile__(				\
 	"1:	ld	%1, [%2]		\n"	\
 		insn				"\n"	\
@@ -65,7 +68,8 @@
 							\
 	: "=&r" (ret), "=&r" (oldval)			\
 	: "r" (uaddr), "r" (oparg), "ir" (-EFAULT)	\
-	: "cc", "memory")
+	: "cc", "memory");				\
+	smp_mb();					\
 
 #endif
 
@@ -151,7 +155,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval,
 	if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
 		return -EFAULT;
 
-	pagefault_disable();
+	smp_mb();
 
 	__asm__ __volatile__(
 #ifdef CONFIG_ARC_HAS_LLSC
@@ -178,7 +182,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval,
 	: "r"(oldval), "r"(newval), "r"(uaddr), "ir"(-EFAULT)
 	: "cc", "memory");
 
-	pagefault_enable();
+	smp_mb();
 
 	*uval = val;
 	return val;
-- 
1.9.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [next] | [standalone]

#1201721

FromDavid Hildenbrand <dahi@linux.vnet.ibm.com>
Date2015-08-06 15:20 +0200
Message-ID<pUtfs-1yv-7@gated-at.bofh.it>
In reply to#1201704
> The atomic ops on futex need to provide the full barrier just like
> regular atomics in kernel.
> 
> Also remove pagefault_enable/disable in futex_atomic_cmpxchg_inatomic()
> as core code already does that
> 
> Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Thomas Gleixner <tglx@linutronix.de>
> Cc: Michel Lespinasse <walken@google.com>
> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
> ---
>  arch/arc/include/asm/futex.h | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arc/include/asm/futex.h b/arch/arc/include/asm/futex.h
> index 70cfe16b742d..160656d0a15a 100644
> --- a/arch/arc/include/asm/futex.h
> +++ b/arch/arc/include/asm/futex.h
> @@ -20,6 +20,7 @@
> 
>  #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\
>  							\
> +	smp_mb();					\
>  	__asm__ __volatile__(				\
>  	"1:	llock	%1, [%2]		\n"	\
>  		insn				"\n"	\
> @@ -40,12 +41,14 @@
>  							\
>  	: "=&r" (ret), "=&r" (oldval)			\
>  	: "r" (uaddr), "r" (oparg), "ir" (-EFAULT)	\
> -	: "cc", "memory")
> +	: "cc", "memory");				\
> +	smp_mb();					\

I think you should drop the ;

> 
>  #else	/* !CONFIG_ARC_HAS_LLSC */
> 
>  #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\
>  							\
> +	smp_mb();					\
>  	__asm__ __volatile__(				\
>  	"1:	ld	%1, [%2]		\n"	\
>  		insn				"\n"	\
> @@ -65,7 +68,8 @@
>  							\
>  	: "=&r" (ret), "=&r" (oldval)			\
>  	: "r" (uaddr), "r" (oparg), "ir" (-EFAULT)	\
> -	: "cc", "memory")
> +	: "cc", "memory");				\
> +	smp_mb();					\

dito

> 
>  #endif
> 
> @@ -151,7 +155,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval,
>  	if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
>  		return -EFAULT;
> 
> -	pagefault_disable();
> +	smp_mb();
> 
>  	__asm__ __volatile__(
>  #ifdef CONFIG_ARC_HAS_LLSC
> @@ -178,7 +182,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval,
>  	: "r"(oldval), "r"(newval), "r"(uaddr), "ir"(-EFAULT)
>  	: "cc", "memory");
> 
> -	pagefault_enable();
> +	smp_mb();
> 
>  	*uval = val;
>  	return val;

Looks like pagefault_() magic is only required for futex_atomic_op_inuser. So
this should be fine (and arc seems to be the only arch left that has in in
_inatomic).

Not sure if you want to change the comment:

/* Compare-xchg with pagefaults disabled.
 *  Notes:
 *      -Best-Effort: Exchg happens only if compare succeeds.

Maybe something like "Compare-xchg: pagefaults have to be disabled by the
caller"

Looks sane to me.

David

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201724

FromVineet Gupta <Vineet.Gupta1@synopsys.com>
Date2015-08-06 15:30 +0200
Message-ID<pUtp8-1JS-15@gated-at.bofh.it>
In reply to#1201721
On Thursday 06 August 2015 06:45 PM, David Hildenbrand wrote:
>> The atomic ops on futex need to provide the full barrier just like
>> regular atomics in kernel.
>>
>> Also remove pagefault_enable/disable in futex_atomic_cmpxchg_inatomic()
>> as core code already does that
>>
>> Cc: David Hildenbrand <dahi@linux.vnet.ibm.com>
>> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Michel Lespinasse <walken@google.com>
>> Signed-off-by: Vineet Gupta <vgupta@synopsys.com>
>> ---
>>  arch/arc/include/asm/futex.h | 12 ++++++++----
>>  1 file changed, 8 insertions(+), 4 deletions(-)
>>
>> diff --git a/arch/arc/include/asm/futex.h b/arch/arc/include/asm/futex.h
>> index 70cfe16b742d..160656d0a15a 100644
>> --- a/arch/arc/include/asm/futex.h
>> +++ b/arch/arc/include/asm/futex.h
>> @@ -20,6 +20,7 @@
>>
>>  #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\
>>  							\
>> +	smp_mb();					\
>>  	__asm__ __volatile__(				\
>>  	"1:	llock	%1, [%2]		\n"	\
>>  		insn				"\n"	\
>> @@ -40,12 +41,14 @@
>>  							\
>>  	: "=&r" (ret), "=&r" (oldval)			\
>>  	: "r" (uaddr), "r" (oparg), "ir" (-EFAULT)	\
>> -	: "cc", "memory")
>> +	: "cc", "memory");				\
>> +	smp_mb();					\
> I think you should drop the ;

OK sure !

>
>>  #else	/* !CONFIG_ARC_HAS_LLSC */
>>
>>  #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\
>>  							\
>> +	smp_mb();					\
>>  	__asm__ __volatile__(				\
>>  	"1:	ld	%1, [%2]		\n"	\
>>  		insn				"\n"	\
>> @@ -65,7 +68,8 @@
>>  							\
>>  	: "=&r" (ret), "=&r" (oldval)			\
>>  	: "r" (uaddr), "r" (oparg), "ir" (-EFAULT)	\
>> -	: "cc", "memory")
>> +	: "cc", "memory");				\
>> +	smp_mb();					\
> dito

OK !

>
>>  #endif
>>
>> @@ -151,7 +155,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval,
>>  	if (!access_ok(VERIFY_WRITE, uaddr, sizeof(int)))
>>  		return -EFAULT;
>>
>> -	pagefault_disable();
>> +	smp_mb();
>>
>>  	__asm__ __volatile__(
>>  #ifdef CONFIG_ARC_HAS_LLSC
>> @@ -178,7 +182,7 @@ futex_atomic_cmpxchg_inatomic(u32 *uval, u32 __user *uaddr, u32 oldval,
>>  	: "r"(oldval), "r"(newval), "r"(uaddr), "ir"(-EFAULT)
>>  	: "cc", "memory");
>>
>> -	pagefault_enable();
>> +	smp_mb();
>>
>>  	*uval = val;
>>  	return val;
> Looks like pagefault_() magic is only required for futex_atomic_op_inuser. So
> this should be fine (and arc seems to be the only arch left that has in in
> _inatomic).
>
> Not sure if you want to change the comment:
>
> /* Compare-xchg with pagefaults disabled.
>  *  Notes:
>  *      -Best-Effort: Exchg happens only if compare succeeds.
>
> Maybe something like "Compare-xchg: pagefaults have to be disabled by the
> caller"

Will do !

> Looks sane to me.

Thx for the quick review David. It seems ARC also needs the preempt disable magic
for !LLSC config. I'll send a separate patch to that effect.

Thx,
-Vineet

>
> David
>
>

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201740

FromPeter Zijlstra <peterz@infradead.org>
Date2015-08-06 15:50 +0200
Message-ID<pUtIu-271-25@gated-at.bofh.it>
In reply to#1201704
On Thu, Aug 06, 2015 at 06:05:20PM +0530, Vineet Gupta wrote:
> The atomic ops on futex need to provide the full barrier just like
> regular atomics in kernel.
> 
> Also remove pagefault_enable/disable in futex_atomic_cmpxchg_inatomic()
> as core code already does that

Urgh, and of course tglx just left for holidays :-)

> +++ b/arch/arc/include/asm/futex.h
> @@ -20,6 +20,7 @@
>  
>  #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\
>  							\
> +	smp_mb();					\
>  	__asm__ __volatile__(				\
>  	"1:	llock	%1, [%2]		\n"	\
>  		insn				"\n"	\
> @@ -40,12 +41,14 @@
>  							\
>  	: "=&r" (ret), "=&r" (oldval)			\
>  	: "r" (uaddr), "r" (oparg), "ir" (-EFAULT)	\
> -	: "cc", "memory")
> +	: "cc", "memory");				\
> +	smp_mb();					\
>  


So:

 - alhpa: only has the first smp_mb(), suggesting RELEASE
 - arm: only has the first smp_mb(), suggesting RELEASE
 - arm64: has store-release + smp_mb(), suggesting full barriers
 - MIPS: has LLSC_MB after, suggesting ACQUIRE
 - powerpc: lwsync before, sync after, full barrier

x86 is of course boring and fully ordered

Looking at the usage site of futex_atomic_op_inuser(), that's in
futex_wake_op() which might suggest RELEASE is indeed sufficient.

Which leaves me puzzled on MIPS, but what do I know.

At the very least this patch isn't wrong, fully ordered is sufficient.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1201775

FromWill Deacon <will.deacon@arm.com>
Date2015-08-06 16:20 +0200
Message-ID<pUubw-2Ur-17@gated-at.bofh.it>
In reply to#1201740
On Thu, Aug 06, 2015 at 02:48:26PM +0100, Peter Zijlstra wrote:
> On Thu, Aug 06, 2015 at 06:05:20PM +0530, Vineet Gupta wrote:
> > The atomic ops on futex need to provide the full barrier just like
> > regular atomics in kernel.
> > 
> > Also remove pagefault_enable/disable in futex_atomic_cmpxchg_inatomic()
> > as core code already does that
> 
> Urgh, and of course tglx just left for holidays :-)

Damn, he's really missing out on this!

> > +++ b/arch/arc/include/asm/futex.h
> > @@ -20,6 +20,7 @@
> >  
> >  #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\
> >  							\
> > +	smp_mb();					\
> >  	__asm__ __volatile__(				\
> >  	"1:	llock	%1, [%2]		\n"	\
> >  		insn				"\n"	\
> > @@ -40,12 +41,14 @@
> >  							\
> >  	: "=&r" (ret), "=&r" (oldval)			\
> >  	: "r" (uaddr), "r" (oparg), "ir" (-EFAULT)	\
> > -	: "cc", "memory")
> > +	: "cc", "memory");				\
> > +	smp_mb();					\
> >  
> 
> 
> So:
> 
>  - alhpa: only has the first smp_mb(), suggesting RELEASE
>  - arm: only has the first smp_mb(), suggesting RELEASE
>  - arm64: has store-release + smp_mb(), suggesting full barriers

I'd be ok relaxing that to smp_mb() but I don't think I'm brave enough
to go all the way to an STLXR. You can lose SC if you combine explicit
barrier instructions with the acquire/release instructions and I dread
to think what userspace is doing...

>  - MIPS: has LLSC_MB after, suggesting ACQUIRE

Yikes, so there's a fun semantic difference there. Maybe we should go
look at glibc (which only uses one of the futex ops in pthread_cond_wait
iirc).

>  - powerpc: lwsync before, sync after, full barrier
> 
> x86 is of course boring and fully ordered
> 
> Looking at the usage site of futex_atomic_op_inuser(), that's in
> futex_wake_op() which might suggest RELEASE is indeed sufficient.
> 
> Which leaves me puzzled on MIPS, but what do I know.
> 
> At the very least this patch isn't wrong, fully ordered is sufficient.

Agreed.

Will
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [next] | [standalone]

#1202613

FromPeter Zijlstra <peterz@infradead.org>
Date2015-08-07 13:50 +0200
Message-ID<pUOjU-6NI-9@gated-at.bofh.it>
In reply to#1201740
On Thu, Aug 06, 2015 at 03:48:26PM +0200, Peter Zijlstra wrote:
> On Thu, Aug 06, 2015 at 06:05:20PM +0530, Vineet Gupta wrote:
> > The atomic ops on futex need to provide the full barrier just like
> > regular atomics in kernel.
> > 
> > Also remove pagefault_enable/disable in futex_atomic_cmpxchg_inatomic()
> > as core code already does that
> 
> Urgh, and of course tglx just left for holidays :-)
> 
> > +++ b/arch/arc/include/asm/futex.h
> > @@ -20,6 +20,7 @@
> >  
> >  #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)\
> >  							\
> > +	smp_mb();					\
> >  	__asm__ __volatile__(				\
> >  	"1:	llock	%1, [%2]		\n"	\
> >  		insn				"\n"	\
> > @@ -40,12 +41,14 @@
> >  							\
> >  	: "=&r" (ret), "=&r" (oldval)			\
> >  	: "r" (uaddr), "r" (oparg), "ir" (-EFAULT)	\
> > -	: "cc", "memory")
> > +	: "cc", "memory");				\
> > +	smp_mb();					\
> >  
> 
> 
> So:
> 
>  - alhpa: only has the first smp_mb(), suggesting RELEASE
>  - arm: only has the first smp_mb(), suggesting RELEASE
>  - arm64: has store-release + smp_mb(), suggesting full barriers
>  - MIPS: has LLSC_MB after, suggesting ACQUIRE
>  - powerpc: lwsync before, sync after, full barrier
> 
> x86 is of course boring and fully ordered
> 
> Looking at the usage site of futex_atomic_op_inuser(), that's in
> futex_wake_op() which might suggest RELEASE is indeed sufficient.
> 
> Which leaves me puzzled on MIPS, but what do I know.

So I _think_ the MIPS code is broken. The mips futex code is from 2006
and the mips smp_mb__before_llsc bits are from 2010, so its well
possible this was missed.

Ralf, David, did I miss the obvious or does the below patch make sense?

---
Subject: MIPS: Fix __futex_atomic_op() for WEAK_REORDERING_BEYOND_LLSC

Current __futex_atomic_op() doesn't have smp_mb__before_llsc(), meaning
it would allow prior load/stores to escape out and re-order against the
ll/sc itself.

While the exact requirements on __futex_atomic_op() are a little unclear
at the moment it must be either RELEASE or fully ordered, without
smp_mb__before_llsc() the MIPS code is neither.

Therefore add smp_mb__before_llsc().

Cc: Ralf Baechle <ralf@linux-mips.org>
Cc: David Daney <ddaney@caviumnetworks.com>
Signed-off-by: Peter Zijlstra (Intel) <peterz@infradead.org>
---
 arch/mips/include/asm/futex.h | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/arch/mips/include/asm/futex.h b/arch/mips/include/asm/futex.h
index 1de190bdfb9c..2b8023b9b661 100644
--- a/arch/mips/include/asm/futex.h
+++ b/arch/mips/include/asm/futex.h
@@ -20,6 +20,8 @@
 
 #define __futex_atomic_op(insn, ret, oldval, uaddr, oparg)		\
 {									\
+	smp_mb__before_llsc();						\
+									\
 	if (cpu_has_llsc && R10000_LLSC_WAR) {				\
 		__asm__ __volatile__(					\
 		"	.set	push				\n"	\
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web