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

[Binoy Jayan]

Hi Thomas,

Thank you for the reply and sharing your insights.

On 21 September 2016 at 21:28, Thomas Gleixner <tglx@xxxxxxxxxxxxx> wrote:

> Sorry. This has nothing to do with changing the hrtimer_base, simply
> because the time base is the same on all cpus.

The condition 'ktime_to_ns(tim) < ktime_to_ns(now)' checks if the timer
has already expired w.r.t. 'soft timeout' value as it does not include
the slack value 'delta_ns'. In that case 'tim_expiry' is normalized to
the current time. (I was under the impression that this inaccuracy
could be because timer was initially running on a different cpu. If that
is not the case, I guess we can use the code mentioned below).

Otherwise it postpones the decision of storing the expiry value
until the hrtimer interrupt. In this case, it calculates the latency
using the hard timeout (which includes the fuzz) as returned by a call
to 'hrtimer_get_expires' in 'latency_hrtimer_timing_stop'.

Since for calculating latency, we use hard timeout value which includes
the slack, and since the actual timeout might have happened in between
the soft and hard timeout, the actual expiry time could be less than
the hard expiry time. This is why latency is checked for negative value
before storing when the trace point is hit.

static inline void latency_hrtimer_timing_start(ktime_t tim)
{
     timer->tim_expiry = tim;
}

static inline void latency_hrtimer_timing_stop(struct hrtimer *timer,
                                                ktime_t basenow)
{
        long latency;
        struct task_struct *task;

        latency = ktime_to_ns(basenow) - hrtimer_get_softexpires_tv64(timer);

        task = timer->function == hrtimer_wakeup ?
                        container_of(timer, struct hrtimer_sleeper,
                                     timer)->task : NULL;
        if (task && latency > 0)   // Now the check for latency may
not be needed
                task->timer_offset = latency;
}

I am using 'hrtimer_get_softexpires_tv64' instead of 'hrtimer_get_expires'
so that 'latency' is never negative. Please let me know if this looks ok.

> It's not Carstens repsonsibility to explain the nature of the change.
> You are submitting that code and so it's your job to provide proper
> explanations and justifications. If you can't do that, then how do you
> think that the review process, which is a feedback loop between the
> reviewer and the submitter, should work?
>
> Answer: It cannot work that way. I hope I don't have to explain why.

Sure, I'll avoid this confusion in the future. I think I should have talked
to him only offline and not here.

Thanks,
Binoy