[PATCH] watchdog: Quiet down the boot messages

[PATCH] watchdog: Quiet down the boot messages

[Don Zickus]

A bunch of bugzillas have complained how noisy the nmi_watchdog is during
boot-up especially with its expected failure cases (like virt and bios
resource contention).

This is my attempt to quiet them down and keep it less confusing for the end
user.  What I did is print the message for cpu0 and save it for future
comparisions.  If future cpus have an identical message as cpu0, then don't
print the redundant info.  However, if a future cpu has a different message,
happily print that loudly.

Before the change, you would see something like:

    ..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1
    CPU0: Intel(R) Core(TM)2 Quad CPU    Q9550  @ 2.83GHz stepping 0a
    Performance Events: PEBS fmt0+, Core2 events, Intel PMU driver.
    ... version:                2
    ... bit width:              40
    ... generic registers:      2
    ... value mask:             000000ffffffffff
    ... max period:             000000007fffffff
    ... fixed-purpose events:   3
    ... event mask:             0000000700000003
    NMI watchdog enabled, takes one hw-pmu counter.
    Booting Node   0, Processors  #1
    NMI watchdog enabled, takes one hw-pmu counter.
     #2
    NMI watchdog enabled, takes one hw-pmu counter.
     #3 Ok.
    NMI watchdog enabled, takes one hw-pmu counter.
    Brought up 4 CPUs
    Total of 4 processors activated (22607.24 BogoMIPS).

After the change, it is simlified to:

    ..TIMER: vector=0x30 apic1=0 pin1=2 apic2=-1 pin2=-1
    CPU0: Intel(R) Core(TM)2 Quad CPU    Q9550  @ 2.83GHz stepping 0a
    Performance Events: PEBS fmt0+, Core2 events, Intel PMU driver.
    ... version:                2
    ... bit width:              40
    ... generic registers:      2
    ... value mask:             000000ffffffffff
    ... max period:             000000007fffffff
    ... fixed-purpose events:   3
    ... event mask:             0000000700000003
    NMI watchdog enabled, takes one hw-pmu counter.
    Booting Node   0, Processors  #1 #2 #3 Ok.
    Brought up 4 CPUs

Reported-and-tested-by: Nathan Zimmer <nzimmer@sgi.com>
Signed-off-by: Don Zickus <dzickus@redhat.com>
---
 kernel/watchdog.c |   20 +++++++++++++++++++-
 1 files changed, 19 insertions(+), 1 deletions(-)

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index e5e1d85..79ff671 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -377,6 +377,14 @@ static int watchdog_nmi_enable(int cpu)
 	struct perf_event_attr *wd_attr;
 	struct perf_event *event = per_cpu(watchdog_ev, cpu);
 
+	/*
+	 * People like the simple clean cpu node info
+	 * on boot.  Simplify the noise from the watchdog
+	 * by only printing messages that are different than
+	 * what cpu0 displayed
+	 */
+	static unsigned long err0 = 0;
+
 	/* is it already setup and enabled? */
 	if (event && event->state > PERF_EVENT_STATE_OFF)
 		goto out;
@@ -390,11 +398,21 @@ static int watchdog_nmi_enable(int cpu)
 
 	/* Try to register using hardware perf events */
 	event = perf_event_create_kernel_counter(wd_attr, cpu, NULL, watchdog_overflow_callback, NULL);
+
+	/* save cpu0 error for future comparision */
+	if (!cpu)
+		err0 = (IS_ERR(event) ? PTR_ERR(event) : 0);
+
 	if (!IS_ERR(event)) {
-		pr_info("enabled, takes one hw-pmu counter.\n");
+		/* only print for cpu0 or different than cpu0 */
+		if (!cpu || err0)
+			pr_info("enabled, takes one hw-pmu counter.\n");
 		goto out_save;
 	}
 
+	/* skip displaying the same error again */
+	if ((PTR_ERR(event) == err0) && cpu)
+		return PTR_ERR(event);
 
 	/* vary the KERN level based on the returned errno */
 	if (PTR_ERR(event) == -EOPNOTSUPP)
-- 
1.7.7.6

Re: [PATCH] watchdog: Quiet down the boot messages

[Joe Perches]

On Thu, 2012-06-07 at 22:15 -0400, Don Zickus wrote:
> A bunch of bugzillas have complained how noisy the nmi_watchdog is during
> boot-up especially with its expected failure cases (like virt and bios
> resource contention).

Hi Don, this seems nicer.

Just some trivial comments below:

> This is my attempt to quiet them down and keep it less confusing for the end
> user.  What I did is print the message for cpu0 and save it for future
> comparisions.

comparisons

[]
> After the change, it is simlified to:

simplified

> diff --git a/kernel/watchdog.c b/kernel/watchdog.c
[]
> @@ -377,6 +377,14 @@ static int watchdog_nmi_enable(int cpu)
>  	struct perf_event_attr *wd_attr;
>  	struct perf_event *event = per_cpu(watchdog_ev, cpu);
>  
> +	/*
> +	 * People like the simple clean cpu node info
> +	 * on boot.  Simplify the noise from the watchdog
> +	 * by only printing messages that are different than
> +	 * what cpu0 displayed
> +	 */

This comment could be shortened by a line
	/*
	 * People like simple and clean cpu node info on boot.
	 * Reduce the watchdog noise by only printing messages
	 * that are different from what cpu0 displayed.
	 */

> +	static unsigned long err0 = 0;

Strictly, this doesn't need initialization.
I think the err0 name is unclear.  Maybe cpu0_err instead;

I think the use of !cpu and cpu is unclear.
It's still a cpu, just index 0.
!cpu should be cpu == 0 and
cpu should be cpu != 0 too.


> @@ -390,11 +398,21 @@ static int watchdog_nmi_enable(int cpu)
>  
>  	/* Try to register using hardware perf events */
>  	event = perf_event_create_kernel_counter(wd_attr, cpu, NULL, watchdog_overflow_callback, NULL);
> +
> +	/* save cpu0 error for future comparision */
> +	if (!cpu)
> +		err0 = (IS_ERR(event) ? PTR_ERR(event) : 0);

	if (cpu == 0 && IS_ERR(event))
		cpu0_err = PTR_ERR(event);

> +
>  	if (!IS_ERR(event)) {
> -		pr_info("enabled, takes one hw-pmu counter.\n");
> +		/* only print for cpu0 or different than cpu0 */

	or if

> +		if (!cpu || err0)

		if (cpu == 0 || cpu0_err)

> +			pr_info("enabled, takes one hw-pmu counter.\n");
>  		goto out_save;
>  	}
>  
> +	/* skip displaying the same error again */
> +	if ((PTR_ERR(event) == err0) && cpu)

	if (cpu > 0 && PTR_ERR(event))
> +		return PTR_ERR(event);
>  
>  	/* vary the KERN level based on the returned errno */
>  	if (PTR_ERR(event) == -EOPNOTSUPP)

cheers, Joe

Re: [PATCH] watchdog: Quiet down the boot messages

[Don Zickus]

On Thu, Jun 07, 2012 at 07:41:14PM -0700, Joe Perches wrote:
> On Thu, 2012-06-07 at 22:15 -0400, Don Zickus wrote:
> > A bunch of bugzillas have complained how noisy the nmi_watchdog is during
> > boot-up especially with its expected failure cases (like virt and bios
> > resource contention).
> 
> Hi Don, this seems nicer.
> 
> Just some trivial comments below:

I like the feedback.  I'll respin the patch and include them.

Thanks,
Don

> 
> > This is my attempt to quiet them down and keep it less confusing for the end
> > user.  What I did is print the message for cpu0 and save it for future
> > comparisions.
> 
> comparisons
> 
> []
> > After the change, it is simlified to:
> 
> simplified
> 
> > diff --git a/kernel/watchdog.c b/kernel/watchdog.c
> []
> > @@ -377,6 +377,14 @@ static int watchdog_nmi_enable(int cpu)
> >  	struct perf_event_attr *wd_attr;
> >  	struct perf_event *event = per_cpu(watchdog_ev, cpu);
> >  
> > +	/*
> > +	 * People like the simple clean cpu node info
> > +	 * on boot.  Simplify the noise from the watchdog
> > +	 * by only printing messages that are different than
> > +	 * what cpu0 displayed
> > +	 */
> 
> This comment could be shortened by a line
> 	/*
> 	 * People like simple and clean cpu node info on boot.
> 	 * Reduce the watchdog noise by only printing messages
> 	 * that are different from what cpu0 displayed.
> 	 */
> 
> > +	static unsigned long err0 = 0;
> 
> Strictly, this doesn't need initialization.
> I think the err0 name is unclear.  Maybe cpu0_err instead;
> 
> I think the use of !cpu and cpu is unclear.
> It's still a cpu, just index 0.
> !cpu should be cpu == 0 and
> cpu should be cpu != 0 too.
> 
> 
> > @@ -390,11 +398,21 @@ static int watchdog_nmi_enable(int cpu)
> >  
> >  	/* Try to register using hardware perf events */
> >  	event = perf_event_create_kernel_counter(wd_attr, cpu, NULL, watchdog_overflow_callback, NULL);
> > +
> > +	/* save cpu0 error for future comparision */
> > +	if (!cpu)
> > +		err0 = (IS_ERR(event) ? PTR_ERR(event) : 0);
> 
> 	if (cpu == 0 && IS_ERR(event))
> 		cpu0_err = PTR_ERR(event);
> 
> > +
> >  	if (!IS_ERR(event)) {
> > -		pr_info("enabled, takes one hw-pmu counter.\n");
> > +		/* only print for cpu0 or different than cpu0 */
> 
> 	or if
> 
> > +		if (!cpu || err0)
> 
> 		if (cpu == 0 || cpu0_err)
> 
> > +			pr_info("enabled, takes one hw-pmu counter.\n");
> >  		goto out_save;
> >  	}
> >  
> > +	/* skip displaying the same error again */
> > +	if ((PTR_ERR(event) == err0) && cpu)
> 
> 	if (cpu > 0 && PTR_ERR(event))
> > +		return PTR_ERR(event);
> >  
> >  	/* vary the KERN level based on the returned errno */
> >  	if (PTR_ERR(event) == -EOPNOTSUPP)
> 
> cheers, Joe
>