lkml.org 
[lkml]   [2020]   [Dec]   [10]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [Intel-gfx] [PATCH v3 2/4] drm/i915/pmu: Use kstat_irqs to get interrupt count
From
Date

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
>

\
 
 \ /
  Last update: 2020-12-10 11:49    [W:2.283 / U:0.000 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site