| 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
| ||
|
Date: Wed, 21 Sep 2016 11:25:25 +0800
From: Baolin Wang <baolin.wang@...aro.org>
To: Thomas Gleixner <tglx@...utronix.de>
Cc: Alessandro Zummo <a.zummo@...ertech.it>,
Alexandre Belloni <alexandre.belloni@...e-electrons.com>,
Steven Rostedt <rostedt@...dmis.org>,
Ingo Molnar <mingo@...hat.com>,
John Stultz <john.stultz@...aro.org>,
Mark Brown <broonie@...nel.org>,
LKML <linux-kernel@...r.kernel.org>, rtc-linux@...glegroups.com
Subject: Re: [PATCH 2/2] time: alarmtimer: Add the trcepoints for alarmtimer
On 21 September 2016 at 06:27, Thomas Gleixner <tglx@...utronix.de> wrote:
> On Sun, 18 Sep 2016, Baolin Wang wrote:
>> +DECLARE_EVENT_CLASS(alarm_setting,
>
> What is alarm_setting? Without looking at the DEFINE_EVENT which uses this
> I cannot decode the meaning.
Will rename it.
>
>> + TP_STRUCT__entry(
>> + __field(unsigned char, second)
>> + __field(unsigned char, minute)
>> + __field(unsigned char, hour)
>> + __field(unsigned char, day)
>> + __field(unsigned char, mon)
>> + __field(unsigned short, year)
>> + __field(unsigned char, alarm_type)
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->second = rtc_time->tm_sec;
>> + __entry->minute = rtc_time->tm_min;
>> + __entry->hour = rtc_time->tm_hour;
>> + __entry->day = rtc_time->tm_mday;
>> + __entry->mon = rtc_time->tm_mon;
>> + __entry->year = rtc_time->tm_year;
>> + __entry->alarm_type = flag;
>
> What's the value of storing the alarm time in RTC format?
As suggested by Steven, change the type of RTC value to save trace buffer.
>
> What's wrong with simply storing the flat u64 based wall clock time?
> Nothing, because you can do the RTC format conversion in user space.
>
>> +DECLARE_EVENT_CLASS(alarm_processing,
>
> Again alarm_processing is not telling me anything.
Is alarm_class OK?
>
>> +
>> + TP_PROTO(struct alarm *alarm, char *process_name),
>
> Why do you want to store process_name? The tracer already tracks the name
> of the process in which context the tracepoint is taken. So what's the
> point of this? Look at the output:
>
> system_server-2976 [003] d..2 1076.605608: alarmtimer_start: process:system_server
>
> Completely pointless duplicated information.
OK. Will remove it.
>
>> +
>> + TP_ARGS(alarm, process_name),
>> +
>> + TP_STRUCT__entry(
>> + __field(unsigned long long, expires)
>> + __field(unsigned char, second)
>> + __field(unsigned char, minute)
>> + __field(unsigned char, hour)
>> + __field(unsigned char, day)
>> + __field(unsigned char, mon)
>> + __field(unsigned short, year)
>> + __field(unsigned char, alarm_type)
>> + __string(name, process_name)
>> + ),
>> +
>> + TP_fast_assign(
>> + __entry->expires = alarm->node.expires.tv64;
>> + __entry->alarm_type = alarm->type;
>> + __assign_str(name, process_name);
>> + __entry->second = rtc_ktime_to_tm(alarm->node.expires).tm_sec;
>> + __entry->minute = rtc_ktime_to_tm(alarm->node.expires).tm_min;
>> + __entry->hour = rtc_ktime_to_tm(alarm->node.expires).tm_hour;
>> + __entry->day = rtc_ktime_to_tm(alarm->node.expires).tm_mday;
>> + __entry->mon = rtc_ktime_to_tm(alarm->node.expires).tm_mon;
>> + __entry->year = rtc_ktime_to_tm(alarm->node.expires).tm_year;
>
> This is utter crap for various reasons:
>
> 1) You store the expiry time over and over and in most cases it's simply
> pointless.
I will remove the 'expires' variable.
>
> - If the timer is started then we want to store the expiry time.
>
> - If the timer fires then the programmed expiry time is available from
> the start trace point and you miss to store the information which is
> really interesting: The actual time at which the timer expires
> (REAL/BOOT)
>
> - If the timer is canceled then the expiry time in the timer is not
> interesting at all. All you care is about the fact that the timer has
> been canceled. The expiry time can still be retrieved from the start
> trace point.
>
> - The restart tracepoint is redundant as well because either you see:
>
> start -> expire -> start or start -> start which is clearly a restart.
>
> If you put the start trace point into alarmtimer_enqueue() then you
> spare the extra code size for two tracepoints because that is used in
> start and restart
Make sense.
>
> See the hrtimer and timer tracepoints for reference.
>
>
> 2) You store the expiry time again in RTC format. Store the information in
> a plain u64 and be done with it.
But I still think the RTC format is more readable for debugging alarm timer.
>
>
>> +DEFINE_EVENT(alarm_processing, alarmtimer_fired,
>> +
>> + TP_PROTO(struct alarm *alarm, char *process_name),
>
> So you hand in a NULL pointer to this tracepoint to have even more useless
> information in the trace.
Will remove 'process_name' parameter.
>
>> @@ -264,6 +270,11 @@ static int alarmtimer_suspend(struct device *dev)
>> now = rtc_tm_to_ktime(tm);
>> now = ktime_add(now, min);
>>
>> + if (trace_alarmtimer_suspend_enabled()) {
>
> What's the point of this conditional? Avoiding rtc_ktime_to_tm() ? Oh well...
>
>> + tm_set = rtc_ktime_to_tm(now);
>> + trace_alarmtimer_suspend(&tm_set, type);
>
> "now" is CLOCK_REALTIME based. You store the type of the alarm timer which
> is the first to expire and therefor is the one setting the RTC value, but
> we don't know which timer it is. Useful - NOT!
We can know the timer by comparing the expire time.
--
Baolin.wang
Best Regards
Powered by blists - more mailing lists