Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Gowthami Thiagarajan <gthiagarajan@marvell.com>
To:     Mark Rutland <mark.rutland@arm.com>
CC:     "will@kernel.org" <will@kernel.org>,
        "linux-arm-kernel@lists.infradead.org" 
        <linux-arm-kernel@lists.infradead.org>,
        "linux-kernel@vger.kernel.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: Re: [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor
 support
Thread-Topic: [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance monitor
 support
Thread-Index: AQHZzSQHepKdKzDa8EaGKWjytks74A==
Date:   Sat, 12 Aug 2023 13:51:00 +0000
Message-ID: <SN1PR18MB21262B71F310B0AF33C2D2EFDB11A@SN1PR18MB2126.namprd18.prod.outlook.com>
References: <20230630120351.1143773-1-gthiagarajan@marvell.com>
 <20230630120351.1143773-4-gthiagarajan@marvell.com>
 <ZMPg8RNxEeHQNdqb@FVFF77S0Q05N>
In-Reply-To: <ZMPg8RNxEeHQNdqb@FVFF77S0Q05N>
Accept-Language: en-IN, en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?WB12u9LQ47rPqtu5JweyibwAR7H1Wy55NXk/WCCX0zzwlN9qbOEKGvku9wIS?=
 =?us-ascii?Q?imH+J6SbB04tW6yv5d73GS0bZI99TqaTfTo/3HQP4HyQB7VsuiYweJlo20Po?=
 =?us-ascii?Q?Up5M+S0B4nXzV4OgpXrirCjpLZ0FXoOqQnSEFioxCR8sKijW84MucGkwOYa9?=
 =?us-ascii?Q?ju/TUxP494uLKOZKsWieXfapfjzRDuau3SrQyxr4ghwxiRbzclBGrG8siug/?=
 =?us-ascii?Q?GiN8lq/JFOBC+T1q5u3ez6b4cIXswPXfJ9NLFZJrSNACeS42tt7UMGhcMmgJ?=
 =?us-ascii?Q?xZ2bqtOeJ2+P/hitk/dnVxR06V2gAUkOCnbnOs/xAMkOZu+fvtv5AGe4SDWS?=
 =?us-ascii?Q?fnK/5N/LsvLWoy+4idMfETh9nHhjWegAr4xOFnQSGArYj0yMMdai43htGukL?=
 =?us-ascii?Q?jCDe2LfLliQ676H59XKt0GVRWEO8ZUq13UFpZlVmzhBGxbr2hydVfHY9ns/u?=
 =?us-ascii?Q?0sPsShSukOl3QnNIR7R41yrbMGSWf46fquFWuFT5anD1Vq6rNcWy45p6L7GR?=
 =?us-ascii?Q?/2ft/5CuzGmLMkjp89iayPVmwwc/6Pc4oZE2OUgI8b3A1tfbJyWX62n2bLRx?=
 =?us-ascii?Q?QEupMIjll4R9tGmRu3wfwQ/5D7ASTH/JpBAOLn7IO1gBvBkKNyrjKKiFnGcz?=
 =?us-ascii?Q?xIZbntEFvRPrXlCuIIBJPAQmGfOMzTLKdHOYOel2Bd78EJdlXP1ur7K8fLcu?=
 =?us-ascii?Q?adhl/cIGZgiE2tE2hVjnCXWA0krpWdkXSN24OZNLo9Z8JEw1TUonn7HINskA?=
 =?us-ascii?Q?VU5TBo/EI2t4tJ2RfRBN91/do4jRacH3q/hufqz2Cgfz5zIdgGBksitbo6sF?=
 =?us-ascii?Q?sd1kqviwP8+pAv1wMIbdIVFV0c1z7EkeYFChPLA/82hASydojOzrol9sTk0w?=
 =?us-ascii?Q?iFi8jR42K54y5ZtoQH54kKAmNE32J5H4MT68nYB0VUegL2700ruW/lrnkfc4?=
 =?us-ascii?Q?GKwSK/gKOMH7LvUJCec4pxSvhkwPLJnQyqiWAkvLKCqlNBPJ78wp1jBDpGPc?=
 =?us-ascii?Q?kadk76RDG/Coe0uopddtwh/V/V4kjzO5fayOLaKPIp0Ydi4Z7wU+934trLSb?=
 =?us-ascii?Q?LtRuY7AjglGSGf3fe1bh8MxLP0NNywtJAwKiCpWVJdiRId0t7iefspRmap/B?=
 =?us-ascii?Q?tI908QlnCwE4Kt552+sS7wrD1YrLZVVYFyDeKykPE7D2BL3Qvpsxx9XZxEBc?=
 =?us-ascii?Q?TOOjdnZGPg1FXRsKerCPaQgTrSr+K0J6ffD2TduKEIsXPMXdQKuv/koLWBr+?=
 =?us-ascii?Q?nllFQWYtA7SehUPfMV1ftTCaAu2cba3TpaZGmYr5lqEcta4NpgcHIxnOYZY3?=
 =?us-ascii?Q?dUAr09eItsBc9bSCtmxrshGW5JoB5Lvq1vuZsfVXih3EHga95cPDL4YoIHKH?=
 =?us-ascii?Q?lbUK5OVdS5bKPZB4Vs7DWd76tXsENF3giTRLWP9dezWHimQfByZf0d5+heMV?=
 =?us-ascii?Q?CZkEtA0UKbqFnk1cPj9R9yKkHRxi/CcCJrnY/cIN/kP606KVGDKR8poJLK5I?=
 =?us-ascii?Q?Vem86gfl2nmz1PrNGSBr+Eq0vWPl5rSCo6FJVq4kwlwagOPEu5BWhlxLsqdL?=
 =?us-ascii?Q?VV32g/5chR9nW/sxmDudAQgpGe9NRBoYSNnhIsQ7?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-OriginatorOrg: marvell.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: SN1PR18MB2126.namprd18.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: b2dcc2b2-ceb1-42ea-0d58-08db9b3b29d5
X-MS-Exchange-CrossTenant-originalarrivaltime: 12 Aug 2023 13:51:00.9928
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 70e1fb47-1155-421d-87fc-2e58f638b6e0
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: v9vpcAzpUE2pKa5DL6rFFEVmr8xi+/47zZEZ/x7uvqiY1X6pM2VMqWd7CHb2t1WHXj8dSv3BF+VhWWsk7nDyatdpD4EzAeSQyTDF/hgbtFo=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SA0PR18MB3645
X-Proofpoint-GUID: gHwuZdF5AUpk2J6v91cmlTcBMlGFrFm4
X-Proofpoint-ORIG-GUID: gHwuZdF5AUpk2J6v91cmlTcBMlGFrFm4
X-Proofpoint-Virus-Version: vendor=baseguard
 engine=ICAP:2.0.267,Aquarius:18.0.957,Hydra:6.0.591,FMLib:17.11.176.26
 definitions=2023-08-12_13,2023-08-10_01,2023-05-22_02
X-Spam-Status: No, score=-1.9 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,RCVD_IN_DNSWL_BLOCKED,SPF_HELO_NONE,SPF_PASS autolearn=ham
        autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

Hi,

> -----Original Message-----
> From: Mark Rutland <mark.rutland@arm.com>
> Sent: Friday, July 28, 2023 9:08 PM
> To: Gowthami Thiagarajan <gthiagarajan@marvell.com>
> Cc: will@kernel.org; linux-arm-kernel@lists.infradead.org; linux-kernel@v=
ger.kernel.org; Sunil Kovvuri
> Goutham <sgoutham@marvell.com>; Bharat Bhushan <bbhushan2@marvell.com>; G=
eorge Cherian
> <gcherian@marvell.com>; Linu Cherian <lcherian@marvell.com>
> Subject: [EXT] Re: [PATCH 3/6] perf/marvell : Odyssey LLC-TAD performance=
 monitor support
>=20
> External Email
>=20
> ----------------------------------------------------------------------
> On Fri, Jun 30, 2023 at 05:33:48PM +0530, Gowthami Thiagarajan wrote:
> > Each TAD provides eight 64-bit counters for monitoring
> > cache behavior.The driver always configures the same counter for
> > all the TADs. The user would end up effectively reserving one of
> > eight counters in every TAD to look across all TADs.
> > The occurrences of events are aggregated and presented to the user
> > at the end of running the workload. The driver does not provide a
> > way for the user to partition TADs so that different TADs are used for
> > different applications.
> >
> > The performance events reflect various internal or interface activities=
.
> > By combining the values from multiple performance counters, cache
> > performance can be measured in terms such as: cache miss rate, cache
> > allocations, interface retry rate, internal resource occupancy, etc.
> >
> > Each supported counter's event and formatting information is exposed
> > to sysfs at /sys/devices/tad/. Use perf tool stat command to measure
> > the pmu events. For instance:
> >
> > perf stat -e tad_hit_ltg,tad_hit_dtg <workload>
> >
> > Signed-off-by: Gowthami Thiagarajan <gthiagarajan@marvell.com>
>=20
> This generally looks ok; I have a few comments below.
>=20
> [...]
>=20
> > +static void tad_pmu_event_counter_stop(struct perf_event *event, int f=
lags)
> > +{
> > +	struct tad_pmu *tad_pmu =3D to_tad_pmu(event->pmu);
> > +	struct hw_perf_event *hwc =3D &event->hw;
> > +	u32 counter_idx =3D hwc->idx;
> > +	int tad_region;
> > +
> > +	/* TAD()_PFC() stop counting on the write
> > +	 * which sets TAD()_PRF()[CNTSEL] =3D=3D 0
> > +	 */
>=20
> Please fix the comment style.
>=20
> Likewise for all other instances within this file.
>=20
> [...]
Ack.
>=20
> > +static int tad_pmu_event_counter_add(struct perf_event *event, int fla=
gs)
> > +{
> > +	struct tad_pmu *tad_pmu =3D to_tad_pmu(event->pmu);
> > +	struct hw_perf_event *hwc =3D &event->hw;
> > +	int idx;
> > +
> > +	/* Get a free counter for this event */
> > +	idx =3D find_first_zero_bit(tad_pmu->counters_map, TAD_MAX_COUNTERS);
> > +	if (idx =3D=3D TAD_MAX_COUNTERS)
> > +		return -EAGAIN;
> > +
> > +	set_bit(idx, tad_pmu->counters_map);
> > +
> > +	hwc->idx =3D idx;
> > +	hwc->state =3D PERF_HES_STOPPED;
> > +	tad_pmu->events[idx] =3D event;
> > +
> > +	if (flags & PERF_EF_START)
> > +		tad_pmu_event_counter_start(event, flags);
> > +
> > +	return 0;
> > +}
> > +
> > +static int tad_pmu_event_init(struct perf_event *event)
> > +{
> > +	struct tad_pmu *tad_pmu =3D to_tad_pmu(event->pmu);
> > +
> > +	if (event->attr.type !=3D event->pmu->type)
> > +		return -ENOENT;
>=20
> Why is this not rejecting smapling events, as patch 1 does?
This is missed here. Will add the check for rejecting sampling events.
>=20
> > +
> > +	if (!event->attr.disabled)
> > +		return -EINVAL;
>=20
> Why?
Just checks the default disabled attribute.
>=20
> > +
> > +	if (event->state !=3D PERF_EVENT_STATE_OFF)
> > +		return -EINVAL;
>=20
> Event groups need to be verified here too.
>=20
> [...]
Ack. Will add the missed checks in the next version.
>=20
> > +static int tad_pmu_probe(struct platform_device *pdev)
> > +{
> > +	struct device *dev =3D &pdev->dev;
> > +	struct tad_region *regions;
> > +	struct tad_pmu *tad_pmu;
> > +	struct resource *res;
> > +	u32 tad_pmu_page_size;
> > +	u32 tad_page_size;
> > +	u32 tad_cnt;
> > +	int i, ret;
> > +	char *name;
> > +
> > +	tad_pmu =3D devm_kzalloc(&pdev->dev, sizeof(*tad_pmu), GFP_KERNEL);
> > +	if (!tad_pmu)
> > +		return -ENOMEM;
> > +
> > +	platform_set_drvdata(pdev, tad_pmu);
> > +
> > +	res =3D platform_get_resource(pdev, IORESOURCE_MEM, 0);
> > +	if (!res) {
> > +		dev_err(&pdev->dev, "Mem resource not found\n");
> > +		return -ENODEV;
> > +	}
> > +
> > +	ret =3D device_property_read_u32(dev, "marvell,tad-page-size", &tad_p=
age_size);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Can't find tad-page-size property\n");
> > +		return ret;
> > +	}
> > +
> > +	ret =3D device_property_read_u32(dev, "marvell,tad-pmu-page-size",
> > +				       &tad_pmu_page_size);
> > +	if (ret) {
> > +		dev_err(&pdev->dev, "Can't find tad-pmu-page-size property\n");
> > +		return ret;
> > +	}
>=20
> Why do you think these properties are necessary?
>=20
> These should almost certainly be provided by IO resources, and shouldn't =
need a
> custom property.

IO resources don't provide all the information in this case. Need to vary t=
hese values
at different boot times. So, kept these custom properties.

Thanks,
Gowthami
>=20
> Thanks,
> Mark.