Re: [RFC][PATCH 4/4] x86, mpx: context-switch new MPX address size MSR

[Ingo Molnar <mingo@kernel.org>]

* Dave Hansen <dave.hansen@linux.intel.com> wrote:

> + * The MPX tables change sizes based on the size of the virtual
> + * (aka. linear) address space.  There is an MSR to tell the CPU
> + * whether we want the legacy-style ones or the larger ones when
> + * we are running with an eXtended virtual address space.
> + */
> +static void switch_mawa(struct mm_struct *prev, struct mm_struct *next)
> +{
> +	/*
> +	 * Note: there is one and only one bit in use in the MSR
> +	 * at this time, so we do not have to be concerned with
> +	 * preseving any of the other bits.  Just write 0 or 1.
> +	 */
> +	unsigned IA32_MPX_LAX_ENABLE_MASK = 0x00000001;
> +
> +	if (!cpu_feature_enabled(X86_FEATURE_MPX))
> +		return;
> +	/*
> +	 * FIXME: do we want a check here for the 5-level paging
> +	 * CR4 bit or CPUID bit, or is the mawa check below OK?
> +	 * It's not obvious what would be the fastest or if it
> +	 * matters.
> +	 */
> +
> +	/*
> +	 * Avoid the relatively costly MSR if we are not changing
> +	 * MAWA state.  All processes not using MPX will have a
> +	 * mpx_mawa_shift()=0, so we do not need to check
> +	 * separately for whether MPX management is enabled.
> +	 */
> +	if (mpx_mawa_shift(prev) == mpx_mawa_shift(next))
> +		return;

Please stop the senseless looking wrappery - if the field is name sensibly then it 
can be accessed directly through mm_struct.

> +
> +	if (mpx_mawa_shift(next)) {
> +		wrmsr(MSR_IA32_MPX_LAX, IA32_MPX_LAX_ENABLE_MASK, 0x0);
> +	} else {
> +		/* clear the enable bit: */
> +		wrmsr(MSR_IA32_MPX_LAX, 0x0, 0x0);
> +	}
> +}
> +
>  void switch_mm_irqs_off(struct mm_struct *prev, struct mm_struct *next,
>  			struct task_struct *tsk)
>  {
> @@ -136,6 +177,7 @@ void switch_mm_irqs_off(struct mm_struct
>  		/* Load per-mm CR4 state */
>  		load_mm_cr4(next);
>  
> +		switch_mawa(prev, next);

This implementation adds about 4-5 unnecessary instructions to the context 
switching hot path of every non-MPX task, even on non-MPX hardware.

Please make sure that this is something like:

	if (unlikely(prev->mpx_msr_val != next->mpx_msr_val))
		switch_mpx(prev, next);

... which reduces the hot path overhead to something like 2 instruction (if we are 
lucky).

This can be put into switch_mpx() and can be inlined - just make sure that on a 
defconfig the generated machine code is sane.

Thanks,

	Ingo

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>