From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from va-2-37.ptr.blmpb.com (va-2-37.ptr.blmpb.com [209.127.231.37]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A946032AADE for ; Tue, 16 Jun 2026 13:30:05 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=209.127.231.37 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781616608; cv=none; b=EWAI488f9Lg+dvR/Cod/nmqHynfksydXngktTTNsT3Oq9AIAlemfxo4Qp8VgRfqXK5qblObe+F4dj2PXGjOrPGNNdGMrMW2wQGe8opUFY7Is8BLRaEZ44k+7E/phb+bH/B4NDV6BbHv744lxK6O08d7UyRsPyqVaL1/FFpHtSnc= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1781616608; c=relaxed/simple; bh=E5SD5fHEMZHJFO3v+C+wlZTZzjYQdC9RF2brMO7jjPk=; h=From:Mime-Version:In-Reply-To:Cc:Subject:Message-Id:Date: Content-Type:To:References; b=Ioib+nJ57kmMYJ/dLyYY/T39zJ6IfUXPArYQxKKNIO+0WZ92RaQOW7EvarB8mmTbSgK/eZSRPHSJS8CRNo6jL6SweGvt+ZB6FSFb/u/eyzgQjBDYKPQJH0qzgEdoG42XZ4aQGw7/H7poSXM53zyB906km7T5SUaouyafijYMYVQ= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=picoheart.com; spf=pass smtp.mailfrom=picoheart.com; dkim=pass (2048-bit key) header.d=picoheart.com header.i=@picoheart.com header.b=W2YKTR3B; arc=none smtp.client-ip=209.127.231.37 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=quarantine dis=none) header.from=picoheart.com Authentication-Results: smtp.subspace.kernel.org; spf=pass smtp.mailfrom=picoheart.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=picoheart.com header.i=@picoheart.com header.b="W2YKTR3B" DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; s=feishu2604151535; d=picoheart.com; t=1781616600; h=from:subject: mime-version:from:date:message-id:subject:to:cc:reply-to:content-type: mime-version:in-reply-to:message-id; bh=ahko22ZEv+alF//ghqD1ZyLcP3MqTwuxU/84c3tL1nc=; b=W2YKTR3BK/THGJMNeyo9x1QuMyEIlmZTFsZ/q/LlceCjx29PfAvav17IX0EItgd8ACcf6Y Xdic5EivoAutgXCA6XVMbtKcs1ehOqZxi5m9PxOSAQlzjRN11ve9WP95F6faHtZ9Byzmhx K5q8sEgWR1TeZ4hkCG/vdFVB66dItoP43bYWINNvb2FFg4KEjqSjg2zCzBj//GfKkFRBsh R/SfKbM97xUENykQz7VClYrzUgXpbeqGFxu9PErsTsHvToVh6clODJPDUg+RVlO+lPJlfS qcU3fb8cUKtXce/WYRC82ghrBlqLs0kmOl41ens7BJRR8k9AUCmazWCSSrZX5A== From: "Yicong Yang" User-Agent: Mozilla Thunderbird Received: from [100.86.221.205] ([120.234.91.246]) by smtp.feishu.cn with ESMTPS; Tue, 16 Jun 2026 21:29:57 +0800 Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: Mime-Version: 1.0 Content-Transfer-Encoding: 7bit In-Reply-To: <20260616123406.4d812fab@jic23-huawei> Cc: , , , , , , , , , , , Subject: Re: [PATCH v1 2/3] perf/dwc_pcie: Support narrowed time-based counter for long time monitoring Message-Id: <8602e60f-98dc-4b5b-8a7f-cf62c1d4a6dc@picoheart.com> Date: Tue, 16 Jun 2026 21:29:55 +0800 Content-Language: en-US Content-Type: text/plain; charset=UTF-8 X-Lms-Return-Path: To: "Jonathan Cameron" X-Original-From: Yicong Yang References: <20260615063459.25361-1-yang.yicong@picoheart.com> <20260615063459.25361-3-yang.yicong@picoheart.com> <20260616123406.4d812fab@jic23-huawei> On 6/16/26 7:34 PM, Jonathan Cameron wrote: > 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? sure, we can make this opt in on our platform first by matching the root port's vendor ID. it's implemented as a module parameter since no discovery mechanism for this, then user can enable this if they found they can only do short time monitoring (symptom attached in the commit) > > 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. I may found that it's hard to regard this as a feature or bug, but the implementation choice of the vendor... > > A few other minor things inline, > will fix in the next version. Thanks. > 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,