Re: [RFC PATCH 1/2] sched: Fix exit_mm vs membarrier
From: peterz
Date: Tue Aug 04 2020 - 10:35:48 EST
On Tue, Jul 28, 2020 at 12:00:09PM -0400, Mathieu Desnoyers wrote:
> exit_mm should issue memory barriers after user-space memory accesses,
> before clearing current->mm, to order user-space memory accesses
> performed prior to exit_mm before clearing tsk->mm, which has the
> effect of skipping the membarrier private expedited IPIs.
>
> The membarrier system call can be issued concurrently with do_exit
> if we have thread groups created with CLONE_VM but not CLONE_THREAD.
I'm still wonder what the exact failure case is though; exit_mm() is on
the exit path (as the name very much implies) and the thread is about to
die. The context switch that follows guarantees a full barrier before we
run anything else again.
> The following comment in exit_mm() seems rather puzzling though:
>
> /* more a memory barrier than a real lock */
> task_lock(current);
>
> So considering that spinlocks are not full memory barriers nowadays,
> some digging into the origins of this comment led me to 2002, at the
> earliest commit tracked by Thomas Gleixner's bitkeeper era's history
> at https://git.kernel.org/pub/scm/linux/kernel/git/tglx/history.git/
>
> At that point, this comment was followed by:
>
> + /* more a memory barrier than a real lock */
> + task_lock(tsk);
> + tsk->mm = NULL;
> + task_unlock(tsk);
>
> Which seems to indicate that grabbing the lock is really about acting as
> a memory barrier, but I wonder if it is meant to be a full barrier or
> just an acquire.
>
> If a full memory barrier happens to be needed even without membarrier,
> perhaps this fix also corrects other issues ? It unclear from the
> comment what this memory barrier orders though. Is it the chain
> exit -> waitpid, or is it related to entering lazy TLB ?
I'm as puzzled by that comment as are you.
It does seem to be required for glorious stuff like get_task_mm()
though.
> Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@xxxxxxxxxxxx>
> Cc: Peter Zijlstra (Intel) <peterz@xxxxxxxxxxxxx>
> Cc: Will Deacon <will@xxxxxxxxxx>
> Cc: Paul E. McKenney <paulmck@xxxxxxxxxx>
> Cc: Nicholas Piggin <npiggin@xxxxxxxxx>
> Cc: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> Cc: Linus Torvalds <torvalds@xxxxxxxxxxxxxxxxxxxx>
> Cc: Alan Stern <stern@xxxxxxxxxxxxxxxxxxx>
> Cc: linux-mm@xxxxxxxxx
> ---
> kernel/exit.c | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/kernel/exit.c b/kernel/exit.c
> index 727150f28103..ce272ec55cdc 100644
> --- a/kernel/exit.c
> +++ b/kernel/exit.c
> @@ -474,6 +474,14 @@ static void exit_mm(void)
> BUG_ON(mm != current->active_mm);
> /* more a memory barrier than a real lock */
> task_lock(current);
> + /*
> + * When a thread stops operating on an address space, the loop
> + * in membarrier_{private,global}_expedited() may not observe
> + * that tsk->mm, and not issue an IPI. Membarrier requires a
> + * memory barrier after accessing user-space memory, before
> + * clearing tsk->mm.
> + */
> + smp_mb();
So like stated above, I'm not sure how missing that IPI is going to be a
problem, we're dying...
> current->mm = NULL;
> mmap_read_unlock(mm);
> enter_lazy_tlb(mm, current);
> --
> 2.11.0
>