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;
Date:   Thu, 20 Oct 2022 12:44:30 +0800
From:   Feng Tang <feng.tang@intel.com>
To:     Thomas Gleixner <tglx@linutronix.de>
CC:     Ingo Molnar <mingo@redhat.com>, Borislav Petkov <bp@alien8.de>,
        "Dave Hansen" <dave.hansen@intel.com>,
        "H . Peter Anvin" <hpa@zytor.com>,
        "Peter Zijlstra" <peterz@infradead.org>, <x86@kernel.org>,
        <linux-kernel@vger.kernel.org>, <rui.zhang@intel.com>,
        <tim.c.chen@intel.com>, Xiongfeng Wang <wangxiongfeng2@huawei.com>,
        Yu Liao <liaoyu15@huawei.com>
Subject: Re: [PATCH v2] x86/tsc: Extend watchdog check exemption to 4-Sockets
 platform
Message-ID: <Y1DSLnfMakCrWux/@feng-clx>
References: <20221013131200.973649-1-feng.tang@intel.com>
 <87tu40p3ws.ffs@tglx>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <87tu40p3ws.ffs@tglx>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?0ROAUykrCr6kPrPw+3r+IYz8mnhZgXLnxbsEjPCZKaO5MfFF+zhDdBlDdk6z?=
 =?us-ascii?Q?GfLy6BCjEy17j2yw7TpNAPRYuU8tsmpNweLug5YYvLJbzHXkXGnWeVu10Nfh?=
 =?us-ascii?Q?HviyVHustRa+IO1I3Rr3r00cq30JSn151VVzsh8Rm224eGxhdlpDF25laaMn?=
 =?us-ascii?Q?WWKTG6YFRSnUj+aFh/3XgvnqWWmOcl6Uee7o0GNUpG4KhHXumPkon8/aROLv?=
 =?us-ascii?Q?Zro15LQ9n4w2JXPOp35vS7lLWNo+9kQUdlq2CVYga4UW2snVoI+ZK31JhE0U?=
 =?us-ascii?Q?8gqw8lEqi53UpsJcebYxGDk/JJu43NehV1OEYuPuJlrMZqzyfc9UXNohb1Xa?=
 =?us-ascii?Q?Bu68AZXyu1MWXT0AMY7bOTY0nLMTBUB8c0ZdP5ovY25jRKGiNqlBdwXh8R2Z?=
 =?us-ascii?Q?rT3W6CZw2B6tql6zGt+53Hkrf5i1Q8pTYUGFwsMxCPBeWvo4FCH3jeqLJj3E?=
 =?us-ascii?Q?y92D4NVcC53OMTW136R8JAcQYAcJL+25QWF+hbFRB8twKIDb0lvHh64w90fg?=
 =?us-ascii?Q?KSw0+AYOCrvLlnzUzlN+jYiU2143UhYxMChI6bUUV7dzlETifW0jL0IbKY6b?=
 =?us-ascii?Q?2U3ORFeseCD8KyZ87BrCvGo3dA4dWez67iZn5NwggJxLYwXvLiptD+2wdUAm?=
 =?us-ascii?Q?a704psG2tD9KqBkCGs1UXIEakUcNr26aw4EUMTLjmQigUXc8rs4GT0xyNvVA?=
 =?us-ascii?Q?eQ9nZOa2CCdEyoA7+vyQPLt1bTgEJckSK4aaNyWmqyG/EqAbeB46ASBcx8D9?=
 =?us-ascii?Q?hmK99uF7FxYzbpYJrSVWft4sLk6DJ3yO2rOopI7MvALUy8W1XhUxFmVPuTon?=
 =?us-ascii?Q?4mwTqZhgSOFkvp9Q2m2EkB4hgSbve7W7MIKDzOF9rJVqVDGeCZJ+u4cdma0p?=
 =?us-ascii?Q?IGa3/KVWE4XloV2tkI9M2q+Dltvo5KxvmG2SxySxmXlbH13yhYvyJ5PVYuSA?=
 =?us-ascii?Q?waX5w9fQNx8Ql/WpRTOVSWFNDa2WyVXsZfZiSesdT0VCk9p/E8wOiQpRiFHU?=
 =?us-ascii?Q?Es5M7c0OkXv6/Y0xfQ+qytKv1zE/2GKXxAI0pJloyLJQTJ0CXYYret73n9mr?=
 =?us-ascii?Q?mdkfxevqj6H0N90tBmf2AZbKPxVHcBdg/h9L0p0Bbo12vGhJDJeRNC6gVT8y?=
 =?us-ascii?Q?x9/lPqigskiMHF4Kc51haGNNXGFIoSSJSHnDB9iEMMHsNBV0gbpFu69RRxku?=
 =?us-ascii?Q?81h+CuXoyjEHeCRchTQ7RFL0kT811VpMac6AY9siDZMZ4kp0SMqctcT/1arl?=
 =?us-ascii?Q?4UKNRRI5OddsK2PoQt/oKUYPcjAqLvLRfY+xE/8H0cddf8txN5nPzJ91mhGb?=
 =?us-ascii?Q?xMzsI/bqzF0OEW/+vI++OMQPfOnVL/f4PnIzxNWl3wigucMUSa06oZuMZ8I+?=
 =?us-ascii?Q?ygyMsUuqMqkxTrNNSg+lclcbqLH5rydZcNnPeOSw2zDhcD7Nx+IliWY09EC1?=
 =?us-ascii?Q?h02f10SQQjgbEgd3GLh8VErtaYLrqGvr5Jj6cfHLd21qnUCK362TMKPHlwe5?=
 =?us-ascii?Q?b5ZUhT+KYukMrT1n1h0gJ1MgQgwrnAXAH8W39Ee4lwHLy9RE+1hC9zHY9tRV?=
 =?us-ascii?Q?lCM/fLG2H2Mv4XSi5Y2ow36lOKjvUAHBh0+LN9ux?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 0b24b7c4-e894-4207-dcf8-08dab255ceb0
X-MS-Exchange-CrossTenant-AuthSource: MN0PR11MB6304.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Oct 2022 04:44:43.8820
 (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: kIf/SZu79Y36kUAfRHDivY32LZheQLynCQjj37Aa0IimPeDRX0tzxF/5ISSESrIrnD65zfjlzvzaPWd+QW+0kg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: PH7PR11MB7002
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-7.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_HI,
        SPF_HELO_NONE,SPF_NONE,URIBL_BLOCKED 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

On Wed, Oct 19, 2022 at 11:18:43AM +0200, Thomas Gleixner wrote:
> On Thu, Oct 13 2022 at 21:12, Feng Tang wrote:
> > There is report again that the tsc clocksource on a 4 sockets x86
> > Skylake server was wrongly judged as 'unstable' by 'jiffies' watchdog,
> > and disabled [1].
> >
> > Commit b50db7095fe0 ("x86/tsc: Disable clocksource watchdog for TSC
> > on qualified platorms") was introduce to deal with these false
> > alarms of tsc unstable issues, covering qualified platforms for 2
> > sockets or smaller ones.
> >
> > Extend the exemption to 4 sockets to fix the issue.
> >
> > We also got similar reports on 8 sockets platform from internal test,
> > but as Peter pointed out, there was tsc sync issues for 8-sockets
> > platform, and it'd better be handled architecture by architecture,
> > instead of directly changing the threshold to 8 here.
> >
> > Rui also proposed another way to disable 'jiffies' as clocksource
> > watchdog [2], which can also solve this specific problem in an
> > architecture independent way, with one limitation that some tsc false
> > alarms are reported by other watchdogs like HPET in post-boot time,
> > while 'jiffies' is mostly used in boot phase before hardware
> > clocksources are initialized.
> 
> HPET is initialized early, but if HPET is disabled or not advertised
> then the only other hardware clocksource is PMTIMER which is initialized
> late via fs_initcall. PMTIMER is initialized late due to broken Pentium
> era chipsets which are sorted with PCI quirks. For anything else we can
> initialize it early. Something like the below.

Thanks for sharing the background and the code! It can reduce the
time of 'jiffies' being a watchdog on client platforms whose HPET
are disabled. And there were still false positive reports for
HPET/PMTIMER as watchdogs, so I still vote to your suggestion of
lifting the check for qualified platforms.

For that, Dave raised the accuracy issue of 'nr_online_nodes' and
we proposed new patch in https://lore.kernel.org/lkml/20221017132942.1646934-1-feng.tang@intel.com/
while the topology_max_packages() still has issue as providing socket
number, and I plan to use 'logical_packages' instead. Do you think
it's in the right direction?

> I'm sure I said this more than once, but I'm happy to repeat myself
> forever:
> 
>   Instead of proliferating lousy hacks, can the X86 vendors finaly get
>   their act together and provide some architected information whether
>   the TSC is trustworthy or not?
 
Yes it will save us a lot of trouble. Maybe better in CPUID info, as
if there is some bug in HW/BIOS, it may get fixed with microcode update.

Thanks,
Feng

> Thanks,
> 
>         tglx
> ---
> 
> --- a/arch/x86/kernel/time.c
> +++ b/arch/x86/kernel/time.c
> @@ -10,6 +10,7 @@
>   *
>   */
>  
> +#include <linux/acpi_pmtmr.h>
>  #include <linux/clocksource.h>
>  #include <linux/clockchips.h>
>  #include <linux/interrupt.h>
> @@ -75,6 +76,14 @@ static void __init setup_default_timer_i
>  void __init hpet_time_init(void)
>  {
>  	if (!hpet_enable()) {
> +		/*
> +		 * Some Pentium chipsets have broken HPETs and need
> +		 * PCI quirks to run before init.
> +		 */
> +		if (boot_cpu_data.x86_vendor != X86_VENDOR_INTEL ||
> +		    boot_cpu_data.family != 5)
> +			init_acpi_pm_clocksource();
> +
>  		if (!pit_timer_init())
>  			return;
>  	}
> --- a/drivers/clocksource/acpi_pm.c
> +++ b/drivers/clocksource/acpi_pm.c
> @@ -30,6 +30,7 @@
>   * in arch/i386/kernel/acpi/boot.c
>   */
>  u32 pmtmr_ioport __read_mostly;
> +static bool pmtmr_initialized __init_data;
>  
>  static inline u32 read_pmtmr(void)
>  {
> @@ -142,7 +143,7 @@ DECLARE_PCI_FIXUP_EARLY(PCI_VENDOR_ID_SE
>   * Some boards have the PMTMR running way too fast. We check
>   * the PMTMR rate against PIT channel 2 to catch these cases.
>   */
> -static int verify_pmtmr_rate(void)
> +static int __init verify_pmtmr_rate(void)
>  {
>  	u64 value1, value2;
>  	unsigned long count, delta;
> @@ -172,14 +173,18 @@ static int verify_pmtmr_rate(void)
>  /* Number of reads we try to get two different values */
>  #define ACPI_PM_READ_CHECKS 10000
>  
> -static int __init init_acpi_pm_clocksource(void)
> +int __init init_acpi_pm_clocksource(void)
>  {
>  	u64 value1, value2;
>  	unsigned int i, j = 0;
> +	int ret;
>  
>  	if (!pmtmr_ioport)
>  		return -ENODEV;
>  
> +	if (pmtmr_initialized)
> +		return 0;
> +
>  	/* "verify" this timing source: */
>  	for (j = 0; j < ACPI_PM_MONOTONICITY_CHECKS; j++) {
>  		udelay(100 * j);
> @@ -210,10 +215,11 @@ static int __init init_acpi_pm_clocksour
>  		return -ENODEV;
>  	}
>  
> -	return clocksource_register_hz(&clocksource_acpi_pm,
> -						PMTMR_TICKS_PER_SEC);
> +	ret = clocksource_register_hz(&clocksource_acpi_pm, PMTMR_TICKS_PER_SEC);
> +	if (!ret)
> +		pmtimer_initialized = true;
> +	return ret;
>  }
> -
>  /* We use fs_initcall because we want the PCI fixups to have run
>   * but we still need to load before device_initcall
>   */
> --- a/include/linux/acpi_pmtmr.h
> +++ b/include/linux/acpi_pmtmr.h
> @@ -13,6 +13,8 @@
>  /* Overrun value */
>  #define ACPI_PM_OVRRUN	(1<<24)
>  
> +extern int __init init_acpi_pm_clocksource(void);
> +
>  #ifdef CONFIG_X86_PM_TIMER
>  
>  extern u32 acpi_pm_read_verified(void);
> 
>