Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Tue, 30 Oct 2018 07:51:26 +0100 (CET)
From:   Thomas Gleixner <tglx@linutronix.de>
To:     Laurence Oberman <loberman@redhat.com>
cc:     linux-kernel@vger.kernel.org, rdunlap@infradead.org
Subject: Re: [PATCH] V6 init/main.c Enable watchdog_thresh control from kernel
 line To: loberman@redhat.com
In-Reply-To: <1540410119-21120-1-git-send-email-loberman@redhat.com>
Message-ID: <alpine.DEB.2.21.1810300713320.5984@nanos.tec.linutronix.de>
References: <1540410119-21120-1-git-send-email-loberman@redhat.com>
User-Agent: Alpine 2.21 (DEB 202 2017-01-01)
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

Laurence,

On Wed, 24 Oct 2018, Laurence Oberman wrote:

your subject line reads a bit strange:

Subject: [PATCH] V6 init/main.c Enable watchdog_thresh control from kernel line To: loberman@redhat.com

Aside of that extra 'To:...', please move the V6 inside the square brackets
together with PATCH so tools can strip off the whole thing.

Also please refrain from using file path as a prefix. It's sufficient to
use 'init' and please add a colon after the prefix to separate it from the
short log string. 'kernel line' reads strange, that should be 'kernel
command line'. 

> Both graphics and serial consoles are exposed to hard lockups
> when handling a large amount of messaging. The kernel watchdog_thresh
> parameter up to now has not been available to be set on the kernel line for
> early boot.
> This patch allows the setting of watchdog_thresh to be increased
> when needed to avoid the hard lockups in the console code.

git grep 'This patch' Documentation/process/

> Signed-off-by: Laurence Oberman <loberman@redhat.com>
> Acked-by: Randy Dunlap <rdunlap@infradead.org>
> ---
>  Documentation/admin-guide/kernel-parameters.txt |  7 +++++++
>  init/main.c                                     | 11 +++++++++++
>  2 files changed, 18 insertions(+)
> 
> diff --git a/Documentation/admin-guide/kernel-parameters.txt b/Documentation/admin-guide/kernel-parameters.txt
> index 4cdcd1a..102382f 100644
> --- a/Documentation/admin-guide/kernel-parameters.txt
> +++ b/Documentation/admin-guide/kernel-parameters.txt
> @@ -4932,6 +4932,13 @@
>  			or other driver-specific files in the
>  			Documentation/watchdog/ directory.
>  
> +	watchdog_thresh=
> +			This parameter allows early boot to change the
> +			value of the watchdog timeout threshold from the default
> +			of 10 seconds to avoid hard lockups.  Example:
> +			watchdog_thresh=30
> +			Default: 10

  Describing a parameter with 'This parameter' is pointless. The 'early
  boot' extra is not really helpful either as the kernel command line
  parameters are evaluated during early boot.

  Changing this parameter does not avoid hard lockups, really. It changes
  the time which has to elapse for a lockup to be detected.

  Aside of that it does not only affect the hard lockup detector it also
  affects the soft lockup detector.

  You also fail to mention that setting this to 0 disables both lockup
  detectors completely. Something like this perhaps:

	watchdog_thresh=
			[KNL]
			Set the hard lockup detector stall duration
			threshold in seconds. The soft lockup detector
			threshold is set to twice the value. A value of 0
			disables both lockup detectors. Default is 10
			seconds.

> --- a/init/main.c
> +++ b/init/main.c
> @@ -1038,6 +1038,17 @@ static int __init set_debug_rodata(char *str)
>  __setup("rodata=", set_debug_rodata);
>  #endif
>  
> +#ifdef CONFIG_LOCKUP_DETECTOR
> +extern int watchdog_thresh;
> +
> +static int __init watchdog_thresh_setup(char *str)
> +{
> +	get_option(&str, &watchdog_thresh);
> +	return 1;
> +}
> +__setup("watchdog_thresh=", watchdog_thresh_setup);
> +#endif

Why are you adding this to init/main.c?

This really belongs into kernel/watchdog.c which also avoids the ifdeffery
and the ugly extern. Then the subject line becomes something like this:

 [PATCH V$N] watchdog/core: Add watchdog_thresh command line parameter

Thanks,

	tglx