From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from bh-25.webhostbox.net ([208.91.199.152]:58042 "EHLO bh-25.webhostbox.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751431AbbKURNr (ORCPT ); Sat, 21 Nov 2015 12:13:47 -0500 Subject: Re: [PATCH 0/6] watchdog: add watchdog pretimeout framework To: Vladimir Zapolskiy , Wim Van Sebroeck References: <1448089910-11453-1-git-send-email-vladimir_zapolskiy@mentor.com> Cc: linux-watchdog@vger.kernel.org From: Guenter Roeck Message-ID: <5650A647.5030005@roeck-us.net> Date: Sat, 21 Nov 2015 09:13:43 -0800 MIME-Version: 1.0 In-Reply-To: <1448089910-11453-1-git-send-email-vladimir_zapolskiy@mentor.com> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-watchdog-owner@vger.kernel.org List-Id: linux-watchdog@vger.kernel.org On 11/20/2015 11:11 PM, Vladimir Zapolskiy wrote: > The change adds a simple watchdog pretimeout framework infrastructure, > its purpose is to allow users to select a desired handling of watchdog > pretimeout events, which may be generated by a watchdog driver. > > The idea of adding this kind of a framework appeared after reviewing > several attempts to add hardcoded pretimeout event handling to some > watchdog driver and after a discussion with Guenter, see > https://lkml.org/lkml/2015/11/4/346 > > By design every watchdog pretimeout governor may be compiled as a > kernel module, a user selects a default watchdog pretimeout > governor during compilation stage and can select another governor in > runtime. > > Watchdogs with WDIOF_PRETIMEOUT capability now have two device > attributes in sysfs: read/write pretimeout_governor attribute and read > only pretimeout_available_governors attribute. > > To throw a pretimeout event for further processing a watchdog driver > should call exported watchdog_notify_pretimeout(wdd) interface. > > In addition to the framework a number of simple watchdog pretimeout > governors are added for review. > Hi Vladimir, Excellent idea. I would suggest to simplify it a bit, though. Use only a single configuration flag, and bundle all governors together with the framework. The governor code isn't that large that it warrants separate modules, much less separate configuration flags. Keep in mind that this will ultimately be used by distributions, and for those an a-b-c choice is always bad. We'll have to find something else to specify the default governor. Maybe make panic the primary default, and support a module parameter to change it. I don't think we should have per-watchdog sysfs attributes to change the governor. A global set of attributes would make more sense. Maybe this is possible through /proc/sys/, or just set it once with a module parameter. If a watchdog driver actually supports pretimeout is a different question. This should simplify the code a lot, since there would always be a well known governor to execute on a pretimeout. If we have to use workqueues, it would have to run on the highest possible priority. I think it would be better to determine on a per-governor basis if a workqueue is needed (eg for userspace events). We don't need one for panic, or for noop. Otherwise we run the risk that the work never executes for the same reason that caused the watchdog to expire in the first place. Does this make sense ? Thanks, Guenter > Vladimir Zapolskiy (6): > watchdog: add watchdog pretimeout framework > watchdog: pretimeout: add noop pretimeout governor > watchdog: pretimeout: add panic pretimeout governor > watchdog: pretimeout: add userspace notifier pretimeout governor > watchdog: pretimeout: add device specific notifier pretimeout governor > watchdog: pretimeout: add ping pretimeout governor > > drivers/watchdog/Kconfig | 91 +++++++++ > drivers/watchdog/Makefile | 10 +- > drivers/watchdog/pretimeout_device.c | 49 +++++ > drivers/watchdog/pretimeout_noop.c | 49 +++++ > drivers/watchdog/pretimeout_panic.c | 49 +++++ > drivers/watchdog/pretimeout_ping.c | 48 +++++ > drivers/watchdog/pretimeout_userspace.c | 49 +++++ > drivers/watchdog/watchdog_core.c | 14 +- > drivers/watchdog/watchdog_pretimeout.c | 348 ++++++++++++++++++++++++++++++++ > drivers/watchdog/watchdog_pretimeout.h | 31 +++ > include/linux/watchdog.h | 12 ++ > 11 files changed, 747 insertions(+), 3 deletions(-) > create mode 100644 drivers/watchdog/pretimeout_device.c > create mode 100644 drivers/watchdog/pretimeout_noop.c > create mode 100644 drivers/watchdog/pretimeout_panic.c > create mode 100644 drivers/watchdog/pretimeout_ping.c > create mode 100644 drivers/watchdog/pretimeout_userspace.c > create mode 100644 drivers/watchdog/watchdog_pretimeout.c > create mode 100644 drivers/watchdog/watchdog_pretimeout.h >