Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <775d8042-639a-1ae6-c099-9411e6027f40@amd.com>
Date:   Sat, 24 Sep 2022 02:05:55 +0530
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:91.0) Gecko/20100101
 Thunderbird/91.13.0
Subject: Re: [PATCH] ACPI: processor idle: Practically limit "Dummy wait"
 workaround to old Intel systems
Content-Language: en-US
To:     Dave Hansen <dave.hansen@intel.com>, linux-kernel@vger.kernel.org
Cc:     Dave Hansen <dave.hansen@linux.intel.com>,
        Len Brown <lenb@kernel.org>,
        Mario Limonciello <Mario.Limonciello@amd.com>,
        Peter Zijlstra <peterz@infradead.org>,
        Borislav Petkov <bp@alien8.de>,
        "Rafael J . Wysocki" <rafael.j.wysocki@intel.com>
References: <20220922184745.3252932-1-dave.hansen@intel.com>
From:   K Prateek Nayak <kprateek.nayak@amd.com>
In-Reply-To: <20220922184745.3252932-1-dave.hansen@intel.com>
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?blV3V29tdVR3TWVWT2UxRjNLbTJzcEJRR3hneXE3SmI1NzhYeUN5MzEwL0xG?=
 =?utf-8?B?QnE4N1RxWXpoQjB3M3VRbHViU1I4Y25WK1pxY0NrVUthcFdELzNNVW4rQ3lH?=
 =?utf-8?B?Sk15NjhIMWxBOFpBZXVXMTA2YW5zRHE2WEN5SHVqazcwSUlEYlVUc2IrTG9x?=
 =?utf-8?B?aEVwOG5lc0Y5OTVJT1U4QnBtOXpTWlNGbXVBMDNmV3gza1JVbmpxcjhiMldM?=
 =?utf-8?B?VmZNbWhKRW1TdHp5U2lBUEhyTkg1R2Rteks1LzErRE1venNVNisvZ2hnbFRt?=
 =?utf-8?B?dVFXUHdDdXVSMnVwdnhwTjZuMnZ3cnVTdnM3ZitUYlhwNFNBeGRQS0ZISGp5?=
 =?utf-8?B?UE8veGNxYnJGRWpjYlMvYnUxMkJCSUxmWStQbTE3VytCUDNTcUpiUmluL2RD?=
 =?utf-8?B?Zjk2MHpxekJTa2NibDhtUXUrK2ttSjJoSTNtMWFqTEFCbVh5djE4ODE1UWY2?=
 =?utf-8?B?dENreUNXeE5aQXFnOS9IcmtIc0Z2RVBlUjZtMSt6T2ZnUnIzL25NU2RoUFZQ?=
 =?utf-8?B?VWtEZ001dTFoUXJJNzlkRWpnVlFKU1RZeDlwa29adkRUM2ZsSXF3cmU5SUdu?=
 =?utf-8?B?QWlZeER6MGZNTHZyLzkvOUV6N2p2MGtkRzVkZWtYbE9XbVRaV29KL0RCTXIw?=
 =?utf-8?B?Ykk1ckpqZFpTSEUybFNRUCtwVE9paTQyTW41OW00STRUenJrUnRrZmd1TFNv?=
 =?utf-8?B?RHZDSXBscnpQS3doODhad2xtcDR3VEREZ3ZQS0JQMVpHRGpZMWYyQWtaL3Jk?=
 =?utf-8?B?dVdMM0huYVBqUDFKN3FmaENhendUdUxUcTBENjBoODc0c0NVYWZKdEVoMUNW?=
 =?utf-8?B?QTBOZ2hxMTl5WHNEY3cxajhRemo0ME9mQmN2SHE1TGZEblB3ZXZUR01kRG1I?=
 =?utf-8?B?S1BHNlFSWW9zd0tBYmhYeGY3b2FhRFlMOThEWFIxRm50TE1BUkptaUd6QjhK?=
 =?utf-8?B?OXdJSEE4QUhRWFZ2L3c3R2tibVNNYktXUFIzZmthaG0yZXoxN2JucWZDOG94?=
 =?utf-8?B?SzlaSmFpZm8rc3d4WlZGd2ZVV1NsUWlXYWx3VWhUcEhKTlVROTd0SnBmZTI3?=
 =?utf-8?B?VkJwK1ZaU2l1UFliNWtsOGd1WHJIejJaNkxzNlp1VEY0TnhmSkRYdHBoakhi?=
 =?utf-8?B?UHFYL2dTUTFYd1dCWjJWcFlTQUlySHB1UklwYUU2Qk5yYlZPZ2hEYzQrQ0Qw?=
 =?utf-8?B?b29zVlZMTFAxUXl6RVlhajZLM1ZvMDlHSFJQcWUrdTgvL0F4T1lGcmU2NW8z?=
 =?utf-8?B?eUcvNEhHMlJRSG5tNHM4SDJtLy90ZSt0M1k1M1dBKzhhL2plVjB0dUh4KzNT?=
 =?utf-8?B?cUsvMlozb1pxUGlHa1NWZDkvUW5LRnB6UWxvbllMU0NsWWFLanNSUzJnUmJ6?=
 =?utf-8?B?Z1BSRWgxb3NXMXV3ZjY5YXNUUDhERU11cGdFT1BzcDQxdm1kckVZL1dYQW1C?=
 =?utf-8?B?Y3MxRGpnaXRlQng5SU9PcGM5TmdyQ3NYSGRGL1dNampNT2s3ZnNIMmFlaTIy?=
 =?utf-8?B?UmF3WEc1SGtFSEZrU1NyYjYrNGZEbHNOWDhjaUl5Zkw4L1h0aG1ETDF0U2R4?=
 =?utf-8?B?eWJCbWhQNG53dFU3RTNsSm0rUEkwOW1oWkM1R1NPV1RIMGN0L0M3aG4xZ1My?=
 =?utf-8?B?a1ZiQmFuQ0tpaGUvN2FNcmdqcnM5c21xdTlYNGh5MDk1ZmR1bkpyOUduL1Mx?=
 =?utf-8?B?aWY4aEdYaE4vMHNEcU9jWS8xTVVLem9xQUhZZjlCRXV2bmJnUk1NTTFWMEl4?=
 =?utf-8?B?WjJTdkpEcjBtL2k4RUxORmFGZ3UvU1hEb3VhdEoxWXF0VXNCUGhNWGt4Yngx?=
 =?utf-8?B?UEE0d1NUZnpVNGtGWW9JOTdyc2s3ZnVyRUJSZ0dBSC9NN2ZwbkFjOEZhQ0xT?=
 =?utf-8?B?OVd3SXpkWndaMWdkTmxzSXBwU1E2My9nbkhwS2FwUmZMb3NyNFduSFdMT0ZC?=
 =?utf-8?B?WVh4M1pTOHNWcVQxc0xvWVpwZDNLREwwTFBOQjRlNkczZmNvOGZQVTFxNzNE?=
 =?utf-8?B?bHRSSmx6TWlvSTlsUWdkZmVCbENybjlnRzdjSUFqNStZVVIxWkd5RFljcVFj?=
 =?utf-8?B?T0EyeFNuQklYTkU5ZU9mZVlad0pBOGlJQm1uWFZGb1oyelo5NW4wVGhnQVdk?=
 =?utf-8?Q?Tzw+L+lhdQ0sd78Le3rwiixw+?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: a9abd4cb-05a7-4d9f-3024-08da9da33e74
X-MS-Exchange-CrossTenant-AuthSource: SN1PR12MB2381.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 23 Sep 2022 20:36:08.3440
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: QHH5MkCe5nHA401YZsIwtUHX7ydcva86wf5su7muHONnzAav021TyJzTiEMchAB0huVij2e6JLbuETZBaDCTsg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CY5PR12MB6431
X-Spam-Status: No, score=-2.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,NICE_REPLY_A,SPF_HELO_PASS,
        SPF_PASS autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hello Dave,

On 9/23/2022 12:17 AM, Dave Hansen wrote:
> Old, circa 2002 chipsets have a bug: they don't go idle when they are
> supposed to.  So, a workaround was added to slow the CPU down and
> ensure that the CPU waits a bit for the chipset to actually go idle.
> This workaround is ancient and has been in place in some form since
> the original kernel ACPI implementation.
> 
> But, this workaround is very painful on modern systems.  The "inl()"
> can take thousands of cycles (see Link: for some more detailed
> numbers and some fun kernel archaeology).
> 
> First and foremost, modern systems should not be using this code.
> Typical Intel systems have not used it in over a decade because it is
> horribly inferior to MWAIT-based idle.
> 
> Despite this, people do seem to be tripping over this workaround on
> AMD system today.
> 
> Limit the "dummy wait" workaround to Intel systems.  Keep Modern AMD
> systems from tripping over the workaround.  Remotely modern Intel
> systems use intel_idle instead of this code and will, in practice,
> remain unaffected by the dummy wait.

I've run 30 runs of tbench with 128 clients on a dual socket Zen3 system
(2 x 64C/128T) and do not see any massive regression like I used to when
we were hitting the dummy wait issue:

Kernel        : baseline      baseline + C2 disabled   baseline + this patch

Min (MB/s)    : 2215.06       33072.10 (+1393.05%)     30519.60 (+1277.82%)
Max (MB/s)    : 32938.80      34399.10                 32699.30
Median (MB/s) : 32191.80      33476.60                 31418.90
AMean (MB/s)  : 22448.55      33649.27 (+49.89%)       31545.93 (+40.52%)
AMean Stddev  : 17526.70      680.14                   1095.39
AMean CoefVar : 78.07%        2.02%                    3.47%

The range is well within the variation we've normally seen with tbench
on the test platform.

> 
> Signed-off-by: Dave Hansen <dave.hansen@linux.intel.com>
> Cc: Len Brown <lenb@kernel.org>
> Cc: Mario Limonciello <Mario.Limonciello@amd.com>
> Cc: Peter Zijlstra (Intel) <peterz@infradead.org>
> Cc: Borislav Petkov <bp@alien8.de>

Can you please add a cc to stable?

Cc: stable@vger.kernel.org

> Suggested-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> Reported-by: K Prateek Nayak <kprateek.nayak@amd.com>
> Link: https://lore.kernel.org/all/20220921063638.2489-1-kprateek.nayak@amd.com/
> ---
>  drivers/acpi/processor_idle.c | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/acpi/processor_idle.c b/drivers/acpi/processor_idle.c
> index 16a1663d02d4..9f40917c49ef 100644
> --- a/drivers/acpi/processor_idle.c
> +++ b/drivers/acpi/processor_idle.c
> @@ -531,10 +531,27 @@ static void wait_for_freeze(void)
>  	/* No delay is needed if we are in guest */
>  	if (boot_cpu_has(X86_FEATURE_HYPERVISOR))
>  		return;
> +	/*
> +	 * Modern (>=Nehalem) Intel systems use ACPI via intel_idle,
> +	 * not this code.  Assume that any Intel systems using this
> +	 * are ancient and may need the dummy wait.  This also assumes
> +	 * that the motivating chipset issue was Intel-only.
> +	 */
> +	if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL)

Based on Andreas's comment, this problem is not limited to Intel chipsets
and affects at least the AMD Athlon on VIA chipset (circa 2006)
(https://lore.kernel.org/lkml/Yyy6l94G0O2B7Yh1@rhlx01.hs-esslingen.de/)
To be on safer side, the exception could be made for AMD Fam 17h+ and also
Hygon as pointed out by Peter, where we know the dummy wait is unnecessary.
Extending the condition you proposed, we can have:

	if (boot_cpu_data.x86_vendor == X86_VENDOR_HYGON ||
	    ((boot_cpu_data.x86_vendor == X86_VENDOR_AMD) &&
	     (boot_cpu_data.x86_model >= 0x17)))
		return;

It is not pretty by any means which is why we can use a x86_BUG_STPCLK to
limit the dummy op to only affected processors. This way, the x86 vendor
check and family check can be avoided in the acpi code. A v2 has been sent
out tackling the problem this way:
https://lore.kernel.org/lkml/20220923153801.9167-1-kprateek.nayak@amd.com/

> +		return;
>  #endif
> -	/* Dummy wait op - must do something useless after P_LVL2 read
> -	   because chipsets cannot guarantee that STPCLK# signal
> -	   gets asserted in time to freeze execution properly. */
> +	/*
> +	 * Dummy wait op - must do something useless after P_LVL2 read
> +	 * because chipsets cannot guarantee that STPCLK# signal gets
> +	 * asserted in time to freeze execution properly
> +	 *
> +	 * This workaround has been in place since the original ACPI
> +	 * implementation was merged, circa 2002.
> +	 *
> +	 * If a profile is pointing to this instruction, please first
> +	 * consider moving your system to a more modern idle
> +	 * mechanism.
> +	 */
>  	inl(acpi_gbl_FADT.xpm_timer_block.address);
>  }
>  

The patch, as it is, solves the problem we've seen on the newer AMD
platforms with large core density that use IOPORT based C-states.

Tested-by: K Prateek Nayak <kprateek.nayak@amd.com>
--
Thanks and Regards,
Prateek