linux-kernel - Re: [RFC PATCH v7 4/5] tracing: Measure delayed hrtimer offset latency

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 21 Sep 2016 11:36:29 +0530
From:   Binoy Jayan <binoy.jayan@...aro.org>
To:     Thomas Gleixner <tglx@...utronix.de>,
        Carsten Emde <C.Emde@...dl.org>
Cc:     "Steven Rostedt (Red Hat)" <rostedt@...dmis.org>,
        Ingo Molnar <mingo@...hat.com>,
        Daniel Wagner <daniel.wagner@...-carit.de>,
        Arnd Bergmann <arnd@...db.de>,
        Linux kernel mailing list <linux-kernel@...r.kernel.org>,
        Masami <masami.hiramatsu@...aro.org>,
        Mark Brown <mark.brown@...aro.org>
Subject: Re: [RFC PATCH v7 4/5] tracing: Measure delayed hrtimer offset latency

On 20 September 2016 at 19:49, Thomas Gleixner <tglx@...utronix.de> wrote:
> On Tue, 20 Sep 2016, Binoy Jayan wrote:
>> +#ifdef CONFIG_TRACE_DELAYED_TIMER_OFFSETS
>> +static inline void latency_hrtimer_timing_start(struct hrtimer *timer,
>> +                                      struct hrtimer_clock_base *new_base,
>> +                                      ktime_t tim)
>> +{
>> +     ktime_t now = new_base->get_time();
>> +
>> +     if (ktime_to_ns(tim) < ktime_to_ns(now))
>> +             timer->tim_expiry = now;
>> +     else
>> +             timer->tim_expiry = ktime_set(0, 0);
>
> You still fail to explain why this get_time() magic is required.
>
> This is executed unconditionally when the config switch is enabled and does
> not depend on whether the trace functionality is enabled or not. So you are
> imposing the extra get_time() call, which can be expensive depending on the
> underlying hardware, on every hrtimer start invocation.
>
> Tracing is supposed to have ZERO impact when it is not used and even when
> it's in use then the impact should be kept as low as possible. The above
> does none of that.
>
> Neither did you provide a proper rationale for this infrastructure in the
> changelog.
>
> You can repost that over and over and it will not go anywhere if you don't
> start to address the review comments I give you.

Hi Thomas,

Sorry, I missed to address this comment from you. From what I understand
why the get_time() is needed is to get the more accurate current time when
the hrtimer base is changed (from the cpu in which it was fired on, to
the current
cpu on which it is currently made to run or restarted) wherein the hrtimer base
needs to be switched to the new cpu provided that it is not running in
pinned mode.

Carsten,

Could you please comment on that?

Thanks,
Binoy