Received-SPF: pass (google.com: domain of linux-kernel+bounces-187889-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Message-ID: <ec8b5d13-3034-4255-ace3-cf1564646da9@suse.com>
Date: Thu, 23 May 2024 20:39:59 +0200
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH] x86/paravirt: Disable virt spinlock when
 CONFIG_PARAVIRT_SPINLOCKS disabled
To: Dave Hansen <dave.hansen@intel.com>, Chen Yu <yu.c.chen@intel.com>,
 Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
 Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>
Cc: Ajay Kaher <ajay.kaher@broadcom.com>,
 Alexey Makhalov <alexey.amakhalov@broadcom.com>,
 Arnd Bergmann <arnd@arndb.de>, "H. Peter Anvin" <hpa@zytor.com>,
 x86@kernel.org, virtualization@lists.linux.dev,
 linux-kernel@vger.kernel.org, Qiuxu Zhuo <qiuxu.zhuo@intel.com>,
 Prem Nath Dey <prem.nath.dey@intel.com>,
 Xiaoping Zhou <xiaoping.zhou@intel.com>
References: <20240516130244.893573-1-yu.c.chen@intel.com>
 <7b8d1dd6-3913-45fe-941e-aac2c15916dc@intel.com>
Content-Language: en-US
From: =?UTF-8?B?SsO8cmdlbiBHcm/Dnw==?= <jgross@suse.com>
In-Reply-To: <7b8d1dd6-3913-45fe-941e-aac2c15916dc@intel.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit

On 23.05.24 18:30, Dave Hansen wrote:
> On 5/16/24 06:02, Chen Yu wrote:
>> 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 set to true on bare-metal.
>> The qspinlock degenerates to test-and-set spinlock, which decrease the
>> performance on bare-metal.
>>
>> Fix this by disabling virt_spin_lock_key if CONFIG_PARAVIRT_SPINLOCKS
>> is not set, or it is on bare-metal.
> 
> This is missing some background:
> 
> 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).
> 
> ... then describe the regression and the fix
> 
>> diff --git a/arch/x86/kernel/paravirt.c b/arch/x86/kernel/paravirt.c
>> index 5358d43886ad..ee51c0949ed8 100644
>> --- a/arch/x86/kernel/paravirt.c
>> +++ b/arch/x86/kernel/paravirt.c
>> @@ -55,7 +55,7 @@ DEFINE_STATIC_KEY_TRUE(virt_spin_lock_key);
>>   
>>   void __init native_pv_lock_init(void)
>>   {
>> -	if (IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) &&
>> +	if (!IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS) ||
>>   	    !boot_cpu_has(X86_FEATURE_HYPERVISOR))
>>   		static_branch_disable(&virt_spin_lock_key);
>>   }
> This gets used at a single site:
> 
>          if (pv_enabled())
>                  goto pv_queue;
> 
>          if (virt_spin_lock(lock))
>                  return;
> 
> which is logically:
> 
> 	if (IS_ENABLED(CONFIG_PARAVIRT_SPINLOCKS))
> 		goto ...; // don't look at virt_spin_lock_key
> 
> 	if (virt_spin_lock_key)
> 		return; // On virt, but non-paravirt.  Did Test-and-Set
> 			// spinlock.
> 
> So I _think_ Arnd was trying to optimize native_pv_lock_init() away when
> it's going to get skipped over anyway by the 'goto'.
> 
> But this took me at least 30 minutes of scratching my head and trying to
> untangle the whole thing.  It's all far too subtle for my taste, and all
> of that to save a few bytes of init text in a configuration that's
> probably not even used very often (PARAVIRT=y, but PARAVIRT_SPINLOCKS=n).
> 
> Let's just keep it simple.  How about the attached patch?

Simple indeed. The attachment is empty. :-p


Juergen