Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1421906 > unrolled thread

Re: [PATCH V4] irq: Track the interrupt timings

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH V4] irq: Track the interrupt timings Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-06-14 15:40 +0200
    Re: [PATCH V4] irq: Track the interrupt timings Nicolas Pitre <nicolas.pitre@linaro.org> - 2016-06-14 17:20 +0200
      Re: [PATCH V4] irq: Track the interrupt timings Daniel Lezcano <daniel.lezcano@linaro.org> - 2016-06-14 17:40 +0200
      Re: [PATCH V4] irq: Track the interrupt timings Thomas Gleixner <tglx@linutronix.de> - 2016-06-14 18:40 +0200

#1421906 — Re: [PATCH V4] irq: Track the interrupt timings

On 06/10/2016 04:52 PM, Thomas Gleixner wrote:

Hi Thomas,

[ ... ]

>> +	now = local_clock();
>> +	prev = timings->timestamp;
>> +	timings->timestamp = now;
>> +
>> +	/*
>> +	 * If it is the first interrupt of the series, we can't
>> +	 * compute an interval, just store the timestamp and exit.
>> +	 */
>> +	if (unlikely(!prev))
>> +		return;
>
> The delta will be large enough that you drop out in the check below. So you
> can spare that conditional.

Good point.

>> +
>> +	diff = now - prev;
>> +
>> +	/*
>> +	 * microsec (actually 1024th of a milisec) precision is good
>> +	 * enough for our purpose.
>> +	 */
>> +	diff >>= 10;
>
> And that shift instruction is required because of the following?
>
>>    	 * Otherwise we know the magnitude of diff is
>> +	 * well within 32 bits.
>
> AFAICT that's pointless. You are not saving anything because NSEC_PER_SEC is
> smaller than 2^32 and your 8 values are not going to overflow 64 bit in the
> sum.
>
>> +	 */
>> +	if (unlikely(diff > USEC_PER_SEC)) {
>> +		memset(timings, 0, sizeof(*timings));
>> +		timings->timestamp = now;
>
> Redundant store.

Yeah, I thought it was more efficient than:
	memset(timings->value, 0, sizeof(timings->value));
	timings->w_index = 0;
	timings->sum = 0;

>> +		return;
>> +	}
>> +
>> +	/* The oldest value corresponds to the next index. */
>> +	timings->w_index = (timings->w_index + 1) & IRQ_TIMINGS_MASK;
>> +
>> +	/*
>> +	 * Remove the oldest value from the summing. If this is the
>> +	 * first time we go through this array slot, the previous
>> +	 * value will be zero and we won't substract anything from the
>> +	 * current sum. Hence this code relies on a zero-ed structure.
>> +	 */
>> +	timings->sum -= timings->values[timings->w_index];
>> +	timings->values[timings->w_index] = diff;
>> +	timings->sum += diff;
>
> Now the real question is whether you really need all that math, checks and
> memsets in the irq hotpath. If you make the storage slightly larger then you
> can just store the values unconditionally in the circular buffer and do all
> the computational stuff when you really it.

Yes, that was one concern when I wrote the code: do some basic 
computation when an interrupt occurs, and the rest after or do the 
entire math when entering idle.

If the storage is a bit larger (let's say 16 values) and there is no 
memset and the sum is not computed, at least we need a count for the 
number of values in the array before this one is fulfilled, otherwise 
the statistics will be wrong as we will take into account the entire 
array with old values, no ?


-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [next] | [standalone]

#1422008

On Tue, 14 Jun 2016, Daniel Lezcano wrote:

> On 06/10/2016 04:52 PM, Thomas Gleixner wrote:
> 
> > > +	timings->sum -= timings->values[timings->w_index];
> > > +	timings->values[timings->w_index] = diff;
> > > +	timings->sum += diff;
> >
> > Now the real question is whether you really need all that math, checks and
> > memsets in the irq hotpath. If you make the storage slightly larger then you
> > can just store the values unconditionally in the circular buffer and do all
> > the computational stuff when you really it.
> 
> Yes, that was one concern when I wrote the code: do some basic computation
> when an interrupt occurs, and the rest after or do the entire math when
> entering idle.
> 
> If the storage is a bit larger (let's say 16 values) and there is no memset
> and the sum is not computed, at least we need a count for the number of values
> in the array before this one is fulfilled, otherwise the statistics will be
> wrong as we will take into account the entire array with old values, no ?

The point is not to change from 8 to 16 entries, but to store raw 64-bit 
timestamps instead of computed 32-bit deltas.  Whether or not those 
timestamps are too far apart and discarded can be done at idle entry 
time.


Nicolas

[toc] | [prev] | [next] | [standalone]

#1422020

On 06/14/2016 05:10 PM, Nicolas Pitre wrote:
> On Tue, 14 Jun 2016, Daniel Lezcano wrote:
>
>> On 06/10/2016 04:52 PM, Thomas Gleixner wrote:
>>
>>>> +	timings->sum -= timings->values[timings->w_index];
>>>> +	timings->values[timings->w_index] = diff;
>>>> +	timings->sum += diff;
>>>
>>> Now the real question is whether you really need all that math, checks and
>>> memsets in the irq hotpath. If you make the storage slightly larger then you
>>> can just store the values unconditionally in the circular buffer and do all
>>> the computational stuff when you really it.
>>
>> Yes, that was one concern when I wrote the code: do some basic computation
>> when an interrupt occurs, and the rest after or do the entire math when
>> entering idle.
>>
>> If the storage is a bit larger (let's say 16 values) and there is no memset
>> and the sum is not computed, at least we need a count for the number of values
>> in the array before this one is fulfilled, otherwise the statistics will be
>> wrong as we will take into account the entire array with old values, no ?
>
> The point is not to change from 8 to 16 entries, but to store raw 64-bit
> timestamps instead of computed 32-bit deltas.  Whether or not those
> timestamps are too far apart and discarded can be done at idle entry
> time.

Ah, ok. That makes sense.

Thanks for the clarification.

  -- Daniel

-- 
  <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog

[toc] | [prev] | [next] | [standalone]

#1422073

On Tue, 14 Jun 2016, Nicolas Pitre wrote:
> On Tue, 14 Jun 2016, Daniel Lezcano wrote:
> > If the storage is a bit larger (let's say 16 values) and there is no memset
> > and the sum is not computed, at least we need a count for the number of values
> > in the array before this one is fulfilled, otherwise the statistics will be
> > wrong as we will take into account the entire array with old values, no ?
> 
> The point is not to change from 8 to 16 entries, but to store raw 64-bit 
> timestamps instead of computed 32-bit deltas.  Whether or not those 
> timestamps are too far apart and discarded can be done at idle entry 
> time.

Correct, and you don't have to know how many timestamps are in the array
simply because if it is cleared at init time, then any not yet set value will
create a large gap, which you filter out.

The point is to make the fast path overhead as small as possible. And if
that's just a store and index increment, then it can be inline and not a
function call.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web