| Message ID | aa9d45b10814dc6b2c59dfb2c1be49f333dae1dc.1723229941.git.robin.murphy@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | perf/arm-cmn: Fixes and updates | expand |
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@arm.com> > --- > 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? 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 >
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@arm.com> >> --- >> 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. 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 >>
On Mon, Aug 19, 2024 at 04:00:06PM +0100, 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@arm.com> > > > --- > > > 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. FWIW, I'm happy with the value being an over-estimate, and with needing a comment. What I'm really after is that the sizing of dtm_idx is clear at the definition of dtm_idx, without an arbitrary-looking number. Mark.
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@arm.com> >>> --- >>> 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 >>> >
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]; s8 dtc_idx[CMN_MAX_DTCS]; u8 num_dns; u8 dtm_offset;
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@arm.com> --- drivers/perf/arm-cmn.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)