| Message ID | 1479531014-25264-2-git-send-email-john.stultz@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
* John Stultz <john.stultz@linaro.org> wrote: > @@ -222,7 +226,7 @@ static int alarmtimer_suspend(struct device *dev) > ktime_t min, now; > unsigned long flags; > struct rtc_device *rtc; > - int i; > + int i, type = 0; > int ret; > > spin_lock_irqsave(&freezer_delta_lock, flags); > @@ -247,8 +251,10 @@ static int alarmtimer_suspend(struct device *dev) > if (!next) > continue; > delta = ktime_sub(next->expires, base->gettime()); > - if (!min.tv64 || (delta.tv64 < min.tv64)) > + if (!min.tv64 || (delta.tv64 < min.tv64)) { > min = delta; > + type = i; > + } > } > if (min.tv64 == 0) > return 0; > @@ -264,6 +270,8 @@ static int alarmtimer_suspend(struct device *dev) > now = rtc_tm_to_ktime(tm); > now = ktime_add(now, min); > > + trace_alarmtimer_suspend(now, type); > + > /* Set alarm, if in the past reject suspend briefly to handle */ > ret = rtc_timer_start(rtc, &rtctimer, now, ktime_set(0, 0)); > if (ret < 0) Hm, there's a weirdness here: if freezer_delta != 0 when alarmtimer_suspend() is called then type might be '0', although it's not alarm_bases[0] this value is picked up from. Wouldn't it be better to initialize 'type' to -1 instead? (And rename it to min_type or so.) That would disambiguate the freezer_delta special case in the trace. (Unless I missed something.) Thanks, Ingo
Hi Ingo, On 21 November 2016 at 16:13, Ingo Molnar <mingo@kernel.org> wrote: > > * John Stultz <john.stultz@linaro.org> wrote: > >> @@ -222,7 +226,7 @@ static int alarmtimer_suspend(struct device *dev) >> ktime_t min, now; >> unsigned long flags; >> struct rtc_device *rtc; >> - int i; >> + int i, type = 0; >> int ret; >> >> spin_lock_irqsave(&freezer_delta_lock, flags); >> @@ -247,8 +251,10 @@ static int alarmtimer_suspend(struct device *dev) >> if (!next) >> continue; >> delta = ktime_sub(next->expires, base->gettime()); >> - if (!min.tv64 || (delta.tv64 < min.tv64)) >> + if (!min.tv64 || (delta.tv64 < min.tv64)) { >> min = delta; >> + type = i; >> + } >> } >> if (min.tv64 == 0) >> return 0; >> @@ -264,6 +270,8 @@ static int alarmtimer_suspend(struct device *dev) >> now = rtc_tm_to_ktime(tm); >> now = ktime_add(now, min); >> >> + trace_alarmtimer_suspend(now, type); >> + >> /* Set alarm, if in the past reject suspend briefly to handle */ >> ret = rtc_timer_start(rtc, &rtctimer, now, ktime_set(0, 0)); >> if (ret < 0) > > Hm, there's a weirdness here: if freezer_delta != 0 when alarmtimer_suspend() is > called then type might be '0', although it's not alarm_bases[0] this value is > picked up from. > > Wouldn't it be better to initialize 'type' to -1 instead? (And rename it to > min_type or so.) Make sense. I will send out new patch to fix this. Thanks for your comments. > > That would disambiguate the freezer_delta special case in the trace. > > (Unless I missed something.) > > Thanks, > > Ingo -- Baolin.wang Best Regards
On Mon, 21 Nov 2016, Baolin Wang wrote: > On 21 November 2016 at 16:13, Ingo Molnar <mingo@kernel.org> wrote: > > Hm, there's a weirdness here: if freezer_delta != 0 when alarmtimer_suspend() is > > called then type might be '0', although it's not alarm_bases[0] this value is > > picked up from. > > > > Wouldn't it be better to initialize 'type' to -1 instead? (And rename it to > > min_type or so.) > > Make sense. I will send out new patch to fix this. Thanks for your comments. Can you please fix the subject line while at it? 'trcepoints' does not look right Thanks, tglx
On 21 November 2016 at 16:56, Thomas Gleixner <tglx@linutronix.de> wrote: > On Mon, 21 Nov 2016, Baolin Wang wrote: >> On 21 November 2016 at 16:13, Ingo Molnar <mingo@kernel.org> wrote: >> > Hm, there's a weirdness here: if freezer_delta != 0 when alarmtimer_suspend() is >> > called then type might be '0', although it's not alarm_bases[0] this value is >> > picked up from. >> > >> > Wouldn't it be better to initialize 'type' to -1 instead? (And rename it to >> > min_type or so.) >> >> Make sense. I will send out new patch to fix this. Thanks for your comments. > > Can you please fix the subject line while at it? 'trcepoints' does not look right Sorry for mistake, I will fix that. Thanks. -- Baolin.wang Best Regards
diff --git a/include/trace/events/alarmtimer.h b/include/trace/events/alarmtimer.h new file mode 100644 index 0000000..61ea556 --- /dev/null +++ b/include/trace/events/alarmtimer.h @@ -0,0 +1,92 @@ +#undef TRACE_SYSTEM +#define TRACE_SYSTEM alarmtimer + +#if !defined(_TRACE_ALARMTIMER_H) || defined(TRACE_HEADER_MULTI_READ) +#define _TRACE_ALARMTIMER_H + +#include <linux/alarmtimer.h> +#include <linux/rtc.h> +#include <linux/tracepoint.h> + +TRACE_DEFINE_ENUM(ALARM_REALTIME); +TRACE_DEFINE_ENUM(ALARM_BOOTTIME); + +#define show_alarm_type(type) __print_flags(type, " | ", \ + { 1 << ALARM_REALTIME, "REALTIME" }, \ + { 1 << ALARM_BOOTTIME, "BOOTTIME" }) + +TRACE_EVENT(alarmtimer_suspend, + + TP_PROTO(ktime_t expires, int flag), + + TP_ARGS(expires, flag), + + TP_STRUCT__entry( + __field(s64, expires) + __field(unsigned char, alarm_type) + ), + + TP_fast_assign( + __entry->expires = expires.tv64; + __entry->alarm_type = flag; + ), + + TP_printk("alarmtimer type:%s expires:%llu", + show_alarm_type((1 << __entry->alarm_type)), + __entry->expires + ) +); + +DECLARE_EVENT_CLASS(alarm_class, + + TP_PROTO(struct alarm *alarm, ktime_t now), + + TP_ARGS(alarm, now), + + TP_STRUCT__entry( + __field(void *, alarm) + __field(unsigned char, alarm_type) + __field(s64, expires) + __field(s64, now) + ), + + TP_fast_assign( + __entry->alarm = alarm; + __entry->alarm_type = alarm->type; + __entry->expires = alarm->node.expires.tv64; + __entry->now = now.tv64; + ), + + TP_printk("alarmtimer:%p type:%s expires:%llu now:%llu", + __entry->alarm, + show_alarm_type((1 << __entry->alarm_type)), + __entry->expires, + __entry->now + ) +); + +DEFINE_EVENT(alarm_class, alarmtimer_fired, + + TP_PROTO(struct alarm *alarm, ktime_t now), + + TP_ARGS(alarm, now) +); + +DEFINE_EVENT(alarm_class, alarmtimer_start, + + TP_PROTO(struct alarm *alarm, ktime_t now), + + TP_ARGS(alarm, now) +); + +DEFINE_EVENT(alarm_class, alarmtimer_cancel, + + TP_PROTO(struct alarm *alarm, ktime_t now), + + TP_ARGS(alarm, now) +); + +#endif /* _TRACE_ALARMTIMER_H */ + +/* This part must be outside protection */ +#include <trace/define_trace.h> diff --git a/kernel/time/alarmtimer.c b/kernel/time/alarmtimer.c index 12dd190..8084e0c 100644 --- a/kernel/time/alarmtimer.c +++ b/kernel/time/alarmtimer.c @@ -26,6 +26,9 @@ #include <linux/workqueue.h> #include <linux/freezer.h> +#define CREATE_TRACE_POINTS +#include <trace/events/alarmtimer.h> + /** * struct alarm_base - Alarm timer bases * @lock: Lock for syncrhonized access to the base @@ -194,6 +197,7 @@ static enum hrtimer_restart alarmtimer_fired(struct hrtimer *timer) } spin_unlock_irqrestore(&base->lock, flags); + trace_alarmtimer_fired(alarm, base->gettime()); return ret; } @@ -222,7 +226,7 @@ static int alarmtimer_suspend(struct device *dev) ktime_t min, now; unsigned long flags; struct rtc_device *rtc; - int i; + int i, type = 0; int ret; spin_lock_irqsave(&freezer_delta_lock, flags); @@ -247,8 +251,10 @@ static int alarmtimer_suspend(struct device *dev) if (!next) continue; delta = ktime_sub(next->expires, base->gettime()); - if (!min.tv64 || (delta.tv64 < min.tv64)) + if (!min.tv64 || (delta.tv64 < min.tv64)) { min = delta; + type = i; + } } if (min.tv64 == 0) return 0; @@ -264,6 +270,8 @@ static int alarmtimer_suspend(struct device *dev) now = rtc_tm_to_ktime(tm); now = ktime_add(now, min); + trace_alarmtimer_suspend(now, type); + /* Set alarm, if in the past reject suspend briefly to handle */ ret = rtc_timer_start(rtc, &rtctimer, now, ktime_set(0, 0)); if (ret < 0) @@ -342,6 +350,8 @@ void alarm_start(struct alarm *alarm, ktime_t start) alarmtimer_enqueue(base, alarm); hrtimer_start(&alarm->timer, alarm->node.expires, HRTIMER_MODE_ABS); spin_unlock_irqrestore(&base->lock, flags); + + trace_alarmtimer_start(alarm, base->gettime()); } EXPORT_SYMBOL_GPL(alarm_start); @@ -390,6 +400,8 @@ int alarm_try_to_cancel(struct alarm *alarm) if (ret >= 0) alarmtimer_dequeue(base, alarm); spin_unlock_irqrestore(&base->lock, flags); + + trace_alarmtimer_cancel(alarm, base->gettime()); return ret; } EXPORT_SYMBOL_GPL(alarm_try_to_cancel);