linux-kernel - Re: [PATCH v8] perf/marvell: Marvell PEM performance monitor support

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240924174046.0000242d@Huawei.com>
Date: Tue, 24 Sep 2024 17:40:46 +0100
From: Jonathan Cameron <Jonathan.Cameron@...wei.com>
To: Gowthami Thiagarajan <gthiagarajan@...vell.com>
CC: <will@...nel.org>, <mark.rutland@....com>,
	<linux-arm-kernel@...ts.infradead.org>, <linux-kernel@...r.kernel.org>,
	<andrew@...n.ch>, <sgoutham@...vell.com>, <lcherian@...vell.com>,
	<gcherian@...vell.com>
Subject: Re: [PATCH v8] perf/marvell: Marvell PEM performance monitor
 support

On Tue, 24 Sep 2024 12:01:26 +0530
Gowthami Thiagarajan <gthiagarajan@...vell.com> wrote:

> PCI Express Interface PMU includes various performance counters
> to monitor the data that is transmitted over the PCIe link. The
> counters track various inbound and outbound transactions which
> includes separate counters for posted/non-posted/completion TLPs.
> Also, inbound and outbound memory read requests along with their
> latencies can also be monitored. Address Translation Services(ATS)events
> such as ATS Translation, ATS Page Request, ATS Invalidation along with
> their corresponding latencies are also supported.
> 
> The performance counters are 64 bits wide.
> 
> For instance,
> perf stat -e ib_tlp_pr <workload>
> tracks the inbound posted TLPs for the workload.
> 
> Signed-off-by: Gowthami Thiagarajan <gthiagarajan@...vell.com>

A few quick comments inline from a superficial look.

Jonathan


> diff --git a/drivers/perf/marvell_pem_pmu.c b/drivers/perf/marvell_pem_pmu.c
> new file mode 100644
> index 000000000000..d3aca94278fb
> --- /dev/null
> +++ b/drivers/perf/marvell_pem_pmu.c
> @@ -0,0 +1,427 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * Marvell PEM(PCIe RC) Performance Monitor Driver
> + *
> + * Copyright (C) 2024 Marvell.
> + */
> +
> +#include <linux/acpi.h>
> +#include <linux/init.h>
> +#include <linux/io.h>
> +#include <linux/module.h>
> +#include <linux/perf_event.h>
> +#include <linux/platform_device.h>
> +
> +/*
> + * Each of these events maps to a free running 64 bit counter
> + * with no event control, but can be reset.
> + *

This blank line adds nothing. I'd drop it.

> + */
> +enum pem_events {
> +	IB_TLP_NPR,
> +	IB_TLP_PR,
> +	IB_TLP_CPL,
> +	IB_TLP_DWORDS_NPR,
> +	IB_TLP_DWORDS_PR,
> +	IB_TLP_DWORDS_CPL,
> +	IB_INFLIGHT,
> +	IB_READS,
> +	IB_REQ_NO_RO_NCB,
> +	IB_REQ_NO_RO_EBUS,
> +	OB_TLP_NPR,
> +	OB_TLP_PR,
> +	OB_TLP_CPL,
> +	OB_TLP_DWORDS_NPR,
> +	OB_TLP_DWORDS_PR,
> +	OB_TLP_DWORDS_CPL,
> +	OB_INFLIGHT,
> +	OB_READS,
> +	OB_MERGES_NPR,
> +	OB_MERGES_PR,
> +	OB_MERGES_CPL,
> +	ATS_TRANS,
> +	ATS_TRANS_LATENCY,
> +	ATS_PRI,
> +	ATS_PRI_LATENCY,
> +	ATS_INV,
> +	ATS_INV_LATENCY,
> +	PEM_EVENTIDS_MAX,

A comma after a MAX entry rarely makes sense. I'd drop it.

> +};



> +static int pem_pmu_offline_cpu(unsigned int cpu, struct hlist_node *node)
> +{
> +	struct pem_pmu *pmu = hlist_entry_safe(node, struct pem_pmu,
> +					       node);

Why wrap? It's under 80 chars anyway.

> +	unsigned int target;
> +
> +	if (cpu != pmu->cpu)
> +		return 0;
> +
> +	target = cpumask_any_but(cpu_online_mask, cpu);
> +	if (target >= nr_cpu_ids)
> +		return 0;
> +
> +	perf_pmu_migrate_context(&pmu->pmu, cpu, target);
> +	pmu->cpu = target;
> +	return 0;
> +}

> +
> +#ifdef CONFIG_ACPI
> +static const struct acpi_device_id pem_pmu_acpi_match[] = {
> +	{"MRVL000E", 0},
> +	{},

No need for trailing comma.

> +};
> +MODULE_DEVICE_TABLE(acpi, pem_pmu_acpi_match);
> +#endif
> +
> +static struct platform_driver pem_pmu_driver = {
> +	.driver	= {
> +		.name   = "pem-pmu",
> +		.acpi_match_table = ACPI_PTR(pem_pmu_acpi_match),

Drop the ACPI_PTR() protection and the ifdefs.

They provide very little advantage and hurt readabilty.
Maybe make sense if the driver supports dt binding but this
one doesn't.

> +		.suppress_bind_attrs = true,
> +	},
> +	.probe		= pem_perf_probe,
> +	.remove		= pem_perf_remove,
> +};

> diff --git a/include/linux/cpuhotplug.h b/include/linux/cpuhotplug.h
> index 9316c39260e0..254491a6d09b 100644
> --- a/include/linux/cpuhotplug.h
> +++ b/include/linux/cpuhotplug.h
> @@ -228,6 +228,7 @@ enum cpuhp_state {
>  	CPUHP_AP_PERF_ARM_APM_XGENE_ONLINE,
>  	CPUHP_AP_PERF_ARM_CAVIUM_TX2_UNCORE_ONLINE,
>  	CPUHP_AP_PERF_ARM_MARVELL_CN10K_DDR_ONLINE,
> +	CPUHP_AP_PERF_ARM_MRVL_PEM_ONLINE,

Silly question but which of the rules at: 
https://elixir.bootlin.com/linux/v6.11/source/include/linux/cpuhotplug.h#L45
means this can't use CPUHP_AP_ONLINE_DYN?

Quite a few recent PMU drivers have used that without issues.

>  	CPUHP_AP_PERF_POWERPC_NEST_IMC_ONLINE,
>  	CPUHP_AP_PERF_POWERPC_CORE_IMC_ONLINE,
>  	CPUHP_AP_PERF_POWERPC_THREAD_IMC_ONLINE,