Re: [RFC PATCH v2 1/2] membarrier: Provide register expedited private command

[Mathieu Desnoyers]

- On Sep 21, 2017, at 11:27 AM, Paul E. McKenney paul...@linux.vnet.ibm.com 
wrote:

> On Thu, Sep 21, 2017 at 03:11:43PM +, Mathieu Desnoyers wrote:
>> - On Sep 21, 2017, at 8:13 AM, Peter Zijlstra pet...@infradead.org wrote:
>> 
>> > On Mon, Sep 18, 2017 at 06:36:47PM -0400, Mathieu Desnoyers wrote:
>> >> @@ -3373,6 +3362,7 @@ static void __sched notrace __schedule(bool preempt)
>> >>  
>> >>   /* Also unlocks the rq: */
>> >>   rq = context_switch(rq, prev, next, );
>> >> + membarrier_arch_sched_in(prev, next);
>> > 
>> > That's wrong iirc; we just switched stacks, you cannot use @prev and
>> > @next.
>> 
>> Yes, it did indeed explode spectacularly at boot in testing. RFC v3 fixes 
>> that.
> 
> Good to hear that there is a useful test.  ;-)

For the issue identified by PeterZ, just booting the machine was enough to
touch a task_struct after it was put, which exploded. After fixing this, I
ran through the membarrier selftests: they run fine.

Then I did a test branch for liburcu [1] which uses the private cmd with
registration. Both 'make check' and 'make regtest' run fine on Power8.

Thanks,

Mathieu

[1] https://github.com/compudj/userspace-rcu-dev/tree/test-urcu-reg-priv

> 
>   Thanx, Paul
> 
>> Thanks,
>> 
>> Mathieu
>> 
>> 
>> > 
>> >>   } else {
>> >>   rq->clock_update_flags &= ~(RQCF_ACT_SKIP|RQCF_REQ_SKIP);
>> > >  rq_unlock_irq(rq, );
>> 
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> http://www.efficios.com

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH v2 1/2] membarrier: Provide register expedited private command

[Mathieu Desnoyers]

- On Sep 21, 2017, at 11:27 AM, Paul E. McKenney paul...@linux.vnet.ibm.com 
wrote:

> On Thu, Sep 21, 2017 at 03:11:43PM +, Mathieu Desnoyers wrote:
>> - On Sep 21, 2017, at 8:13 AM, Peter Zijlstra pet...@infradead.org wrote:
>> 
>> > On Mon, Sep 18, 2017 at 06:36:47PM -0400, Mathieu Desnoyers wrote:
>> >> @@ -3373,6 +3362,7 @@ static void __sched notrace __schedule(bool preempt)
>> >>  
>> >>   /* Also unlocks the rq: */
>> >>   rq = context_switch(rq, prev, next, );
>> >> + membarrier_arch_sched_in(prev, next);
>> > 
>> > That's wrong iirc; we just switched stacks, you cannot use @prev and
>> > @next.
>> 
>> Yes, it did indeed explode spectacularly at boot in testing. RFC v3 fixes 
>> that.
> 
> Good to hear that there is a useful test.  ;-)

For the issue identified by PeterZ, just booting the machine was enough to
touch a task_struct after it was put, which exploded. After fixing this, I
ran through the membarrier selftests: they run fine.

Then I did a test branch for liburcu [1] which uses the private cmd with
registration. Both 'make check' and 'make regtest' run fine on Power8.

Thanks,

Mathieu

[1] https://github.com/compudj/userspace-rcu-dev/tree/test-urcu-reg-priv

> 
>   Thanx, Paul
> 
>> Thanks,
>> 
>> Mathieu
>> 
>> 
>> > 
>> >>   } else {
>> >>   rq->clock_update_flags &= ~(RQCF_ACT_SKIP|RQCF_REQ_SKIP);
>> > >  rq_unlock_irq(rq, );
>> 
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> http://www.efficios.com

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH v2 1/2] membarrier: Provide register expedited private command

[Paul E. McKenney]

On Thu, Sep 21, 2017 at 03:11:43PM +, Mathieu Desnoyers wrote:
> - On Sep 21, 2017, at 8:13 AM, Peter Zijlstra pet...@infradead.org wrote:
> 
> > On Mon, Sep 18, 2017 at 06:36:47PM -0400, Mathieu Desnoyers wrote:
> >> @@ -3373,6 +3362,7 @@ static void __sched notrace __schedule(bool preempt)
> >>  
> >>/* Also unlocks the rq: */
> >>rq = context_switch(rq, prev, next, );
> >> +  membarrier_arch_sched_in(prev, next);
> > 
> > That's wrong iirc; we just switched stacks, you cannot use @prev and
> > @next.
> 
> Yes, it did indeed explode spectacularly at boot in testing. RFC v3 fixes 
> that.

Good to hear that there is a useful test.  ;-)

Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> 
> > 
> >>} else {
> >>rq->clock_update_flags &= ~(RQCF_ACT_SKIP|RQCF_REQ_SKIP);
> > >   rq_unlock_irq(rq, );
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
> 

Re: [RFC PATCH v2 1/2] membarrier: Provide register expedited private command

[Paul E. McKenney]

On Thu, Sep 21, 2017 at 03:11:43PM +, Mathieu Desnoyers wrote:
> - On Sep 21, 2017, at 8:13 AM, Peter Zijlstra pet...@infradead.org wrote:
> 
> > On Mon, Sep 18, 2017 at 06:36:47PM -0400, Mathieu Desnoyers wrote:
> >> @@ -3373,6 +3362,7 @@ static void __sched notrace __schedule(bool preempt)
> >>  
> >>/* Also unlocks the rq: */
> >>rq = context_switch(rq, prev, next, );
> >> +  membarrier_arch_sched_in(prev, next);
> > 
> > That's wrong iirc; we just switched stacks, you cannot use @prev and
> > @next.
> 
> Yes, it did indeed explode spectacularly at boot in testing. RFC v3 fixes 
> that.

Good to hear that there is a useful test.  ;-)

Thanx, Paul

> Thanks,
> 
> Mathieu
> 
> 
> > 
> >>} else {
> >>rq->clock_update_flags &= ~(RQCF_ACT_SKIP|RQCF_REQ_SKIP);
> > >   rq_unlock_irq(rq, );
> 
> -- 
> Mathieu Desnoyers
> EfficiOS Inc.
> http://www.efficios.com
> 

Re: [RFC PATCH v2 1/2] membarrier: Provide register expedited private command

[Mathieu Desnoyers]

- On Sep 21, 2017, at 8:13 AM, Peter Zijlstra pet...@infradead.org wrote:

> On Mon, Sep 18, 2017 at 06:36:47PM -0400, Mathieu Desnoyers wrote:
>> @@ -3373,6 +3362,7 @@ static void __sched notrace __schedule(bool preempt)
>>  
>>  /* Also unlocks the rq: */
>>  rq = context_switch(rq, prev, next, );
>> +membarrier_arch_sched_in(prev, next);
> 
> That's wrong iirc; we just switched stacks, you cannot use @prev and
> @next.

Yes, it did indeed explode spectacularly at boot in testing. RFC v3 fixes that.

Thanks,

Mathieu


> 
>>  } else {
>>  rq->clock_update_flags &= ~(RQCF_ACT_SKIP|RQCF_REQ_SKIP);
> > rq_unlock_irq(rq, );

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH v2 1/2] membarrier: Provide register expedited private command

[Mathieu Desnoyers]

- On Sep 21, 2017, at 8:13 AM, Peter Zijlstra pet...@infradead.org wrote:

> On Mon, Sep 18, 2017 at 06:36:47PM -0400, Mathieu Desnoyers wrote:
>> @@ -3373,6 +3362,7 @@ static void __sched notrace __schedule(bool preempt)
>>  
>>  /* Also unlocks the rq: */
>>  rq = context_switch(rq, prev, next, );
>> +membarrier_arch_sched_in(prev, next);
> 
> That's wrong iirc; we just switched stacks, you cannot use @prev and
> @next.

Yes, it did indeed explode spectacularly at boot in testing. RFC v3 fixes that.

Thanks,

Mathieu


> 
>>  } else {
>>  rq->clock_update_flags &= ~(RQCF_ACT_SKIP|RQCF_REQ_SKIP);
> > rq_unlock_irq(rq, );

-- 
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com

Re: [RFC PATCH v2 1/2] membarrier: Provide register expedited private command

[Peter Zijlstra]

On Mon, Sep 18, 2017 at 06:36:47PM -0400, Mathieu Desnoyers wrote:
> @@ -3373,6 +3362,7 @@ static void __sched notrace __schedule(bool preempt)
>  
>   /* Also unlocks the rq: */
>   rq = context_switch(rq, prev, next, );
> + membarrier_arch_sched_in(prev, next);

That's wrong iirc; we just switched stacks, you cannot use @prev and
@next.

>   } else {
>   rq->clock_update_flags &= ~(RQCF_ACT_SKIP|RQCF_REQ_SKIP);
>   rq_unlock_irq(rq, );

Re: [RFC PATCH v2 1/2] membarrier: Provide register expedited private command

[Peter Zijlstra]

On Mon, Sep 18, 2017 at 06:36:47PM -0400, Mathieu Desnoyers wrote:
> @@ -3373,6 +3362,7 @@ static void __sched notrace __schedule(bool preempt)
>  
>   /* Also unlocks the rq: */
>   rq = context_switch(rq, prev, next, );
> + membarrier_arch_sched_in(prev, next);

That's wrong iirc; we just switched stacks, you cannot use @prev and
@next.

>   } else {
>   rq->clock_update_flags &= ~(RQCF_ACT_SKIP|RQCF_REQ_SKIP);
>   rq_unlock_irq(rq, );

[RFC PATCH v2 1/2] membarrier: Provide register expedited private command

[Mathieu Desnoyers]

Provide a new command allowing processes to register their intent to use
the private expedited command.

This allows PowerPC to skip the full memory barrier in switch_mm(), and
only issue the barrier when scheduling into a task belonging to a
process that has registered to use expedited private.

Processes are now required to register before using
MEMBARRIER_CMD_PRIVATE_EXPEDITED, otherwise that command returns EPERM.

[ Build tested on PowerPC. Waiting on test machine to come back to life
  for runtime testing. ]

Changes since v1:
- Use test_ti_thread_flag(next, ...) instead of test_thread_flag() in
  powerpc membarrier_arch_sched_in(), given that we want to specifically
  check the next thread state.
- Add missing ARCH_HAS_MEMBARRIER_HOOKS in Kconfig.
- Use task_thread_info() to pass thread_info from task to
  *_ti_thread_flag().

Signed-off-by: Mathieu Desnoyers 
CC: Peter Zijlstra 
CC: Paul E. McKenney 
CC: Boqun Feng 
CC: Andrew Hunter 
CC: Maged Michael 
CC: gro...@google.com
CC: Avi Kivity 
CC: Benjamin Herrenschmidt 
CC: Paul Mackerras 
CC: Michael Ellerman 
CC: Dave Watson 
CC: Alan Stern 
CC: Will Deacon 
CC: Andy Lutomirski 
CC: linux-a...@vger.kernel.org
---
 MAINTAINERS|  2 ++
 arch/powerpc/Kconfig   |  1 +
 arch/powerpc/include/asm/membarrier.h  | 40 ++
 arch/powerpc/include/asm/thread_info.h |  3 +++
 arch/powerpc/kernel/Makefile   |  2 ++
 arch/powerpc/kernel/membarrier.c   | 45 ++
 fs/exec.c  |  1 +
 include/linux/mm_types.h   |  3 +++
 include/linux/sched/mm.h   | 45 ++
 include/uapi/linux/membarrier.h| 23 +++--
 init/Kconfig   |  3 +++
 kernel/fork.c  |  2 ++
 kernel/sched/core.c| 16 +++-
 kernel/sched/membarrier.c  | 25 ---
 14 files changed, 187 insertions(+), 24 deletions(-)
 create mode 100644 arch/powerpc/include/asm/membarrier.h
 create mode 100644 arch/powerpc/kernel/membarrier.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ef65785cdff2..ac2e2623c34f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8805,6 +8805,8 @@ L:linux-kernel@vger.kernel.org
 S: Supported
 F: kernel/sched/membarrier.c
 F: include/uapi/linux/membarrier.h
+F: arch/powerpc/kernel/membarrier.c
+F: arch/powerpc/include/asm/membarrier.h
 
 MEMORY MANAGEMENT
 L: linux...@kvack.org
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 809c468edab1..6f44c5f74f71 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -138,6 +138,7 @@ config PPC
select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_FORTIFY_SOURCE
select ARCH_HAS_GCOV_PROFILE_ALL
+   select ARCH_HAS_MEMBARRIER_HOOKS
select ARCH_HAS_SCALED_CPUTIME  if VIRT_CPU_ACCOUNTING_NATIVE
select ARCH_HAS_SG_CHAIN
select ARCH_HAS_TICK_BROADCAST  if GENERIC_CLOCKEVENTS_BROADCAST
diff --git a/arch/powerpc/include/asm/membarrier.h 
b/arch/powerpc/include/asm/membarrier.h
new file mode 100644
index ..43a8b264244d
--- /dev/null
+++ b/arch/powerpc/include/asm/membarrier.h
@@ -0,0 +1,40 @@
+#ifndef _ASM_POWERPC_MEMBARRIER_H
+#define _ASM_POWERPC_MEMBARRIER_H
+
+static inline void membarrier_arch_sched_in(struct task_struct *prev,
+   struct task_struct *next)
+{
+   /*
+* Only need the full barrier when switching between processes.
+*/
+   if (likely(!test_ti_thread_flag(task_thread_info(next),
+   TIF_MEMBARRIER_PRIVATE_EXPEDITED)
+   || prev->mm == next->mm))
+   return;
+
+   /*
+* The membarrier system call requires a full memory barrier
+* after storing to rq->curr, before going back to user-space.
+*/
+   smp_mb();
+}
+static inline void membarrier_arch_fork(struct task_struct *t,
+   unsigned long clone_flags)
+{
+   /*
+* Coherence of TIF_MEMBARRIER_PRIVATE_EXPEDITED against thread
+* fork is protected by siglock. membarrier_arch_fork is called
+* with siglock held.
+*/
+   if (t->mm->membarrier_private_expedited)
+   set_ti_thread_flag(task_thread_info(t),
+   TIF_MEMBARRIER_PRIVATE_EXPEDITED);
+}
+static inline void membarrier_arch_execve(struct task_struct *t)
+{
+   clear_ti_thread_flag(task_thread_info(t),
+   TIF_MEMBARRIER_PRIVATE_EXPEDITED);
+}
+void 

[RFC PATCH v2 1/2] membarrier: Provide register expedited private command

[Mathieu Desnoyers]

Provide a new command allowing processes to register their intent to use
the private expedited command.

This allows PowerPC to skip the full memory barrier in switch_mm(), and
only issue the barrier when scheduling into a task belonging to a
process that has registered to use expedited private.

Processes are now required to register before using
MEMBARRIER_CMD_PRIVATE_EXPEDITED, otherwise that command returns EPERM.

[ Build tested on PowerPC. Waiting on test machine to come back to life
  for runtime testing. ]

Changes since v1:
- Use test_ti_thread_flag(next, ...) instead of test_thread_flag() in
  powerpc membarrier_arch_sched_in(), given that we want to specifically
  check the next thread state.
- Add missing ARCH_HAS_MEMBARRIER_HOOKS in Kconfig.
- Use task_thread_info() to pass thread_info from task to
  *_ti_thread_flag().

Signed-off-by: Mathieu Desnoyers 
CC: Peter Zijlstra 
CC: Paul E. McKenney 
CC: Boqun Feng 
CC: Andrew Hunter 
CC: Maged Michael 
CC: gro...@google.com
CC: Avi Kivity 
CC: Benjamin Herrenschmidt 
CC: Paul Mackerras 
CC: Michael Ellerman 
CC: Dave Watson 
CC: Alan Stern 
CC: Will Deacon 
CC: Andy Lutomirski 
CC: linux-a...@vger.kernel.org
---
 MAINTAINERS|  2 ++
 arch/powerpc/Kconfig   |  1 +
 arch/powerpc/include/asm/membarrier.h  | 40 ++
 arch/powerpc/include/asm/thread_info.h |  3 +++
 arch/powerpc/kernel/Makefile   |  2 ++
 arch/powerpc/kernel/membarrier.c   | 45 ++
 fs/exec.c  |  1 +
 include/linux/mm_types.h   |  3 +++
 include/linux/sched/mm.h   | 45 ++
 include/uapi/linux/membarrier.h| 23 +++--
 init/Kconfig   |  3 +++
 kernel/fork.c  |  2 ++
 kernel/sched/core.c| 16 +++-
 kernel/sched/membarrier.c  | 25 ---
 14 files changed, 187 insertions(+), 24 deletions(-)
 create mode 100644 arch/powerpc/include/asm/membarrier.h
 create mode 100644 arch/powerpc/kernel/membarrier.c

diff --git a/MAINTAINERS b/MAINTAINERS
index ef65785cdff2..ac2e2623c34f 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -8805,6 +8805,8 @@ L:linux-kernel@vger.kernel.org
 S: Supported
 F: kernel/sched/membarrier.c
 F: include/uapi/linux/membarrier.h
+F: arch/powerpc/kernel/membarrier.c
+F: arch/powerpc/include/asm/membarrier.h
 
 MEMORY MANAGEMENT
 L: linux...@kvack.org
diff --git a/arch/powerpc/Kconfig b/arch/powerpc/Kconfig
index 809c468edab1..6f44c5f74f71 100644
--- a/arch/powerpc/Kconfig
+++ b/arch/powerpc/Kconfig
@@ -138,6 +138,7 @@ config PPC
select ARCH_HAS_ELF_RANDOMIZE
select ARCH_HAS_FORTIFY_SOURCE
select ARCH_HAS_GCOV_PROFILE_ALL
+   select ARCH_HAS_MEMBARRIER_HOOKS
select ARCH_HAS_SCALED_CPUTIME  if VIRT_CPU_ACCOUNTING_NATIVE
select ARCH_HAS_SG_CHAIN
select ARCH_HAS_TICK_BROADCAST  if GENERIC_CLOCKEVENTS_BROADCAST
diff --git a/arch/powerpc/include/asm/membarrier.h 
b/arch/powerpc/include/asm/membarrier.h
new file mode 100644
index ..43a8b264244d
--- /dev/null
+++ b/arch/powerpc/include/asm/membarrier.h
@@ -0,0 +1,40 @@
+#ifndef _ASM_POWERPC_MEMBARRIER_H
+#define _ASM_POWERPC_MEMBARRIER_H
+
+static inline void membarrier_arch_sched_in(struct task_struct *prev,
+   struct task_struct *next)
+{
+   /*
+* Only need the full barrier when switching between processes.
+*/
+   if (likely(!test_ti_thread_flag(task_thread_info(next),
+   TIF_MEMBARRIER_PRIVATE_EXPEDITED)
+   || prev->mm == next->mm))
+   return;
+
+   /*
+* The membarrier system call requires a full memory barrier
+* after storing to rq->curr, before going back to user-space.
+*/
+   smp_mb();
+}
+static inline void membarrier_arch_fork(struct task_struct *t,
+   unsigned long clone_flags)
+{
+   /*
+* Coherence of TIF_MEMBARRIER_PRIVATE_EXPEDITED against thread
+* fork is protected by siglock. membarrier_arch_fork is called
+* with siglock held.
+*/
+   if (t->mm->membarrier_private_expedited)
+   set_ti_thread_flag(task_thread_info(t),
+   TIF_MEMBARRIER_PRIVATE_EXPEDITED);
+}
+static inline void membarrier_arch_execve(struct task_struct *t)
+{
+   clear_ti_thread_flag(task_thread_info(t),
+   TIF_MEMBARRIER_PRIVATE_EXPEDITED);
+}
+void membarrier_arch_register_private_expedited(struct task_struct *t);
+
+#endif /* _ASM_POWERPC_MEMBARRIER_H */
diff --git a/arch/powerpc/include/asm/thread_info.h 
b/arch/powerpc/include/asm/thread_info.h
index a941cc6fc3e9..2a208487724b 100644
--- a/arch/powerpc/include/asm/thread_info.h
+++ b/arch/powerpc/include/asm/thread_info.h
@@