Re: [RFC PATCH v7 4/5] tracing: Measure delayed hrtimer offset latency
On Thu, Sep 22, 2016 at 07:28:01PM +0200, Thomas Gleixner wrote: > I'm not blaming you, but I blame the responsible persons inside your > company who task you with that and expect that I'm going to do their work > of explaining to you how that code works. > @Arnd, @Mark: I'm starting to get seriously grumpy about that. Really sorry about that, obviously that's not the intention. We'll try to avoid this happening in future. signature.asc Description: PGP signature
Re: [RFC PATCH v7 4/5] tracing: Measure delayed hrtimer offset latency
Binoy, On Thu, 22 Sep 2016, Binoy Jayan wrote: > 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. You are halfways coming close to the point why this is done, but your conclusion is completely wrong. The correct keyword is "expired", but anything else is just random speculation. > (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). We are not playing a guessing game here. > 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. No it does not. And it won't look correct until you finally sit down and decode and understand the functionality behind this code. I'm not going to continue this, as it's not my job to explain you the code which you are trying to submit. I'm not blaming you, but I blame the responsible persons inside your company who task you with that and expect that I'm going to do their work of explaining to you how that code works. @Arnd, @Mark: I'm starting to get seriously grumpy about that. Thanks, tglx
Re: [RFC PATCH v7 4/5] tracing: Measure delayed hrtimer offset latency
Hi Thomas,
Thank you for the reply and sharing your insights.
On 21 September 2016 at 21:28, Thomas Gleixner 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
Re: [RFC PATCH v7 4/5] tracing: Measure delayed hrtimer offset latency
On Wed, 21 Sep 2016, Binoy Jayan wrote: > On 20 September 2016 at 19:49, Thomas Gleixner wrote: > > You still fail to explain why this get_time() magic is required. > > 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. Sorry. This has nothing to do with changing the hrtimer_base, simply because the time base is the same on all cpus. > Carsten, Could you please comment on that? 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. Thanks, tglx
Re: [RFC PATCH v7 4/5] tracing: Measure delayed hrtimer offset latency
On 20 September 2016 at 19:49, Thomas Gleixner 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
Re: [RFC PATCH v7 4/5] tracing: Measure delayed hrtimer offset latency
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.
Thanks,
tglx
