linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Ulrich Obergfell <uobergfe@redhat.com>
To: linux-kernel@vger.kernel.org
Cc: akpm@linux-foundation.org, dzickus@redhat.com,
	atomlin@redhat.com, uobergfe@redhat.com, jolsa@kernel.org,
	mhocko@suse.cz, eranian@google.com, cmetcalf@ezchip.com,
	fweisbec@gmail.com
Subject: [PATCH 0/4] watchdog: avoid races in watchdog_nmi_{en|disable} functions
Date: Sat,  1 Aug 2015 14:49:21 +0200	[thread overview]
Message-ID: <1438433365-2979-1-git-send-email-uobergfe@redhat.com> (raw)

Originally watchdog_nmi_enable(cpu) and watchdog_nmi_disable(cpu) were
only called in watchdog thread context. However, the following commits
utilize these functions outside of watchdog thread context too.

  commit 9809b18fcf6b8d8ec4d3643677345907e6b50eca
  Author: Michal Hocko <mhocko@suse.cz>
  Date:   Tue Sep 24 15:27:30 2013 -0700

      watchdog: update watchdog_thresh properly

  commit b3738d29323344da3017a91010530cf3a58590fc
  Author: Stephane Eranian <eranian@google.com>
  Date:   Mon Nov 17 20:07:03 2014 +0100

      watchdog: Add watchdog enable/disable all functions

Hence, it is now possible that these functions execute concurrently with
the same 'cpu' argument. This concurrency is problematic because per-cpu
'watchdog_ev' can be accessed/modified without adequate synchronization.

The patch series aims to address the above problem. However, instead of
introducing locks to protect per-cpu 'watchdog_ev' a different approach
is taken: Invoke these functions by parking and unparking the watchdog
threads (to ensure they are always called in watchdog thread context).

  static struct smp_hotplug_thread watchdog_threads = {
           ...
          .park   = watchdog_disable, // calls watchdog_nmi_disable()
          .unpark = watchdog_enable,  // calls watchdog_nmi_enable()
  };

Both previously mentioned commits call these functions in a similar way
and thus in principle contain some duplicate code. The patch series also
avoids this duplication by providing a commonly usable mechanism.

- Patch 1/4 introduces the watchdog_{park|unpark}_threads functions that
  park/unpark all watchdog threads specified in 'watchdog_cpumask'. They
  are intended to be called inside of kernel/watchdog.c only.

- Patch 2/4 introduces the watchdog_{suspend|resume} functions which can
  be utilized by external callers to deactivate the hard and soft lockup
  detector temporarily.

- Patch 3/4 utilizes watchdog_{park|unpark}_threads to replace some code
  that was introduced by commit 9809b18fcf6b8d8ec4d3643677345907e6b50eca.

- Patch 4/4 utilizes watchdog_{suspend|resume} to replace some code that
  was introduced by commit b3738d29323344da3017a91010530cf3a58590fc.

A few corner cases should be mentioned here for completeness.

- kthread_park() of watchdog/N could hang if cpu N is already locked up.
  However, if watchdog is enabled the lockup will be detected anyway.

- kthread_unpark() of watchdog/N could hang if cpu N got locked up after
  kthread_park(). The occurrence of this scenario should be _very_ rare
  in practice, in particular because it is not expected that temporary
  deactivation will happen frequently, and if it happens at all it is
  expected that the duration of deactivation will be short.

Ulrich Obergfell (4):
  watchdog: introduce watchdog_park_threads() and
    watchdog_unpark_threads()
  watchdog: introduce watchdog_suspend() and watchdog_resume()
  watchdog: use park/unpark functions in update_watchdog_all_cpus()
  watchdog: use suspend/resume interface in fixup_ht_bug()

 arch/x86/kernel/cpu/perf_event_intel.c |   9 +-
 include/linux/nmi.h                    |   2 +
 include/linux/watchdog.h               |   8 --
 kernel/watchdog.c                      | 157 +++++++++++++++++++--------------
 4 files changed, 100 insertions(+), 76 deletions(-)

-- 
1.7.11.7


             reply	other threads:[~2015-08-01 12:46 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2015-08-01 12:49 Ulrich Obergfell [this message]
2015-08-01 12:49 ` [PATCH 1/4] watchdog: introduce watchdog_park_threads() and watchdog_unpark_threads() Ulrich Obergfell
2015-08-04 13:26   ` Michal Hocko
2015-08-04 15:20     ` Ulrich Obergfell
2015-08-01 12:49 ` [PATCH 2/4] watchdog: introduce watchdog_suspend() and watchdog_resume() Ulrich Obergfell
2015-08-01 14:04   ` Guenter Roeck
2015-08-01 14:39     ` Ulrich Obergfell
2015-08-01 14:47       ` Guenter Roeck
2015-08-04 22:58   ` Andrew Morton
2015-08-05  8:10     ` Ulrich Obergfell
2015-08-01 12:49 ` [PATCH 3/4] watchdog: use park/unpark functions in update_watchdog_all_cpus() Ulrich Obergfell
2015-08-01 12:49 ` [PATCH 4/4] watchdog: use suspend/resume interface in fixup_ht_bug() Ulrich Obergfell
2015-08-04 13:31   ` Michal Hocko
2015-08-04 14:27     ` Don Zickus
2015-08-04 14:44       ` Michal Hocko
2015-08-04 14:59       ` Ulrich Obergfell
2015-08-05 22:17   ` Andrew Morton
2015-08-02 10:54 ` [PATCH 0/4] watchdog: avoid races in watchdog_nmi_{en|disable} functions Aaron Tomlin
2015-08-02 22:14 ` Jiri Olsa

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1438433365-2979-1-git-send-email-uobergfe@redhat.com \
    --to=uobergfe@redhat.com \
    --cc=akpm@linux-foundation.org \
    --cc=atomlin@redhat.com \
    --cc=cmetcalf@ezchip.com \
    --cc=dzickus@redhat.com \
    --cc=eranian@google.com \
    --cc=fweisbec@gmail.com \
    --cc=jolsa@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mhocko@suse.cz \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).