RE: [EXT] Re: [PATCH 1/6] perf/marvell: Marvell PEM performance monitor support

Gowthami Thiagarajan
2023-08-12T03:43:47Z

Hi,

> -----Original Message-----
> From: Mark Rutland <mark.rutland@arm.com>
> Sent: Friday, July 28, 2023 8:32 PM
> To: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> Cc: will@kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@vger.kernel.org; Sunil Kovvuri
> Goutham <sgoutham@marvell.com>; Bharat Bhushan <bbhushan2@marvell.com>; George Cherian
> <gcherian@marvell.com>; Linu Cherian <lcherian@marvell.com>
> Subject: [EXT] Re: [PATCH 1/6] perf/marvell: Marvell PEM performance monitor support
>
> External Email
>
> ----------------------------------------------------------------------
> Hi,
>
> On Fri, Jun 30, 2023 at 05:33:46PM +0530, Gowthami Thiagarajan 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: Linu Cherian <lcherian@marvell.com>
> > Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
>
> This generally looks fine; I have a few comments below.
>
> [...]
>
> > diff --git a/drivers/perf/marvell_pem_pmu.c b/drivers/perf/marvell_pem_pmu.c
> > new file mode 100644
> > index 000000000000..fb27112aa7d4
> > --- /dev/null
> > +++ b/drivers/perf/marvell_pem_pmu.c
> > @@ -0,0 +1,433 @@
> > +// SPDX-License-Identifier: GPL-2.0
> > +/* Marvell PEM(PCIe RC) Performance Monitor Driver
> > + *
> > + * Copyright (C) 2023 Marvell.
> > + */
>
> Nit: please follow the preferred coding style for comments. This should have a
> newline immediately after the '/*', e.g.
>
> /*
> * Marvell PEM(PCIe RC) Performance Monitor Driver
> *
> * Copyright (C) 2023 Marvell.
> */
>
> Likewise for all other multi-line comments.

Ack.
>
> > +#include <linux/acpi.h>
> > +#include <linux/init.h>
> > +#include <linux/io.h>
> > +#include <linux/module.h>
> > +#include <linux/of.h>
> > +#include <linux/of_address.h>
> > +#include <linux/of_device.h>
> > +#include <linux/perf_event.h>
> > +
> > +/* Each of these events maps to a free running 64 bit counter
> > + * with no event control, but can be reset.
> > + *
> > + */
> > +enum pem_events {
> > + IB_TLP_NPR,
> > + IB_TLP_PR,
> > + IB_TLP_CPL,
>
> > +static u64 eventid_to_offset_table[] = {
> > + 0x0,
> > + 0x8,
> > + 0x10,
>
> I assume the event IDs are the values in the pem_events enum, so please use
> array initalizers here to make that clear, e.g.
>
> static u64 eventid_to_offset_table[] = {
> [IB_TLP_NPR] = 0x0,
> [IB_TLP_PR] = 0x8,
> [IB_TLP_CPL] 0x10,
> ...
> };
>
> [...]

Ack. Yes. IDs refer to the pem_events.

>
> > +static int pem_perf_event_init(struct perf_event *event)
> > +{
> > + struct pem_pmu *pmu = to_pem_pmu(event->pmu);
> > + struct hw_perf_event *hwc = &event->hw;
> > +
> > + if (event->attr.type != event->pmu->type)
> > + return -ENOENT;
> > +
> > + if (is_sampling_event(event)) {
>
> Don't we also need to check for:
>
> event->attach_state & PERF_ATTACH_TASK

Ack. Will add this check.

>
> > + dev_info(pmu->dev, "Sampling not supported!\n");
> > + return -EOPNOTSUPP;
> > + }
>
> Please delete this dev_info().

Ack.
>
> > +
> > + if (event->cpu < 0) {
> > + dev_warn(pmu->dev, "Can't provide per-task data!\n");
> > + return -EOPNOTSUPP;
> > + }
>
> Likewise, please delete this dev_warn().
Ack.
>
> > +
> > + /* We must NOT create groups containing mixed PMUs */
> > + if (event->group_leader->pmu != event->pmu &&
> > + !is_software_event(event->group_leader))
> > + return -EINVAL;
> > +
> > + /* Set ownership of event to one CPU, same event can not be observed
> > + * on multiple cpus at same time.
> > + */
>
> Please fix this comment style (or delete the comment).
Ack.

-Thanks,
Gowthami
>
> > + event->cpu = pmu->cpu;
> > + hwc->idx = -1;
> > + return 0;
> > +}
>
> Thanks,
> Mark.