linux-kernel - Re: [PATCH] kernel/watchdog.c: Do not hardcode CPU 0 as the initial thread

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]

Date:   Tue, 29 Nov 2016 16:07:29 -0500
From:   Don Zickus <dzickus@...hat.com>
To:     Prarit Bhargava <prarit@...hat.com>
Cc:     linux-kernel@...r.kernel.org, Borislav Petkov <bp@...e.de>,
        Tejun Heo <tj@...nel.org>,
        Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>,
        Thomas Gleixner <tglx@...utronix.de>,
        Andi Kleen <ak@...ux.intel.com>,
        Joshua Hunt <johunt@...mai.com>,
        Ingo Molnar <mingo@...nel.org>,
        Babu Moger <babu.moger@...cle.com>
Subject: Re: [PATCH] kernel/watchdog.c: Do not hardcode CPU 0 as the initial
 thread

On Tue, Nov 29, 2016 at 08:15:21AM -0500, Prarit Bhargava wrote:
> When CONFIG_BOOTPARAM_HOTPLUG_CPU0 is enabled, the socket containing the
> boot cpu can be replaced.  During the hot add event, the message
> 
> NMI watchdog: enabled on all CPUs, permanently consumes one hw-PMU counter.
> 
> is output implying that the NMI watchdog was disabled at some point.  This
> is not the case and the message has caused confusion for users of systems
> that support the removal of the boot cpu socket.
> 
> The watchdog code is coded to assume that cpu 0 is always the first cpu to
> initialize the watchdog, and the last to stop its watchdog thread.  That
> is not the case for initializing if cpu 0 has been removed and added.  The
> removal case has never been correct because the smpboot code will remove
> the watchdog threads starting with the lowest cpu number.
> 
> This patch adds watchdog_cpus to track the number of cpus with active NMI
> watchdog threads so that the first and last thread can be used to set and
> clear the value of firstcpu_err.  firstcpu_err is set when the first
> watchdog thread is enabled, and cleared when the last watchdog thread is
> disabled.
> 
> This patch is based on top of linux-next akpm-base.

Most of this patch is just renaming cpu0_err to firstcpu_err.  The main
trick is the global atomic watchdog_cpus and the small cleanup in
watchdog_nmi_disable.

Looks good to me.  Will try to test and verify things later this week.

Cheers,
Don

> 
> Signed-off-by: Prarit Bhargava <prarit@...hat.com>
> Cc: Borislav Petkov <bp@...e.de>
> Cc: Tejun Heo <tj@...nel.org>
> Cc: Don Zickus <dzickus@...hat.com>
> Cc: Hidehiro Kawai <hidehiro.kawai.ez@...achi.com>
> Cc: Thomas Gleixner <tglx@...utronix.de>
> Cc: Andi Kleen <ak@...ux.intel.com>
> Cc: Joshua Hunt <johunt@...mai.com>
> Cc: Ingo Molnar <mingo@...nel.org>
> Cc: Babu Moger <babu.moger@...cle.com>
> ---
>  kernel/watchdog_hld.c |   25 +++++++++++++++----------
>  1 file changed, 15 insertions(+), 10 deletions(-)
> 
> diff --git a/kernel/watchdog_hld.c b/kernel/watchdog_hld.c
> index 84016c8aee6b..30761f7504ef 100644
> --- a/kernel/watchdog_hld.c
> +++ b/kernel/watchdog_hld.c
> @@ -134,12 +134,14 @@ static void watchdog_overflow_callback(struct perf_event *event,
>   * Reduce the watchdog noise by only printing messages
>   * that are different from what cpu0 displayed.
>   */
> -static unsigned long cpu0_err;
> +static unsigned long firstcpu_err;
> +static atomic_t watchdog_cpus;
>  
>  int watchdog_nmi_enable(unsigned int cpu)
>  {
>  	struct perf_event_attr *wd_attr;
>  	struct perf_event *event = per_cpu(watchdog_ev, cpu);
> +	int firstcpu = 0;
>  
>  	/* nothing to do if the hard lockup detector is disabled */
>  	if (!(watchdog_enabled & NMI_WATCHDOG_ENABLED))
> @@ -153,19 +155,22 @@ int watchdog_nmi_enable(unsigned int cpu)
>  	if (event != NULL)
>  		goto out_enable;
>  
> +	if (atomic_inc_return(&watchdog_cpus) == 1)
> +		firstcpu = 1;
> +
>  	wd_attr = &wd_hw_attr;
>  	wd_attr->sample_period = hw_nmi_get_sample_period(watchdog_thresh);
>  
>  	/* 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 == 0 && IS_ERR(event))
> -		cpu0_err = PTR_ERR(event);
> +	/* save the first cpu's error for future comparision */
> +	if (firstcpu && IS_ERR(event))
> +		firstcpu_err = PTR_ERR(event);
>  
>  	if (!IS_ERR(event)) {
> -		/* only print for cpu0 or different than cpu0 */
> -		if (cpu == 0 || cpu0_err)
> +		/* only print for the first cpu initialized */
> +		if (firstcpu || firstcpu_err)
>  			pr_info("enabled on all CPUs, permanently consumes one hw-PMU counter.\n");
>  		goto out_save;
>  	}
> @@ -183,7 +188,7 @@ int watchdog_nmi_enable(unsigned int cpu)
>  	smp_mb__after_atomic();
>  
>  	/* skip displaying the same error again */
> -	if (cpu > 0 && (PTR_ERR(event) == cpu0_err))
> +	if (!firstcpu && (PTR_ERR(event) == firstcpu_err))
>  		return PTR_ERR(event);
>  
>  	/* vary the KERN level based on the returned errno */
> @@ -219,9 +224,9 @@ void watchdog_nmi_disable(unsigned int cpu)
>  
>  		/* should be in cleanup, but blocks oprofile */
>  		perf_event_release_kernel(event);
> -	}
> -	if (cpu == 0) {
> +
>  		/* watchdog_nmi_enable() expects this to be zero initially. */
> -		cpu0_err = 0;
> +		if (atomic_dec_and_test(&watchdog_cpus))
> +			firstcpu_err = 0;
>  	}
>  }
> -- 
> 1.7.9.3
>