Re: [PATCH 2/2] x86 mpx: fix potential performance issue on unmaps

[Ingo Molnar <mingo@kernel.org>]

* Dave Hansen <dave@sr71.net> wrote:

> 
> From: Dave Hansen <dave.hansen@linux.intel.com>
> 
> The 3.19 release window saw some TLB modifications merged which
> caused a performance regression.  They were fixed in commit
> 045bbb9fa.
> 
> Once that fix was applied, I also noticed that there was a small
> but intermittent regression still present.  It was not present
> consistently enough to bisect reliably, but I'm fairly confident
> that it came from (my own) MPX patches.  The source was reading
> a relatively unused field in the mm_struct via arch_unmap.
> 
> I also noted that this code was in the main instruction flow of
> do_munmap() and probably had more icache impact than we want.
> 
> This patch does two things:
> 1. Adds a static (via Kconfig) and dynamic (via cpuid) check
>    for MPX with cpu_feature_enabled().  This keeps us from
>    reading that cacheline in the mm and trades it for a check
>    of the global CPUID variables at least on CPUs without MPX.
> 2. Adds an unlikely() to ensure that the MPX call ends up out
>    of the main instruction flow in do_munmap().  I've added
>    a detailed comment about why this was done and why we want
>    it even on systems where MPX is present.
> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> ---
> 
>  b/arch/x86/include/asm/mmu_context.h |   20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
> 
> diff -puN arch/x86/include/asm/mmu_context.h~fix-mpx-regression-on-unmap arch/x86/include/asm/mmu_context.h
> --- a/arch/x86/include/asm/mmu_context.h~fix-mpx-regression-on-unmap	2014-12-22 12:06:18.677928330 -0800
> +++ b/arch/x86/include/asm/mmu_context.h	2014-12-22 12:06:18.680928465 -0800
> @@ -130,7 +130,25 @@ static inline void arch_bprm_mm_init(str
>  static inline void arch_unmap(struct mm_struct *mm, struct vm_area_struct *vma,
>  			      unsigned long start, unsigned long end)
>  {
> -	mpx_notify_unmap(mm, vma, start, end);
> +	/*
> +	 * mpx_notify_unmap() goes and reads a rarely-hot
> +	 * cacheline in the mm_struct.  That can be expensive
> +	 * enough to be seen in profiles.
> +	 *
> +	 * The mpx_notify_unmap() call and its contents have been
> +	 * observed to affect munmap() performance on hardware
> +	 * where MPX is not present.
> +	 *
> +	 * The unlikely() optimizes for the fast case: no MPX
> +	 * in the CPU, or no MPX use in the process.  Even if
> +	 * we get this wrong (in the unlikely event that MPX
> +	 * is widely enabled on some system) the overhead of
> +	 * MPX itself (reading bounds tables) is expected to
> +	 * overwhelm the overhead of getting this unlikely()
> +	 * consistently wrong.
> +	 */
> +	if (unlikely(cpu_feature_enabled(X86_FEATURE_MPX)))
> +		mpx_notify_unmap(mm, vma, start, end);
>  }

Hm, so this patch still does not help people who have an MPX 
capable CPU but don't have (or don't have many) MPX using apps. 
What about them?

Thanks,

	Ingo