Skip to contentSkip to conversation

watchdog_hld cleanup and async model for arm64

PATCH Superseded Pingfan Liu 5 participants 21 messages Sep 15, 2021 – Sep 22, 2021

Subsystems (3)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore
#0 lore raw

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.

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 introducing an extra parameter notifier of watchdog_nmi_probe().

In this series, [1-2/5] are trivial cleanup. [3-5/5] is for this async model.

[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: Julien Thierry <[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]

Pingfan Liu (5):
kernel/watchdog: remove useless WATCHDOG_DEFAULT kernel/watchdog_hld: clarify the condition in
hardlockup_detector_event_create()
kernel/watchdog: adapt the watchdog_hld interface for async model kernel/watchdog_hld: simplify the detecting of hld watchdog arm64/watchdog_hld: enable hard lockup on arm64 platform

 arch/arm64/Kconfig                  |  3 ++
 arch/arm64/include/asm/perf_event.h |  5 ++
 arch/arm64/kernel/Makefile          |  1 +
 arch/arm64/kernel/perf_event.c      | 14 ++++-
 arch/arm64/kernel/watchdog_hld.c    | 83 +++++++++++++++++++++++++++++
 drivers/perf/arm_pmu.c              |  5 ++
 include/linux/nmi.h                 | 20 ++++---
 kernel/watchdog.c                   | 69 ++++++++++++++++++------
 kernel/watchdog_hld.c               | 26 +++------
 9 files changed, 181 insertions(+), 45 deletions(-)
 create mode 100644 arch/arm64/kernel/watchdog_hld.c
Signature · 1 line
2.31.1
#1 lore raw

PATCH 1/5 kernel/watchdog: remove useless WATCHDOG_DEFAULT

Since no reference to WATCHDOG_DEFAULT, remove it.

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]

---
 kernel/watchdog.c | 2 --
 1 file changed, 2 deletions(-)
kernel/watchdog.c +0 -2
index ad912511a0c0..e2a9e3331416 100644
@@ -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
 
Signature · 1 line
2.31.1
#2 lore raw

PATCH 2/5 kernel/watchdog_hld: clarify the condition in hardlockup_detector_event_create()

In reply to Pingfan Liu

hardlockup_detector_event_create() indirectly calls kmem_cache_alloc_node(), which is blockable.

So here, the really planned context is is_percpu_thread().

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]>
Cc Sumit Garg <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>

To: [email protected]

---
 kernel/watchdog_hld.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
kernel/watchdog_hld.c +4 -1
index 247bf0b1582c..6876e796dbf5 100644
@@ -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 */
+	BUG_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
#3 lore raw

Re: [PATCH 2/5] kernel/watchdog_hld: clarify the condition in hardlockup_detector_event_create()

Quoted 10 lines · On Wed, 15 Sep 2021 11:51:00 +0800 Pingfan Liu <[email protected]> wrote:

hardlockup_detector_event_create() indirectly calls kmem_cache_alloc_node(), which is blockable.

So here, the really planned context is is_percpu_thread().

...

kernel/watchdog_hld.c static void watchdog_overflow_callback(struct perf_event *event, · line 173

Quoted 10 lines
kernel/watchdog_hld.c +3 -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 */
+	BUG_ON(!is_percpu_thread());

Can we avoid adding the BUG()? Find a way to emit a WARNing and then permit the kernel to continue?

Quoted 3 lines
kernel/watchdog_hld.c +1 -0
+	cpu = raw_smp_processor_id();
 	wd_attr = &wd_hw_attr;
 	wd_attr->sample_period = hw_nmi_get_sample_period(watchdog_thresh);
#4 lore raw

Re: [PATCH 2/5] kernel/watchdog_hld: clarify the condition in hardlockup_detector_event_create()

Quoted 20 lines · Andrew Morton
Quoted 18 lines · On Wed, 15 Sep 2021 11:51:00 +0800 Pingfan Liu <[email protected]> wrote:

hardlockup_detector_event_create() indirectly calls kmem_cache_alloc_node(), which is blockable.

So here, the really planned context is is_percpu_thread().

...

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 6 lines

kernel/watchdog_hld.c static void watchdog_overflow_callback(struct perf_event *event, · line 173

Quoted 2 lines
kernel/watchdog_hld.c +2 -0
+	/* This function plans to execute in cpu bound kthread */
+	BUG_ON(!is_percpu_thread());

Can we avoid adding the BUG()? Find a way to emit a WARNing and then permit the kernel to continue?

Yes, WARN_ON() can work in this case.

Thanks,

Pingfan

maintainer
#5 lore raw

Re: [PATCH 2/5] kernel/watchdog_hld: clarify the condition in hardlockup_detector_event_create()

In reply to Pingfan Liu
Quoted 27 lines · Pingfan Liu

hardlockup_detector_event_create() indirectly calls kmem_cache_alloc_node(), which is blockable.

So here, the really planned context is is_percpu_thread().

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]>
Cc Sumit Garg <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>

To: [email protected]

---
 kernel/watchdog_hld.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
kernel/watchdog_hld.c +0 -0
index 247bf0b1582c..6876e796dbf5 100644
@@ -165,10 +165,13 @@ static void watchdog_overflow_callback(struct perf_event *event,
 
 static int hardlockup_detector_event_create(void)
 {

kernel/watchdog_hld.c static void watchdog_overflow_callback(struct perf_event *event, · line 176

Quoted 10 lines
kernel/watchdog_hld.c +4 -1
-	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 */
+	BUG_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);

This patch makes no sense.

#6 lore raw

Re: [PATCH 2/5] kernel/watchdog_hld: clarify the condition in hardlockup_detector_event_create()

Quoted 33 lines · Peter Zijlstra
Quoted 32 lines · Pingfan Liu

hardlockup_detector_event_create() indirectly calls kmem_cache_alloc_node(), which is blockable.

So here, the really planned context is is_percpu_thread().

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]>
Cc Sumit Garg <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>

To: [email protected]

---
 kernel/watchdog_hld.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
kernel/watchdog_hld.c +1 -1
index 247bf0b1582c..6876e796dbf5 100644
@@ -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 7 lines

kernel/watchdog_hld.c static void watchdog_overflow_callback(struct perf_event *event, · line 176

Quoted 5 lines
kernel/watchdog_hld.c +3 -0
+	/* This function plans to execute in cpu bound kthread */
+	BUG_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);

This patch makes no sense.

This patch aims to disable any attempt such as using get_cpu()/put_cpu() to shut up the check_preemption_disabled().

But if anybody is familiar with the integration of watchdog_hld and cpuhp, he should know the right way without this BUG_ON() or warn.

Do you still think it is pointless?

Thanks,

Pingfan

#7 lore raw

Re: [PATCH 2/5] kernel/watchdog_hld: clarify the condition in hardlockup_detector_event_create()

Quoted 42 lines · Pingfan Liu
Quoted 33 lines · Peter Zijlstra
Quoted 32 lines · Pingfan Liu

hardlockup_detector_event_create() indirectly calls kmem_cache_alloc_node(), which is blockable.

So here, the really planned context is is_percpu_thread().

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]>
Cc Sumit Garg <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>

To: [email protected]

---
 kernel/watchdog_hld.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
kernel/watchdog_hld.c +1 -1
index 247bf0b1582c..6876e796dbf5 100644
@@ -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 7 lines

kernel/watchdog_hld.c static void watchdog_overflow_callback(struct perf_event *event, · line 176

Quoted 5 lines
kernel/watchdog_hld.c +3 -0
+	/* This function plans to execute in cpu bound kthread */
+	BUG_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);

This patch makes no sense.

Quoted 2 lines

This patch aims to disable any attempt such as using get_cpu()/put_cpu() to shut up the check_preemption_disabled().

I have to say that the description of the problem is really cryptic. Please, provide more context, code paths, sample code, next time.

Well, I probably got it. The code might sleep. But it should run on the same CPU even after waking up. You try to achieve this by running the code in a process that is bound to a single CPU.

IMHO, this is not reliable. Anyone could change the affinity of the process in the meantime.

I see two solutions. Either avoid the sleep or making sure that the code access per-CPU variables on the same CPU all the time.

For example, you might use

	*per_cpu_ptr(watchdog_ev, cpu) = evt;

instead of

	this_cpu_write(watchdog_ev, evt);

Best Regards,
Petr

#8 lore raw

Re: [PATCH 2/5] kernel/watchdog_hld: clarify the condition in hardlockup_detector_event_create()

Quoted 43 lines · Petr Mladek
Quoted 42 lines · Pingfan Liu
Quoted 40 lines · Peter Zijlstra
Quoted 27 lines · Pingfan Liu

hardlockup_detector_event_create() indirectly calls kmem_cache_alloc_node(), which is blockable.

So here, the really planned context is is_percpu_thread().

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]>
Cc Sumit Garg <[email protected]>
Cc Will Deacon <[email protected]>
Cc Mark Rutland <[email protected]>

To: [email protected]

---
 kernel/watchdog_hld.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)
kernel/watchdog_hld.c +0 -0
index 247bf0b1582c..6876e796dbf5 100644
@@ -165,10 +165,13 @@ static void watchdog_overflow_callback(struct perf_event *event,
 
 static int hardlockup_detector_event_create(void)
 {

kernel/watchdog_hld.c static void watchdog_overflow_callback(struct perf_event *event, · line 176

Quoted 10 lines
kernel/watchdog_hld.c +4 -1
-	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 */
+	BUG_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);

This patch makes no sense.

Quoted 6 lines
Quoted 2 lines

This patch aims to disable any attempt such as using get_cpu()/put_cpu() to shut up the check_preemption_disabled().

I have to say that the description of the problem is really cryptic. Please, provide more context, code paths, sample code, next time.

Sorry, I will be more straight forward. And I had thought commit log had mentioned it.

Quoted 1 line

Well, I probably got it. The code might sleep. But it should run on the

And you do get it.

Quoted 6 lines

same CPU even after waking up. You try to achieve this by running the code in a process that is bound to a single CPU.

IMHO, this is not reliable. Anyone could change the affinity of the process in the meantime.

No, it is not. As the code says: PF_NO_SETAFFINITY. static inline bool is_percpu_thread(void)
{

#ifdef CONFIG_SMP
	return (current->flags & PF_NO_SETAFFINITY) &&
		(current->nr_cpus_allowed  == 1);
#else
	return true;

#endif
}

This is critical for cpuhp_* (kernel/cpu.c). And hardlockup_detector_event_create() should be planned to run on such a kthread.

Thanks,

Pingfan

Quoted 14 lines

I see two solutions. Either avoid the sleep or making sure that the code access per-CPU variables on the same CPU all the time.

For example, you might use

	*per_cpu_ptr(watchdog_ev, cpu) = evt;

instead of

	this_cpu_write(watchdog_ev, evt);

Best Regards,
Petr

#9 lore raw

PATCH 3/5 kernel/watchdog: adapt the watchdog_hld interface for async model

In reply to 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 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 introducing an extra parameter notifier of watchdog_nmi_probe().

Note after this patch, the async model, which is utilized by the next patch, does not take effect yet.

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 Julien Thierry <[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]

---
 include/linux/nmi.h | 12 +++++++++--
 kernel/watchdog.c   | 49 +++++++++++++++++++++++++++++++++++----------
 2 files changed, 48 insertions(+), 13 deletions(-)
include/linux/nmi.h +10 -2
index 750c7f395ca9..70665fa6e0a9 100644
@@ -78,8 +78,10 @@ static inline void reset_hung_task_detector(void) { }
  */
 #define NMI_WATCHDOG_ENABLED_BIT   0
 #define SOFT_WATCHDOG_ENABLED_BIT  1
+#define NMI_WATCHDOG_UNDETERMINED_BIT  2
 #define NMI_WATCHDOG_ENABLED      (1 << NMI_WATCHDOG_ENABLED_BIT)
 #define SOFT_WATCHDOG_ENABLED     (1 << SOFT_WATCHDOG_ENABLED_BIT)
+#define NMI_WATCHDOG_UNDETERMINED    (1 << NMI_WATCHDOG_UNDETERMINED_BIT)
 
 #if defined(CONFIG_HARDLOCKUP_DETECTOR)
 extern void hardlockup_detector_disable(void);
@@ -116,10 +118,16 @@ static inline int hardlockup_detector_perf_init(void) { return 0; }
 # endif
 #endif
 
+struct watchdog_nmi_status {
+	unsigned int cpu;
+	int status;
+};
+
+typedef void (*watchdog_nmi_status_reporter)(struct watchdog_nmi_status *);
 void watchdog_nmi_stop(void);
 void watchdog_nmi_start(void);
-int watchdog_nmi_probe(void);
-int watchdog_nmi_enable(unsigned int cpu);
+int watchdog_nmi_probe(watchdog_nmi_status_reporter notifier);
+void watchdog_nmi_enable(unsigned int cpu);
 void watchdog_nmi_disable(unsigned int cpu);
 
 /**
kernel/watchdog.c +38 -11

98 lines not on this page · raw message

Signature · 1 line
2.31.1
maintainer
#10 lore raw

Re: [PATCH 3/5] kernel/watchdog: adapt the watchdog_hld interface for async model

Quoted 6 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 before smp_init().

Quoted 8 lines

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 introducing an extra parameter notifier of watchdog_nmi_probe().

Note after this patch, the async model, which is utilized by the next patch, does not take effect yet.

I can't make any sense of what you're trying to do..

kernel/watchdog.c void __weak watchdog_nmi_disable(unsigned int cpu) · line 114

Quoted 5 lines
kernel/watchdog.c +5 -0
+static void watchdog_nmi_report_capability(struct watchdog_nmi_status *data)
+{
+	/* Set status to 1 temporary to block any further access */
+	if (atomic_cmpxchg((atomic_t *)&nmi_watchdog_status, -EBUSY, 1)
+			== -EBUSY) {

But this..

Quoted 13 lines
kernel/watchdog.c +11 -0
+		if (!data->status) {
+			nmi_watchdog_status = 0;
+			lockup_detector_update_enable();
+		} else {
+			nmi_watchdog_status = -ENODEV;
+			/* turn offf watchdog_enabled forever */
+			lockup_detector_update_enable();
+			pr_info("Perf NMI watchdog permanently disabled\n");
+		}
+	}
+}

kernel/watchdog.c static void watchdog_enable(unsigned int cpu) · line 498

Quoted 6 lines
kernel/watchdog.c +2 -1
@@ -467,7 +494,8 @@ static void watchdog_enable(unsigned int cpu)
 	/* Initialize timestamp */
 	update_touch_ts();
 	/* Enable the perf event */
-	if (watchdog_enabled & NMI_WATCHDOG_ENABLED)
+	if (watchdog_enabled &
+			(NMI_WATCHDOG_ENABLED | NMI_WATCHDOG_UNDETERMINED))

and this, are horrible indenting.

#11 lore raw

Re: [PATCH 3/5] kernel/watchdog: adapt the watchdog_hld interface for async model

Quoted 10 lines · Peter Zijlstra
Quoted 9 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 before smp_init().

But it is easy to take an opposite approach by enabling watchdog_hld to get the capability of PMU async.

Quoted 8 lines
Quoted 5 lines

The async model is achieved by introducing an extra parameter notifier of watchdog_nmi_probe().

Note after this patch, the async model, which is utilized by the next patch, does not take effect yet.

I can't make any sense of what you're trying to do..

Sorry for a bad expression. what I mean is: this patch [3/5] provides an framework for async model. But since watchdog_nmi_probe() still return 0 or -ENODEV after this patch, the code's behavior is the same as original.

Does it make sense to you?

Quoted 8 lines

kernel/watchdog.c void __weak watchdog_nmi_disable(unsigned int cpu) · line 114

Quoted 5 lines
kernel/watchdog.c +5 -0
+static void watchdog_nmi_report_capability(struct watchdog_nmi_status *data)
+{
+	/* Set status to 1 temporary to block any further access */
+	if (atomic_cmpxchg((atomic_t *)&nmi_watchdog_status, -EBUSY, 1)
+			== -EBUSY) {

But this..

Oh, check other codes, for a wrapped condition, blanks should be better choice.

Quoted 12 lines
Quoted 11 lines
kernel/watchdog.c +11 -0
+		if (!data->status) {
+			nmi_watchdog_status = 0;
+			lockup_detector_update_enable();
+		} else {
+			nmi_watchdog_status = -ENODEV;
+			/* turn offf watchdog_enabled forever */
+			lockup_detector_update_enable();
+			pr_info("Perf NMI watchdog permanently disabled\n");
+		}
+	}
+}
Quoted 9 lines

kernel/watchdog.c static void watchdog_enable(unsigned int cpu) · line 498

Quoted 7 lines
kernel/watchdog.c +2 -1
@@ -467,7 +494,8 @@ static void watchdog_enable(unsigned int cpu)
 	/* Initialize timestamp */
 	update_touch_ts();
 	/* Enable the perf event */
-	if (watchdog_enabled & NMI_WATCHDOG_ENABLED)
+	if (watchdog_enabled &
+			(NMI_WATCHDOG_ENABLED | NMI_WATCHDOG_UNDETERMINED))

and this, are horrible indenting.

Ditto.

Thanks for your comment and review.

Regards,

Pingfan

#12 lore raw

Re: [PATCH 3/5] kernel/watchdog: adapt the watchdog_hld interface for async model

In reply to Pingfan Liu
Quoted 8 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 before smp_init().

But it is easy to take an opposite approach by enabling watchdog_hld to get the capability of PMU async.

This is another cryptic description. I have probably got it after looking at the 5th patch (was not Cc :-(

Quoted 2 lines

The async model is achieved by introducing an extra parameter notifier of watchdog_nmi_probe().

I would say that the code is horrible and looks too complex.

What about simply calling watchdog_nmi_probe() and lockup_detector_setup() once again when watchdog_nmi_probe() failed in lockup_detector_init()?

Or do not call lockup_detector_init() at all in
kernel_init_freeable() when PMU is not ready yet.

Best Regards,
Petr

#13 lore raw

Re: [PATCH 3/5] kernel/watchdog: adapt the watchdog_hld interface for async model

Quoted 16 lines · Petr Mladek
Quoted 8 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 before smp_init().

But it is easy to take an opposite approach by enabling watchdog_hld to get the capability of PMU async.

This is another cryptic description. I have probably got it after looking at the 5th patch (was not Cc :-(

Quoted 2 lines

The async model is achieved by introducing an extra parameter notifier of watchdog_nmi_probe().

Quoted 8 lines

I would say that the code is horrible and looks too complex.

What about simply calling watchdog_nmi_probe() and lockup_detector_setup() once again when watchdog_nmi_probe() failed in lockup_detector_init()?

Or do not call lockup_detector_init() at all in
kernel_init_freeable() when PMU is not ready yet.

BTW: It is an overkill to create your own kthread just to run some code just once. And you implemeted it a wrong way. The kthread must wait in a loop until someone else stop it and read the exit code.

The easiest solution is to queue a work into system_wq for this.

I was not Cc for the 5th patch, so I write it here.

Best Regards,
Petr

#14 lore raw

Re: [PATCH 3/5] kernel/watchdog: adapt the watchdog_hld interface for async model

Quoted 23 lines · Petr Mladek
Quoted 22 lines · Petr Mladek
Quoted 8 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 before smp_init().

But it is easy to take an opposite approach by enabling watchdog_hld to get the capability of PMU async.

This is another cryptic description. I have probably got it after looking at the 5th patch (was not Cc :-(

Quoted 2 lines

The async model is achieved by introducing an extra parameter notifier of watchdog_nmi_probe().

I would say that the code is horrible and looks too complex.

What about simply calling watchdog_nmi_probe() and lockup_detector_setup() once again when watchdog_nmi_probe() failed in lockup_detector_init()?

Quoted 5 lines
Quoted 2 lines

Or do not call lockup_detector_init() at all in
kernel_init_freeable() when PMU is not ready yet.

BTW: It is an overkill to create your own kthread just to run some code just once. And you implemeted it a wrong way. The kthread

I had thought about queue_work_on() in watchdog_nmi_enable(). But since this work will block the worker kthread for this cpu. So finally, another worker kthread should be created for other work.

But now, I think queue_work_on() may be more neat.

Quoted 3 lines

must wait in a loop until someone else stop it and read the exit code.

Is this behavior mandotory? Since this kthread can decide the exit condition by itself.

And a quick through static int kthread(void *_create), I am not aware of any problem with it.

Quoted 4 lines

The easiest solution is to queue a work into system_wq for this.

I was not Cc for the 5th patch, so I write it here.

Sorry for the inconvenience and Cc you now in case that you have further comments. I will cc you in the next version.

Appreciate for all of your suggestions and comments

Thanks,

Pingfan

#15 lore raw

Re: [PATCH 3/5] kernel/watchdog: adapt the watchdog_hld interface for async model

Quoted 27 lines · Pingfan Liu
Quoted 26 lines · Petr Mladek
Quoted 24 lines · Petr Mladek
Quoted 8 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 before smp_init().

But it is easy to take an opposite approach by enabling watchdog_hld to get the capability of PMU async.

This is another cryptic description. I have probably got it after looking at the 5th patch (was not Cc :-(

Quoted 2 lines

The async model is achieved by introducing an extra parameter notifier of watchdog_nmi_probe().

I would say that the code is horrible and looks too complex.

What about simply calling watchdog_nmi_probe() and lockup_detector_setup() once again when watchdog_nmi_probe() failed in lockup_detector_init()?

Or do not call lockup_detector_init() at all in
kernel_init_freeable() when PMU is not ready yet.

Quoted 6 lines
Quoted 2 lines

BTW: It is an overkill to create your own kthread just to run some code just once. And you implemeted it a wrong way. The kthread

I had thought about queue_work_on() in watchdog_nmi_enable(). But since this work will block the worker kthread for this cpu. So finally, another worker kthread should be created for other work.

This is not a problem. workqueues use a pool of workers that are already created and can be used when one worker gets blocked.

Quoted 7 lines

But now, I think queue_work_on() may be more neat.

Quoted 3 lines

must wait in a loop until someone else stop it and read the exit code.

Is this behavior mandotory? Since this kthread can decide the exit condition by itself.

I am pretty sure. Unfortunately, I can't find it in the documentation.

My view is the following. Each process has a task_struct. The scheduler needs task_struct so that it can switch processes. The task_struct must still exist when the process exits. The scheduler puts the task into TASK_DEAD state. Another process has to read the exit code and destroy the task struct.

See, do_exit() in kernel/exit.c. It ends with do_dead_task(). It is the point when the process goes into TASK_DEAD state.

For a good example, see lib/test_vmalloc.c. The kthread waits until anyone want him to stop:

static int test_func(void *private)
{
[...]

	/*
	 * Wait for the kthread_stop() call.
	 */
	while (!kthread_should_stop())
		msleep(10);

	return 0;
}

The kthreads are started and stopped in:

static void do_concurrent_test(void)
{
[...]

	for (i = 0; i < nr_threads; i++) {
[...]
		t->task = kthread_run(test_func, t, "vmalloc_test/%d", i);
[...]
	/*
	 * Sleep quiet until all workers are done with 1 second
	 * interval. Since the test can take a lot of time we
	 * can run into a stack trace of the hung task. That is
	 * why we go with completion_timeout and HZ value.
	 */
	do {
		ret = wait_for_completion_timeout(&test_all_done_comp, HZ);
	} while (!ret);
[...]
	for (i = 0; i < nr_threads; i++) {
[...]
		if (!IS_ERR(t->task))
			kthread_stop(t->task);

[...]
}

You do not have to solve this if you use the system workqueue (system_wq).

Best Regards,
Petr

#16 lore raw

Re: [PATCH 3/5] kernel/watchdog: adapt the watchdog_hld interface for async model

Quoted 1 line · Petr Mladek

On Fri 2021-09-17 23:41:31, Pingfan Liu wrote:

[...]

Quoted 8 lines
Quoted 4 lines

I had thought about queue_work_on() in watchdog_nmi_enable(). But since this work will block the worker kthread for this cpu. So finally, another worker kthread should be created for other work.

This is not a problem. workqueues use a pool of workers that are already created and can be used when one worker gets blocked.

Yes, you are right. The creation is dynamic and immune to blocking.

Quoted 8 lines
Quoted 7 lines

But now, I think queue_work_on() may be more neat.

Quoted 3 lines

must wait in a loop until someone else stop it and read the exit code.

Is this behavior mandotory? Since this kthread can decide the exit condition by itself.

Quoted 9 lines

I am pretty sure. Unfortunately, I can't find it in the documentation.

My view is the following. Each process has a task_struct. The scheduler needs task_struct so that it can switch processes. The task_struct must still exist when the process exits. The scheduler puts the task into TASK_DEAD state. Another process has to read the exit code and destroy the task struct.

Thanks for bringing up this, and I have an opportunity to think about it.

The core of the problem is put_task_struct(), and who releases the
last one.
It should be: finish_task_switch()->put_task_struct_rcu_user()->delayed_put_task_struct()->put_task_struct(), if (unlikely(prev_state == TASK_DEAD)). It does not depend on another task.

Quoted 42 lines

See, do_exit() in kernel/exit.c. It ends with do_dead_task(). It is the point when the process goes into TASK_DEAD state.

For a good example, see lib/test_vmalloc.c. The kthread waits until anyone want him to stop:

static int test_func(void *private)
{
[...]

	/*
	 * Wait for the kthread_stop() call.
	 */
	while (!kthread_should_stop())
		msleep(10);

	return 0;
}

The kthreads are started and stopped in:

static void do_concurrent_test(void)
{
[...]

	for (i = 0; i < nr_threads; i++) {
[...]
		t->task = kthread_run(test_func, t, "vmalloc_test/%d", i);
[...]
	/*
	 * Sleep quiet until all workers are done with 1 second
	 * interval. Since the test can take a lot of time we
	 * can run into a stack trace of the hung task. That is
	 * why we go with completion_timeout and HZ value.
	 */
	do {
		ret = wait_for_completion_timeout(&test_all_done_comp, HZ);
	} while (!ret);
[...]
	for (i = 0; i < nr_threads; i++) {
[...]
		if (!IS_ERR(t->task))
			kthread_stop(t->task);
Quoted 2 lines

[...]
}

They are good and elegant examples.

Quoted 5 lines

You do not have to solve this if you use the system workqueue (system_wq).

Yes, workqueue is a better choice.

Thanks for your great patience.

Regards,

Pingfan

#17 lore raw

Re: [PATCH 3/5] kernel/watchdog: adapt the watchdog_hld interface for async model

In reply to Petr Mladek
Quoted 13 lines · Petr Mladek
Quoted 8 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 before smp_init().

But it is easy to take an opposite approach by enabling watchdog_hld to get the capability of PMU async.

This is another cryptic description. I have probably got it after looking at the 5th patch (was not Cc :-(

Quoted 9 lines
Quoted 2 lines

The async model is achieved by introducing an extra parameter notifier of watchdog_nmi_probe().

I would say that the code is horrible and looks too complex.

What about simply calling watchdog_nmi_probe() and lockup_detector_setup() once again when watchdog_nmi_probe() failed in lockup_detector_init()?

It may work. But there is still a way to report the PMU NMI capability to watchdog layer accurately. And the API should be extened somehow.

I am thinking something, maybe I can model in another way.

Quoted 3 lines

Or do not call lockup_detector_init() at all in
kernel_init_freeable() when PMU is not ready yet.

This may be not a good choice. Since lockup_detector_init() had better be ready as early as possible, especially before drivers.

Thanks,

Pingfan

#18 lore raw

PATCH 4/5 kernel/watchdog_hld: simplify the detecting of hld watchdog

In reply to Pingfan Liu

By utilizing the new interface model, the stages of probe and enable can be merged, which saves the pair of perf_event alloc and free.

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]

---
 include/linux/nmi.h   |  8 ++------
 kernel/watchdog.c     | 16 ++++++++++++++--
 kernel/watchdog_hld.c | 21 +++------------------
 3 files changed, 19 insertions(+), 26 deletions(-)
include/linux/nmi.h +2 -6
index 70665fa6e0a9..e41cdf3edba7 100644
@@ -101,20 +101,16 @@ extern void arch_touch_nmi_watchdog(void);
 extern void hardlockup_detector_perf_stop(void);
 extern void hardlockup_detector_perf_restart(void);
 extern void hardlockup_detector_perf_disable(void);
-extern void hardlockup_detector_perf_enable(void);
+extern int hardlockup_detector_perf_enable(void);
 extern void hardlockup_detector_perf_cleanup(void);
-extern int hardlockup_detector_perf_init(void);
 #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 int hardlockup_detector_perf_enable(void) { return -ENODEV; }
 static inline void hardlockup_detector_perf_cleanup(void) { }
 # if !defined(CONFIG_HAVE_NMI_WATCHDOG)
-static inline int hardlockup_detector_perf_init(void) { return -ENODEV; }
 static inline void arch_touch_nmi_watchdog(void) {}
-# else
-static inline int hardlockup_detector_perf_init(void) { return 0; }
 # endif
 #endif
 
kernel/watchdog.c +14 -2
index 4ab71943d65f..3f5efbd5961c 100644
@@ -99,7 +99,17 @@ static watchdog_nmi_status_reporter status_reporter;
  */
 void __weak watchdog_nmi_enable(unsigned int cpu)
 {
-	hardlockup_detector_perf_enable();
+	struct watchdog_nmi_status data;
+	int ret;
+
+	ret = hardlockup_detector_perf_enable();
+	/* No concurrent risk because BP executes this before smp_init() */
+	if (watchdog_enabled & NMI_WATCHDOG_UNDETERMINED
+			&& status_reporter) {
+		data.cpu = cpu;
+		data.status = ret;
+		(*status_reporter)(&data);
+	}
 }
 
 void __weak watchdog_nmi_disable(unsigned int cpu)
@@ -130,7 +140,9 @@ static void watchdog_nmi_report_capability(struct watchdog_nmi_status *data)
  */
 int __weak __init watchdog_nmi_probe(watchdog_nmi_status_reporter notifier)
 {
-	return hardlockup_detector_perf_init();
+	status_reporter = notifier;
+
+	return -EBUSY;
 }
 
 /**
kernel/watchdog_hld.c +3 -18
index 6876e796dbf5..2894778fbc6d 100644
@@ -190,16 +190,17 @@ static int hardlockup_detector_event_create(void)
 /**
  * hardlockup_detector_perf_enable - Enable the local event
  */
-void hardlockup_detector_perf_enable(void)
+int hardlockup_detector_perf_enable(void)
 {
 	if (hardlockup_detector_event_create())
-		return;
+		return -ENODEV;
 
 	/* use original value for check */
 	if (!atomic_fetch_inc(&watchdog_cpus))
 		pr_info("Enabled. Permanently consumes one hw-PMU counter.\n");
 
 	perf_event_enable(this_cpu_read(watchdog_ev));
+	return 0;
 }
 
 /**
@@ -281,19 +282,3 @@ void __init hardlockup_detector_perf_restart(void)
 			perf_event_enable(event);
 	}
 }
-
-/**
- * hardlockup_detector_perf_init - Probe whether NMI event is available at all
- */
-int __init hardlockup_detector_perf_init(void)
-{
-	int ret = hardlockup_detector_event_create();
-
-	if (ret) {
-		pr_info("Perf NMI watchdog permanently disabled\n");
-	} else {
-		perf_event_release_kernel(this_cpu_read(watchdog_ev));
-		this_cpu_write(watchdog_ev, NULL);
-	}
-	return ret;
-}
Signature · 1 line
2.31.1
#19 lore raw

PATCH 5/5 arm64/watchdog_hld: enable hard lockup on arm64 platform

In reply to Pingfan Liu

With watchdog_hld armed with the async model, arm64 can probe and enable perf NMI after smp_init(). At the boot stage, all of cpus arm hard lockup detector in the async model.

In this patch, the function hw_nmi_get_sample_period() is borrowed from [1], credit goes to Sumit.

[1]: http://lore.kernel.org/linux-arm-kernel/[email protected]

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Peter Zijlstra <[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 Julien Thierry <[email protected]>
Cc Kees Cook <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Sami Tolvanen <[email protected]>

To: [email protected]
Cc: [email protected]

---
 arch/arm64/Kconfig                  |  3 ++
 arch/arm64/include/asm/perf_event.h |  5 ++
 arch/arm64/kernel/Makefile          |  1 +
 arch/arm64/kernel/perf_event.c      | 14 ++++-
 arch/arm64/kernel/watchdog_hld.c    | 83 +++++++++++++++++++++++++++++
 drivers/perf/arm_pmu.c              |  5 ++
 6 files changed, 109 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm64/kernel/watchdog_hld.c
arch/arm64/Kconfig +3 -0
index 62c3c1d2190f..0f49e58a9dd8 100644
@@ -193,6 +193,9 @@ config ARM64
 	select HAVE_PERF_EVENTS
 	select HAVE_PERF_REGS
 	select HAVE_PERF_USER_STACK_DUMP
+	select HAVE_PERF_EVENTS_NMI
+	select HAVE_HARDLOCKUP_DETECTOR_PERF \
+		if PERF_EVENTS && HW_PERF_EVENTS && HAVE_PERF_EVENTS_NMI
 	select HAVE_REGS_AND_STACK_ACCESS_API
 	select HAVE_FUNCTION_ARG_ACCESS_API
 	select HAVE_FUTEX_CMPXCHG if FUTEX
arch/arm64/include/asm/perf_event.h +5 -0
index 4ef6f19331f9..712a75f432f0 100644
@@ -6,6 +6,7 @@
 #ifndef __ASM_PERF_EVENT_H
 #define __ASM_PERF_EVENT_H
 
+#include <linux/wait.h>
 #include <asm/stack_pointer.h>
 #include <asm/ptrace.h>
 
@@ -259,4 +260,8 @@ extern unsigned long perf_misc_flags(struct pt_regs *regs);
 	(regs)->pstate = PSR_MODE_EL1h;	\
 }
 
+extern bool arm_pmu_initialized;
+extern wait_queue_head_t arm_pmu_wait;
+extern bool check_pmu_nmi_ability(void);
+
 #endif
arch/arm64/kernel/Makefile +1 -0
index 3f1490bfb938..789c2fe5bb90 100644
@@ -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 +12 -2
index d07788dad388..c4144cea0d55 100644
@@ -24,6 +24,9 @@
 #include <linux/sched_clock.h>
 #include <linux/smp.h>
 
+bool arm_pmu_initialized;
+DECLARE_WAIT_QUEUE_HEAD(arm_pmu_wait);
+
 /* ARMv8 Cortex-A53 specific event types. */
 #define ARMV8_A53_PERFCTR_PREF_LINEFILL				0xC2
 
@@ -1284,10 +1287,17 @@ 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);
+
+	arm_pmu_initialized = true;
+	wake_up_all(&arm_pmu_wait);
+
+	return ret;
 }
 device_initcall(armv8_pmu_driver_init)
 
arch/arm64/kernel/watchdog_hld.c new file +83 -0

83 lines not on this page · raw message

drivers/perf/arm_pmu.c +5 -0
index 3cbc3baf087f..e08961b37922 100644
@@ -105,6 +105,11 @@ static DEFINE_PER_CPU(const struct pmu_irq_ops *, cpu_irq_ops);
 
 static bool has_nmi;
 
+bool check_pmu_nmi_ability(void)
+{
+	return has_nmi;
+}
+
 static inline u64 arm_pmu_event_max_period(struct perf_event *event)
 {
 	if (event->hw.flags & ARMPMU_EVT_64BIT)
Signature · 1 line
2.31.1
#20 lore raw

Re: [PATCH 5/5] arm64/watchdog_hld: enable hard lockup on arm64 platform

On Wed, Sep 15, 2021 at 11:51:03AM +0800, Pingfan Liu wrote:

+ Petr Mladek

Quoted 230 lines

With watchdog_hld armed with the async model, arm64 can probe and enable perf NMI after smp_init(). At the boot stage, all of cpus arm hard lockup detector in the async model.

In this patch, the function hw_nmi_get_sample_period() is borrowed from [1], credit goes to Sumit.

[1]: http://lore.kernel.org/linux-arm-kernel/[email protected]

Cc Catalin Marinas <[email protected]>
Cc Will Deacon <[email protected]>
Cc Peter Zijlstra <[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 Julien Thierry <[email protected]>
Cc Kees Cook <[email protected]>
Cc Masahiro Yamada <[email protected]>
Cc Sami Tolvanen <[email protected]>

To: [email protected]
Cc: [email protected]

---
 arch/arm64/Kconfig                  |  3 ++
 arch/arm64/include/asm/perf_event.h |  5 ++
 arch/arm64/kernel/Makefile          |  1 +
 arch/arm64/kernel/perf_event.c      | 14 ++++-
 arch/arm64/kernel/watchdog_hld.c    | 83 +++++++++++++++++++++++++++++
 drivers/perf/arm_pmu.c              |  5 ++
 6 files changed, 109 insertions(+), 2 deletions(-)
 create mode 100644 arch/arm64/kernel/watchdog_hld.c
arch/arm64/Kconfig +3 -0
index 62c3c1d2190f..0f49e58a9dd8 100644
@@ -193,6 +193,9 @@ config ARM64
 	select HAVE_PERF_EVENTS
 	select HAVE_PERF_REGS
 	select HAVE_PERF_USER_STACK_DUMP
+	select HAVE_PERF_EVENTS_NMI
+	select HAVE_HARDLOCKUP_DETECTOR_PERF \
+		if PERF_EVENTS && HW_PERF_EVENTS && HAVE_PERF_EVENTS_NMI
 	select HAVE_REGS_AND_STACK_ACCESS_API
 	select HAVE_FUNCTION_ARG_ACCESS_API
 	select HAVE_FUTEX_CMPXCHG if FUTEX
arch/arm64/include/asm/perf_event.h +5 -0
index 4ef6f19331f9..712a75f432f0 100644
@@ -6,6 +6,7 @@
 #ifndef __ASM_PERF_EVENT_H
 #define __ASM_PERF_EVENT_H
 
+#include <linux/wait.h>
 #include <asm/stack_pointer.h>
 #include <asm/ptrace.h>
 
@@ -259,4 +260,8 @@ extern unsigned long perf_misc_flags(struct pt_regs *regs);
 	(regs)->pstate = PSR_MODE_EL1h;	\
 }
 
+extern bool arm_pmu_initialized;
+extern wait_queue_head_t arm_pmu_wait;
+extern bool check_pmu_nmi_ability(void);
+
 #endif
arch/arm64/kernel/Makefile +1 -0
index 3f1490bfb938..789c2fe5bb90 100644
@@ -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 +12 -2
index d07788dad388..c4144cea0d55 100644
@@ -24,6 +24,9 @@
 #include <linux/sched_clock.h>
 #include <linux/smp.h>
 
+bool arm_pmu_initialized;
+DECLARE_WAIT_QUEUE_HEAD(arm_pmu_wait);
+
 /* ARMv8 Cortex-A53 specific event types. */
 #define ARMV8_A53_PERFCTR_PREF_LINEFILL				0xC2
 
@@ -1284,10 +1287,17 @@ 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);
+
+	arm_pmu_initialized = true;
+	wake_up_all(&arm_pmu_wait);
+
+	return ret;
 }
 device_initcall(armv8_pmu_driver_init)
 
arch/arm64/kernel/watchdog_hld.c new file +83 -0
new file mode 100644
index 000000000000..18bfa62f1058
@@ -0,0 +1,83 @@
+// SPDX-License-Identifier: GPL-2.0
+#include <linux/kthread.h>
+#include <linux/sched.h>
+#include <linux/nmi.h>
+#include <linux/cpufreq.h>
+#include <asm/perf_event.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;
+}
+
+static watchdog_nmi_status_reporter status_reporter;
+
+static int hld_enabled_thread_fun(void *unused)
+{
+	struct watchdog_nmi_status status;
+	watchdog_nmi_status_reporter local_reporter;
+	int ret;
+
+	wait_event(arm_pmu_wait, arm_pmu_initialized);
+	status.cpu = raw_smp_processor_id();
+
+	if (!check_pmu_nmi_ability()) {
+		status.status = -ENODEV;
+		goto report;
+	}
+
+	ret = hardlockup_detector_perf_enable();
+	status.status = ret;
+
+report:
+	local_reporter = (watchdog_nmi_status_reporter)atomic64_fetch_and(0,
+				(atomic64_t *)&status_reporter);
+	if (local_reporter)
+		(*local_reporter)(&status);
+
+	return 0;
+}
+
+/* As for watchdog_nmi_disable(), using the default implement */
+void watchdog_nmi_enable(unsigned int cpu)
+{
+	struct task_struct *t;
+
+	/* PMU is not ready */
+	if (!arm_pmu_initialized) {
+		t = kthread_create_on_cpu(hld_enabled_thread_fun, NULL, cpu,
+			       "arm64_hld.%u");
+		if (IS_ERR(t))
+			return;
+		wake_up_process(t);
+		return;
+	}
+
+	/* For hotplug in cpu */
+	hardlockup_detector_perf_enable();
+}
+
+int __init watchdog_nmi_probe(watchdog_nmi_status_reporter notifier)
+{
+	/* On arm64, arm pmu is not ready at this stage */
+	status_reporter = notifier;
+	return -EBUSY;
+}
+
drivers/perf/arm_pmu.c +5 -0
index 3cbc3baf087f..e08961b37922 100644
@@ -105,6 +105,11 @@ static DEFINE_PER_CPU(const struct pmu_irq_ops *, cpu_irq_ops);
 
 static bool has_nmi;
 
+bool check_pmu_nmi_ability(void)
+{
+	return has_nmi;
+}
+
 static inline u64 arm_pmu_event_max_period(struct perf_event *event)
 {
 	if (event->hw.flags & ARMPMU_EVT_64BIT)
Signature · 7 lines
2.31.1


_______________________________________________
linux-arm-kernel mailing list
[email protected]
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel