watchdog_hld cleanup and async model for arm64
Subsystems (3)
v2 cover watchdog_hld cleanup and async model for arm64
hard lockup detector is helpful to diagnose unpaired irq enable/disable. Sumit has tried with a series, and the last one is V5 [1]. Since it lasts a long time without any update, I takes a retry, which addresses the delay intialization of watchdog_hld.
( To: Sumit, I think the main body of [4/4] is contributed from you,so I keep you as the author, please let me know if you dislike it and my modification.)
There is an obstacle to integrate arm64 hw perf event into watchdog_hld. When lockup_detector_init()->watchdog_nmi_probe(), on arm64, PMU is not ready until device_initcall(armv8_pmu_driver_init). And it is deeply integrated with the driver model and cpuhp. Hence it is hard to push the initialization of armv8_pmu_driver_init() before smp_init().
But it is easy to take an opposite approach by enabling watchdog_hld to get the capability of PMU async.
The async model is achieved by expanding watchdog_nmi_probe() with -EBUSY, and a re-initializing work_struct which waits on a wait_queue_head.
In this series, [1-2/4] are trivial cleanup. [3-4/4] is for this async model.
v1 > v2:
uplift the async model from hard lockup layer to watchdog layter.
The benefit is simpler code, the drawback is re-initialize means wasted alloc/free.
[1]: http://lore.kernel.org/linux-arm-kernel/[email protected]
Cc: Sumit Garg <[email protected]>
Cc: Catalin Marinas <[email protected]>
Cc: Will Deacon <[email protected]>
Cc: Ingo Molnar <[email protected]>
Cc: Arnaldo Carvalho de Melo <[email protected]>
Cc: Mark Rutland <[email protected]>
Cc: Alexander Shishkin <[email protected]>
Cc: Jiri Olsa <[email protected]>
Cc: Namhyung Kim <[email protected]>
Cc: Marc Zyngier <[email protected]>
Cc: Kees Cook <[email protected]>
Cc: Masahiro Yamada <[email protected]>
Cc: Sami Tolvanen <[email protected]>
Cc: Petr Mladek <[email protected]>
Cc: Andrew Morton <[email protected]>
Cc: Wang Qing <[email protected]>
Cc: "Peter Zijlstra (Intel)" <[email protected]>
Cc: Santosh Sivaraj <[email protected]>
To: [email protected]
To: [email protected]
*** BLURB HERE ***
Pingfan Liu (3):
kernel/watchdog: trival cleanups
kernel/watchdog_hld: clarify the condition in
hardlockup_detector_event_create()
kernel/watchdog: adapt the watchdog_hld interface for async model
Sumit Garg (1):
arm64: Enable perf events based hard lockup detector
arch/arm64/Kconfig | 2 ++ arch/arm64/kernel/Makefile | 1 + arch/arm64/kernel/perf_event.c | 11 +++++++-- arch/arm64/kernel/watchdog_hld.c | 36 +++++++++++++++++++++++++++ drivers/perf/arm_pmu.c | 5 ++++ include/linux/nmi.h | 5 +++- include/linux/perf/arm_pmu.h | 2 ++ kernel/watchdog.c | 42 +++++++++++++++++++++++++++----- kernel/watchdog_hld.c | 5 +++- 9 files changed, 99 insertions(+), 10 deletions(-) create mode 100644 arch/arm64/kernel/watchdog_hld.c
Signature · 1 line
2.31.1
PATCH v2 1/4 kernel/watchdog: trival cleanups
No reference to WATCHDOG_DEFAULT, remove it.
And nobody cares about the return value of watchdog_nmi_enable(), changing its prototype to void.
--- include/linux/nmi.h | 2 +- kernel/watchdog.c | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-)
include/linux/nmi.h +1 -1
@@ -119,7 +119,7 @@ static inline int hardlockup_detector_perf_init(void) { return 0; }void watchdog_nmi_stop (void );void watchdog_nmi_start (void );int watchdog_nmi_probe (void ); -int watchdog_nmi_enable (unsigned int cpu); +void watchdog_nmi_enable (unsigned int cpu);void watchdog_nmi_disable (unsigned int cpu);/**
kernel/watchdog.c +1 -4
@@ -30,10 +30,8 @@static DEFINE_MUTEX (watchdog_mutex);#if defined(CONFIG_HARDLOCKUP_DETECTOR ) || defined(CONFIG_HAVE_NMI_WATCHDOG ) -# define WATCHDOG_DEFAULT (SOFT_WATCHDOG_ENABLED | NMI_WATCHDOG_ENABLED)# define NMI_WATCHDOG_DEFAULT 1#else -# define WATCHDOG_DEFAULT (SOFT_WATCHDOG_ENABLED)# define NMI_WATCHDOG_DEFAULT 0#endif
@@ -95,10 +93,9 @@ __setup("nmi_watchdog=", hardlockup_panic_setup);* softlockup watchdog start and stop. The arch must select the * SOFTLOCKUP_DETECTOR Kconfig. */ -int __weak watchdog_nmi_enable (unsigned int cpu) +void __weak watchdog_nmi_enable (unsigned int cpu) {hardlockup_detector_perf_enable (); -return 0 ; }void __weak watchdog_nmi_disable (unsigned int cpu)
Signature · 1 line
2.31.1
Re: [PATCHv2 1/4] kernel/watchdog: trival cleanups
Quoted 47 lines · Pingfan Liu
No reference to
WATCHDOG_DEFAULT, remove it.And nobody cares about the return value of
watchdog_nmi_enable(), changing its prototype to void.Signed-off-by Pingfan Liu <[email protected]>Cc Petr Mladek <[email protected]>Cc Andrew Morton <[email protected]>Cc Wang Qing <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Santosh Sivaraj <[email protected]>--- include/linux/nmi.h | 2 +- kernel/watchdog.c | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-)include/linux/nmi.h +1
-1@@ -119,7 +119,7 @@ static inline int hardlockup_detector_perf_init(void) { return 0; }void watchdog_nmi_stop (void );void watchdog_nmi_start (void );int watchdog_nmi_probe (void ); -int watchdog_nmi_enable (unsigned int cpu); +void watchdog_nmi_enable (unsigned int cpu);void watchdog_nmi_disable (unsigned int cpu);/** kernel/watchdog.c +0
-2@@ -30,10 +30,8 @@static DEFINE_MUTEX (watchdog_mutex);#if defined(CONFIG_HARDLOCKUP_DETECTOR ) || defined(CONFIG_HAVE_NMI_WATCHDOG ) -# define WATCHDOG_DEFAULT (SOFT_WATCHDOG_ENABLED | NMI_WATCHDOG_ENABLED)# define NMI_WATCHDOG_DEFAULT 1#else -# define WATCHDOG_DEFAULT (SOFT_WATCHDOG_ENABLED)# define NMI_WATCHDOG_DEFAULT 0#endif
kernel/watchdog.c __setup("nmi_watchdog=", hardlockup_panic_setup); · line 96
Quoted 5 lines
kernel/watchdog.c +1
-1@@ -95,10 +93,9 @@ __setup("nmi_watchdog=", hardlockup_panic_setup);* softlockup watchdog start and stop. The arch must select the * SOFTLOCKUP_DETECTOR Kconfig. */ -int __weak watchdog_nmi_enable (unsigned int cpu) +void __weak watchdog_nmi_enable (unsigned int cpu)
It is __weak. spart specific implementation is in arch/sparc/kernel/nmi.c. It has to be updated as well.
Best Regards,
Petr
Re: [PATCHv2 1/4] kernel/watchdog: trival cleanups
Quoted 47 lines · Petr Mladek
Quoted 46 lines · Pingfan Liu
No reference to
WATCHDOG_DEFAULT, remove it.And nobody cares about the return value of
watchdog_nmi_enable(), changing its prototype to void.Signed-off-by Pingfan Liu <[email protected]>Cc Petr Mladek <[email protected]>Cc Andrew Morton <[email protected]>Cc Wang Qing <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Santosh Sivaraj <[email protected]>--- include/linux/nmi.h | 2 +- kernel/watchdog.c | 5 +---- 2 files changed, 2 insertions(+), 5 deletions(-)include/linux/nmi.h +1
-1@@ -119,7 +119,7 @@ static inline int hardlockup_detector_perf_init(void) { return 0; }void watchdog_nmi_stop (void );void watchdog_nmi_start (void );int watchdog_nmi_probe (void ); -int watchdog_nmi_enable (unsigned int cpu); +void watchdog_nmi_enable (unsigned int cpu);void watchdog_nmi_disable (unsigned int cpu);/** kernel/watchdog.c +0
-2@@ -30,10 +30,8 @@static DEFINE_MUTEX (watchdog_mutex);#if defined(CONFIG_HARDLOCKUP_DETECTOR ) || defined(CONFIG_HAVE_NMI_WATCHDOG ) -# define WATCHDOG_DEFAULT (SOFT_WATCHDOG_ENABLED | NMI_WATCHDOG_ENABLED)# define NMI_WATCHDOG_DEFAULT 1#else -# define WATCHDOG_DEFAULT (SOFT_WATCHDOG_ENABLED)# define NMI_WATCHDOG_DEFAULT 0#endif
Quoted 10 lines
kernel/watchdog.c __setup("nmi_watchdog=", hardlockup_panic_setup); · line 96
Quoted 6 lines
kernel/watchdog.c +1
-1@@ -95,10 +93,9 @@ __setup("nmi_watchdog=", hardlockup_panic_setup);* softlockup watchdog start and stop. The arch must select the * SOFTLOCKUP_DETECTOR Kconfig. */ -int __weak watchdog_nmi_enable (unsigned int cpu) +void __weak watchdog_nmi_enable (unsigned int cpu)It is
__weak. spart specific implementation is inarch/sparc/kernel/nmi.c. It has to be updated as well.
Oops, I will fix it.
Thanks,
Pingfan
PATCH v2 2/4 kernel/watchdog_hld: clarify the condition in hardlockup_detector_event_create()
In reply to Pingfan LiuAs for the context, there are two arguments to change debug_smp_processor_id() to is_percpu_thread().
-1. watchdog_ev is percpu, and migration will frustrate the attempt
which try to bind a watchdog_ev to a cpu by protecting this func inside the pair of preempt_disable()/preempt_enable().
-2. hardlockup_detector_event_create() indirectly callskmem_cache_alloc_node(), which is blockable.
So here, spelling out the really planned context "is_percpu_thread()".
--- kernel/watchdog_hld.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)
kernel/watchdog_hld.c +4 -1
@@ -165,10 +165,13 @@ static void watchdog_overflow_callback(struct perf_event *event,static int hardlockup_detector_event_create (void ) { -unsigned int cpu =smp_processor_id (); +unsigned int cpu;struct perf_event_attr *wd_attr;struct perf_event *evt; +/* This function plans to execute in cpu bound kthread */ +WARN_ON (!is_percpu_thread ()); + cpu =raw_smp_processor_id (); wd_attr = &wd_hw_attr; wd_attr->sample_period =hw_nmi_get_sample_period (watchdog_thresh);
Signature · 1 line
2.31.1
Re: [PATCHv2 2/4] kernel/watchdog_hld: clarify the condition in hardlockup_detector_event_create()
Quoted 11 lines · Pingfan Liu
As for the context, there are two arguments to change
debug_smp_processor_id()tois_percpu_thread().-1.
watchdog_evis percpu, and migration will frustrate the attempt
which try to bind awatchdog_evto a cpu by protecting this func inside the pair of preempt_disable()/preempt_enable().-2.
hardlockup_detector_event_create()indirectly callskmem_cache_alloc_node(), which is blockable.So here, spelling out the really planned context "
is_percpu_thread()".
The description is pretty hard to understand. I would suggest something like:
Subject: kernel/watchdog_hld: Ensure CPU-bound context when creating hardlockup detector event
hardlockup_detector_event_create() should create perf_event on the current CPU. Preemption could not get disabled because perf_event_create_kernel_counter() allocates memory. Instead, the CPU locality is achieved by processing the code in a per-CPU bound kthread.
Add a check to prevent mistakes when calling the code in another code path.
Quoted 17 lines
Signed-off-by Pingfan Liu <[email protected]>Cc Petr Mladek <[email protected]>Cc Andrew Morton <[email protected]>Cc Wang Qing <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Santosh Sivaraj <[email protected]>--- kernel/watchdog_hld.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)kernel/watchdog_hld.c +0
-0
kernel/watchdog_hld.c static void watchdog_overflow_callback(struct perf_event *event, · line 172
Quoted 9 lines
kernel/watchdog_hld.c +2
-1@@ -165,10 +165,13 @@ static void watchdog_overflow_callback(struct perf_event *event,static int hardlockup_detector_event_create (void ) { -unsigned int cpu =smp_processor_id (); +unsigned int cpu;struct perf_event_attr *wd_attr;struct perf_event *evt; +/* This function plans to execute in cpu bound kthread */
This does not explain why it is needed. I suggest something like:
/* * Preemption is not disabled because memory will be allocated. * Ensure CPU-locality by calling this in per-CPU kthread. */
kernel/watchdog_hld.c static void watchdog_overflow_callback(struct perf_event *event, · line 177
Quoted 5 lines
kernel/watchdog_hld.c +2
-0+WARN_ON (!is_percpu_thread ()); + cpu =raw_smp_processor_id (); wd_attr = &wd_hw_attr; wd_attr->sample_period =hw_nmi_get_sample_period (watchdog_thresh);
Othrewise the change looks good to me.
Best Regards,
Petr
Re: [PATCHv2 2/4] kernel/watchdog_hld: clarify the condition in hardlockup_detector_event_create()
Quoted 19 lines · Petr Mladek
Quoted 11 lines · Pingfan Liu
As for the context, there are two arguments to change
debug_smp_processor_id()tois_percpu_thread().-1.
watchdog_evis percpu, and migration will frustrate the attempt
which try to bind awatchdog_evto a cpu by protecting this func inside the pair of preempt_disable()/preempt_enable().-2.
hardlockup_detector_event_create()indirectly callskmem_cache_alloc_node(), which is blockable.So here, spelling out the really planned context "
is_percpu_thread()".The description is pretty hard to understand. I would suggest something like:
Subject:
kernel/watchdog_hld: Ensure CPU-bound context when creating hardlockup detector event
Quoted 9 lines
hardlockup_detector_event_create()should createperf_eventon the current CPU. Preemption could not get disabled becauseperf_event_create_kernel_counter()allocates memory. Instead, the CPU locality is achieved by processing the code in a per-CPU bound kthread.Add a check to prevent mistakes when calling the code in another code path.
Appreciate for that. I will use it.
Quoted 25 lines
Quoted 25 lines
Signed-off-by Pingfan Liu <[email protected]>Cc Petr Mladek <[email protected]>Cc Andrew Morton <[email protected]>Cc Wang Qing <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Santosh Sivaraj <[email protected]>--- kernel/watchdog_hld.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)kernel/watchdog_hld.c +1
-1@@ -165,10 +165,13 @@ static void watchdog_overflow_callback(struct perf_event *event,static int hardlockup_detector_event_create (void ) { -unsigned int cpu =smp_processor_id (); +unsigned int cpu;struct perf_event_attr *wd_attr;struct perf_event *evt;
Quoted 9 lines
kernel/watchdog_hld.c static void watchdog_overflow_callback(struct perf_event *event, · line 172
Quoted 1 line
kernel/watchdog_hld.c +1
-0+/* This function plans to execute in cpu bound kthread */ This does not explain why it is needed. I suggest something like:
/* * Preemption is not disabled because memory will be allocated. * Ensure CPU-locality by calling this in per-CPU kthread. */
It sounds good. I will use it.
Quoted 9 lines
kernel/watchdog_hld.c static void watchdog_overflow_callback(struct perf_event *event, · line 177
Quoted 6 lines
kernel/watchdog_hld.c +2
-0+WARN_ON (!is_percpu_thread ()); + cpu =raw_smp_processor_id (); wd_attr = &wd_hw_attr; wd_attr->sample_period =hw_nmi_get_sample_period (watchdog_thresh);Othrewise the change looks good to me.
Thank for your help.
Regards,
Pingfan
Quoted 5 lines
_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
PATCH v2 3/4 kernel/watchdog: adapt the watchdog_hld interface for async model
In reply to Pingfan LiuWhen lockup_detector_init()->watchdog_nmi_probe(), PMU may be not ready yet. E.g. on arm64, PMU is not ready until device_initcall(armv8_pmu_driver_init). And it is deeply integrated with the driver model and cpuhp. Hence it is hard to push this initialization before smp_init().
But it is easy to take an opposite approach by enabling watchdog_hld to get the capability of PMU async.
The async model is achieved by expanding watchdog_nmi_probe() with -EBUSY, and a re-initializing work_struct which waits on a wait_queue_head.
--- include/linux/nmi.h | 3 +++ kernel/watchdog.c | 37 +++++++++++++++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 2 deletions(-)
include/linux/nmi.h +3 -0
@@ -118,6 +118,9 @@ static inline int hardlockup_detector_perf_init(void) { return 0; }void watchdog_nmi_stop (void );void watchdog_nmi_start (void ); + +extern bool hld_detector_delay_initialized; +extern struct wait_queue_head hld_detector_wait;int watchdog_nmi_probe (void );void watchdog_nmi_enable (unsigned int cpu);void watchdog_nmi_disable (unsigned int cpu);
kernel/watchdog.c +35 -2
@@ -103,7 +103,10 @@ void __weak watchdog_nmi_disable(unsigned int cpu)hardlockup_detector_perf_disable (); } -/* Return 0, if a NMI watchdog is available. Error code otherwise */ +/* +* Return 0, if a NMI watchdog is available. -EBUSY if not ready. +* Other negative value if not support. +*/ int __weak __init watchdog_nmi_probe (void ) {return hardlockup_detector_perf_init ();
@@ -739,15 +742,45 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write, }#endif /* CONFIG_SYSCTL */ +static void lockup_detector_delay_init (struct work_struct *work); +bool hld_detector_delay_initialized__initdata ; + +struct wait_queue_head hld_detector_wait__initdata = +__WAIT_QUEUE_HEAD_INITIALIZER (hld_detector_wait); + +static struct work_struct detector_work__initdata = +__WORK_INITIALIZER (detector_work, lockup_detector_delay_init); + +static void __init lockup_detector_delay_init (struct work_struct *work) +{ +int ret; + +wait_event (hld_detector_wait, hld_detector_delay_initialized); + ret =watchdog_nmi_probe (); +if (!ret) { + nmi_watchdog_available = true; +lockup_detector_setup (); + }else { +WARN_ON (ret == -EBUSY ); +pr_info ("Perf NMI watchdog permanently disabled\n" ); + } +} +void __init lockup_detector_init (void ) { +int ret; +if (tick_nohz_full_enabled ())pr_info ("Disabling watchdog on nohz_full cores by default\n" );cpumask_copy (&watchdog_cpumask,housekeeping_cpumask (HK_FLAG_TIMER )); -if (!watchdog_nmi_probe ()) + ret =watchdog_nmi_probe (); +if (!ret) nmi_watchdog_available = true; +else if (ret == -EBUSY ) +queue_work_on (smp_processor_id (), system_wq, &detector_work); +lockup_detector_setup (); }
Signature · 1 line
2.31.1
Re: [PATCHv2 3/4] kernel/watchdog: adapt the watchdog_hld interface for async model
Quoted 80 lines · Pingfan Liu
When lockup_detector_init()->watchdog_nmi_probe(), PMU may be not ready yet. E.g. on arm64, PMU is not ready until
device_initcall(armv8_pmu_driver_init). And it is deeply integrated with the driver model and cpuhp. Hence it is hard to push this initialization beforesmp_init().But it is easy to take an opposite approach by enabling
watchdog_hldto get the capability of PMU async.The async model is achieved by expanding
watchdog_nmi_probe()with-EBUSY, and a re-initializingwork_structwhich waits on await_queue_head.Signed-off-by Pingfan Liu <[email protected]>Cc Sumit Garg <[email protected]>Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Ingo Molnar <[email protected]>Cc Arnaldo Carvalho de Melo <[email protected]>Cc Mark Rutland <[email protected]>Cc Alexander Shishkin <[email protected]>Cc Jiri Olsa <[email protected]>Cc Namhyung Kim <[email protected]>Cc Marc Zyngier <[email protected]>Cc Kees Cook <[email protected]>Cc Masahiro Yamada <[email protected]>Cc Sami Tolvanen <[email protected]>Cc Petr Mladek <[email protected]>Cc Andrew Morton <[email protected]>Cc Wang Qing <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Santosh Sivaraj <[email protected]>--- include/linux/nmi.h | 3 +++ kernel/watchdog.c | 37 +++++++++++++++++++++++++++++++++++-- 2 files changed, 38 insertions(+), 2 deletions(-)include/linux/nmi.h +3
-0@@ -118,6 +118,9 @@ static inline int hardlockup_detector_perf_init(void) { return 0; }void watchdog_nmi_stop (void );void watchdog_nmi_start (void ); + +extern bool hld_detector_delay_initialized; +extern struct wait_queue_head hld_detector_wait;int watchdog_nmi_probe (void );void watchdog_nmi_enable (unsigned int cpu);void watchdog_nmi_disable (unsigned int cpu);kernel/watchdog.c +12
-1@@ -103,7 +103,10 @@ void __weak watchdog_nmi_disable(unsigned int cpu)hardlockup_detector_perf_disable (); } -/* Return 0, if a NMI watchdog is available. Error code otherwise */ +/* +* Return 0, if a NMI watchdog is available. -EBUSY if not ready. +* Other negative value if not support. +*/ int __weak __init watchdog_nmi_probe (void ) {return hardlockup_detector_perf_init ();@@ -739,15 +742,45 @@ int proc_watchdog_cpumask(struct ctl_table *table, int write, }#endif /* CONFIG_SYSCTL */ +static void lockup_detector_delay_init (struct work_struct *work); +bool hld_detector_delay_initialized__initdata ; + +struct wait_queue_head hld_detector_wait__initdata = +__WAIT_QUEUE_HEAD_INITIALIZER (hld_detector_wait); + +static struct work_struct detector_work__initdata = +__WORK_INITIALIZER (detector_work, lockup_detector_delay_init);
kernel/watchdog.c int proc_watchdog_cpumask(struct ctl_table *table, int write, · line 762
Quoted 10 lines
kernel/watchdog.c +10
-0+ +static void __init lockup_detector_delay_init (struct work_struct *work) +{ +int ret; + +wait_event (hld_detector_wait, hld_detector_delay_initialized); + ret =watchdog_nmi_probe (); +if (!ret) { + nmi_watchdog_available = true; +lockup_detector_setup ();
Is it really safe to call the entire lockup_detector_setup() later?
It manipulates also softlockup detector. And more importantly, the original call is before smp_init(). It means that it was running when only single CPU was on.
It seems that x86 has some problem with hardlockup detector as well. It later manipulates only the hardlockup detector. Also it uses cpus_read_lock() to prevent races with CPU hotplug, see fixup_ht_bug().
Quoted 12 lines
kernel/watchdog.c +8
-0+ }else { +WARN_ON (ret == -EBUSY ); +pr_info ("Perf NMI watchdog permanently disabled\n" ); + } +} +void __init lockup_detector_init (void ) { +int ret; +if (tick_nohz_full_enabled ())pr_info ("Disabling watchdog on nohz_full cores by default\n" );
kernel/watchdog.c int proc_watchdog_cpumask(struct ctl_table *table, int write, · line 783
Quoted 10 lines
kernel/watchdog.c +4
-1cpumask_copy (&watchdog_cpumask,housekeeping_cpumask (HK_FLAG_TIMER )); -if (!watchdog_nmi_probe ()) + ret =watchdog_nmi_probe (); +if (!ret) nmi_watchdog_available = true; +else if (ret == -EBUSY ) +queue_work_on (smp_processor_id (), system_wq, &detector_work);
IMHO, this is not acceptable. It will block one worker until someone wakes it. Only arm64 will have a code to wake up the work and only when pmu is successfully initialized. In all other cases, the worker will stay blocked forever.
The right solution is to do it the other way. Queue the work from arm64-specific code when armv8_pmu_driver_init() succeeded.
Also I suggest to flush the work to make sure that it is finished before __init code gets removed.
The open question is what code the work will call. As mentioned above, I am not sure that lockup_detector_delay_init() is safe. IMHO, we need to manipulate only hardlockup detector and we have to serialize it against CPU hotplug.
Best Regards,
Petr
Re: [PATCHv2 3/4] kernel/watchdog: adapt the watchdog_hld interface for async model
On Tue, Oct 05, 2021 at 09:03:17AM +0200, Petr Mladek wrote:
[...]
Quoted 22 lines
Quoted 8 lines
kernel/watchdog.c +8
-0+static void lockup_detector_delay_init (struct work_struct *work); +bool hld_detector_delay_initialized__initdata ; + +struct wait_queue_head hld_detector_wait__initdata = +__WAIT_QUEUE_HEAD_INITIALIZER (hld_detector_wait); + +static struct work_struct detector_work__initdata = +__WORK_INITIALIZER (detector_work, lockup_detector_delay_init);kernel/watchdog.c int proc_watchdog_cpumask(struct ctl_table *table, int write, · line 762
Quoted 10 lines
kernel/watchdog.c +10
-0+ +static void __init lockup_detector_delay_init (struct work_struct *work) +{ +int ret; + +wait_event (hld_detector_wait, hld_detector_delay_initialized); + ret =watchdog_nmi_probe (); +if (!ret) { + nmi_watchdog_available = true; +lockup_detector_setup ();Is it really safe to call the entire
lockup_detector_setup()later?
Quoted 4 lines
It manipulates also softlockup detector. And more importantly, the original call is before
smp_init(). It means that it was running when only single CPU was on.
For the race analysis, lockup_detector_reconfigure() is on the centre stage. Since proc_watchdog_update() can call lockup_detector_reconfigure() to re-initialize both soft and hard lockup detector, so the race issue should be already taken into consideration.
Quoted 5 lines
It seems that x86 has some problem with hardlockup detector as well. It later manipulates only the hardlockup detector. Also it uses
cpus_read_lock()to prevent races with CPU hotplug, seefixup_ht_bug().
Yes. But hardlockup_detector_perf_{stop,start}() can not meet the requirement, since no perf_event is created yet. So there is no handy interface to re-initialize hardlockup detector directly.
Quoted 17 lines
Quoted 17 lines
kernel/watchdog.c +8
-0+ }else { +WARN_ON (ret == -EBUSY ); +pr_info ("Perf NMI watchdog permanently disabled\n" ); + } +} +void __init lockup_detector_init (void ) { +int ret; +if (tick_nohz_full_enabled ())pr_info ("Disabling watchdog on nohz_full cores by default\n" );cpumask_copy (&watchdog_cpumask,housekeeping_cpumask (HK_FLAG_TIMER ));
Quoted 12 lines
kernel/watchdog.c int proc_watchdog_cpumask(struct ctl_table *table, int write, · line 783
Quoted 6 lines
kernel/watchdog.c +4
-1-if (!watchdog_nmi_probe ()) + ret =watchdog_nmi_probe (); +if (!ret) nmi_watchdog_available = true; +else if (ret == -EBUSY ) +queue_work_on (smp_processor_id (), system_wq, &detector_work);IMHO, this is not acceptable. It will block one worker until someone wakes it. Only arm64 will have a code to wake up the work and only when pmu is successfully initialized. In all other cases, the worker will stay blocked forever.
What about consider -EBUSY and hld_detector_delay_initialized as a unit?
If watchdog_nmi_probe() returns -EBUSY, then
set the state of ld_detector_delay_initialized as "waiting", and then moved to state "finished".
And at the end of do_initcalls(), check the state is "finished". If not, then throw a warning and wake up the worker.
Quoted 3 lines
The right solution is to do it the other way. Queue the work from arm64-specific code when
armv8_pmu_driver_init()succeeded.
Could it be better if watchdog can provide a common framework for future extension instead of arch specific? The 2nd argument is to avoid the message "Perf NMI watchdog permanently disabled" while later enabling it. (Please see lockup_detector_init()->watchdog_nmi_probe()->hardlockup_detector_perf_init(), but if providing arch specific probe method, it can be avoided)
Quoted 3 lines
Also I suggest to flush the work to make sure that it is finished before
__initcode gets removed.
Good point, and very interesting. I will look into it.
Quoted 6 lines
The open question is what code the work will call. As mentioned above, I am not sure that
lockup_detector_delay_init()is safe. IMHO, we need to manipulate only hardlockup detector and we have to serialize it against CPU hotplug.
As explained ahead, it has already consider the race against CPU hotplug.
Thanks,
Pingfan
Re: [PATCHv2 3/4] kernel/watchdog: adapt the watchdog_hld interface for async model
Quoted 71 lines · Pingfan Liu
On Tue, Oct 05, 2021 at 09:03:17AM +0200, Petr Mladek wrote:
[...]Quoted 22 lines
Quoted 8 lines
kernel/watchdog.c +8
-0+static void lockup_detector_delay_init (struct work_struct *work); +bool hld_detector_delay_initialized__initdata ; + +struct wait_queue_head hld_detector_wait__initdata = +__WAIT_QUEUE_HEAD_INITIALIZER (hld_detector_wait); + +static struct work_struct detector_work__initdata = +__WORK_INITIALIZER (detector_work, lockup_detector_delay_init);kernel/watchdog.c int proc_watchdog_cpumask(struct ctl_table *table, int write, · line 762
Quoted 10 lines
kernel/watchdog.c +10
-0+ +static void __init lockup_detector_delay_init (struct work_struct *work) +{ +int ret; + +wait_event (hld_detector_wait, hld_detector_delay_initialized); + ret =watchdog_nmi_probe (); +if (!ret) { + nmi_watchdog_available = true; +lockup_detector_setup ();Is it really safe to call the entire
lockup_detector_setup()later?Quoted 4 lines
It manipulates also softlockup detector. And more importantly, the original call is before
smp_init(). It means that it was running when only single CPU was on.For the race analysis,
lockup_detector_reconfigure()is on the centre stage. Sinceproc_watchdog_update()can calllockup_detector_reconfigure()to re-initialize both soft and hard lockup detector, so the race issue should be already taken into consideration.Quoted 5 lines
It seems that x86 has some problem with hardlockup detector as well. It later manipulates only the hardlockup detector. Also it uses
cpus_read_lock()to prevent races with CPU hotplug, seefixup_ht_bug().Yes. But hardlockup_detector_perf_{stop,start}() can not meet the requirement, since no
perf_eventis created yet. So there is no handy interface to re-initialize hardlockup detector directly.Quoted 17 lines
Quoted 17 lines
kernel/watchdog.c +8
-0+ }else { +WARN_ON (ret == -EBUSY ); +pr_info ("Perf NMI watchdog permanently disabled\n" ); + } +} +void __init lockup_detector_init (void ) { +int ret; +if (tick_nohz_full_enabled ())pr_info ("Disabling watchdog on nohz_full cores by default\n" );cpumask_copy (&watchdog_cpumask,housekeeping_cpumask (HK_FLAG_TIMER ));Quoted 12 lines
kernel/watchdog.c int proc_watchdog_cpumask(struct ctl_table *table, int write, · line 783
Quoted 6 lines
kernel/watchdog.c +4
-1-if (!watchdog_nmi_probe ()) + ret =watchdog_nmi_probe (); +if (!ret) nmi_watchdog_available = true; +else if (ret == -EBUSY ) +queue_work_on (smp_processor_id (), system_wq, &detector_work);IMHO, this is not acceptable. It will block one worker until someone wakes it. Only arm64 will have a code to wake up the work and only when pmu is successfully initialized. In all other cases, the worker will stay blocked forever.
Quoted 1 line
What about consider
-EBUSYandhld_detector_delay_initializedas a unit?
^^^ unity
Quoted 9 lines
If
watchdog_nmi_probe()returns-EBUSY, then
set the state ofld_detector_delay_initializedas "waiting", and then moved to state "finished".And at the end of
do_initcalls(), check the state is "finished". If not, then throw a warning and wake up the worker.Quoted 3 lines
The right solution is to do it the other way. Queue the work from arm64-specific code when
armv8_pmu_driver_init()succeeded.
Quoted 7 lines
Could it be better if watchdog can provide a common framework for future extension instead of arch specific? The 2nd argument is to avoid the message "Perf NMI watchdog permanently disabled" while later enabling it. (Please see lockup_detector_init()->watchdog_nmi_probe()->hardlockup_detector_perf_init(), but if providing arch specific probe method, it can be avoided)
Sorry for poor expression. I have not explained it completely for the second point.
Since using arch specific watchdog_nmi_probe() to avoid misleading message "Perf NMI watchdog permanently disabled", then -EBUSY should be returned. And from watchdog level, it should know how to handle error, that is to say queue_work_on(smp_processor_id(), system_wq, &detector_work).
Thanks,
Pingfan
Quoted 18 lines
Quoted 3 lines
Also I suggest to flush the work to make sure that it is finished before
__initcode gets removed.Good point, and very interesting. I will look into it.
Quoted 6 lines
The open question is what code the work will call. As mentioned above, I am not sure that
lockup_detector_delay_init()is safe. IMHO, we need to manipulate only hardlockup detector and we have to serialize it against CPU hotplug.As explained ahead, it has already consider the race against CPU hotplug.
Thanks,
Pingfan
PATCH v2 4/4 arm64: Enable perf events based hard lockup detector
In reply to Pingfan LiuFrom: Sumit Garg <[email protected]>
With the recent feature added to enable perf events to use pseudo NMIs as interrupts on platforms which support GICv3 or later, its now been possible to enable hard lockup detector (or NMI watchdog) on arm64 platforms. So enable corresponding support.
One thing to note here is that normally lockup detector is initialized just after the early initcalls but PMU on arm64 comes up much later as device_initcall(). So we need to re-initialize lockup detection once PMU has been initialized.
[1]: http://lore.kernel.org/linux-arm-kernel/[email protected]
(Pingfan: adapt it to watchdog_hld async model based on [1])
--- arch/arm64/Kconfig | 2 ++ arch/arm64/kernel/Makefile | 1 + arch/arm64/kernel/perf_event.c | 11 ++++++++-- arch/arm64/kernel/watchdog_hld.c | 36 ++++++++++++++++++++++++++++++++ drivers/perf/arm_pmu.c | 5 +++++ include/linux/perf/arm_pmu.h | 2 ++ 6 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 arch/arm64/kernel/watchdog_hld.c
arch/arm64/Kconfig +2 -0
@@ -189,6 +189,8 @@ config ARM64 select HAVE_NMI select HAVE_PATA_PLATFORM select HAVE_PERF_EVENTS + select HAVE_PERF_EVENTS_NMI if ARM64_PSEUDO_NMI + select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI select HAVE_PERF_REGS select HAVE_PERF_USER_STACK_DUMP select HAVE_REGS_AND_STACK_ACCESS_API
arch/arm64/kernel/Makefile +1 -0
@@ -46,6 +46,7 @@ obj-$(CONFIG_MODULES) += module.o obj-$(CONFIG_ARM64_MODULE_PLTS) += module-plts.o obj-$(CONFIG_PERF_EVENTS) += perf_regs.o perf_callchain.o obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o +obj-$(CONFIG_HARDLOCKUP_DETECTOR_PERF) += watchdog_hld.o obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o obj-$(CONFIG_CPU_PM) += sleep.o suspend.o obj-$(CONFIG_CPU_IDLE) += cpuidle.o
arch/arm64/kernel/perf_event.c +9 -2
@@ -23,6 +23,7 @@#include <linux/platform_device.h> #include <linux/sched_clock.h> #include <linux/smp.h> +#include <linux/nmi.h> /* ARMv8 Cortex-A53 specific event types. */ #define ARMV8_A53_PERFCTR_PREF_LINEFILL 0xC2
@@ -1284,10 +1285,16 @@ static struct platform_driver armv8_pmu_driver = {static int __init armv8_pmu_driver_init (void ) { +int ret; +if (acpi_disabled) -return platform_driver_register (&armv8_pmu_driver); + ret =platform_driver_register (&armv8_pmu_driver);else -return arm_pmu_acpi_probe (armv8_pmuv3_init); + ret =arm_pmu_acpi_probe (armv8_pmuv3_init); + + hld_detector_delay_initialized = true; +wake_up (&hld_detector_wait); +return ret; }device_initcall (armv8_pmu_driver_init)
arch/arm64/kernel/watchdog_hld.c new file +36 -0
@@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/nmi.h> +#include <linux/cpufreq.h> +#include <linux/perf/arm_pmu.h> + +/* +* Safe maximum CPU frequency in case a particular platform doesn't implement +* cpufreq driver. Although, architecture doesn't put any restrictions on +* maximum frequency but 5 GHz seems to be safe maximum given the available +* Arm CPUs in the market which are clocked much less than 5 GHz. On the other +* hand, we can't make it much higher as it would lead to a large hard-lockup +* detection timeout on parts which are running slower (eg. 1GHz on +* Developerbox) and doesn't possess a cpufreq driver. +*/ +#define SAFE_MAX_CPU_FREQ 5000000000UL // 5 GHz +u64 hw_nmi_get_sample_period (int watchdog_thresh) +{ +unsigned int cpu =smp_processor_id (); +unsigned long max_cpu_freq; + + max_cpu_freq =cpufreq_get_hw_max_freq (cpu) *1000UL ; +if (!max_cpu_freq) + max_cpu_freq =SAFE_MAX_CPU_FREQ ; + +return (u64 )max_cpu_freq * watchdog_thresh; +} + +int __init watchdog_nmi_probe (void ) +{ +if (!hld_detector_delay_initialized) +return -EBUSY ; +else if (!arm_pmu_irq_is_nmi ()) +return -ENODEV ; + +return hardlockup_detector_perf_init (); +}
drivers/perf/arm_pmu.c +5 -0
@@ -697,6 +697,11 @@ static int armpmu_get_cpu_irq(struct arm_pmu *pmu, int cpu)return per_cpu (hw_events->irq, cpu); } +bool arm_pmu_irq_is_nmi (void ) +{ +return has_nmi; +} +/* * PMU hardware loses all context when a CPU goes offline. * When a CPU is hotplugged back in, since some hardware registers are
include/linux/perf/arm_pmu.h +2 -0
@@ -163,6 +163,8 @@ int arm_pmu_acpi_probe(armpmu_init_fn init_fn);static inline int arm_pmu_acpi_probe (armpmu_init_fn init_fn) {return 0 ; }#endif +bool arm_pmu_irq_is_nmi (void ); +/* Internal functions only for core arm_pmu code */ struct arm_pmu *armpmu_alloc (void );struct arm_pmu *armpmu_alloc_atomic (void );
Signature · 1 line
2.31.1
Re: [PATCHv2 4/4] arm64: Enable perf events based hard lockup detector
Quoted 3 lines · Pingfan Liu
From: Sumit Garg <[email protected]>
To Sumit, I think credits should go to you and keep you as the author.
Please let me know if you dislike it.
Thanks,
Pingfan
Quoted 174 lines
With the recent feature added to enable perf events to use pseudo NMIs as interrupts on platforms which support GICv3 or later, its now been possible to enable hard lockup detector (or NMI watchdog) on arm64 platforms. So enable corresponding support.
One thing to note here is that normally lockup detector is initialized just after the early initcalls but PMU on arm64 comes up much later as
device_initcall(). So we need to re-initialize lockup detection once PMU has been initialized.[1]: http://lore.kernel.org/linux-arm-kernel/[email protected]
Signed-off-by Sumit Garg <[email protected]>(Pingfan: adapt it to
watchdog_hldasync model based on [1])Signed-off-by Pingfan Liu <[email protected]>Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Ingo Molnar <[email protected]>Cc Arnaldo Carvalho de Melo <[email protected]>Cc Mark Rutland <[email protected]>Cc Alexander Shishkin <[email protected]>Cc Jiri Olsa <[email protected]>Cc Namhyung Kim <[email protected]>Cc Marc Zyngier <[email protected]>Cc Kees Cook <[email protected]>Cc Masahiro Yamada <[email protected]>Cc Sami Tolvanen <[email protected]>Cc Petr Mladek <[email protected]>Cc Andrew Morton <[email protected]>Cc Wang Qing <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Santosh Sivaraj <[email protected]>--- arch/arm64/Kconfig | 2 ++ arch/arm64/kernel/Makefile | 1 + arch/arm64/kernel/perf_event.c | 11 ++++++++-- arch/arm64/kernel/watchdog_hld.c | 36 ++++++++++++++++++++++++++++++++ drivers/perf/arm_pmu.c | 5 +++++ include/linux/perf/arm_pmu.h | 2 ++ 6 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 arch/arm64/kernel/watchdog_hld.carch/arm64/Kconfig +2
-0@@ -189,6 +189,8 @@ config ARM64 select HAVE_NMI select HAVE_PATA_PLATFORM select HAVE_PERF_EVENTS + select HAVE_PERF_EVENTS_NMI if ARM64_PSEUDO_NMI + select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI select HAVE_PERF_REGS select HAVE_PERF_USER_STACK_DUMP select HAVE_REGS_AND_STACK_ACCESS_APIarch/arm64/kernel/Makefile +1
-0@@ -46,6 +46,7 @@ obj-$(CONFIG_MODULES) += module.o obj-$(CONFIG_ARM64_MODULE_PLTS) += module-plts.o obj-$(CONFIG_PERF_EVENTS) += perf_regs.o perf_callchain.o obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o +obj-$(CONFIG_HARDLOCKUP_DETECTOR_PERF) += watchdog_hld.o obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o obj-$(CONFIG_CPU_PM) += sleep.o suspend.o obj-$(CONFIG_CPU_IDLE) += cpuidle.oarch/arm64/kernel/perf_event.c +9
-2@@ -23,6 +23,7 @@#include <linux/platform_device.h> #include <linux/sched_clock.h> #include <linux/smp.h> +#include <linux/nmi.h> /* ARMv8 Cortex-A53 specific event types. */ #define ARMV8_A53_PERFCTR_PREF_LINEFILL 0xC2@@ -1284,10 +1285,16 @@ static struct platform_driver armv8_pmu_driver = {static int __init armv8_pmu_driver_init (void ) { +int ret; +if (acpi_disabled) -return platform_driver_register (&armv8_pmu_driver); + ret =platform_driver_register (&armv8_pmu_driver);else -return arm_pmu_acpi_probe (armv8_pmuv3_init); + ret =arm_pmu_acpi_probe (armv8_pmuv3_init); + + hld_detector_delay_initialized = true; +wake_up (&hld_detector_wait); +return ret; }device_initcall (armv8_pmu_driver_init)arch/arm64/kernel/watchdog_hld.c new file +36
-0@@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/nmi.h> +#include <linux/cpufreq.h> +#include <linux/perf/arm_pmu.h> + +/* +* Safe maximum CPU frequency in case a particular platform doesn't implement +* cpufreq driver. Although, architecture doesn't put any restrictions on +* maximum frequency but 5 GHz seems to be safe maximum given the available +* Arm CPUs in the market which are clocked much less than 5 GHz. On the other +* hand, we can't make it much higher as it would lead to a large hard-lockup +* detection timeout on parts which are running slower (eg. 1GHz on +* Developerbox) and doesn't possess a cpufreq driver. +*/ +#define SAFE_MAX_CPU_FREQ 5000000000UL // 5 GHz +u64 hw_nmi_get_sample_period (int watchdog_thresh) +{ +unsigned int cpu =smp_processor_id (); +unsigned long max_cpu_freq; + + max_cpu_freq =cpufreq_get_hw_max_freq (cpu) *1000UL ; +if (!max_cpu_freq) + max_cpu_freq =SAFE_MAX_CPU_FREQ ; + +return (u64 )max_cpu_freq * watchdog_thresh; +} + +int __init watchdog_nmi_probe (void ) +{ +if (!hld_detector_delay_initialized) +return -EBUSY ; +else if (!arm_pmu_irq_is_nmi ()) +return -ENODEV ; + +return hardlockup_detector_perf_init (); +}drivers/perf/arm_pmu.c +5
-0@@ -697,6 +697,11 @@ static int armpmu_get_cpu_irq(struct arm_pmu *pmu, int cpu)return per_cpu (hw_events->irq, cpu); } +bool arm_pmu_irq_is_nmi (void ) +{ +return has_nmi; +} +/* * PMU hardware loses all context when a CPU goes offline. * When a CPU is hotplugged back in, since some hardware registers are include/linux/perf/arm_pmu.h +2
-1@@ -163,6 +163,8 @@ int arm_pmu_acpi_probe(armpmu_init_fn init_fn);static inline int arm_pmu_acpi_probe (armpmu_init_fn init_fn) {return 0 ; }#endif +bool arm_pmu_irq_is_nmi (void ); +/* Internal functions only for core arm_pmu code */ struct arm_pmu *armpmu_alloc (void );struct arm_pmu *armpmu_alloc_atomic (void ); --2.31.1
Re: [PATCHv2 4/4] arm64: Enable perf events based hard lockup detector
Hi Pingfan,
Quoted 7 lines · Pingfan Liu
Quoted 3 lines · Pingfan Liu
From: Sumit Garg <[email protected]>
To Sumit, I think credits should go to you and keep you as the author.
Thanks, I am fine with it. If you like then you can add your "Co-developed-by" as well.
Quoted 14 lines
Please let me know if you dislike it.
Thanks,
Pingfan
Quoted 9 lines
With the recent feature added to enable perf events to use pseudo NMIs as interrupts on platforms which support GICv3 or later, its now been possible to enable hard lockup detector (or NMI watchdog) on arm64 platforms. So enable corresponding support.
One thing to note here is that normally lockup detector is initialized just after the early initcalls but PMU on arm64 comes up much later as
device_initcall(). So we need to re-initialize lockup detection once PMU has been initialized.
This needs to be updated to reflect delayed initialization instead.
-Sumit
Quoted 165 lines
Quoted 165 lines
[1]: http://lore.kernel.org/linux-arm-kernel/[email protected]
Signed-off-by Sumit Garg <[email protected]>(Pingfan: adapt it to
watchdog_hldasync model based on [1])Signed-off-by Pingfan Liu <[email protected]>Cc Catalin Marinas <[email protected]>Cc Will Deacon <[email protected]>Cc Ingo Molnar <[email protected]>Cc Arnaldo Carvalho de Melo <[email protected]>Cc Mark Rutland <[email protected]>Cc Alexander Shishkin <[email protected]>Cc Jiri Olsa <[email protected]>Cc Namhyung Kim <[email protected]>Cc Marc Zyngier <[email protected]>Cc Kees Cook <[email protected]>Cc Masahiro Yamada <[email protected]>Cc Sami Tolvanen <[email protected]>Cc Petr Mladek <[email protected]>Cc Andrew Morton <[email protected]>Cc Wang Qing <[email protected]>Cc "Peter Zijlstra (Intel)" <[email protected]>Cc Santosh Sivaraj <[email protected]>--- arch/arm64/Kconfig | 2 ++ arch/arm64/kernel/Makefile | 1 + arch/arm64/kernel/perf_event.c | 11 ++++++++-- arch/arm64/kernel/watchdog_hld.c | 36 ++++++++++++++++++++++++++++++++ drivers/perf/arm_pmu.c | 5 +++++ include/linux/perf/arm_pmu.h | 2 ++ 6 files changed, 55 insertions(+), 2 deletions(-) create mode 100644 arch/arm64/kernel/watchdog_hld.carch/arm64/Kconfig +2
-0@@ -189,6 +189,8 @@ config ARM64 select HAVE_NMI select HAVE_PATA_PLATFORM select HAVE_PERF_EVENTS + select HAVE_PERF_EVENTS_NMI if ARM64_PSEUDO_NMI + select HAVE_HARDLOCKUP_DETECTOR_PERF if PERF_EVENTS && HAVE_PERF_EVENTS_NMI select HAVE_PERF_REGS select HAVE_PERF_USER_STACK_DUMP select HAVE_REGS_AND_STACK_ACCESS_APIarch/arm64/kernel/Makefile +1
-0@@ -46,6 +46,7 @@ obj-$(CONFIG_MODULES) += module.o obj-$(CONFIG_ARM64_MODULE_PLTS) += module-plts.o obj-$(CONFIG_PERF_EVENTS) += perf_regs.o perf_callchain.o obj-$(CONFIG_HW_PERF_EVENTS) += perf_event.o +obj-$(CONFIG_HARDLOCKUP_DETECTOR_PERF) += watchdog_hld.o obj-$(CONFIG_HAVE_HW_BREAKPOINT) += hw_breakpoint.o obj-$(CONFIG_CPU_PM) += sleep.o suspend.o obj-$(CONFIG_CPU_IDLE) += cpuidle.oarch/arm64/kernel/perf_event.c +9
-2@@ -23,6 +23,7 @@#include <linux/platform_device.h> #include <linux/sched_clock.h> #include <linux/smp.h> +#include <linux/nmi.h> /* ARMv8 Cortex-A53 specific event types. */ #define ARMV8_A53_PERFCTR_PREF_LINEFILL 0xC2@@ -1284,10 +1285,16 @@ static struct platform_driver armv8_pmu_driver = {static int __init armv8_pmu_driver_init (void ) { +int ret; +if (acpi_disabled) -return platform_driver_register (&armv8_pmu_driver); + ret =platform_driver_register (&armv8_pmu_driver);else -return arm_pmu_acpi_probe (armv8_pmuv3_init); + ret =arm_pmu_acpi_probe (armv8_pmuv3_init); + + hld_detector_delay_initialized = true; +wake_up (&hld_detector_wait); +return ret; }device_initcall (armv8_pmu_driver_init)arch/arm64/kernel/watchdog_hld.c new file +36
-0@@ -0,0 +1,36 @@ +// SPDX-License-Identifier: GPL-2.0 +#include <linux/nmi.h> +#include <linux/cpufreq.h> +#include <linux/perf/arm_pmu.h> + +/* +* Safe maximum CPU frequency in case a particular platform doesn't implement +* cpufreq driver. Although, architecture doesn't put any restrictions on +* maximum frequency but 5 GHz seems to be safe maximum given the available +* Arm CPUs in the market which are clocked much less than 5 GHz. On the other +* hand, we can't make it much higher as it would lead to a large hard-lockup +* detection timeout on parts which are running slower (eg. 1GHz on +* Developerbox) and doesn't possess a cpufreq driver. +*/ +#define SAFE_MAX_CPU_FREQ 5000000000UL // 5 GHz +u64 hw_nmi_get_sample_period (int watchdog_thresh) +{ +unsigned int cpu =smp_processor_id (); +unsigned long max_cpu_freq; + + max_cpu_freq =cpufreq_get_hw_max_freq (cpu) *1000UL ; +if (!max_cpu_freq) + max_cpu_freq =SAFE_MAX_CPU_FREQ ; + +return (u64 )max_cpu_freq * watchdog_thresh; +} + +int __init watchdog_nmi_probe (void ) +{ +if (!hld_detector_delay_initialized) +return -EBUSY ; +else if (!arm_pmu_irq_is_nmi ()) +return -ENODEV ; + +return hardlockup_detector_perf_init (); +}drivers/perf/arm_pmu.c +5
-0@@ -697,6 +697,11 @@ static int armpmu_get_cpu_irq(struct arm_pmu *pmu, int cpu)return per_cpu (hw_events->irq, cpu); } +bool arm_pmu_irq_is_nmi (void ) +{ +return has_nmi; +} +/* * PMU hardware loses all context when a CPU goes offline. * When a CPU is hotplugged back in, since some hardware registers are include/linux/perf/arm_pmu.h +2
-1@@ -163,6 +163,8 @@ int arm_pmu_acpi_probe(armpmu_init_fn init_fn);static inline int arm_pmu_acpi_probe (armpmu_init_fn init_fn) {return 0 ; }#endif +bool arm_pmu_irq_is_nmi (void ); +/* Internal functions only for core arm_pmu code */ struct arm_pmu *armpmu_alloc (void );struct arm_pmu *armpmu_alloc_atomic (void ); --2.31.1
Re: [PATCHv2 4/4] arm64: Enable perf events based hard lockup detector
Quoted 7 lines · Sumit Garg
Hi Pingfan,
Quoted 3 lines · Pingfan Liu
Quoted 1 line · Pingfan Liu
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
From: Sumit Garg <[email protected]>
To Sumit, I think credits should go to you and keep you as the author.
Thanks, I am fine with it. If you like then you can add your "Co-developed-by" as well.
OK, I will.
Quoted 7 lines
Quoted 4 lines
Quoted 4 lines
One thing to note here is that normally lockup detector is initialized just after the early initcalls but PMU on arm64 comes up much later as
device_initcall(). So we need to re-initialize lockup detection once PMU has been initialized.This needs to be updated to reflect delayed initialization instead.
I will update the commit log accordingly.
Thanks,
Pingfan