linux-kernel - Re: [PATCH v8 2/3] watchdog: add watchdog

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: <2132240307.4651379.1429690855807.JavaMail.zimbra@redhat.com>
Date:	Wed, 22 Apr 2015 04:20:55 -0400 (EDT)
From:	Ulrich Obergfell <uobergfe@...hat.com>
To:	Chris Metcalf <cmetcalf@...hip.com>
Cc:	Frederic Weisbecker <fweisbec@...il.com>,
	Don Zickus <dzickus@...hat.com>,
	Ingo Molnar <mingo@...nel.org>,
	Andrew Morton <akpm@...ux-foundation.org>,
	Andrew Jones <drjones@...hat.com>,
	chai wen <chaiw.fnst@...fujitsu.com>,
	Fabian Frederick <fabf@...net.be>,
	Aaron Tomlin <atomlin@...hat.com>,
	Ben Zhang <benzh@...omium.org>,
	Christoph Lameter <cl@...ux.com>,
	Gilad Ben-Yossef <gilad@...yossef.com>,
	Steven Rostedt <rostedt@...dmis.org>,
	linux-kernel@...r.kernel.org, Jonathan Corbet <corbet@....net>,
	linux-doc@...r.kernel.org, Thomas Gleixner <tglx@...utronix.de>,
	Peter Zijlstra <peterz@...radead.org>
Subject: Re: [PATCH v8 2/3] watchdog: add watchdog_cpumask sysctl to assist
 nohz


Chris,

in principle the change looks o.k. to me, even though I'm not really familiar
with the watchdog_nmi_disable_all() and watchdog_nmi_enable_all() functions.
It is my understanding that those functions are only called once via 'initcall'
early during kernel startup as shown in the following flow of execution:

kernel_init
{
  kernel_init_freeable
  {
    lockup_detector_init
    {
      cpumask_andnot(watchdog_cpumask, cpu_possible_mask,tick_nohz_full_mask)
      watchdog_enable_all_cpus
        smpboot_register_percpu_thread(&watchdog_threads)
        smpboot_update_cpumask_percpu_thread(&watchdog_threads,watchdog_cpumask)
          // here we make sure that watchdog threads don't run on nohz_full CPUs
          // only the watchdog threads of housekeeping CPUs keep on running
    }

    do_basic_setup
      do_initcalls
        do_initcall_level
          do_one_initcall
            fixup_ht_bug // subsys_initcall(fixup_ht_bug)
            {
              watchdog_nmi_disable_all
                // here we disable NMI watchdog only on housekeeping CPUs
                for_each_cpu_and(cpu,cpu_online_mask,watchdog_cpumask)
                    watchdog_nmi_disable

              watchdog_nmi_enable_all
                // here we enable NMI watchdog only on housekeeping CPUs
                for_each_cpu_and(cpu,cpu_online_mask,watchdog_cpumask)
                    watchdog_nmi_enable
            }
  }
}

It seems crucial that lockup_detector_init() is executed before fixup_ht_bug().


Regards,

Uli


On 04/16/2015 06:46 AM, Ulrich Obergfell wrote:
> if a user changes watchdog parameters in /proc/sys/kernel, the watchdog threads
> are not stopped and restarted in all cases. Parameters can also be changed 'on
> the fly', for example like 'watchdog_thresh' in the following flow of execution:
>
>    proc_watchdog_thresh
>      proc_watchdog_update
>        if (watchdog_enabled && watchdog_thresh)
>            watchdog_enable_all_cpus
>              if (!watchdog_running) {
>                  // watchdog threads are already running so we don't get here
>              } else {
>                  update_watchdog_all_cpus
>                    for_each_online_cpu <-----------------------------.
>                      update_watchdog                                 |
>                        watchdog_nmi_disable                          |
>                        watchdog_nmi_enable                           |
>              }                                                       |
>                                                                      |
> I think we would not want to call watchdog_nmi_enable() for each_online_  CPU,
> but rather for each CPU that has an_unparked_  watchdog thread (i.e. where the
> watchdog mechanism is actually enabled).

How about something like this?  I'll fold it into v9 of the patchset.
Thanks!

diff --git a/kernel/watchdog.c b/kernel/watchdog.c
index 0c5a37cdbedd..a4e1c9a2e769 100644
--- a/kernel/watchdog.c
+++ b/kernel/watchdog.c
@@ -61,6 +61,10 @@ static cpumask_var_t watchdog_cpumask_for_smpboot;
  static cpumask_var_t watchdog_cpumask;
  unsigned long *watchdog_cpumask_bits;
  
+/* Helper for online, unparked cpus. */
+#define for_each_watchdog_cpu(cpu) \
+	for_each_cpu_and((cpu), cpu_online_mask, watchdog_cpumask)
+
  static int __read_mostly watchdog_running;
  static u64 __read_mostly sample_period;
  
@@ -209,7 +213,7 @@ void touch_all_softlockup_watchdogs(void)
  	 * do we care if a 0 races with a timestamp?
  	 * all it means is the softlock check starts one cycle later
  	 */
-	for_each_online_cpu(cpu)
+	for_each_watchdog_cpu(cpu)
  		per_cpu(watchdog_touch_ts, cpu) = 0;
  }
  
@@ -616,7 +620,7 @@ void watchdog_nmi_enable_all(void)
  		return;
  
  	get_online_cpus();
-	for_each_online_cpu(cpu)
+	for_each_watchdog_cpu(cpu)
  		watchdog_nmi_enable(cpu);
  	put_online_cpus();
  }
@@ -629,7 +633,7 @@ void watchdog_nmi_disable_all(void)
  		return;
  
  	get_online_cpus();
-	for_each_online_cpu(cpu)
+	for_each_watchdog_cpu(cpu)
  		watchdog_nmi_disable(cpu);
  	put_online_cpus();
  }
@@ -688,7 +692,7 @@ static void update_watchdog_all_cpus(void)
  	int cpu;
  
  	get_online_cpus();
-	for_each_online_cpu(cpu)
+	for_each_watchdog_cpu(cpu)
  		update_watchdog(cpu);
  	put_online_cpus();
  }

-- 
Chris Metcalf, EZChip Semiconductor
http://www.ezchip.com

--
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/