Received-SPF: pass (google.com: domain of linux-kernel+bounces-189289-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Date: Sat, 25 May 2024 14:03:26 +0800
From: Chen Yu <yu.c.chen@intel.com>
To: Dave Hansen <dave.hansen@intel.com>, Juergen Gross <jgross@suse.com>
CC: Thomas Gleixner <tglx@linutronix.de>, Ingo Molnar <mingo@redhat.com>,
	Borislav Petkov <bp@alien8.de>, Dave Hansen <dave.hansen@linux.intel.com>,
	Juergen Gross <jgross@suse.com>, 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>
Subject: Re: [PATCH] x86/paravirt: Disable virt spinlock when
 CONFIG_PARAVIRT_SPINLOCKS disabled
Message-ID: <ZlF/Lrr4nWqVpoc8@chenyu5-mobl2>
References: <20240516130244.893573-1-yu.c.chen@intel.com>
 <7b8d1dd6-3913-45fe-941e-aac2c15916dc@intel.com>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <7b8d1dd6-3913-45fe-941e-aac2c15916dc@intel.com>
Precedence: bulk
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?wCUWavelJYKTcaY7cRl/4i3/ZOAFmghDy7bzWOheZ1gJt6xR38RhXIxdPTgo?=
 =?us-ascii?Q?wqYM6/1ivUEnQPltBPi1WmwAxvytVbuF1KOUh41rzKKjKT8zcHnKpA5cGhnS?=
 =?us-ascii?Q?UnsQtIyExDgeys9QWMgYrlggUUwwA5QgXf0h1keuVDhyUtIB4t7vK7XCmeAX?=
 =?us-ascii?Q?NI/AOzjK30eUON+flsnbGa5ugWaRpfOXvKqn1N3jj5wLjn5KqypLt2b9CzGt?=
 =?us-ascii?Q?a6I/hx/0VqAc7/Fp4YCSe/Xtz/Mv50DO5jXsvFGkyNYoq1krLCo+ziGQ3aT1?=
 =?us-ascii?Q?PSl+T0CahP93P+rFDQRkq1vXxHy3a4KLhn2zar34GDZcr0gOQQMZJFqTFz3g?=
 =?us-ascii?Q?7PORdbPHIHmizz/nogkc1z5EdnDePsgnqqHsJyaCATpuF15qFCoPvE6XGoDA?=
 =?us-ascii?Q?sE50bzgYkcNi3VjiSVa2jiVPEq/iePc8uKTgNa0HdXGHRinLVzRuP3NLnP4u?=
 =?us-ascii?Q?bzUNXiJcpqXlgGLAKKScvdFcHu2Mkfa1nAaijMEKQVketTpQB9Gn/vQSGTZ/?=
 =?us-ascii?Q?67SEidLMG9BUi6j7eIGdRdV5eVWVwD/ruJQJKJEBipGSb28dWYakAZbH8It6?=
 =?us-ascii?Q?TuvfysjhsJfGXcODkGfp4X2W+v0caiqoJlZBTm1v7VTrXH3mb021HcwJyWWi?=
 =?us-ascii?Q?6eAhYi5DRVd7AWb2+P9AJ0Umv4XYtGeLbAKsPXM3kbpPaNKK3e9t9e5wkVNY?=
 =?us-ascii?Q?RJ7Wg0haLfCmkNCU3gabYFJX987/VVxB4OZhUKyBvCaa1Z+vsIdU5q67/VUc?=
 =?us-ascii?Q?GBlHgxANi0ae2K34oV3robdUKoMJoRZCPUcwICCv6h9On53+aZk9qMXU+Ad+?=
 =?us-ascii?Q?8CQzd6NT5OOkeBdLi2Ag4bHsvF9d37y/jj5W0lpznye1nQjFCHwSToooDIJ7?=
 =?us-ascii?Q?vytqlb3HzNPI48hsbmPDxQSyf/ZodhlRD9puBM6OpWFgM/CxSSr9kAftAwdb?=
 =?us-ascii?Q?29h+dwxXtcXAnHQZG8ENK6de95rPtGrupvUOHmnSbHNHPUX0P9Z+cXPUJjwd?=
 =?us-ascii?Q?kNTyoM7r+PYVDkuXIITCkjCWuvVJwC02j83IdS8feJYBsCwQ+KCBGoHL7cHY?=
 =?us-ascii?Q?++/E44D2wWSKBCHJuvYJUk4rCYQREr1aoZ0cxDqGBzxnyVsW5FRe4hQHhKqt?=
 =?us-ascii?Q?CMmQFc7+xc9xvIXDElPAzLUsreEqoPWX3/6hxniYdDGUrZCvPOgyAw7UknK3?=
 =?us-ascii?Q?YEWz0gAzeul2x36IZ1Q0P0f8rZCNF1Fz1Z/hEdzEfAEHUv+m/g8+IhO65/mv?=
 =?us-ascii?Q?Xd8jyR5u76R0OsVtwI3Pbj9jvCkgJ3ZLnwlMqL8onrZ0hSXappXdANnV+DQA?=
 =?us-ascii?Q?1QX6501SjM3Z5mH8+TI9DgoqBPMVHbNvWRGKi71VnV8rWTf1ZxrwmHuYhb6L?=
 =?us-ascii?Q?QKiYGDlfhrZ/aPMfYNHTKGneetudw0liI0FR7MjPLn+5HSO00FzkZFT6t2iI?=
 =?us-ascii?Q?s8sEwq5QhuktXQw6P9kbLg2onrkNvTAw2rk0Frml0hNhFzski/vBsSsGXCls?=
 =?us-ascii?Q?1S9dlswFyRc1AvzcrlSoHslDxEagyFFx8nn/lczCuF8SebpBmbFtyK9wxVsf?=
 =?us-ascii?Q?EF0F7lcnhPjfvQ2F+FLi/Xtm5q/6CTQdD5Mc7GtQ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0a7620c4-0448-4a1f-5274-08dc7c806f02
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB6020.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 25 May 2024 06:03:44.3681
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: ddwh/sKDpPvTULLGWzsQlgyHD6AfdIRdE59i286oSRnS9NO93oaoYnYfrsmLPBFdY8s47VCob7LmqjE+vF3XYw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: MW5PR11MB5931
X-OriginatorOrg: intel.com

On 2024-05-23 at 09:30:59 -0700, 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
>
Thanks Juergen for your review.

And thanks Dave for the write up, I'll refine the log according to your suggestion. 

> > 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.
>

Thanks for the description in detail, my original change might break the
"X86_FEATURE_HYPERVISOR + NO_CONFIG_PARAVIRT_SPINLOCKS " case that, the guest
can not fall into test-and-set.
 
> 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?

Yes, this one works, I'll refine it.

thanks,
Chenyu