lkml.org 
[lkml]   [2017]   [Jan]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [RFC][PATCH 4/4] x86, mpx: context-switch new MPX address size MSR

* 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

\
 
 \ /
  Last update: 2017-01-27 09:33    [W:0.042 / U:0.956 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site