Re: [PATCH 3/8] perf/arm-cmn: Ensure dtm_idx is big enough

[Ilkka Koskinen]

On Mon, 19 Aug 2024, Robin Murphy wrote:

> On 16/08/2024 11:14 am, Mark Rutland wrote:
> > On Fri, Aug 09, 2024 at 08:15:42PM +0100, Robin Murphy wrote:
> > > While CMN_MAX_DIMENSION was bumped to 12 for CMN-650, that only supports
> > > up to a 10x10 mesh, so bumping dtm_idx to 256 bits at the time worked
> > > out OK in practice. However CMN-700 did finally support up to 144 XPs,
> > > and thus needs a worst-case 288 bits of dtm_idx for an aggregated XP
> > > event on a maxed-out config. Oops.
> > >
> > > Fixes: 23760a014417 ("perf/arm-cmn: Add CMN-700 support")
> > > Signed-off-by: Robin Murphy <robin.murphy@xxxxxxx>
> > > ---
> > >   drivers/perf/arm-cmn.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/perf/arm-cmn.c b/drivers/perf/arm-cmn.c
> > > index 0e2e12e2f4fb..c9a2b21a7aec 100644
> > > --- a/drivers/perf/arm-cmn.c
> > > +++ b/drivers/perf/arm-cmn.c
> > > @@ -563,7 +563,7 @@ static void arm_cmn_debugfs_init(struct arm_cmn *cmn, 
> > > int id) {}
> > >     struct arm_cmn_hw_event {
> > >   	struct arm_cmn_node *dn;
> > > -	u64 dtm_idx[4];
> > > +	u64 dtm_idx[5];
> >
> > Can't we size this based on CMN_MAX_DIMENSION (or CMN_MAX_XPS or
> > CMN_MAX_DTMS), to make that clear?
>
> Fair enough, I did go back and forth a little on that idea, but reached the 
> opposite conclusion that documenting this with the assert to deliberately 
> make it *not* look straightforward was nicer than wrestling with an accurate 
> name for the logical quantity here, which strictly would be something like 
> CMN_MAX_NODES_PER_TYPE_WE_CARE_ABOUT (there can already be up to 256 RN-Fs, 
> but those aren't visible to the PMU).
>
> I'll have another think on that approach - I do concur that the assert isn't 
> *functionally* any better than automatically sizing the array.

I'm ok with the patch but automatic sizing would be nice as there would be 
one less thing to verify when the mesh size is increased again.

Cheers, Ilkka

> Thanks,
> Robin.
>
> >  From the desciription in the commit message it sounds like you need 2 *
> > CMN_MAX_XPS bits, i.e.
> >
> > 	#define DTM_IDX_BITS	(2 * CMN_MAX_XPS)
> >
> > 	u64 dtm_idx[DIV_ROUND_UP(DTM_IDX_BITS, 64)];
> >
> > Mark.
> >
> > >   	s8 dtc_idx[CMN_MAX_DTCS];
> > >   	u8 num_dns;
> > >   	u8 dtm_offset;
> > > --
> > > 2.39.2.101.g768bb238c484.dirty