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

Ingo Molnar
2014-12-23T09:14:47Z

* 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