Re: [RFC PATCH v3 1/2] drivers/perf: hisi: Add description for HNS3 PMU driver

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Sat, 8 May 2021 17:44:55 +0800
Guangbin Huang <huangguangbin2@huawei.com> wrote:

> HNS3 PMU End Point device is supported on HiSilicon HIP09 platform, so
> add document hns3-pmu.rst to provide guidance on how to use it.
> 
> Signed-off-by: Guangbin Huang <huangguangbin2@huawei.com>

A few question and suggestions inline.

Thanks,

Jonathan

> ---
>  Documentation/admin-guide/perf/hns3-pmu.rst | 129 ++++++++++++++++++++++++++++
>  1 file changed, 129 insertions(+)
>  create mode 100644 Documentation/admin-guide/perf/hns3-pmu.rst
> 
> diff --git a/Documentation/admin-guide/perf/hns3-pmu.rst b/Documentation/admin-guide/perf/hns3-pmu.rst
> new file mode 100644
> index 0000000..9695688
> --- /dev/null
> +++ b/Documentation/admin-guide/perf/hns3-pmu.rst
> @@ -0,0 +1,129 @@
> +======================================
> +HNS3 Performance Monitoring Unit (PMU)
> +======================================
> +
> +HNS3(HiSilicon network system 3) Performance Monitoring Unit (PMU) is an
> +End Point device to collect performance statistics of HiSilicon SoC NIC.
> +On Hip09, each SICL(Super I/O cluster) has one PMU device.
> +
> +HNS3 PMU is supported to collect performance statistics of bandwidth,

Perhaps replace with:

HNS3 PMU supports collection of performance statistics such as bandwidth,
latency, packet rate and interrupt rate.

> +latency, packet rate and interrupt rate.
> +
> +Each HNS3 PMU supports up to 8 hardware events.

Do we have hardware versions that support less?  If not replace with:

Each HNS3 PMU supports 8 hardware events.

> +
> +HNS3 PMU driver
> +===============
> +
> +The HNS3 PMU driver registers a perf PMU with the name of its device id.::
> +
> +  /sys/devices/hns3_pmu_<device_id>
> +
> +The device_id is read from hardware register, it contains information of
> +chip_id(bit 31:2) and SICL_ID(bit 1:0). One chip may have one or more SICL.
> +
> +PMU driver provides description of available events, filter modes, format,
> +identifier and cpumask in sysfs.
> +
> +The "events" directory describes the event code and subevent code of all
> +supported events shown in perf list.
> +
> +The "filtermode" directory describes the supported filter modes of each
> +event.
> +
> +The "format" directory describes all formats of the config (events) and
> +config1 (filter options) fields of the perf_event_attr structure.
> +
> +The "identifier" file shows version of PMU hardware device.
> +
> +Example usage of checking event code and subevent code::
> +
> +  $# cat /sys/devices/hns3_pmu_0/events/bw_igu_ssu
> +  config=0x0000
> +
> +The upper 8 bits of config is event code, lower 8 bits of config is
> +subevent code.

Could you use an example that isn't 0x0000?   That way you could illustrate
this comment by saying something like:

   config=0x0103

The upper 8 bits of config (here 0x01) are the event code. The lower 8 bits
of config (here 0x03) are the subevent code.

> +
> +Example usage of checking supported filter mode::
> +
> +  $# cat /sys/devices/hns3_pmu_0/filtermode/bw_igu_ssu
> +  filter mode supported: global/port/port-tc/

This formatting seems unusual for a sysfs attribute are there similar
examples already in use?

> +
> +Example usage of perf::
> +
> +  $# perf list
> +  hns3_pmu_0/bw_igu_ssu/ [kernel PMU event]
> +  ------------------------------------------
> +
> +  $# perf stat -a -e hns3_pmu_0/bw_igu_ssu,global=1/ -I 1000
> +  or
> +  $# perf stat -a -e hns3_pmu_0/event=0,subevent=0,global=1/ -I 1000
> +
> +The current driver does not support sampling. So "perf record" is unsupported.

One sentence:
The current driver does not support sampling, so "perf record" is unsupported.

Given this is effectively an uncore device, I'd imagine there is no sensible way
of supporting sampling?  In which case I'd just say it is not supported.

> +Also attach to a task is unsupported for HNS3 PMU.
> +
> +Filter modes
> +--------------
> +
> +1. global mode
> +PMU collect performance statistic of all functions of IO DIE. Set the

statistics for all

> +"global" filter option to 1 will enable this mode.

I'm assuming HNS related functions of the IO die?  Perhaps "HNS3 PCI functions"
is worth saying rather than just "functions"

> +Example usage of perf::
> +
> +  $# perf stat -a -e hns3_pmu_0/event=0,subevent=0,global=1/ -I 1000
> +
> +2. port mode
> +PMU collect performance statistic of one whole physical port. The port id
> +is same as mac id. The "tc" filter option must be set to 0xF in this mode.
> +Example usage of perf::
> +
> +  $# perf stat -a -e hns3_pmu_0/event=0,subevent=0,port=0,tc=0xF/ -I 1000
> +
> +3. port-tc mode
> +PMU collect performance statistic of one tc of physical port. The port id
> +is same as mac id. The "tc" filter option must be set to 0 ~ 7 in this
> +mode.

Say what tc stands for - I would assume traffic class?

> +Example usage of perf::
> +
> +  $# perf stat -a -e hns3_pmu_0/event=0,subevent=0,port=0,tc=0/ -I 1000
> +
> +4. func mode
> +PMU collect performance statistic of one PF/VF. The function id is BDF of
> +PF/VF, its conversion formula::
> +
> +  func = (bus << 8) + (device << 3) + (function)
> +
> +for example:
> +  BDF         func
> +  35:00.0    0x3500
> +  35:00.1    0x3501
> +  35:01.0    0x3508
> +
> +In this mode, the "queue" filter option must be set to 0xFFFF.
> +Example usage of perf::
> +
> +  $# perf stat -a -e hns3_pmu_0/event=0,subevent=0,bdf=0x3500,queue=0xFFFF/ -I 1000
> +
> +5. func-queue mode
> +PMU collect performance statistic of one queue of PF/VF. The function id
> +is BDF of PF/VF, its conversion formula::
> +
> +  func = (bus << 8) + (device << 3) + (function)
> +
> +In this mode, the "queue" filter option must be set to the exact queue id
> +of function.
> +Example usage of perf::
> +
> +  $# perf stat -a -e hns3_pmu_0/event=0,subevent=0,bdf=0x3500,queue=0/ -I 1000
> +
> +

Drop one blank line here for consistency

> +6. func-intr mode
> +PMU collect performance statistic of one interrupt of PF/VF. The function
> +id is BDF of PF/VF, its conversion formula::
> +
> +  func = (bus << 8) + (device << 3) + (function)
> +
> +In this mode, the "intr" filter option must be set to the exact interrupt
> +id of function.
> +Example usage of perf::
> +
> +  $# perf stat -a -e hns3_pmu_0/event=0,subevent=0,bdf=0x3500,intr=0/ -I 1000


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel