On 25.06.24 г. 17:50 ч., Chen Yu wrote:
On 2024-06-25 at 16:42:11 +0300, Nikolay Borisov wrote:On 25.06.24 г. 15:54 ч., Chen Yu wrote:The kernel can change spinlock behavior when running as a guest. But this guest-friendly behavior causes performance problems on bare metal. So there's a 'virt_spin_lock_key' static key to switch between the two modes. The static key is always enabled by default (run in guest mode) and should be disabled for bare metal (and in some guests that want native behavior). Performance drop is reported when running encode/decode workload and BenchSEE cache sub-workload. Bisect points to commit ce0a1b608bfc ("x86/paravirt: Silence unused native_pv_lock_init() function warning"). When CONFIG_PARAVIRT_SPINLOCKS is disabled the virt_spin_lock_key is incorrectly set to true on bare metal. The qspinlock degenerates to test-and-set spinlock, which decrease the performance on bare metal. Set the default value of virt_spin_lock_key to false. If booting in a VM, enable this key. Later during the VM initialization, if other high-efficient spinlock is preferred(paravirt-spinlock eg), the virt_spin_lock_key is disabled accordingly. The relation is described as below: X86_FEATURE_HYPERVISOR Y Y Y N CONFIG_PARAVIRT_SPINLOCKS Y Y N Y/N PV spinlock Y N N Y/N virt_spin_lock_key N N Y N -DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key); +DEFINE_STATIC_KEY_FALSE(virt_spin_lock_key); void __init native_pv_lock_init(void) { - if (IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) &&Actually now shouldn't the CONFIG_PARAVIRT_SPINLOCKS check be retained? Otherwise we'll have the virtspinlock enabled even if we are a guest but CONFIG_PARAVIRT_SPINLOCKS is disabled, no ?It seems to be the expected behavior? If CONFIG_PARAVIRT_SPINLOCKS is disabled, should the virt_spin_lock_key be enabled in the guest?
No, but if it's disabled and we are under a hypervisor shouldn't the virt spinlock be kept disabled? As it stands now everytime we are under a hypervisor the virt spinlock is enabled irrespective of the PARAVIRT_SPINLOCK config state.
The previous behavior before commit ce0a1b608bfc ("x86/paravirt: Silence unused
native_pv_lock_init() function warning"): kvm_spinlock_init() is NULL if
CONFIG_PARAVIRT_SPINLOCKS is disabled, and static_branch_disable(&virt_spin_lock_key)
can not be invoked, so the virt_spin_lock_key keeps enabled.
thanks,
Chenyu
