Message-ID: <949466399.31003.1504115633691.JavaMail.zimbra@efficios.com>
Date:   Wed, 30 Aug 2017 17:53:53 +0000 (UTC)
From:   Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
To:     "Paul E. McKenney" <paulmck@...ux.vnet.ibm.com>
Cc:     Peter Zijlstra <peterz@...radead.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        Boqun Feng <boqun.feng@...il.com>,
        Andrew Hunter <ahh@...gle.com>,
        maged michael <maged.michael@...il.com>,
        gromer <gromer@...gle.com>, Avi Kivity <avi@...lladb.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Michael Ellerman <mpe@...erman.id.au>,
        Dave Watson <davejwatson@...com>,
        Andy Lutomirski <luto@...nel.org>,
        Will Deacon <will.deacon@....com>,
        Hans Boehm <hboehm@...gle.com>
Subject: Re: [RFC PATCH 2/2] membarrier: provide register sync core cmd

----- On Aug 28, 2017, at 1:49 PM, Paul E. McKenney paulmck@...ux.vnet.ibm.com wrote:

> On Sun, Aug 27, 2017 at 09:39:54PM +0000, Mathieu Desnoyers wrote:
>> ----- On Aug 27, 2017, at 2:00 PM, Paul E. McKenney paulmck@...ux.vnet.ibm.com
>> wrote:
>> 
>> > On Sun, Aug 27, 2017 at 08:52:58PM +0000, Mathieu Desnoyers wrote:
>> >> ----- On Aug 27, 2017, at 1:35 PM, Paul E. McKenney paulmck@...ux.vnet.ibm.com
>> >> wrote:
>> >> 
>> >> > On Sun, Aug 27, 2017 at 12:54:04PM -0700, Mathieu Desnoyers wrote:
>> >> >> Add a new MEMBARRIER_CMD_REGISTER_SYNC_CORE command to the membarrier
>> >> >> system call. It allows processes to register their intent to have their
>> >> >> threads issue core serializing barriers in addition to memory barriers
>> >> >> whenever a membarrier command is performed.
>> >> >> 
>> >> >> It is relevant for reclaim of JIT code, which requires to issue core
>> >> >> serializing barriers on all threads running on behalf of a process
>> >> >> after ensuring the old code is not visible anymore, before re-using
>> >> >> memory for new code.
>> >> >> 
>> >> >> When a processes returns from a MEMBARRIER_CMD_REGISTER_SYNC_CORE
>> >> >> command, it is guaranteed that all following MEMBARRIER_CMD_SHARED and
>> >> >> MEMBARRIER_CMD_PRIVATE_EXPEDITED issue core serializing barriers, in
>> >> >> addition to the memory barriers, on all of its running threads.
>> >> > 
>> >> > I have queued both of these for testing and review:
>> >> > 
>> >> >  git://git.kernel.org/pub/scm/linux/kernel/git/paulmck/linux-rcu.git
>> >> >  f54cf5123eb9 ("membarrier: Speed up single-threaded private cmd")
>> >> >  0d6eb99818da ("membarrier: Provide register sync core cmd")
>> >> > 
>> >> > Could the people who asked for this please test it?  The second commit
>> >> > is non-obvious enough to need some careful review and intensive testing
>> >> > before I can in good conscious pass it upstream.
>> >> 
>> >> Thanks Paul!
>> >> 
>> >> Can you pick up my v2 instead ? I added benchmarks in the changelog and
>> >> documented the new command in the membarrier public header. No changes
>> >> otherwise.
>> > 
>> > It looks like 0day Test Robot also wants some additional header files.
>> > Could you please send me a v3 with its complaints addressed?
>> 
>> I sent a separate patch implementing sync_core() on xtensa. It should
>> be applied before this patch.
> 
> And it looks like we also need a sync_core() on every CPU family other
> than x86, which already has one.  Do we want to make this capability
> depend on an ARCH_ kconfig option, or should we just bit the bullet and
> implement sync_core() everywhere?
> 
> (I had to drop this commit due to downstream complaints.  Left to myself,
> I would take the ARCH_ approach, then add it back in, but please let me
> know how you would like to proceed.)

I plan to go with the ARCH_ approach indeed.

Thanks,

Mathieu

> 
>							Thanx, Paul
> 
>> Hopefully we can get some testing from people with xtensa testing
>> environments.
>> 
>> Thanks,
>> 
>> Mathieu
>> 
>> > 
>> >							Thanx, Paul
>> > 
>> >> Thanks,
>> >> 
>> >> Mathieu
>> >> 
>> >> > 
>> >> >							Thanx, Paul
>> >> > 
>> >> >> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@...icios.com>
>> >> >> CC: Peter Zijlstra <peterz@...radead.org>
>> >> >> CC: Paul E. McKenney <paulmck@...ux.vnet.ibm.com>
>> >> >> CC: Boqun Feng <boqun.feng@...il.com>
>> >> >> CC: Andrew Hunter <ahh@...gle.com>
>> >> >> CC: Maged Michael <maged.michael@...il.com>
>> >> >> CC: gromer@...gle.com
>> >> >> CC: Avi Kivity <avi@...lladb.com>
>> >> >> CC: Benjamin Herrenschmidt <benh@...nel.crashing.org>
>> >> >> CC: Paul Mackerras <paulus@...ba.org>
>> >> >> CC: Michael Ellerman <mpe@...erman.id.au>
>> >> >> CC: Dave Watson <davejwatson@...com>
>> >> >> CC: Andy Lutomirski <luto@...nel.org>
>> >> >> CC: Will Deacon <will.deacon@....com>
>> >> >> CC: Hans Boehm <hboehm@...gle.com>
>> >> >> ---
>> >> >>  fs/exec.c                       |  1 +
>> >> >>  include/linux/sched.h           | 52 +++++++++++++++++++++++++++++++++++++++++
>> >> >>  include/uapi/linux/membarrier.h |  1 +
>> >> >>  kernel/fork.c                   |  2 ++
>> >> >>  kernel/sched/core.c             |  3 +++
>> >> >>  kernel/sched/membarrier.c       | 37 ++++++++++++++++++++++++++---
>> >> >>  6 files changed, 93 insertions(+), 3 deletions(-)
>> >> >> 
>> >> >> diff --git a/fs/exec.c b/fs/exec.c
>> >> >> index 62175cbcc801..a4ab3253bac7 100644
>> >> >> --- a/fs/exec.c
>> >> >> +++ b/fs/exec.c
>> >> >> @@ -1794,6 +1794,7 @@ static int do_execveat_common(int fd, struct filename
>> >> >> *filename,
>> >> >>  	/* execve succeeded */
>> >> >>  	current->fs->in_exec = 0;
>> >> >>  	current->in_execve = 0;
>> >> >> +	membarrier_execve(current);
>> >> >>  	acct_update_integrals(current);
>> >> >>  	task_numa_free(current);
>> >> >>  	free_bprm(bprm);
>> >> >> diff --git a/include/linux/sched.h b/include/linux/sched.h
>> >> >> index 8337e2db0bb2..b1ecdc4e8b84 100644
>> >> >> --- a/include/linux/sched.h
>> >> >> +++ b/include/linux/sched.h
>> >> >> @@ -1086,6 +1086,9 @@ struct task_struct {
>> >> >>  	/* Used by LSM modules for access restriction: */
>> >> >>  	void				*security;
>> >> >>  #endif
>> >> >> +#ifdef CONFIG_MEMBARRIER
>> >> >> +	int membarrier_sync_core;
>> >> >> +#endif
>> >> >> 
>> >> >>  	/*
>> >> >>  	 * New fields for task_struct should be added above here, so that
>> >> >> @@ -1623,4 +1626,53 @@ extern long sched_getaffinity(pid_t pid, struct cpumask
>> >> >> *mask);
>> >> >>  #define TASK_SIZE_OF(tsk)	TASK_SIZE
>> >> >>  #endif
>> >> >> 
>> >> >> +#ifdef CONFIG_MEMBARRIER
>> >> >> +static inline void membarrier_fork(struct task_struct *t,
>> >> >> +		unsigned long clone_flags)
>> >> >> +{
>> >> >> +	/*
>> >> >> +	 * Coherence of membarrier_sync_core against thread fork is
>> >> >> +	 * protected by siglock. membarrier_fork is called with siglock
>> >> >> +	 * held.
>> >> >> +	 */
>> >> >> +	t->membarrier_sync_core = current->membarrier_sync_core;
>> >> >> +}
>> >> >> +static inline void membarrier_execve(struct task_struct *t)
>> >> >> +{
>> >> >> +	t->membarrier_sync_core = 0;
>> >> >> +}
>> >> >> +static inline void membarrier_sched_out(struct task_struct *t)
>> >> >> +{
>> >> >> +	/*
>> >> >> +	 * Core serialization is performed before the memory barrier
>> >> >> +	 * preceding the store to rq->curr.
>> >> >> +	 */
>> >> >> +	if (unlikely(READ_ONCE(t->membarrier_sync_core)))
>> >> >> +		sync_core();
>> >> >> +}
>> >> >> +static inline void membarrier_sched_in(struct task_struct *t)
>> >> >> +{
>> >> >> +	/*
>> >> >> +	 * Core serialization is performed after the memory barrier
>> >> >> +	 * following the store to rq->curr.
>> >> >> +	 */
>> >> >> +	if (unlikely(READ_ONCE(t->membarrier_sync_core)))
>> >> >> +		sync_core();
>> >> >> +}
>> >> >> +#else
>> >> >> +static inline void membarrier_fork(struct task_struct *t,
>> >> >> +		unsigned long clone_flags)
>> >> >> +{
>> >> >> +}
>> >> >> +static inline void membarrier_execve(struct task_struct *t)
>> >> >> +{
>> >> >> +}
>> >> >> +static inline void membarrier_sched_out(struct task_struct *t)
>> >> >> +{
>> >> >> +}
>> >> >> +static inline void membarrier_sched_in(struct task_struct *t)
>> >> >> +{
>> >> >> +}
>> >> >> +#endif
>> >> >> +
>> >> >>  #endif
>> >> >> diff --git a/include/uapi/linux/membarrier.h b/include/uapi/linux/membarrier.h
>> >> >> index 6d47b3249d8a..bc26fcf8fb7b 100644
>> >> >> --- a/include/uapi/linux/membarrier.h
>> >> >> +++ b/include/uapi/linux/membarrier.h
>> >> >> @@ -67,6 +67,7 @@ enum membarrier_cmd {
>> >> >>  	/* reserved for MEMBARRIER_CMD_SHARED_EXPEDITED (1 << 1) */
>> >> >>  	/* reserved for MEMBARRIER_CMD_PRIVATE (1 << 2) */
>> >> >>  	MEMBARRIER_CMD_PRIVATE_EXPEDITED	= (1 << 3),
>> >> >> +	MEMBARRIER_CMD_REGISTER_SYNC_CORE	= (1 << 4),
>> >> >>  };
>> >> >> 
>> >> >>  #endif /* _UAPI_LINUX_MEMBARRIER_H */
>> >> >> diff --git a/kernel/fork.c b/kernel/fork.c
>> >> >> index 17921b0390b4..713b3c932671 100644
>> >> >> --- a/kernel/fork.c
>> >> >> +++ b/kernel/fork.c
>> >> >> @@ -1840,6 +1840,8 @@ static __latent_entropy struct task_struct *copy_process(
>> >> >>  	 */
>> >> >>  	copy_seccomp(p);
>> >> >> 
>> >> >> +	membarrier_fork(p, clone_flags);
>> >> >> +
>> >> >>  	/*
>> >> >>  	 * Process group and session signals need to be delivered to just the
>> >> >>  	 * parent before the fork or both the parent and the child after the
>> >> >> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
>> >> >> index 0e36d9960d91..3ca27d066950 100644
>> >> >> --- a/kernel/sched/core.c
>> >> >> +++ b/kernel/sched/core.c
>> >> >> @@ -3292,6 +3292,8 @@ static void __sched notrace __schedule(bool preempt)
>> >> >>  	local_irq_disable();
>> >> >>  	rcu_note_context_switch(preempt);
>> >> >> 
>> >> >> +	membarrier_sched_out(prev);
>> >> >> +
>> >> >>  	/*
>> >> >>  	 * Make sure that signal_pending_state()->signal_pending() below
>> >> >>  	 * can't be reordered with __set_current_state(TASK_INTERRUPTIBLE)
>> >> >> @@ -3364,6 +3366,7 @@ static void __sched notrace __schedule(bool preempt)
>> >> >> 
>> >> >>  		/* Also unlocks the rq: */
>> >> >>  		rq = context_switch(rq, prev, next, &rf);
>> >> >> +		membarrier_sched_in(next);
>> >> >>  	} else {
>> >> >>  		rq->clock_update_flags &= ~(RQCF_ACT_SKIP|RQCF_REQ_SKIP);
>> >> >>  		rq_unlock_irq(rq, &rf);
>> >> >> diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
>> >> >> index 7eec6914d2d2..f128caf64b49 100644
>> >> >> --- a/kernel/sched/membarrier.c
>> >> >> +++ b/kernel/sched/membarrier.c
>> >> >> @@ -25,12 +25,16 @@
>> >> >>   * Bitmask made from a "or" of all commands within enum membarrier_cmd,
>> >> >>   * except MEMBARRIER_CMD_QUERY.
>> >> >>   */
>> >> >> -#define MEMBARRIER_CMD_BITMASK	\
>> >> >> -	(MEMBARRIER_CMD_SHARED | MEMBARRIER_CMD_PRIVATE_EXPEDITED)
>> >> >> +#define MEMBARRIER_CMD_BITMASK			\
>> >> >> +	(MEMBARRIER_CMD_SHARED			\
>> >> >> +	| MEMBARRIER_CMD_PRIVATE_EXPEDITED	\
>> >> >> +	| MEMBARRIER_CMD_REGISTER_SYNC_CORE)
>> >> >> 
>> >> >>  static void ipi_mb(void *info)
>> >> >>  {
>> >> >> -	smp_mb();	/* IPIs should be serializing but paranoid. */
>> >> >> +	/* IPIs should be serializing but paranoid. */
>> >> >> +	smp_mb();
>> >> >> +	sync_core();
>> >> >>  }
>> >> >> 
>> >> >>  static void membarrier_private_expedited(void)
>> >> >> @@ -96,6 +100,30 @@ static void membarrier_private_expedited(void)
>> >> >>  	smp_mb();	/* exit from system call is not a mb */
>> >> >>  }
>> >> >> 
>> >> >> +static void membarrier_register_sync_core(void)
>> >> >> +{
>> >> >> +	struct task_struct *p = current, *t;
>> >> >> +
>> >> >> +	if (get_nr_threads(p) == 1) {
>> >> >> +		p->membarrier_sync_core = 1;
>> >> >> +		return;
>> >> >> +	}
>> >> >> +
>> >> >> +	/*
>> >> >> +	 * Coherence of membarrier_sync_core against thread fork is
>> >> >> +	 * protected by siglock.
>> >> >> +	 */
>> >> >> +	spin_lock(&p->sighand->siglock);
>> >> >> +	for_each_thread(p, t)
>> >> >> +		WRITE_ONCE(t->membarrier_sync_core, 1);
>> >> >> +	spin_unlock(&p->sighand->siglock);
>> >> >> +	/*
>> >> >> +	 * Ensure all future scheduler execution will observe the new
>> >> >> +	 * membarrier_sync_core state for this process.
>> >> >> +	 */
>> >> >> +	synchronize_sched();
>> >> >> +}
>> >> >> +
>> >> >>  /**
>> >> >>   * sys_membarrier - issue memory barriers on a set of threads
>> >> >>   * @cmd:   Takes command values defined in enum membarrier_cmd.
>> >> >> @@ -146,6 +174,9 @@ SYSCALL_DEFINE2(membarrier, int, cmd, int, flags)
>> >> >>  	case MEMBARRIER_CMD_PRIVATE_EXPEDITED:
>> >> >>  		membarrier_private_expedited();
>> >> >>  		return 0;
>> >> >> +	case MEMBARRIER_CMD_REGISTER_SYNC_CORE:
>> >> >> +		membarrier_register_sync_core();
>> >> >> +		return 0;
>> >> >>  	default:
>> >> >>  		return -EINVAL;
>> >> >>  	}
>> >> >> --
>> >> >> 2.11.0
>> >> 
>> >> --
>> >> Mathieu Desnoyers
>> >> EfficiOS Inc.
>> >> http://www.efficios.com
>> 
>> --
>> Mathieu Desnoyers
>> EfficiOS Inc.
>> http://www.efficios.com

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

Powered by blists - more mailing lists