From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752068AbdASKlA (ORCPT ); Thu, 19 Jan 2017 05:41:00 -0500 Received: from imslp-west.kjsl.com ([65.50.211.133]:44604 "EHLO bombadil.infradead.org" rhost-flags-OK-FAIL-OK-OK) by vger.kernel.org with ESMTP id S1751946AbdASKk6 (ORCPT ); Thu, 19 Jan 2017 05:40:58 -0500 Date: Thu, 19 Jan 2017 11:14:21 +0100 From: Peter Zijlstra To: Suravee Suthikulpanit Cc: linux-kernel@vger.kernel.org, iommu@lists.linux-foundation.org, joro@8bytes.org, bp@alien8.de, mingo@redhat.com Subject: Re: [PATCH v7 1/7] perf/amd/iommu: Misc fix up perf_iommu_read Message-ID: <20170119101421.GI6485@twins.programming.kicks-ass.net> References: <1484019227-11473-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1484019227-11473-2-git-send-email-Suravee.Suthikulpanit@amd.com> <20170111115735.GI3107@twins.programming.kicks-ass.net> <8d529523-21b4-d917-e83f-ed616a29083c@amd.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <8d529523-21b4-d917-e83f-ed616a29083c@amd.com> User-Agent: Mutt/1.5.23.1 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sun, Jan 15, 2017 at 09:36:10AM +0700, Suravee Suthikulpanit wrote: > Peter, > > On 1/11/17 18:57, Peter Zijlstra wrote: > >On Mon, Jan 09, 2017 at 09:33:41PM -0600, Suravee Suthikulpanit wrote: > >>This patch contains the following minor fixup: > >> * Fixed overflow handling since u64 delta would lose the MSB sign bit. > > > >Please explain.. afaict this actually introduces a bug. > > I'm changing the u64 to s64 ..... (see below) > > > > >>diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c > >>index b28200d..f387baf 100644 > >>--- a/arch/x86/events/amd/iommu.c > >>+++ b/arch/x86/events/amd/iommu.c > >>@@ -319,29 +319,30 @@ static void perf_iommu_start(struct perf_event *event, int flags) > >> > >> static void perf_iommu_read(struct perf_event *event) > >> { > >>- u64 count = 0ULL; > >>- u64 prev_raw_count = 0ULL; > >>- u64 delta = 0ULL; > >>+ u64 cnt, prev; > >>+ s64 delta; > > .... (here) because we had a discussion (https://lkml.org/lkml/2016/2/18/325), > and you suggested the following: > > Your overflow handling is broken, you want delta to be s64. Otherwise: > > delta >>= COUNTER_SHIFT; > > ends up as a SHR and you loose the MSB sign bits. Yah, I was wrong :-), please see: 7f612a7f0bc1 ("perf/x86: Fix full width counter, counter overflow") > >> struct hw_perf_event *hwc = &event->hw; > >> pr_debug("perf: amd_iommu:perf_iommu_read\n"); > >> > >> amd_iommu_pc_get_set_reg_val(_GET_DEVID(event), > >> _GET_BANK(event), _GET_CNTR(event), > >>- IOMMU_PC_COUNTER_REG, &count, false); > >>+ IOMMU_PC_COUNTER_REG, &cnt, false); > >> > >> /* IOMMU pc counter register is only 48 bits */ > >>- count &= 0xFFFFFFFFFFFFULL; > >>+ cnt &= GENMASK_ULL(48, 0); > >> > >>- prev_raw_count = local64_read(&hwc->prev_count); > >>- if (local64_cmpxchg(&hwc->prev_count, prev_raw_count, > >>- count) != prev_raw_count) > >>- return; > >>+ prev = local64_read(&hwc->prev_count); > >> > >>- /* Handling 48-bit counter overflowing */ > >>- delta = (count << COUNTER_SHIFT) - (prev_raw_count << COUNTER_SHIFT); > >>+ /* > >>+ * Since we do not enable counter overflow interrupts, > >>+ * we do not have to worry about prev_count changing on us. > >>+ */ > > > >So you cannot group this event with a software event that reads this > >from their sample? > > Not sure if I understand you point here. When you say sample, I assume you mean > the profiling mode used w/ perf record. These counters are not supported for > sampling mode. So, we only perf stat (i.e. counting mode). But you can group them with a software event, right? And then that software event can do sampling (using a timer for instance) and read the other counters in the group using PERF_SAMPLE_READ and PERF_FORMAT_GROUP. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Zijlstra Subject: Re: [PATCH v7 1/7] perf/amd/iommu: Misc fix up perf_iommu_read Date: Thu, 19 Jan 2017 11:14:21 +0100 Message-ID: <20170119101421.GI6485@twins.programming.kicks-ass.net> References: <1484019227-11473-1-git-send-email-Suravee.Suthikulpanit@amd.com> <1484019227-11473-2-git-send-email-Suravee.Suthikulpanit@amd.com> <20170111115735.GI3107@twins.programming.kicks-ass.net> <8d529523-21b4-d917-e83f-ed616a29083c@amd.com> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Content-Disposition: inline In-Reply-To: <8d529523-21b4-d917-e83f-ed616a29083c-5C7GfCeVMHo@public.gmane.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org Errors-To: iommu-bounces-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org To: Suravee Suthikulpanit Cc: iommu-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org, mingo-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org, bp-Gina5bIWoIWzQB+pC5nmwQ@public.gmane.org, linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org List-Id: iommu@lists.linux-foundation.org On Sun, Jan 15, 2017 at 09:36:10AM +0700, Suravee Suthikulpanit wrote: > Peter, > > On 1/11/17 18:57, Peter Zijlstra wrote: > >On Mon, Jan 09, 2017 at 09:33:41PM -0600, Suravee Suthikulpanit wrote: > >>This patch contains the following minor fixup: > >> * Fixed overflow handling since u64 delta would lose the MSB sign bit. > > > >Please explain.. afaict this actually introduces a bug. > > I'm changing the u64 to s64 ..... (see below) > > > > >>diff --git a/arch/x86/events/amd/iommu.c b/arch/x86/events/amd/iommu.c > >>index b28200d..f387baf 100644 > >>--- a/arch/x86/events/amd/iommu.c > >>+++ b/arch/x86/events/amd/iommu.c > >>@@ -319,29 +319,30 @@ static void perf_iommu_start(struct perf_event *event, int flags) > >> > >> static void perf_iommu_read(struct perf_event *event) > >> { > >>- u64 count = 0ULL; > >>- u64 prev_raw_count = 0ULL; > >>- u64 delta = 0ULL; > >>+ u64 cnt, prev; > >>+ s64 delta; > > .... (here) because we had a discussion (https://lkml.org/lkml/2016/2/18/325), > and you suggested the following: > > Your overflow handling is broken, you want delta to be s64. Otherwise: > > delta >>= COUNTER_SHIFT; > > ends up as a SHR and you loose the MSB sign bits. Yah, I was wrong :-), please see: 7f612a7f0bc1 ("perf/x86: Fix full width counter, counter overflow") > >> struct hw_perf_event *hwc = &event->hw; > >> pr_debug("perf: amd_iommu:perf_iommu_read\n"); > >> > >> amd_iommu_pc_get_set_reg_val(_GET_DEVID(event), > >> _GET_BANK(event), _GET_CNTR(event), > >>- IOMMU_PC_COUNTER_REG, &count, false); > >>+ IOMMU_PC_COUNTER_REG, &cnt, false); > >> > >> /* IOMMU pc counter register is only 48 bits */ > >>- count &= 0xFFFFFFFFFFFFULL; > >>+ cnt &= GENMASK_ULL(48, 0); > >> > >>- prev_raw_count = local64_read(&hwc->prev_count); > >>- if (local64_cmpxchg(&hwc->prev_count, prev_raw_count, > >>- count) != prev_raw_count) > >>- return; > >>+ prev = local64_read(&hwc->prev_count); > >> > >>- /* Handling 48-bit counter overflowing */ > >>- delta = (count << COUNTER_SHIFT) - (prev_raw_count << COUNTER_SHIFT); > >>+ /* > >>+ * Since we do not enable counter overflow interrupts, > >>+ * we do not have to worry about prev_count changing on us. > >>+ */ > > > >So you cannot group this event with a software event that reads this > >from their sample? > > Not sure if I understand you point here. When you say sample, I assume you mean > the profiling mode used w/ perf record. These counters are not supported for > sampling mode. So, we only perf stat (i.e. counting mode). But you can group them with a software event, right? And then that software event can do sampling (using a timer for instance) and read the other counters in the group using PERF_SAMPLE_READ and PERF_FORMAT_GROUP.