| 1 | The kernel can change spinlock behavior when running as a guest. But | 1 | The kernel can change spinlock behavior when running as a guest. But |
|---|---|---|---|
| 2 | this guest-friendly behavior causes performance problems on bare metal. | 2 | this guest-friendly behavior causes performance problems on bare metal. |
| 3 | So there's a 'virt_spin_lock_key' static key to switch between the two | 3 | So there's a 'virt_spin_lock_key' static key to switch between the two |
| 4 | modes. | 4 | modes. |
| 5 | 5 | ||
| 6 | The static key is always enabled by default (run in guest mode) and | 6 | In current code, the static key is always enabled by default when |
| 7 | should be disabled for bare metal (and in some guests that want native | 7 | running in guest mode. The key is disabled for bare metal (and in |
| 8 | behavior). | 8 | some guests that want native behavior). |
| 9 | 9 | ||
| 10 | Performance drop is reported when running encode/decode workload and | 10 | Large performance regression is reported when running encode/decode |
| 11 | BenchSEE cache sub-workload. | 11 | workload and BenchSEE cache sub-workload on the bare metal. |
| 12 | Bisect points to commit ce0a1b608bfc ("x86/paravirt: Silence unused | 12 | Bisect points to commit ce0a1b608bfc ("x86/paravirt: Silence unused |
| 13 | native_pv_lock_init() function warning"). When CONFIG_PARAVIRT_SPINLOCKS | 13 | native_pv_lock_init() function warning"). When CONFIG_PARAVIRT_SPINLOCKS |
| 14 | is disabled the virt_spin_lock_key is incorrectly set to true on bare | 14 | is disabled, the virt_spin_lock_key is incorrectly set to true on bare |
| 15 | metal. The qspinlock degenerates to test-and-set spinlock, which | 15 | metal. The qspinlock degenerates to test-and-set spinlock, which |
| 16 | decrease the performance on bare metal. | 16 | decrease the performance on bare metal. |
| 17 | 17 | ||
| 18 | Fix this by disabling virt_spin_lock_key if it is on bare metal, | 18 | Set the default value of virt_spin_lock_key to false. If booting in |
| 19 | regardless of CONFIG_PARAVIRT_SPINLOCKS. | 19 | a VM, enable this key. Later during the VM initialization, if other |
| 20 | high-efficient spinlock is detected(paravirt-spinlock eg), the | ||
| 21 | virt_spin_lock_key is disabled. According to the description above, | ||
| 22 | the final effect will be as followed: | ||
| 23 | |||
| 24 | X86_FEATURE_HYPERVISOR Y Y Y N | ||
| 25 | CONFIG_PARAVIRT_SPINLOCKS Y Y N Y/N | ||
| 26 | PV spinlock Y N N Y/N | ||
| 27 | |||
| 28 | virt_spin_lock_key N N Y N | ||
| 29 | |||
| 30 | To summarize, the virt_spin_lock_key is disabled on the bare metal | ||
| 31 | no matter what other condidtion is. And the virt_spin_lock_key is | ||
| 32 | also disabled when other spinlock mechanism is detected in the VM | ||
| 33 | guest. | ||
| 20 | 34 | ||
| 21 | Fixes: ce0a1b608bfc ("x86/paravirt: Silence unused native_pv_lock_init() function warning") | 35 | Fixes: ce0a1b608bfc ("x86/paravirt: Silence unused native_pv_lock_init() function warning") |
| 22 | Suggested-by: Dave Hansen <dave.hansen@linux.intel.com> | 36 | Suggested-by: Dave Hansen <dave.hansen@linux.intel.com> |
| 23 | Suggested-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> | 37 | Suggested-by: Qiuxu Zhuo <qiuxu.zhuo@intel.com> |
| 38 | Suggested-by: Nikolay Borisov <nik.borisov@suse.com> | ||
| 24 | Reported-by: Prem Nath Dey <prem.nath.dey@intel.com> | 39 | Reported-by: Prem Nath Dey <prem.nath.dey@intel.com> |
| 25 | Reported-by: Xiaoping Zhou <xiaoping.zhou@intel.com> | 40 | Reported-by: Xiaoping Zhou <xiaoping.zhou@intel.com> |
| 26 | Reviewed-by: Juergen Gross <jgross@suse.com> | 41 | Reviewed-by: Nikolay Borisov <nik.borisov@suse.com> |
| 27 | Signed-off-by: Chen Yu <yu.c.chen@intel.com> | 42 | Signed-off-by: Chen Yu <yu.c.chen@intel.com> |
| 28 | --- | 43 | --- |
| 44 | v3->v4: | ||
| 45 | Refine the commit log. | ||
| 46 | Added Reviewed-by tag from Nikolay. | ||
| 47 | v2->v3: | ||
| 48 | Change the default value of virt_spin_lock_key from true to false. | ||
| 49 | Enable this key when it is in the VM, and disable it when needed. | ||
| 50 | This makes the code more readable. (Nikolay Borisov) | ||
| 51 | Dropped Reviewed-by because the code has been changed. | ||
| 29 | v1->v2: | 52 | v1->v2: |
| 30 | Refine the commit log per Dave's suggestion. | 53 | Refine the commit log per Dave's suggestion. |
| 31 | Simplify the fix by directly disabling the virt_spin_lock_key on bare metal. | 54 | Simplify the fix by directly disabling the virt_spin_lock_key on bare metal. |
| 32 | Collect Reviewed-by from Juergen. | 55 | Collect Reviewed-by from Juergen. |
| 33 | --- | 56 | --- |
| 34 | arch/x86/kernel/paravirt.c | 3 +-- | 57 | arch/x86/include/asm/qspinlock.h | 4 ++-- |
| 35 | 1 file changed, 1 insertion(+), 2 deletions(-) | 58 | arch/x86/kernel/paravirt.c | 7 +++---- |
| 59 | 2 files changed, 5 insertions(+), 6 deletions(-) | ||
| 36 | 60 | ||
| 61 | diff --git a/arch/x86/include/asm/qspinlock.h b/arch/x86/include/asm/qspinlock.h | ||
| 62 | index XXXXXXX..XXXXXXX 100644 | ||
| 63 | --- a/arch/x86/include/asm/qspinlock.h | ||
| 64 | +++ b/arch/x86/include/asm/qspinlock.h | ||
| 65 | @@ -XXX,XX +XXX,XX @@ static inline bool vcpu_is_preempted(long cpu) | ||
| 66 | |||
| 67 | #ifdef CONFIG_PARAVIRT | ||
| 68 | /* | ||
| 69 | - * virt_spin_lock_key - enables (by default) the virt_spin_lock() hijack. | ||
| 70 | + * virt_spin_lock_key - disables (by default) the virt_spin_lock() hijack. | ||
| 71 | * | ||
| 72 | * Native (and PV wanting native due to vCPU pinning) should disable this key. | ||
| 73 | * It is done in this backwards fashion to only have a single direction change, | ||
| 74 | * which removes ordering between native_pv_spin_init() and HV setup. | ||
| 75 | */ | ||
| 76 | -DECLARE_STATIC_KEY_TRUE(virt_spin_lock_key); | ||
| 77 | +DECLARE_STATIC_KEY_FALSE(virt_spin_lock_key); | ||
| 78 | |||
| 79 | /* | ||
| 80 | * Shortcut for the queued_spin_lock_slowpath() function that allows | ||
| 37 | diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c | 81 | diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c |
| 38 | index XXXXXXX..XXXXXXX 100644 | 82 | index XXXXXXX..XXXXXXX 100644 |
| 39 | --- a/arch/x86/kernel/paravirt.c | 83 | --- a/arch/x86/kernel/paravirt.c |
| 40 | +++ b/arch/x86/kernel/paravirt.c | 84 | +++ b/arch/x86/kernel/paravirt.c |
| 41 | @@ -XXX,XX +XXX,XX @@ DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key); | 85 | @@ -XXX,XX +XXX,XX @@ DEFINE_ASM_FUNC(pv_native_irq_enable, "sti", .noinstr.text); |
| 86 | DEFINE_ASM_FUNC(pv_native_read_cr2, "mov %cr2, %rax", .noinstr.text); | ||
| 87 | #endif | ||
| 88 | |||
| 89 | -DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key); | ||
| 90 | +DEFINE_STATIC_KEY_FALSE(virt_spin_lock_key); | ||
| 42 | 91 | ||
| 43 | void __init native_pv_lock_init(void) | 92 | void __init native_pv_lock_init(void) |
| 44 | { | 93 | { |
| 45 | - if (IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) && | 94 | - if (IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) && |
| 46 | - !boot_cpu_has(X86_FEATURE_HYPERVISOR)) | 95 | - !boot_cpu_has(X86_FEATURE_HYPERVISOR)) |
| 47 | + if (!boot_cpu_has(X86_FEATURE_HYPERVISOR)) | 96 | - static_branch_disable(&virt_spin_lock_key); |
| 48 | static_branch_disable(&virt_spin_lock_key); | 97 | + if (boot_cpu_has(X86_FEATURE_HYPERVISOR)) |
| 98 | + static_branch_enable(&virt_spin_lock_key); | ||
| 49 | } | 99 | } |
| 50 | 100 | ||
| 101 | static void native_tlb_remove_table(struct mmu_gather *tlb, void *table) | ||
| 51 | -- | 102 | -- |
| 52 | 2.25.1 | 103 | 2.25.1 | diff view generated by jsdifflib |