From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.9 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,HK_RANDOM_FROM,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id E9A5CC4361B for ; Thu, 10 Dec 2020 10:47:56 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id 9E80223D98 for ; Thu, 10 Dec 2020 10:47:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1730351AbgLJKrr (ORCPT ); Thu, 10 Dec 2020 05:47:47 -0500 Received: from mga01.intel.com ([192.55.52.88]:14595 "EHLO mga01.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2389008AbgLJKr2 (ORCPT ); Thu, 10 Dec 2020 05:47:28 -0500 IronPort-SDR: 3DQ5cz0pGvnkGCg3HtsKNemPimHYB74sbnSCgxxB0mTTAWNxZ5O32b5oFn5p8yoEw4qgx3A/C8 qzPRetHQWbBg== X-IronPort-AV: E=McAfee;i="6000,8403,9830"; a="192546249" X-IronPort-AV: E=Sophos;i="5.78,408,1599548400"; d="scan'208";a="192546249" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by fmsmga101.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Dec 2020 02:45:42 -0800 IronPort-SDR: 8B5JILFNwgpLoTaLek6lDBxucy5SOqjmME7GMXrefffU9h3aOhRW/kkWr7tMkBT7Ul/eo4ThxS Po3iPyr1yLcg== X-IronPort-AV: E=Sophos;i="5.78,408,1599548400"; d="scan'208";a="319007397" Received: from nabuhijl-mobl.ger.corp.intel.com (HELO [10.251.185.230]) ([10.251.185.230]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Dec 2020 02:45:37 -0800 Subject: Re: [Intel-gfx] [PATCH v3 2/4] drm/i915/pmu: Use kstat_irqs to get interrupt count To: Joonas Lahtinen , Jerry Snitselaar , Thomas Gleixner , linux-kernel@vger.kernel.org, Tvrtko Ursulin , Chris Wilson Cc: Jason Gunthorpe , Peter Zijlstra , intel-gfx@lists.freedesktop.org, Matthew Garrett , James Bottomley , David Airlie , Jarkko Sakkinen , dri-devel@lists.freedesktop.org, linux-integrity@vger.kernel.org, Peter Huewe References: <20201205014340.148235-1-jsnitsel@redhat.com> <20201205014340.148235-3-jsnitsel@redhat.com> <875z5e99ez.fsf@nanos.tec.linutronix.de> <160758677957.5062.15497765500689083558@jlahtine-mobl.ger.corp.intel.com> From: Tvrtko Ursulin Organization: Intel Corporation UK Plc Message-ID: Date: Thu, 10 Dec 2020 10:45:34 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <160758677957.5062.15497765500689083558@jlahtine-mobl.ger.corp.intel.com> Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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 > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.9 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,HK_RANDOM_FROM,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,USER_AGENT_SANE_1 autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id D0819C4361B for ; Thu, 10 Dec 2020 10:45:46 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 5384123D6A for ; Thu, 10 Dec 2020 10:45:46 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 5384123D6A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=dri-devel-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 709946E40F; Thu, 10 Dec 2020 10:45:45 +0000 (UTC) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id BDE966E40A; Thu, 10 Dec 2020 10:45:43 +0000 (UTC) IronPort-SDR: i8QLw6wSmbyutX+z1x7pe/a5f3hdlkGwF9dSPgZZkMBIMF5mZB04QkslSmcy0143I/YNft831W kpoRjfcB23kA== X-IronPort-AV: E=McAfee;i="6000,8403,9830"; a="235827967" X-IronPort-AV: E=Sophos;i="5.78,408,1599548400"; d="scan'208";a="235827967" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Dec 2020 02:45:42 -0800 IronPort-SDR: 8B5JILFNwgpLoTaLek6lDBxucy5SOqjmME7GMXrefffU9h3aOhRW/kkWr7tMkBT7Ul/eo4ThxS Po3iPyr1yLcg== X-IronPort-AV: E=Sophos;i="5.78,408,1599548400"; d="scan'208";a="319007397" Received: from nabuhijl-mobl.ger.corp.intel.com (HELO [10.251.185.230]) ([10.251.185.230]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Dec 2020 02:45:37 -0800 Subject: Re: [Intel-gfx] [PATCH v3 2/4] drm/i915/pmu: Use kstat_irqs to get interrupt count To: Joonas Lahtinen , Jerry Snitselaar , Thomas Gleixner , linux-kernel@vger.kernel.org, Tvrtko Ursulin , Chris Wilson References: <20201205014340.148235-1-jsnitsel@redhat.com> <20201205014340.148235-3-jsnitsel@redhat.com> <875z5e99ez.fsf@nanos.tec.linutronix.de> <160758677957.5062.15497765500689083558@jlahtine-mobl.ger.corp.intel.com> From: Tvrtko Ursulin Organization: Intel Corporation UK Plc Message-ID: Date: Thu, 10 Dec 2020 10:45:34 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <160758677957.5062.15497765500689083558@jlahtine-mobl.ger.corp.intel.com> Content-Language: en-US X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: David Airlie , Peter Zijlstra , intel-gfx@lists.freedesktop.org, Matthew Garrett , James Bottomley , Jason Gunthorpe , Jarkko Sakkinen , dri-devel@lists.freedesktop.org, linux-integrity@vger.kernel.org, Peter Huewe Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" 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 > 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 > _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-9.9 required=3.0 tests=BAYES_00, HEADER_FROM_DIFFERENT_DOMAINS,HK_RANDOM_FROM,INCLUDES_PATCH, MAILING_LIST_MULTI,NICE_REPLY_A,SPF_HELO_NONE,SPF_PASS,URIBL_BLOCKED, USER_AGENT_SANE_1 autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2EA58C433FE for ; Thu, 10 Dec 2020 10:45:46 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id 7E4BC23D6A for ; Thu, 10 Dec 2020 10:45:45 +0000 (UTC) DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org 7E4BC23D6A Authentication-Results: mail.kernel.org; dmarc=fail (p=none dis=none) header.from=linux.intel.com Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=intel-gfx-bounces@lists.freedesktop.org Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id DE0A26E40A; Thu, 10 Dec 2020 10:45:44 +0000 (UTC) Received: from mga06.intel.com (mga06.intel.com [134.134.136.31]) by gabe.freedesktop.org (Postfix) with ESMTPS id BDE966E40A; Thu, 10 Dec 2020 10:45:43 +0000 (UTC) IronPort-SDR: i8QLw6wSmbyutX+z1x7pe/a5f3hdlkGwF9dSPgZZkMBIMF5mZB04QkslSmcy0143I/YNft831W kpoRjfcB23kA== X-IronPort-AV: E=McAfee;i="6000,8403,9830"; a="235827967" X-IronPort-AV: E=Sophos;i="5.78,408,1599548400"; d="scan'208";a="235827967" Received: from fmsmga008.fm.intel.com ([10.253.24.58]) by orsmga104.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Dec 2020 02:45:42 -0800 IronPort-SDR: 8B5JILFNwgpLoTaLek6lDBxucy5SOqjmME7GMXrefffU9h3aOhRW/kkWr7tMkBT7Ul/eo4ThxS Po3iPyr1yLcg== X-IronPort-AV: E=Sophos;i="5.78,408,1599548400"; d="scan'208";a="319007397" Received: from nabuhijl-mobl.ger.corp.intel.com (HELO [10.251.185.230]) ([10.251.185.230]) by fmsmga008-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 10 Dec 2020 02:45:37 -0800 To: Joonas Lahtinen , Jerry Snitselaar , Thomas Gleixner , linux-kernel@vger.kernel.org, Tvrtko Ursulin , Chris Wilson References: <20201205014340.148235-1-jsnitsel@redhat.com> <20201205014340.148235-3-jsnitsel@redhat.com> <875z5e99ez.fsf@nanos.tec.linutronix.de> <160758677957.5062.15497765500689083558@jlahtine-mobl.ger.corp.intel.com> From: Tvrtko Ursulin Organization: Intel Corporation UK Plc Message-ID: Date: Thu, 10 Dec 2020 10:45:34 +0000 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.10.0 MIME-Version: 1.0 In-Reply-To: <160758677957.5062.15497765500689083558@jlahtine-mobl.ger.corp.intel.com> Content-Language: en-US Subject: Re: [Intel-gfx] [PATCH v3 2/4] drm/i915/pmu: Use kstat_irqs to get interrupt count X-BeenThere: intel-gfx@lists.freedesktop.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Intel graphics driver community testing & development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: David Airlie , Peter Zijlstra , intel-gfx@lists.freedesktop.org, Matthew Garrett , James Bottomley , Jason Gunthorpe , Jarkko Sakkinen , dri-devel@lists.freedesktop.org, linux-integrity@vger.kernel.org, Peter Huewe Content-Transfer-Encoding: 7bit Content-Type: text/plain; charset="us-ascii"; Format="flowed" Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" 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 > 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 > _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx