Re: [PATCH v8 1/4] docs: perf: Add description for Synopsys DesignWare PCIe PMU driver

[Jonathan Cameron]

On Wed, 18 Oct 2023 09:19:51 +0800
Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx> wrote:

> On 2023/10/17 17:16, Jonathan Cameron wrote:
> > On Tue, 17 Oct 2023 09:32:32 +0800
> > Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx> wrote:
> >   
> >> Alibaba's T-Head Yitan 710 SoC includes Synopsys' DesignWare Core PCIe
> >> controller which implements which implements PMU for performance and
> >> functional debugging to facilitate system maintenance.
> >>
> >> Document it to provide guidance on how to use it.
> >>
> >> Signed-off-by: Shuai Xue <xueshuai@xxxxxxxxxxxxxxxxx>
> >> Reviewed-by: Baolin Wang <baolin.wang@xxxxxxxxxxxxxxxxx>  
> > 
> > A few minor things inline and one question that I'd like a comment on
> > for my understanding at least! (why not multiply the counter by 16 and
> > make the maths simpler?)
> > 
> > With those tidied up,
> > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> >   
> 
> Thank you for providing prompt feedback and valuable comments to me.
> (please also see my replies inline)
> 
> Best Regards,
> Shuai
> 
> > 
> >   
> >> ---
> >>  .../admin-guide/perf/dwc_pcie_pmu.rst         | 94 +++++++++++++++++++
> >>  Documentation/admin-guide/perf/index.rst      |  1 +
> >>  2 files changed, 95 insertions(+)
> >>  create mode 100644 Documentation/admin-guide/perf/dwc_pcie_pmu.rst
> >>
> >> diff --git a/Documentation/admin-guide/perf/dwc_pcie_pmu.rst b/Documentation/admin-guide/perf/dwc_pcie_pmu.rst
> >> new file mode 100644
> >> index 000000000000..eac1b6f36450
> >> --- /dev/null
> >> +++ b/Documentation/admin-guide/perf/dwc_pcie_pmu.rst
> >> @@ -0,0 +1,94 @@
> >> +======================================================================
> >> +Synopsys DesignWare Cores (DWC) PCIe Performance Monitoring Unit (PMU)
> >> +======================================================================
> >> +
> >> +DesignWare Cores (DWC) PCIe PMU
> >> +===============================
> >> +
> >> +The PMU is a PCIe configuration space register block provided by each PCIe Root
> >> +Port in a Vendor-Specific Extended Capability named RAS D.E.S (Debug, Error
> >> +injection, and Statistics).
> >> +
> >> +As the name indicates, the RAS DES capability supports system level
> >> +debugging, AER error injection, and collection of statistics. To facilitate
> >> +collection of statistics, Synopsys DesignWare Cores PCIe controller
> >> +provides the following two features:
> >> +
> >> +- one 64-bit counter for Time Based Analysis (RX/TX data throughput and
> >> +  time spent in each low-power LTSSM state) and
> >> +- one 32-bit counter for Event Counting (error and non-error events for
> >> +  a specified lane)
> >> +
> >> +Note: There is no interrupt for counter overflow.
> >> +
> >> +Time Based Analysis
> >> +-------------------
> >> +
> >> +Using this feature you can obtain information regarding RX/TX data
> >> +throughput and time spent in each low-power LTSSM state by the controller.
> >> +The PMU measures data in two categories:
> >> +
> >> +- Group#0: Percentage of time the controller stays in LTSSM states.
> >> +- Group#1: Amount of data processed (Units of 16 bytes).
> >> +
> >> +Lane Event counters
> >> +-------------------
> >> +
> >> +Using this feature you can obtain Error and Non-Error information in
> >> +specific lane by the controller. The PMU event is select by:
> >> +
> >> +- Group i
> >> +- Event j within the Group i
> >> +- and Lane k  
> > The and here is a little confusing. I'd rework as
> > The PMU event is selected by all of:
> > - Group i
> > - Event j within the Group i
> > - Lane k  
> 
> Will rework it in next version.
> 
> >   
> >> +
> >> +Some of the event only exist for specific configurations.  
> > 
> > events  
> 
> Sorry for typo, will fix it.
> 
> >   
> >> +
> >> +DesignWare Cores (DWC) PCIe PMU Driver
> >> +=======================================
> >> +
> >> +This driver adds PMU devices for each PCIe Root Port named based on the BDF of
> >> +the Root Port. For example,
> >> +
> >> +    30:03.0 PCI bridge: Device 1ded:8000 (rev 01)
> >> +
> >> +the PMU device name for this Root Port is dwc_rootport_3018.
> >> +
> >> +The DWC PCIe PMU driver registers a perf PMU driver, which provides
> >> +description of available events and configuration options in sysfs, see
> >> +/sys/bus/event_source/devices/dwc_rootport_{bdf}.
> >> +
> >> +The "format" directory describes format of the config fields of the
> >> +perf_event_attr structure. The "events" directory provides configuration
> >> +templates for all documented events.  For example,
> >> +"Rx_PCIe_TLP_Data_Payload" is an equivalent of "eventid=0x22,type=0x1".
> >> +
> >> +The "perf list" command shall list the available events from sysfs, e.g.::
> >> +
> >> +    $# perf list | grep dwc_rootport
> >> +    <...>
> >> +    dwc_rootport_3018/Rx_PCIe_TLP_Data_Payload/        [Kernel PMU event]
> >> +    <...>
> >> +    dwc_rootport_3018/rx_memory_read,lane=?/               [Kernel PMU event]
> >> +
> >> +Time Based Analysis Event Usage
> >> +-------------------------------
> >> +
> >> +Example usage of counting PCIe RX TLP data payload (Units of 16 bytes)::
> >> +
> >> +    $# perf stat -a -e dwc_rootport_3018/Rx_PCIe_TLP_Data_Payload/
> >> +
> >> +The average RX/TX bandwidth can be calculated using the following formula:
> >> +
> >> +    PCIe RX Bandwidth = PCIE_RX_DATA * 16B / Measure_Time_Window
> >> +    PCIe TX Bandwidth = PCIE_TX_DATA * 16B / Measure_Time_Window  
> > 
> > Silly question (sorry I didn't raise it earlier) but can we make the interface
> > more intuitive by just multiplying the counter value at point of read by 16?  
> 
> Really a good suggestion, and it is very convenient for end perf users.
> But the unit of 16 is only applied to group#1 as described in Time Based Analysis
> section.

How hard would it be to just apply it to those events?
Userspace doesn't care what the hardware does underneath - it just wants to get
moderately intuitive data back. Having the end user deal with this oddity + even
the need to document it seems to me to be unnecessary burden given how simple it
is (I assume) to remove the oddity.

> 
> So I prefer to left the unit part to end users.
>