Re: [Intel-gfx] [PATCH v3 2/4] drm/i915/pmu: Use kstat_irqs to get interrupt count

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 10/12/2020 07:53, Joonas Lahtinen wrote:
> + Tvrtko and Chris for comments
> 
> Code seems to be added in:
> 
> commit 0cd4684d6ea9a4ffec33fc19de4dd667bb90d0a5
> Author: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Date:   Tue Nov 21 18:18:50 2017 +0000
> 
>      drm/i915/pmu: Add interrupt count metric
> 
> I think later in the thread there was a suggestion to replace this with
> simple counter increment in IRQ handler.

It was indeed unsafe until recent b00bccb3f0bb ("drm/i915/pmu: Handle 
PCI unbind") but now should be fine.

If kstat_irqs does not get exported it is easy enough for i915 to keep a 
local counter. Reasoning was very infrequent per cpu summation is much 
cheaper than very frequent atomic add. Up to thousands of interrupts per 
second vs "once per second" PMU read kind of thing.

Regards,

Tvrtko

> Quoting Thomas Gleixner (2020-12-06 18:38:44)
>> On Fri, Dec 04 2020 at 18:43, Jerry Snitselaar wrote:
>>
>>> Now that kstat_irqs is exported, get rid of count_interrupts in
>>> i915_pmu.c
>>> --- a/drivers/gpu/drm/i915/i915_pmu.c
>>> +++ b/drivers/gpu/drm/i915/i915_pmu.c
>>> @@ -423,22 +423,6 @@ static enum hrtimer_restart i915_sample(struct hrtimer *hrtimer)
>>>        return HRTIMER_RESTART;
>>>   }
>>>   
>>> -static u64 count_interrupts(struct drm_i915_private *i915)
>>> -{
>>> -     /* open-coded kstat_irqs() */
>>> -     struct irq_desc *desc = irq_to_desc(i915->drm.pdev->irq);
>>> -     u64 sum = 0;
>>> -     int cpu;
>>> -
>>> -     if (!desc || !desc->kstat_irqs)
>>> -             return 0;
>>> -
>>> -     for_each_possible_cpu(cpu)
>>> -             sum += *per_cpu_ptr(desc->kstat_irqs, cpu);
>>> -
>>> -     return sum;
>>> -}
>>
>> May I ask why this has been merged in the first place?
>>
>> Nothing in a driver has ever to fiddle with the internals of an irq
>> descriptor. We have functions for properly accessing them. Just because
>> C allows to fiddle with everything is not a justification. If the
>> required function is not exported then adding the export with a proper
>> explanation is not asked too much.
>>
>> Also this lacks protection or at least a comment why this can be called
>> safely and is not subject to a concurrent removal of the irq descriptor.
>> The same problem exists when calling kstat_irqs(). It's even documented
>> at the top of the function.
>>
>> Thanks,
>>
>>          tglx
>>
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx
>