Re: MIPS: We need to clear MMU contexts of all other processes when asid_cache(cpu) wraps to 0.

[James Hogan <james.hogan@imgtec.com>]

[-- Attachment #1: Type: text/plain, Size: 8711 bytes --]

Hi,

On Sun, Jul 10, 2016 at 01:04:47PM +0000, yhb@ruijie.com.cn wrote:
> From cd1eb951d4a7f01aaa24d2fb902f06b73ef4f608 Mon Sep 17 00:00:00 2001
> From: yhb <yhb@ruijie.com.cn>
> Date: Sun, 10 Jul 2016 20:43:05 +0800
> Subject: [PATCH] MIPS: We need to clear MMU contexts of all other processes
>  when asid_cache(cpu) wraps to 0.
> 
> Suppose that asid_cache(cpu) wraps to 0 every n days.
> case 1:
> (1)Process 1 got ASID 0x101.
> (2)Process 1 slept for n days.
> (3)asid_cache(cpu) wrapped to 0x101, and process 2 got ASID 0x101.
> (4)Process 1 is woken,and ASID of process 1 is same as ASID of process 2.
> 
> case 2:
> (1)Process 1 got ASID 0x101 on CPU 1.
> (2)Process 1 migrated to CPU 2.
> (3)Process 1 migrated to CPU 1 after n days.
> (4)asid_cache on CPU 1 wrapped to 0x101, and process 2 got ASID 0x101.
> (5)Process 1 is scheduled, and ASID of process 1 is same as ASID of process 2.
> 
> So we need to clear MMU contexts of all other processes when asid_cache(cpu) wraps to 0.
> 
> Signed-off-by: yhb <yhb@ruijie.com.cn>
> ---
>  arch/blackfin/kernel/trace.c               |  7 ++--
>  arch/frv/mm/mmu-context.c                  |  6 ++--
>  arch/mips/include/asm/mmu_context.h        | 53 ++++++++++++++++++++++++++++--
>  arch/um/kernel/reboot.c                    |  5 +--
>  block/blk-cgroup.c                         |  6 ++--
>  block/blk-ioc.c                            | 17 ++++++----
>  drivers/staging/android/ion/ion.c          |  5 +--
>  drivers/staging/android/lowmemorykiller.c  | 15 +++++----
>  drivers/staging/lustre/lustre/ptlrpc/sec.c |  5 +--
>  drivers/tty/tty_io.c                       |  6 ++--
>  fs/coredump.c                              |  5 +--
>  fs/exec.c                                  | 17 ++++++----
>  fs/file.c                                  | 16 +++++----
>  fs/fs_struct.c                             | 16 +++++----
>  fs/hugetlbfs/inode.c                       |  6 ++--
>  fs/namespace.c                             |  5 +--
>  fs/proc/array.c                            |  5 +--
>  fs/proc/base.c                             | 40 +++++++++++++---------
>  fs/proc/internal.h                         |  5 +--
>  fs/proc/proc_net.c                         |  6 ++--
>  fs/proc/task_mmu.c                         |  5 +--
>  fs/proc_namespace.c                        |  9 ++---
>  include/linux/cpuset.h                     |  8 ++---
>  include/linux/nsproxy.h                    |  6 ++--
>  include/linux/oom.h                        |  3 +-
>  include/linux/sched.h                      |  8 ++---
>  ipc/namespace.c                            |  5 +--
>  kernel/cgroup.c                            |  5 +--
>  kernel/cpu.c                               |  5 +--
>  kernel/cpuset.c                            |  7 ++--
>  kernel/exit.c                              | 19 +++++++----
>  kernel/fork.c                              | 32 +++++++++++-------
>  kernel/kcmp.c                              |  5 +--
>  kernel/nsproxy.c                           |  5 +--
>  kernel/ptrace.c                            | 11 ++++---
>  kernel/sched/debug.c                       |  5 +--
>  kernel/sys.c                               | 16 +++++----
>  kernel/utsname.c                           |  5 +--
>  mm/memcontrol.c                            |  5 +--
>  mm/mempolicy.c                             | 46 ++++++++++++++++----------
>  mm/mmu_context.c                           | 10 +++---
>  mm/oom_kill.c                              | 37 ++++++++++++---------
>  net/core/net_namespace.c                   | 11 ++++---
>  net/core/netclassid_cgroup.c               |  6 ++--
>  net/core/netprio_cgroup.c                  |  5 +--
>  45 files changed, 337 insertions(+), 188 deletions(-)

[snip / reorder]

> diff --git a/include/linux/sched.h b/include/linux/sched.h
> index 52c4847..9e643fd 100644
> --- a/include/linux/sched.h
> +++ b/include/linux/sched.h
> @@ -2769,14 +2769,14 @@ static inline int thread_group_empty(struct task_struct *p)
>   * It must not be nested with write_lock_irq(&tasklist_lock),
>   * neither inside nor outside.
>   */
> -static inline void task_lock(struct task_struct *p)
> +static inline void task_lock(struct task_struct *p, unsigned long *irqflags)
>  {
> -	spin_lock(&p->alloc_lock);
> +	spin_lock_irqsave(&p->alloc_lock, *irqflags);

Since most of the patch is relating to this change, which is only a
means to an end, I suggest some changes if you stick to this approach
(but see my comments below too):

1) Please separate the change to use _irqsave/_irqrestore (and pass in
irqflags parameter) from whatever else this patch contains (presumably
only the arch/mips/include/asm/mmu_context.h change).

2) Please provide some explanation for why the irqsave change is
necessary. Presumably its due to the desire to lock tasks between irq
context (when context switching, in order to clear all *other* task's
asids) and the various other places.

3) This will affect other arches which don't need the irqsave at all,
which will bloat the kernel slightly unnecessarily. We should consider
if it can/should be made MIPS specific, and whether it can be avoided
entirely.

[snip / reorder]

> diff --git a/arch/mips/include/asm/mmu_context.h b/arch/mips/include/asm/mmu_context.h
> index 45914b5..68966b5 100644
> --- a/arch/mips/include/asm/mmu_context.h
> +++ b/arch/mips/include/asm/mmu_context.h
> @@ -12,6 +12,7 @@
>  #define _ASM_MMU_CONTEXT_H
>  
>  #include <linux/errno.h>
> +#include <linux/oom.h>/* find_lock_task_mm */
>  #include <linux/sched.h>
>  #include <linux/smp.h>
>  #include <linux/slab.h>
> @@ -97,6 +98,52 @@ static inline void enter_lazy_tlb(struct mm_struct *mm, struct task_struct *tsk)
>  #define ASID_VERSION_MASK  ((unsigned long)~(ASID_MASK|(ASID_MASK-1)))
>  #define ASID_FIRST_VERSION ((unsigned long)(~ASID_VERSION_MASK) + 1)
>  
> +/*
> + * Yu Huabing
> + * Suppose that asid_cache(cpu) wraps to 0 every n days.
> + * case 1:
> + * (1)Process 1 got ASID 0x101.
> + * (2)Process 1 slept for n days.
> + * (3)asid_cache(cpu) wrapped to 0x101, and process 2 got ASID 0x101.
> + * (4)Process 1 is woken,and ASID of process 1 is same as ASID of process 2.
> + *
> + * case 2:
> + * (1)Process 1 got ASID 0x101 on CPU 1.
> + * (2)Process 1 migrated to CPU 2.
> + * (3)Process 1 migrated to CPU 1 after n days.
> + * (4)asid_cache on CPU 1 wrapped to 0x101, and process 2 got ASID 0x101.
> + * (5)Process 1 is scheduled,and ASID of process 1 is same as ASID of process 2.
> + *
> + * So we need to clear MMU contexts of all other processes when asid_cache(cpu)
> + * wraps to 0.
> + *
> + * This function might be called from hardirq context or process context.
> + */
> +static inline void clear_other_mmu_contexts(struct mm_struct *mm,
> +						unsigned long cpu)
> +{
> +	struct task_struct *p;
> +	unsigned long irqflags;
> +
> +	read_lock(&tasklist_lock);
> +	for_each_process(p) {
> +		struct task_struct *t;
> +
> +		/*
> +		 * Main thread might exit, but other threads may still have
> +		 * a valid mm. Find one.
> +		 */
> +		t = find_lock_task_mm(p, &irqflags);
> +		if (!t)
> +			continue;
> +
> +		if (t->mm != mm)
> +			cpu_context(cpu, t->mm) = 0;
> +		task_unlock(t, &irqflags);
> +	}
> +	read_unlock(&tasklist_lock);
> +}
> +
>  /* Normal, classic MIPS get_new_mmu_context */
>  static inline void
>  get_new_mmu_context(struct mm_struct *mm, unsigned long cpu)
> @@ -112,8 +159,10 @@ get_new_mmu_context(struct mm_struct *mm, unsigned long cpu)
>  #else
>  		local_flush_tlb_all();	/* start new asid cycle */
>  #endif
> -		if (!asid)		/* fix version if needed */
> -			asid = ASID_FIRST_VERSION;
> +		if (!asid) {
> +			asid = ASID_FIRST_VERSION; /* fix version if needed */
> +			clear_other_mmu_contexts(mm, cpu);
> +		}
>  	}
>  
>  	cpu_context(cpu, mm) = asid_cache(cpu) = asid;

Thank you for pointing out this issue. Clearly it needs to be fixed.

Would it be sufficient/better though to expand the ASID to 64-bit with
e.g. u64 (i.e. even on 32-bit MIPS kernels), and maybe enforce that the
ASID is stored shifted to the least significant bits, rather than in a
form that can be directly written to EntryHi?

Even at 2GHz with an ASID generated every CPU cycle (completely
unrealistic behaviour), it'd still take 292 years before we'd hit this
problem.

That would increase overhead slightly in the common cases around ASID
handling rather than the exceptional overflow case, but would avoid the
additional overhead needed around task locking.

Cheers
James

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]