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;
From:   Song Liu <songliubraving@meta.com>
To:     Yonghong Song <yhs@meta.com>
CC:     Song Liu <song@kernel.org>, LKML <linux-kernel@vger.kernel.org>,
        Kernel Team <kernel-team@meta.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Peter Zijlstra <peterz@infradead.org>
Subject: Re: [PATCH v4] watchdog: Allow nmi watchdog to use "ref-cycles" event
Thread-Topic: [PATCH v4] watchdog: Allow nmi watchdog to use "ref-cycles"
 event
Thread-Index: AQHZiR9WEaR3xoFFfEGCLaxGjWAWL69fljCAgADhp4A=
Date:   Thu, 18 May 2023 20:12:27 +0000
Message-ID: <E206E9A5-7B13-42BB-A78A-CF329DB667CC@fb.com>
References: <20230518002555.1114189-1-song@kernel.org>
 <437cc3c8-73e0-dbc2-2998-39310e1597e9@meta.com>
In-Reply-To: <437cc3c8-73e0-dbc2-2998-39310e1597e9@meta.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?VOpfP18c3HW2zvg8k9F+zGfY3UXWRYrCbS80lGYCAL/P6J115D2VIvgtljcV?=
 =?us-ascii?Q?1bL+jZ76bfpPm+h6fQev7K4ye7pn9NWqWyzJN7F44jIFxU3/Ddr4WZYLYQi1?=
 =?us-ascii?Q?2lQ6kr+ptU+UATmFYbOxyw5WR1Xukd205HRZHSj2c+TrLFk6L8/xJGLril49?=
 =?us-ascii?Q?HHhOvVJqIWNPBNfr900Q3LbGYEkgkwEJwHSCrjPR53GQZcQ3c7isizyceqmE?=
 =?us-ascii?Q?LYwXQElx4JaYNtNL7BIwQ2PnjiCokWqC8djEwRUf/XKOc1ooCBPYRK4Sv6S9?=
 =?us-ascii?Q?6hf3SGSnCNe356d+L7phOn+g3Stpzz+wUBtzbdP0UsvHrX5mkqrOJXVItBZu?=
 =?us-ascii?Q?y6N4I6htPJhC5wgTOxwrNncKC1vpDsgNdhVY9IAN6M3fCNsjtuKOSGoAtisp?=
 =?us-ascii?Q?XRJan5I5xe/wsqI1ynxDOSnPiGamRGaQuVemOx3n9IwndJbqerO038+nHY/X?=
 =?us-ascii?Q?eoq9JERvDovu6Gicu7igFFvT0RliRqt8DWX6Jpi7FZokaXLN5hxteT6Zq+gE?=
 =?us-ascii?Q?rjEZDFfk1ySz8ColbZq+JID5PrUpQQG/0GG8qEkDa4wiIbiUnqh6JP8h+/Lg?=
 =?us-ascii?Q?SDSvh4qrURe26X2Z/KL893QpEaDMXogPaT3PrnCf5Mixh9k5nFeIFWaJ3Uii?=
 =?us-ascii?Q?FBvoKWOaVYN0WnJVZ6EmdxlytHFMpIgGLWgf+MoYamiAHJqCy5RCWx5ztH1Y?=
 =?us-ascii?Q?VB1dzdCC3RVHS1bsMNWxwHKLE9MKksNq3SXv5HgZV53bcoagvm8t2WzIgXii?=
 =?us-ascii?Q?DZg0ozZyRjJAKW+7bu5+guaaEziLFYt6vwO8Dr4l4076FjToeuD1m2bwu6M+?=
 =?us-ascii?Q?QpHEUuD1NCsV2KXQ6M+kr285ikkAqOgoVoduUICnhHNU2JfAf1bSpylD5Y12?=
 =?us-ascii?Q?neZ50nZv+lh18+6hOCFyYiEJH7YjJJz3evt2j7u6GPFGAJ+BYH9sX+7iZ/L+?=
 =?us-ascii?Q?X3mWgVdS9qtP1I2DKjvb4I4BVcYJ8y5MRRIpGnXio/YMjhorbLOCnPa6ToGx?=
 =?us-ascii?Q?SEieiCAR0OgXDGHbfjwYBt/xSkajV70jlRxmz0FNFfGIslopZdDbLiG+gwAP?=
 =?us-ascii?Q?HTsr2krfT7yvW2nexc/+ht//6Ly2iwqTdLlHhQLma8PqPO5uWVctcybDvl3v?=
 =?us-ascii?Q?+t9uKCGd+Seu4B/JXssXdgbO3t39eJKJ1EO0+73oLrKtrw2lnWbZLtxWitrZ?=
 =?us-ascii?Q?vWys3JOKTODxLsfRMQQ9BMpojMFd7Z8Tn2KuT4CIvNP5wzfDnePJP16G6UmB?=
 =?us-ascii?Q?Jp6ET0Ud6AvIH9FJ+9as+DJ75HS+GbOfn0DSv57s0/xGP8qFWnTSCSvzUsPF?=
 =?us-ascii?Q?iEQu90irlvGemp9TcQLgrg+CM5Wyy7CO8evLln4n8el2k110TMObICZ7i0Kp?=
 =?us-ascii?Q?J/b72jvmihKE+zpV73lxIsHGgtDitwACVCLnkKgWqQMU45JqXsoXBKvqpN4U?=
 =?us-ascii?Q?MxHAtYVmjfnWHCTQ/m8nTTTPP1sMOrxLs/HwVLRMsnEE1X8XWumJlRl3t6ws?=
 =?us-ascii?Q?5+tlYzfMX0Xn6eiNRBDG6Cw6pUOt84VjnRpDkVV+AkeHLmdm63kYrIdzDvpw?=
 =?us-ascii?Q?w++20cHDvHMvcsY6Iq/oAEKcMd294M0hFETNyNGZxJtEIfCRAMijgBUEormw?=
 =?us-ascii?Q?Lw0XT911ybesXBi6CEkKhJk=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-ID: <9A548A6EA520BA45A72CB6FDA3D9CC9A@namprd15.prod.outlook.com>
MIME-Version: 1.0
X-OriginatorOrg: meta.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SA1PR15MB5109.namprd15.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: ad9c89c0-fec6-4fdc-f3ab-08db57dc3403
X-MS-Exchange-CrossTenant-originalarrivaltime: 18 May 2023 20:12:27.9900
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 8ae927fe-1255-47a7-a2af-5f3a069daaa2
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 1TAViswlvQBn9ArMJloyzQ5NqXW0F8Jb/FE5fNl0zNUibIzy2O8GHXd/qjh9dz70I89cXPI+TBM3xdme2u1knQ==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA1PR15MB4723
X-Proofpoint-GUID: Tlyqw_ssMnyjpjGP2n5VP7QTVbf7Z_uz
X-Proofpoint-ORIG-GUID: Tlyqw_ssMnyjpjGP2n5VP7QTVbf7Z_uz
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.254,Aquarius:18.0.957,Hydra:6.0.573,FMLib:17.11.170.22
 definitions=2023-05-18_15,2023-05-17_02,2023-02-09_01
X-Spam-Status: No, score=-2.8 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_LOW,
        RCVD_IN_MSPIKE_H3,RCVD_IN_MSPIKE_WL,SPF_HELO_NONE,SPF_PASS,
        T_SCC_BODY_TEXT_LINE 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 May 17, 2023, at 11:44 PM, Yonghong Song <yhs@meta.com> wrote:
> 
> 
> 
> On 5/17/23 5:25 PM, Song Liu wrote:
>> NMI watchdog permanently consumes one hardware counters per CPU on the
>> system. For systems that use many hardware counters, this causes more
>> aggressive time multiplexing of perf events.
>> OTOH, some CPUs (mostly Intel) support "ref-cycles" event, which is rarely
>> used. Add kernel cmdline arg nmi_watchdog=ref-cycles to configure the
>> watchdog to use "ref-cycles" event instead of "cycles".
> 
> Maybe list some example how this new option will used?

In most case, all we need is to add "nmi_watchdog=refcycles" to kernel args.

> 
>> Cc: Andrew Morton <akpm@linux-foundation.org>
>> Cc: Peter Zijlstra <peterz@infradead.org>
>> Signed-off-by: Song Liu <song@kernel.org>
>> ---
>> Changes in v4:
>> Fix compile error for !CONFIG_HARDLOCKUP_DETECTOR_PERF. (kernel test bot)
>> Changes in v3:
>> Pivot the design to use kernel arg nmi_watchdog=ref-cycles (Peter)
>> ---
>>  Documentation/admin-guide/kernel-parameters.txt | 5 +++--
>>  include/linux/nmi.h                             | 2 ++
>>  kernel/watchdog.c                               | 2 ++
>>  kernel/watchdog_hld.c                           | 9 +++++++++
>>  4 files changed, 16 insertions(+), 2 deletions(-)
>> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
>> index 9e5bab29685f..d378e23dad7c 100644
>> --- a/Documentation/admin-guide/kernel-parameters.txt
>> +++ b/Documentation/admin-guide/kernel-parameters.txt
>> @@ -3593,10 +3593,12 @@
>>   Format: [state][,regs][,debounce][,die]
>>     nmi_watchdog= [KNL,BUGS=X86] Debugging features for SMP kernels
>> - Format: [panic,][nopanic,][num]
>> + Format: [panic,][nopanic,][ref-cycles][num]
>>   Valid num: 0 or 1
>>   0 - turn hardlockup detector in nmi_watchdog off
>>   1 - turn hardlockup detector in nmi_watchdog on
>> + ref-cycles - configure the watchdog with perf event
>> +              "ref-cycles" instead of "cycles"
>>   When panic is specified, panic when an NMI watchdog
>>   timeout occurs (or 'nopanic' to not panic on an NMI
>>   watchdog, if CONFIG_BOOTPARAM_HARDLOCKUP_PANIC is set)
>> @@ -7097,4 +7099,3 @@
>>   memory, and other data can't be written using
>>   xmon commands.
>>   off xmon is disabled.
>> -
>> diff --git a/include/linux/nmi.h b/include/linux/nmi.h
>> index 048c0b9aa623..edfd1bcce0f6 100644
>> --- a/include/linux/nmi.h
>> +++ b/include/linux/nmi.h
>> @@ -102,12 +102,14 @@ extern void hardlockup_detector_perf_disable(void);
>>  extern void hardlockup_detector_perf_enable(void);
>>  extern void hardlockup_detector_perf_cleanup(void);
>>  extern int hardlockup_detector_perf_init(void);
>> +extern void hardlockup_config_perf_event(const char *str);
>>  #else
>>  static inline void hardlockup_detector_perf_stop(void) { }
>>  static inline void hardlockup_detector_perf_restart(void) { }
>>  static inline void hardlockup_detector_perf_disable(void) { }
>>  static inline void hardlockup_detector_perf_enable(void) { }
>>  static inline void hardlockup_detector_perf_cleanup(void) { }
>> +static inline void hardlockup_config_perf_event(const char *str) { }
>>  # if !defined(CONFIG_HAVE_NMI_WATCHDOG)
>>  static inline int hardlockup_detector_perf_init(void) { return -ENODEV; }
>>  static inline void arch_touch_nmi_watchdog(void) {}
>> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
>> index 8e61f21e7e33..fed4f0be8e1a 100644
>> --- a/kernel/watchdog.c
>> +++ b/kernel/watchdog.c
>> @@ -81,6 +81,8 @@ static int __init hardlockup_panic_setup(char *str)
>>   nmi_watchdog_user_enabled = 0;
>>   else if (!strncmp(str, "1", 1))
>>   nmi_watchdog_user_enabled = 1;
>> + else if (!strncmp(str, "ref-cycles", 10))
> 
> str vs. 'ref-cycles' is tested here.
> 
>> + hardlockup_config_perf_event(str);
>>   return 1;
>>  }
>>  __setup("nmi_watchdog=", hardlockup_panic_setup);
>> diff --git a/kernel/watchdog_hld.c b/kernel/watchdog_hld.c
>> index 247bf0b1582c..4deca58ba6ed 100644
>> --- a/kernel/watchdog_hld.c
>> +++ b/kernel/watchdog_hld.c
>> @@ -294,3 +294,12 @@ int __init hardlockup_detector_perf_init(void)
>>   }
>>   return ret;
>>  }
>> +
>> +/**
>> + * hardlockup_config_perf_event - Overwrite config of wd_hw_attr
>> + */
>> +void __init hardlockup_config_perf_event(const char *str)
>> +{
>> + if (!strncmp(str, "ref-cycles", 10))
> 
> It is unnecessarily tested again here.

In that case, we can probably rename the function as 
hardlockup_use_ref_cycles(). I am fine with either way. 

Thanks,
Song