Messages in this thread | | | Date | Thu, 16 Jun 2022 06:44:35 -0700 | From | Guenter Roeck <> | Subject | Re: [PATCH V4 17/20] watchdog/dev: Add tracepoints |
| |
On 6/16/22 01:44, Daniel Bristot de Oliveira wrote: > Add a set of tracepoints, enabling the observability of the watchdog > device interactions with user-space. > > The events are: > watchdog:watchdog_open > watchdog:watchdog_close > watchdog:watchdog_start > watchdog:watchdog_stop > watchdog:watchdog_set_timeout > watchdog:watchdog_ping > watchdog:watchdog_nowayout > watchdog:watchdog_set_keep_alive > watchdog:watchdog_keep_alive > watchdog:watchdog_set_pretimeout > watchdog:watchdog_pretimeout > > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc: Jonathan Corbet <corbet@lwn.net> > Cc: Steven Rostedt <rostedt@goodmis.org> > Cc: Ingo Molnar <mingo@redhat.com> > Cc: Thomas Gleixner <tglx@linutronix.de> > Cc: Peter Zijlstra <peterz@infradead.org> > Cc: Will Deacon <will@kernel.org> > Cc: Catalin Marinas <catalin.marinas@arm.com> > Cc: Marco Elver <elver@google.com> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: "Paul E. McKenney" <paulmck@kernel.org> > Cc: Shuah Khan <skhan@linuxfoundation.org> > Cc: Gabriele Paoloni <gpaoloni@redhat.com> > Cc: Juri Lelli <juri.lelli@redhat.com> > Cc: Clark Williams <williams@redhat.com> > Cc: linux-doc@vger.kernel.org > Cc: linux-kernel@vger.kernel.org > Cc: linux-trace-devel@vger.kernel.org > Signed-off-by: Daniel Bristot de Oliveira <bristot@kernel.org> > --- > drivers/watchdog/watchdog_dev.c | 43 ++++++++++- > drivers/watchdog/watchdog_pretimeout.c | 2 + > include/linux/watchdog.h | 7 +- > include/trace/events/watchdog.h | 101 +++++++++++++++++++++++++ > 4 files changed, 143 insertions(+), 10 deletions(-) > create mode 100644 include/trace/events/watchdog.h > > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c > index 54903f3c851e..2f28dc5ab763 100644 > --- a/drivers/watchdog/watchdog_dev.c > +++ b/drivers/watchdog/watchdog_dev.c > @@ -44,6 +44,9 @@ > #include <linux/watchdog.h> /* For watchdog specific items */ > #include <linux/uaccess.h> /* For copy_to_user/put_user/... */ > > +#define CREATE_TRACE_POINTS > +#include <trace/events/watchdog.h> > + > #include "watchdog_core.h" > #include "watchdog_pretimeout.h" > > @@ -130,9 +133,11 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd) > if (watchdog_need_worker(wdd)) { > ktime_t t = watchdog_next_keepalive(wdd); > > - if (t > 0) > + if (t > 0) { > hrtimer_start(&wd_data->timer, t, > HRTIMER_MODE_REL_HARD); > + trace_watchdog_set_keep_alive(wdd, ktime_to_ms(t)); > + } > } else { > hrtimer_cancel(&wd_data->timer); > } > @@ -141,7 +146,7 @@ static inline void watchdog_update_worker(struct watchdog_device *wdd) > static int __watchdog_ping(struct watchdog_device *wdd) > { > struct watchdog_core_data *wd_data = wdd->wd_data; > - ktime_t earliest_keepalive, now; > + ktime_t earliest_keepalive, now, next_keepalive; > int err; > > earliest_keepalive = ktime_add(wd_data->last_hw_keepalive, > @@ -149,14 +154,16 @@ static int __watchdog_ping(struct watchdog_device *wdd) > now = ktime_get(); > > if (ktime_after(earliest_keepalive, now)) { > - hrtimer_start(&wd_data->timer, > - ktime_sub(earliest_keepalive, now), > + next_keepalive = ktime_sub(earliest_keepalive, now); > + hrtimer_start(&wd_data->timer, next_keepalive, > HRTIMER_MODE_REL_HARD); > + trace_watchdog_set_keep_alive(wdd, ktime_to_ms(next_keepalive)); > return 0; > } > > wd_data->last_hw_keepalive = now; > > + trace_watchdog_ping(wdd); > if (wdd->ops->ping) > err = wdd->ops->ping(wdd); /* ping the watchdog */ > else > @@ -215,6 +222,7 @@ static void watchdog_ping_work(struct kthread_work *work) > wd_data = container_of(work, struct watchdog_core_data, work); > > mutex_lock(&wd_data->lock); > + trace_watchdog_keep_alive(wd_data->wdd); > if (watchdog_worker_should_ping(wd_data)) > __watchdog_ping(wd_data->wdd); > mutex_unlock(&wd_data->lock); > @@ -250,6 +258,8 @@ static int watchdog_start(struct watchdog_device *wdd) > > set_bit(_WDOG_KEEPALIVE, &wd_data->status); > > + trace_watchdog_start(wdd); > + > started_at = ktime_get(); > if (watchdog_hw_running(wdd) && wdd->ops->ping) { > err = __watchdog_ping(wdd); > @@ -294,6 +304,7 @@ static int watchdog_stop(struct watchdog_device *wdd) > return -EBUSY; > } > > + trace_watchdog_stop(wdd); > if (wdd->ops->stop) { > clear_bit(WDOG_HW_RUNNING, &wdd->status); > err = wdd->ops->stop(wdd); > @@ -367,6 +378,7 @@ static int watchdog_set_timeout(struct watchdog_device *wdd, > if (watchdog_timeout_invalid(wdd, timeout)) > return -EINVAL; > > + trace_watchdog_set_timeout(wdd, timeout);
The driver has no obligation to set the timeout to the requested value. It might be more valuable to report both the requested and the actual values.
> if (wdd->ops->set_timeout) { > err = wdd->ops->set_timeout(wdd, timeout); > } else { > @@ -399,6 +411,8 @@ static int watchdog_set_pretimeout(struct watchdog_device *wdd, > if (watchdog_pretimeout_invalid(wdd, timeout)) > return -EINVAL; > > + trace_watchdog_set_pretimeout(wdd, timeout); > +
Again, the driver has no obligation to set the timeout to the requested value.
> if (wdd->ops->set_pretimeout && (wdd->info->options & WDIOF_PRETIMEOUT)) > err = wdd->ops->set_pretimeout(wdd, timeout); > else > @@ -430,6 +444,23 @@ static int watchdog_get_timeleft(struct watchdog_device *wdd, > return 0; > } > > +/** > + * watchdog_set_nowayout - set nowaout bit > + * @wdd: The watchdog device to set nowayoutbit > + * @nowayout A boolean on/off switcher > + * > + * If nowayout boolean is true, the nowayout option is set. No action is > + * taken if nowayout is false. > + */ > +void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout) > +{ > + if (nowayout) { > + set_bit(WDOG_NO_WAY_OUT, &wdd->status); > + trace_watchdog_nowayout(wdd); > + } > +} > +EXPORT_SYMBOL(watchdog_set_nowayout); > + > #ifdef CONFIG_WATCHDOG_SYSFS > static ssize_t nowayout_show(struct device *dev, struct device_attribute *attr, > char *buf) > @@ -861,6 +892,8 @@ static int watchdog_open(struct inode *inode, struct file *file) > goto out_clear; > } > > + trace_watchdog_open(wdd); > + > err = watchdog_start(wdd); > if (err < 0) > goto out_mod; > @@ -883,6 +916,7 @@ static int watchdog_open(struct inode *inode, struct file *file) > return stream_open(inode, file); > > out_mod: > + trace_watchdog_close(wdd); > module_put(wd_data->wdd->ops->owner); > out_clear: > clear_bit(_WDOG_DEV_OPEN, &wd_data->status); > @@ -944,6 +978,7 @@ static int watchdog_release(struct inode *inode, struct file *file) > /* make sure that /dev/watchdog can be re-opened */ > clear_bit(_WDOG_DEV_OPEN, &wd_data->status); > > + trace_watchdog_close(wdd); > done: > running = wdd && watchdog_hw_running(wdd); > mutex_unlock(&wd_data->lock); > diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c > index 376a495ab80c..58c391ed2205 100644 > --- a/drivers/watchdog/watchdog_pretimeout.c > +++ b/drivers/watchdog/watchdog_pretimeout.c > @@ -8,6 +8,7 @@ > #include <linux/spinlock.h> > #include <linux/string.h> > #include <linux/watchdog.h> > +#include <trace/events/watchdog.h> > > #include "watchdog_core.h" > #include "watchdog_pretimeout.h" > @@ -107,6 +108,7 @@ void watchdog_notify_pretimeout(struct watchdog_device *wdd) > return; > } > > + trace_watchdog_pretimeout(wdd); > wdd->gov->pretimeout(wdd); > spin_unlock_irqrestore(&pretimeout_lock, flags); > } > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h > index 99660197a36c..11d93407e492 100644 > --- a/include/linux/watchdog.h > +++ b/include/linux/watchdog.h > @@ -139,12 +139,7 @@ static inline bool watchdog_hw_running(struct watchdog_device *wdd) > return test_bit(WDOG_HW_RUNNING, &wdd->status); > } > > -/* Use the following function to set the nowayout feature */ > -static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout) > -{ > - if (nowayout) > - set_bit(WDOG_NO_WAY_OUT, &wdd->status); > -} > +void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout); > > /* Use the following function to stop the watchdog on reboot */ > static inline void watchdog_stop_on_reboot(struct watchdog_device *wdd) > diff --git a/include/trace/events/watchdog.h b/include/trace/events/watchdog.h > new file mode 100644 > index 000000000000..145cd6cfaa02 > --- /dev/null > +++ b/include/trace/events/watchdog.h > @@ -0,0 +1,101 @@ > +/* SPDX-License-Identifier: GPL-2.0 */ > +#undef TRACE_SYSTEM > +#define TRACE_SYSTEM watchdog > + > +#if !defined(_TRACE_WATCHDOG_H) || defined(TRACE_HEADER_MULTI_READ) > +#define _TRACE_WATCHDOG_H > + > +#include <linux/tracepoint.h> > + > +/* > + * These are all events whose sole argument is the watchdog id. > + */ > +DECLARE_EVENT_CLASS(dev_operations_template, > + > + TP_PROTO(struct watchdog_device *wdd), > + > + TP_ARGS(wdd), > + > + TP_STRUCT__entry( > + __field(__u32, id) > + ), > + > + TP_fast_assign( > + __entry->id = wdd->id; > + ), > + > + TP_printk("id=%d", > + __entry->id) > +); > + > +DEFINE_EVENT(dev_operations_template, watchdog_open, > + TP_PROTO(struct watchdog_device *wdd), > + TP_ARGS(wdd)); > + > +DEFINE_EVENT(dev_operations_template, watchdog_close, > + TP_PROTO(struct watchdog_device *wdd), > + TP_ARGS(wdd)); > + > +DEFINE_EVENT(dev_operations_template, watchdog_start, > + TP_PROTO(struct watchdog_device *wdd), > + TP_ARGS(wdd)); > + > +DEFINE_EVENT(dev_operations_template, watchdog_stop, > + TP_PROTO(struct watchdog_device *wdd), > + TP_ARGS(wdd)); > + > +DEFINE_EVENT(dev_operations_template, watchdog_ping, > + TP_PROTO(struct watchdog_device *wdd), > + TP_ARGS(wdd)); > + > +DEFINE_EVENT(dev_operations_template, watchdog_nowayout, > + TP_PROTO(struct watchdog_device *wdd), > + TP_ARGS(wdd)); > + > +DEFINE_EVENT(dev_operations_template, watchdog_keep_alive, > + TP_PROTO(struct watchdog_device *wdd), > + TP_ARGS(wdd)); > + > +DEFINE_EVENT(dev_operations_template, watchdog_pretimeout, > + TP_PROTO(struct watchdog_device *wdd), > + TP_ARGS(wdd)); > + > +/* > + * These are all events with a device ID and a given timeout. > + */ > +DECLARE_EVENT_CLASS(watchdog_timeout_template, > + > + TP_PROTO(struct watchdog_device *wdd, u64 timeout), > + > + TP_ARGS(wdd, timeout), > + > + TP_STRUCT__entry( > + __field(__u32, id) > + __field(__u64, timeout)
Why u64 ? timeout is unsigned long.
> + ), > + > + TP_fast_assign( > + __entry->id = wdd->id; > + __entry->timeout = timeout; > + ), > + > + TP_printk("id=%d timeout=%llus", > + __entry->id, __entry->timeout) > +); > + > +DEFINE_EVENT(watchdog_timeout_template, watchdog_set_timeout, > + TP_PROTO(struct watchdog_device *wdd, u64 timeout), > + TP_ARGS(wdd, timeout)); > + > +DEFINE_EVENT(watchdog_timeout_template, watchdog_set_pretimeout, > + TP_PROTO(struct watchdog_device *wdd, u64 timeout), > + TP_ARGS(wdd, timeout)); > + > +DEFINE_EVENT(watchdog_timeout_template, watchdog_set_keep_alive, > + TP_PROTO(struct watchdog_device *wdd, u64 timeout), > + TP_ARGS(wdd, timeout)); > + > +#endif /* _TRACE_WATCHDOG_H */ > + > +/* This part must be outside protection */ > +#include <trace/define_trace.h>
| |