Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Date:   Wed, 7 Oct 2020 17:07:04 +0200
From:   Peter Zijlstra <peterz@infradead.org>
To:     Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Cc:     Boqun Feng <boqun.feng@gmail.com>, linux-kernel@vger.kernel.org,
        Will Deacon <will@kernel.org>,
        "Paul E . McKenney" <paulmck@kernel.org>,
        Nicholas Piggin <npiggin@gmail.com>,
        Andy Lutomirski <luto@amacapital.net>,
        Andrew Morton <akpm@linux-foundation.org>
Subject: Re: [RFC PATCH 2/3] sched: membarrier: cover kthread_use_mm (v3)
Message-ID: <20201007150704.GH2628@hirez.programming.kicks-ass.net>
References: <20200924172508.8724-1-mathieu.desnoyers@efficios.com>
 <20200924172508.8724-3-mathieu.desnoyers@efficios.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20200924172508.8724-3-mathieu.desnoyers@efficios.com>
Precedence: bulk

On Thu, Sep 24, 2020 at 01:25:07PM -0400, Mathieu Desnoyers wrote:

> diff --git a/kernel/sched/core.c b/kernel/sched/core.c
> index 2d95dc3f4644..bab6f4f2809f 100644
> --- a/kernel/sched/core.c
> +++ b/kernel/sched/core.c
> @@ -3736,6 +3736,8 @@ context_switch(struct rq *rq, struct task_struct *prev,
>  	 */
>  	arch_start_context_switch(prev);
>  
> +	membarrier_switch_mm(rq, prev->mm, next->mm);
> +
>  	/*
>  	 * kernel -> kernel   lazy + transfer active
>  	 *   user -> kernel   lazy + mmgrab() active
> @@ -3752,7 +3754,6 @@ context_switch(struct rq *rq, struct task_struct *prev,
>  		else
>  			prev->active_mm = NULL;
>  	} else {                                        // to user
> -		membarrier_switch_mm(rq, prev->active_mm, next->mm);
>  		/*
>  		 * sys_membarrier() requires an smp_mb() between setting
>  		 * rq->curr / membarrier_switch_mm() and returning to userspace.

I was thinking... do we need the above, when:

> diff --git a/kernel/sched/membarrier.c b/kernel/sched/membarrier.c
> index 8bc8b8a888b7..e5246580201b 100644
> --- a/kernel/sched/membarrier.c
> +++ b/kernel/sched/membarrier.c
> @@ -112,13 +112,9 @@ static int membarrier_global_expedited(void)
>  		    MEMBARRIER_STATE_GLOBAL_EXPEDITED))
>  			continue;
>  
> -		/*
> -		 * Skip the CPU if it runs a kernel thread. The scheduler
> -		 * leaves the prior task mm in place as an optimization when
> -		 * scheduling a kthread.
> -		 */
> +		/* Skip the CPU if it runs the idle thread. */
>  		p = rcu_dereference(cpu_rq(cpu)->curr);
> -		if (p->flags & PF_KTHREAD)

We retain this in the form:

		if ((p->flags & PF_KTHREAD) && !p-mm)
			continue;

> +		if (is_idle_task(p))
>  			continue;
>  
>  		__cpumask_set_cpu(cpu, tmpmask);

Specifically, we only care about kthreads when they're between
kthread_use_mm() / kthread_unuse_mm(), and in that case they will have
updated state already.

It's too late in the day to be sure about the memory ordering though;
but if we see !->mm, they'll do/have-done switch_mm() which implies
sufficient barriers().

Hmm?