linux-kernel - Re: [PATCH 2/2] nmi_watchdog: properly configure for software events

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20100217072040.GA24587@elte.hu>
Date:	Wed, 17 Feb 2010 08:20:40 +0100
From:	Ingo Molnar <mingo@...e.hu>
To:	Don Zickus <dzickus@...hat.com>
Cc:	linux-kernel@...r.kernel.org, peterz@...radead.org,
	gorcunov@...il.com, aris@...hat.com, paulus@...ba.org
Subject: Re: [PATCH 2/2] nmi_watchdog: properly configure for software events


* Don Zickus <dzickus@...hat.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@...hat.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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/