From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-alma10-1.taild15c8.ts.net [100.103.45.18]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 9E21A2D8DD0 for ; Tue, 16 Jun 2026 11:34:16 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=100.103.45.18 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781609657; cv=none; b=TjmkoG7pWGQTqoiMwMU2uof5jwRT/MWGGnVJ82g9akx3Ud5Mltqu5VWVkDcwzgFIjqPS3kQ5L72hq3FDF44f8LFm7noxzYt1NetxGvqdcBchI9TYxeRBExp5cK3nhOkznJseH+jSq/6IjS4Te6AQFh+5VjetwL/5t7070izVnCM= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781609657; c=relaxed/simple; bh=cuEriRGYvRLY6Vn+idpimBBXieYnTd9aVxD4F5eGvUs=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=Ue0u9rUttADYoFKORz2yMCSDy0rLlPDiYLIFD5M6i+wcLMpociXKF/qIAQ8HEyGUXKM1E2RltUK2lql6eSLvLaJU+ZnUGBo8K+fKLBJxlobsKWfOUys97E3mZZUA5S9YCAPjR25OB2NvJx0zNse44tfdHIYdRLoSQNzHEjZ3khU= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=mdff/3x3; arc=none smtp.client-ip=100.103.45.18 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="mdff/3x3" Received: by smtp.kernel.org (Postfix) with ESMTPSA id B27881F000E9; Tue, 16 Jun 2026 11:34:11 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=kernel.org; s=k20260515; t=1781609656; bh=OiA4aSwidDSqEl2VmIacX3fGdlN3JPQMWMZnNeM1x/Q=; h=Date:From:To:Cc:Subject:In-Reply-To:References; b=mdff/3x3UaAf+6HeJ92OUO7gdTr7h8d87mFiJLAxlgThyTFohS35asP2kASq/H1VM WtTcXxqvbVKfEaipBT8m4naV28XKDhrl8wi1OdyQELMEs4VAIddJpMhNK5/0W1ksOv yA2kukJJz9YyOxuGNMk8RzMYx/QUoXWKrNJvx/P81AY+DapSEiQGYVFjXj0o0xMo1H kKCY50MSrRsWpOzGtPSEGc6HYjw1UkHgoBWAxxm2cMW1bnyHW7V56DlIzp/BrlX8/t w4sMPkao8+senS6Y7tUaz47aoFzlS0Omh6HPUA/eL0X/EoTPxk+3muPfqaZIjbCqzw 1JlZhnMH+1ueA== Date: Tue, 16 Jun 2026 12:34:06 +0100 From: Jonathan Cameron To: "Yicong Yang" Cc: , , , , , , , , , , Subject: Re: [PATCH v1 2/3] perf/dwc_pcie: Support narrowed time-based counter for long time monitoring Message-ID: <20260616123406.4d812fab@jic23-huawei> In-Reply-To: <20260615063459.25361-3-yang.yicong@picoheart.com> References: <20260615063459.25361-1-yang.yicong@picoheart.com> <20260615063459.25361-3-yang.yicong@picoheart.com> X-Mailer: Claws Mail 4.4.0 (GTK 3.24.52; x86_64-pc-linux-gnu) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Mon, 15 Jun 2026 14:34:58 +0800 "Yicong Yang" wrote: > From: Yufan Dou > > The DWC PCIe Time-Based Analysis Data Register (the counter for time-based > events) is architected as 64-bit, but some hardware implementations do not > implement the full width. On these implementations the counter stops after > reaching its implemented width. This will limit the usage for short time > monitoring only. The counter will only cover ~15s for monitoring RX TLP > payloads on our platform. > > Add an optional hrtimer that fires every 2 seconds. It'll take the role > as the counter overflow interrupt to read-update-reset the counter and > event counts to break the limits of the narrow counters. It'll only > apply on timer-based counter. The 2 seconds update period is the half > of the maximum counting period (4s) of the time-based counter under > period counting mode of the hardware. > > Because fully-implemented 64-bit counters do not need this workaround, > expose it via the module parameter timer_enable (default N) so that > users can opt in only on affected platforms. Hi, That bit seems less than ideal. Given there is matching by the vsec in the previous patch, can we make this automatically opt in on appropriate platforms? That would require the vendor to not use the same vsec for devices that don't have this quirk so kind of depends what is already out there. If this is a 'feature' rather than a 'bug' then longer term it would ideally be discoverable from a capability style register. A few other minor things inline, Thanks, Jonathan > > Before this patch, when counting fio for 10m the counts is incorrect: > root@localhost:/tmp# echo 0 > /sys/module/dwc_pcie_pmu/parameters/timer_enable > root@localhost:/tmp# perf stat -e dwc_rootport_20000/rx_pcie_tlp_data_payload/ -- fio --runtime=10m fio_job.config > [...] > Run status group 0 (all jobs): > READ: bw=5594MiB/s (5865MB/s), 5594MiB/s-5594MiB/s (5865MB/s-5865MB/s), io=3278GiB (3519GB), run=600010-600010msec > [...] > Performance counter stats for 'system wide': > 137,438,953,456 dwc_rootport_20000/rx_pcie_tlp_data_payload/ > > After this patch the counts is as expected: > root@localhost:/tmp# echo 1 > /sys/module/dwc_pcie_pmu/parameters/timer_enable > root@localhost:/tmp# perf stat -e dwc_rootport_20000/rx_pcie_tlp_data_payload/ -- fio --runtime=10m fio_job.config > [...] > Run status group 0 (all jobs): > READ: bw=5632MiB/s (5905MB/s), 5632MiB/s-5632MiB/s (5905MB/s-5905MB/s), io=3300GiB (3543GB), run=600013-600013msec > [...] > Performance counter stats for 'system wide': > 3,543,850,268,576 dwc_rootport_20000/rx_pcie_tlp_data_payload/ > > Signed-off-by: Yufan Dou > Signed-off-by: Yicong Yang > +static enum hrtimer_restart dwc_pcie_pmu_hrtimer_callback(struct hrtimer *hrtimer) > +{ > + struct dwc_pcie_pmu *pcie_pmu = container_of(hrtimer, struct dwc_pcie_pmu, hrtimer); > + struct perf_event *event = pcie_pmu->time_based_event; > + struct hw_perf_event *hwc; > + > + if (!event) > + return HRTIMER_NORESTART; > + > + hwc = &event->hw; > + if (hwc->state & PERF_HES_STOPPED) > + return HRTIMER_NORESTART; > + > + dwc_pcie_pmu_event_update(event); > + dwc_pcie_pmu_reset_time_based_counter(event); > + hrtimer_forward_now(hrtimer, ns_to_ktime(DWC_PCIE_PMU_TIMER_PERIOD_NS)); Small readability thing but I would put a blank line here. > + return HRTIMER_RESTART; > +} > + > static int dwc_pcie_pmu_event_init(struct perf_event *event) > { > struct dwc_pcie_pmu *pcie_pmu = to_dwc_pcie_pmu(event->pmu); > @@ -480,8 +526,13 @@ static void dwc_pcie_pmu_event_start(struct perf_event *event, int flags) > > if (type == DWC_PCIE_LANE_EVENT) > dwc_pcie_pmu_lane_event_enable(pcie_pmu, event, true); > - else if (type == DWC_PCIE_TIME_BASE_EVENT) > + else if (type == DWC_PCIE_TIME_BASE_EVENT) { Same as below - if () { } else if { } > dwc_pcie_pmu_time_based_event_enable(pcie_pmu, true); > + if (timer_enable) > + hrtimer_start(&pcie_pmu->hrtimer, > + ns_to_ktime(DWC_PCIE_PMU_TIMER_PERIOD_NS), > + HRTIMER_MODE_REL_PINNED_HARD); > + } > } > > static void dwc_pcie_pmu_event_stop(struct perf_event *event, int flags) > @@ -497,8 +548,10 @@ static void dwc_pcie_pmu_event_stop(struct perf_event *event, int flags) > > if (type == DWC_PCIE_LANE_EVENT) > dwc_pcie_pmu_lane_event_enable(pcie_pmu, event, false); > - else if (type == DWC_PCIE_TIME_BASE_EVENT) > + else if (type == DWC_PCIE_TIME_BASE_EVENT) { Style wise, all legs should have {} if any need it, so add them to the if above as well. > dwc_pcie_pmu_time_based_event_enable(pcie_pmu, false); > + hrtimer_cancel(&pcie_pmu->hrtimer); > + } > > hwc->state |= PERF_HES_STOPPED | PERF_HES_UPTODATE; > } > @@ -726,6 +779,8 @@ static int dwc_pcie_pmu_probe(struct platform_device *plat_dev) > pcie_pmu->ras_des_offset = vsec; > pcie_pmu->nr_lanes = pcie_get_width_cap(pdev); > pcie_pmu->on_cpu = -1; > + hrtimer_setup(&pcie_pmu->hrtimer, dwc_pcie_pmu_hrtimer_callback, > + CLOCK_MONOTONIC, HRTIMER_MODE_REL_PINNED_HARD); > pcie_pmu->pmu = (struct pmu){ > .name = name, > .parent = &plat_dev->dev,