* [PATCH 2/2] nmi_watchdog: properly configure for software events
@ 2010-02-16 22:02 Don Zickus
2010-02-17 7:20 ` Ingo Molnar
2010-02-17 12:11 ` [tip:perf/nmi] nmi_watchdog: Properly " tip-bot for Don Zickus
0 siblings, 2 replies; 3+ messages in thread
From: Don Zickus @ 2010-02-16 22:02 UTC (permalink / raw)
To: linux-kernel; +Cc: Don Zickus, mingo, peterz, gorcunov, aris, paulus
Paul Mackerras brought up a good point that when fallbacking to
software events, I may have been lucky in my configuration.
Modified the code to explicit provide a new configuration for
software events.
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
kernel/nmi_watchdog.c | 23 ++++++++++++++++++-----
1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/kernel/nmi_watchdog.c b/kernel/nmi_watchdog.c
index 633b230..3c75cbf 100644
--- a/kernel/nmi_watchdog.c
+++ b/kernel/nmi_watchdog.c
@@ -61,7 +61,7 @@ static int __init setup_nmi_watchdog(char *str)
}
__setup("nmi_watchdog=", setup_nmi_watchdog);
-struct perf_event_attr wd_attr = {
+struct perf_event_attr wd_hw_attr = {
.type = PERF_TYPE_HARDWARE,
.config = PERF_COUNT_HW_CPU_CYCLES,
.size = sizeof(struct perf_event_attr),
@@ -69,6 +69,14 @@ struct perf_event_attr wd_attr = {
.disabled = 1,
};
+struct perf_event_attr wd_sw_attr = {
+ .type = PERF_TYPE_SOFTWARE,
+ .config = PERF_COUNT_SW_CPU_CLOCK,
+ .size = sizeof(struct perf_event_attr),
+ .pinned = 1,
+ .disabled = 1,
+};
+
void wd_overflow(struct perf_event *event, int nmi,
struct perf_sample_data *data,
struct pt_regs *regs)
@@ -105,6 +113,7 @@ void wd_overflow(struct perf_event *event, int nmi,
static int enable_nmi_watchdog(int cpu)
{
struct perf_event *event;
+ struct perf_event_attr *wd_attr;
event = per_cpu(nmi_watchdog_ev, cpu);
if (event && event->state > PERF_EVENT_STATE_OFF)
@@ -112,11 +121,15 @@ static int enable_nmi_watchdog(int cpu)
if (event == NULL) {
/* Try to register using hardware perf events first */
- wd_attr.sample_period = hw_nmi_get_sample_period();
- event = perf_event_create_kernel_counter(&wd_attr, cpu, -1, wd_overflow);
+ wd_attr = &wd_hw_attr;
+ wd_attr->sample_period = hw_nmi_get_sample_period();
+ event = perf_event_create_kernel_counter(wd_attr, cpu, -1, wd_overflow);
if (IS_ERR(event)) {
- wd_attr.type = PERF_TYPE_SOFTWARE;
- event = perf_event_create_kernel_counter(&wd_attr, cpu, -1, wd_overflow);
+ /* hardware doesn't exist or not supported, fallback to software events */
+ printk("nmi_watchdog: hardware not available, trying software events\n");
+ wd_attr = &wd_sw_attr;
+ wd_attr->sample_period = NSEC_PER_SEC;
+ event = perf_event_create_kernel_counter(wd_attr, cpu, -1, wd_overflow);
if (IS_ERR(event)) {
printk(KERN_ERR "nmi watchdog failed to create perf event on %i: %p\n", cpu, event);
return -1;
--
1.6.6.83.gc9a2
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH 2/2] nmi_watchdog: properly configure for software events
2010-02-16 22:02 [PATCH 2/2] nmi_watchdog: properly configure for software events Don Zickus
@ 2010-02-17 7:20 ` Ingo Molnar
2010-02-17 12:11 ` [tip:perf/nmi] nmi_watchdog: Properly " tip-bot for Don Zickus
1 sibling, 0 replies; 3+ messages in thread
From: Ingo Molnar @ 2010-02-17 7:20 UTC (permalink / raw)
To: Don Zickus; +Cc: linux-kernel, peterz, gorcunov, aris, paulus
* Don Zickus <dzickus@redhat.com> wrote:
> Paul Mackerras brought up a good point that when fallbacking to
> software events, I may have been lucky in my configuration.
>
> Modified the code to explicit provide a new configuration for
> software events.
>
> Signed-off-by: Don Zickus <dzickus@redhat.com>
> ---
> kernel/nmi_watchdog.c | 23 ++++++++++++++++++-----
> 1 files changed, 18 insertions(+), 5 deletions(-)
Applied, thanks Don!
btw., a few minor code cleanliness nits:
>
> diff --git a/kernel/nmi_watchdog.c b/kernel/nmi_watchdog.c
> index 633b230..3c75cbf 100644
> --- a/kernel/nmi_watchdog.c
> +++ b/kernel/nmi_watchdog.c
> @@ -61,7 +61,7 @@ static int __init setup_nmi_watchdog(char *str)
> }
> __setup("nmi_watchdog=", setup_nmi_watchdog);
>
> -struct perf_event_attr wd_attr = {
> +struct perf_event_attr wd_hw_attr = {
> .type = PERF_TYPE_HARDWARE,
> .config = PERF_COUNT_HW_CPU_CYCLES,
> .size = sizeof(struct perf_event_attr),
> @@ -69,6 +69,14 @@ struct perf_event_attr wd_attr = {
> .disabled = 1,
> };
>
> +struct perf_event_attr wd_sw_attr = {
> + .type = PERF_TYPE_SOFTWARE,
> + .config = PERF_COUNT_SW_CPU_CLOCK,
> + .size = sizeof(struct perf_event_attr),
> + .pinned = 1,
> + .disabled = 1,
> +};
Please use the vertically aligned initializer style we are using in arch/x86
and in the perf code as well:
struct perf_event_attr wd_sw_attr = {
.type = PERF_TYPE_SOFTWARE,
.config = PERF_COUNT_SW_CPU_CLOCK,
.size = sizeof(struct perf_event_attr),
.pinned = 1,
.disabled = 1,
};
Plus see below the errors and warnings that scripts/checkpatch -f
kernel/nmi_watchdog.c found. Most of those (except the col 80 warnings which
you should ignore) should be fixed i suspect.
Thanks,
Ingo
ERROR: code indent should use tabs where possible
#53: FILE: nmi_watchdog.c:53:
+ if (!strncmp(str, "panic", 5)) {$
ERROR: code indent should use tabs where possible
#54: FILE: nmi_watchdog.c:54:
+ panic_on_timeout = 1;$
ERROR: code indent should use tabs where possible
#55: FILE: nmi_watchdog.c:55:
+ str = strchr(str, ',');$
ERROR: code indent should use tabs where possible
#56: FILE: nmi_watchdog.c:56:
+ if (!str)$
ERROR: code indent should use tabs where possible
#57: FILE: nmi_watchdog.c:57:
+ return 1;$
ERROR: code indent should use tabs where possible
#58: FILE: nmi_watchdog.c:58:
+ ++str;$
ERROR: code indent should use tabs where possible
#59: FILE: nmi_watchdog.c:59:
+ }$
ERROR: code indent should use tabs where possible
#60: FILE: nmi_watchdog.c:60:
+ return 1;$
CHECK: __setup appears un-documented -- check Documentation/kernel-parameters.txt
#62: FILE: nmi_watchdog.c:62:
+__setup("nmi_watchdog=", setup_nmi_watchdog);
ERROR: space required after that ',' (ctx:VxV)
#98: FILE: nmi_watchdog.c:98:
+ per_cpu(alert_counter,cpu) += 1;
^
ERROR: space required after that ',' (ctx:VxV)
#99: FILE: nmi_watchdog.c:99:
+ if (per_cpu(alert_counter,cpu) == 5) {
^
WARNING: braces {} are not necessary for any arm of this statement
#100: FILE: nmi_watchdog.c:100:
+ if (panic_on_timeout) {
[...]
+ } else {
[...]
WARNING: line over 80 characters
#101: FILE: nmi_watchdog.c:101:
+ panic("NMI Watchdog detected LOCKUP on cpu %d", cpu);
WARNING: line over 80 characters
#103: FILE: nmi_watchdog.c:103:
+ WARN(1, "NMI Watchdog detected LOCKUP on cpu %d", cpu);
ERROR: space required after that ',' (ctx:VxV)
#107: FILE: nmi_watchdog.c:107:
+ per_cpu(alert_counter,cpu) = 0;
^
WARNING: line over 80 characters
#126: FILE: nmi_watchdog.c:126:
+ event = perf_event_create_kernel_counter(wd_attr, cpu, -1, wd_overflow);
WARNING: line over 80 characters
#128: FILE: nmi_watchdog.c:128:
+ /* hardware doesn't exist or not supported, fallback to software events */
WARNING: printk() should include KERN_ facility level
#129: FILE: nmi_watchdog.c:129:
+ printk("nmi_watchdog: hardware not available, trying software events\n");
WARNING: line over 80 characters
#132: FILE: nmi_watchdog.c:132:
+ event = perf_event_create_kernel_counter(wd_attr, cpu, -1, wd_overflow);
WARNING: line over 80 characters
#134: FILE: nmi_watchdog.c:134:
+ printk(KERN_ERR "nmi watchdog failed to create perf event on %i: %p\n", cpu, event);
WARNING: printk() should include KERN_ facility level
#185: FILE: nmi_watchdog.c:185:
+ printk("NMI watchdog failed configuration, "
CHECK: __setup appears un-documented -- check Documentation/kernel-parameters.txt
#238: FILE: nmi_watchdog.c:238:
+__setup("nonmi_watchdog", nonmi_watchdog_setup);
total: 11 errors, 9 warnings, 2 checks, 258 lines checked
kernel/nmi_watchdog.c has style problems, please review. If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
^ permalink raw reply [flat|nested] 3+ messages in thread
* [tip:perf/nmi] nmi_watchdog: Properly configure for software events
2010-02-16 22:02 [PATCH 2/2] nmi_watchdog: properly configure for software events Don Zickus
2010-02-17 7:20 ` Ingo Molnar
@ 2010-02-17 12:11 ` tip-bot for Don Zickus
1 sibling, 0 replies; 3+ messages in thread
From: tip-bot for Don Zickus @ 2010-02-17 12:11 UTC (permalink / raw)
To: linux-tip-commits
Cc: linux-kernel, acme, paulus, hpa, mingo, a.p.zijlstra, efault,
fweisbec, tglx, mingo, dzickus
Commit-ID: 96ca4028aca0638d0e11dcbfabc4283054072933
Gitweb: http://git.kernel.org/tip/96ca4028aca0638d0e11dcbfabc4283054072933
Author: Don Zickus <dzickus@redhat.com>
AuthorDate: Tue, 16 Feb 2010 17:02:25 -0500
Committer: Ingo Molnar <mingo@elte.hu>
CommitDate: Wed, 17 Feb 2010 08:16:02 +0100
nmi_watchdog: Properly configure for software events
Paul Mackerras brought up a good point that when fallbacking to
software events, I may have been lucky in my configuration.
Modified the code to explicit provide a new configuration for
software events.
Suggested-by: Paul Mackerras <paulus@samba.org>
Signed-off-by: Don Zickus <dzickus@redhat.com>
Cc: gorcunov@gmail.com
Cc: aris@redhat.com
Cc: Peter Zijlstra <a.p.zijlstra@chello.nl>
Cc: Mike Galbraith <efault@gmx.de>
Cc: Arnaldo Carvalho de Melo <acme@redhat.com>
Cc: Frederic Weisbecker <fweisbec@gmail.com>
LKML-Reference: <1266357745-26671-1-git-send-email-dzickus@redhat.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
kernel/nmi_watchdog.c | 23 ++++++++++++++++++-----
1 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/kernel/nmi_watchdog.c b/kernel/nmi_watchdog.c
index 633b230..3c75cbf 100644
--- a/kernel/nmi_watchdog.c
+++ b/kernel/nmi_watchdog.c
@@ -61,7 +61,7 @@ static int __init setup_nmi_watchdog(char *str)
}
__setup("nmi_watchdog=", setup_nmi_watchdog);
-struct perf_event_attr wd_attr = {
+struct perf_event_attr wd_hw_attr = {
.type = PERF_TYPE_HARDWARE,
.config = PERF_COUNT_HW_CPU_CYCLES,
.size = sizeof(struct perf_event_attr),
@@ -69,6 +69,14 @@ struct perf_event_attr wd_attr = {
.disabled = 1,
};
+struct perf_event_attr wd_sw_attr = {
+ .type = PERF_TYPE_SOFTWARE,
+ .config = PERF_COUNT_SW_CPU_CLOCK,
+ .size = sizeof(struct perf_event_attr),
+ .pinned = 1,
+ .disabled = 1,
+};
+
void wd_overflow(struct perf_event *event, int nmi,
struct perf_sample_data *data,
struct pt_regs *regs)
@@ -105,6 +113,7 @@ void wd_overflow(struct perf_event *event, int nmi,
static int enable_nmi_watchdog(int cpu)
{
struct perf_event *event;
+ struct perf_event_attr *wd_attr;
event = per_cpu(nmi_watchdog_ev, cpu);
if (event && event->state > PERF_EVENT_STATE_OFF)
@@ -112,11 +121,15 @@ static int enable_nmi_watchdog(int cpu)
if (event == NULL) {
/* Try to register using hardware perf events first */
- wd_attr.sample_period = hw_nmi_get_sample_period();
- event = perf_event_create_kernel_counter(&wd_attr, cpu, -1, wd_overflow);
+ wd_attr = &wd_hw_attr;
+ wd_attr->sample_period = hw_nmi_get_sample_period();
+ event = perf_event_create_kernel_counter(wd_attr, cpu, -1, wd_overflow);
if (IS_ERR(event)) {
- wd_attr.type = PERF_TYPE_SOFTWARE;
- event = perf_event_create_kernel_counter(&wd_attr, cpu, -1, wd_overflow);
+ /* hardware doesn't exist or not supported, fallback to software events */
+ printk("nmi_watchdog: hardware not available, trying software events\n");
+ wd_attr = &wd_sw_attr;
+ wd_attr->sample_period = NSEC_PER_SEC;
+ event = perf_event_create_kernel_counter(wd_attr, cpu, -1, wd_overflow);
if (IS_ERR(event)) {
printk(KERN_ERR "nmi watchdog failed to create perf event on %i: %p\n", cpu, event);
return -1;
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2010-02-17 12:11 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-02-16 22:02 [PATCH 2/2] nmi_watchdog: properly configure for software events Don Zickus
2010-02-17 7:20 ` Ingo Molnar
2010-02-17 12:11 ` [tip:perf/nmi] nmi_watchdog: Properly " tip-bot for Don Zickus
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).