Re: [PATCH v3 2/2] drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.

[Will Deacon]

On Tue, Aug 13, 2019 at 12:03:45PM +0100, Mark Rutland wrote:
> On Tue, Aug 13, 2019 at 04:25:15PM +0530, Ganapatrao Kulkarni wrote:
> > On Mon, Aug 12, 2019 at 5:31 PM Mark Rutland <mark.rutland@xxxxxxx> wrote:
> > >
> > > On Tue, Jul 23, 2019 at 09:16:28AM +0000, Ganapatrao Kulkarni wrote:
> > > > CCPI2 is a low-latency high-bandwidth serial interface for connecting
> > > > ThunderX2 processors. This patch adds support to capture CCPI2 perf events.
> > >
> > > It would be worth pointing out in the commit message how the CCPI2
> > > counters differ from the others. I realise you have that in the body of
> > > patch 1, but it's critical information when reviewing this patch...
> > 
> > Ok, I will add in next version.
> > >
> > > >
> > > > Signed-off-by: Ganapatrao Kulkarni <gkulkarni@xxxxxxxxxxx>
> > > > ---
> > > >  drivers/perf/thunderx2_pmu.c | 248 ++++++++++++++++++++++++++++++-----
> > > >  1 file changed, 214 insertions(+), 34 deletions(-)
> > > >
> > > > diff --git a/drivers/perf/thunderx2_pmu.c b/drivers/perf/thunderx2_pmu.c
> > > > index 43d76c85da56..a4e1273eafa3 100644
> > > > --- a/drivers/perf/thunderx2_pmu.c
> > > > +++ b/drivers/perf/thunderx2_pmu.c
> > > > @@ -17,22 +17,31 @@
> > > >   */
> > > >
> > > >  #define TX2_PMU_MAX_COUNTERS         4
> > >
> > > Shouldn't this be 8 now?
> > 
> > It is kept unchanged to 4(as suggested by Will), which is same for
> > both L3 and DMC.
> > For CCPI2 this macro is not used.
> 
> Hmmm....
> 
> I disagree with that suggestion given that this also affects the
> active_counters bitmap size (and thus it is not correctly sized as of
> this patch), and it doesn't really save us much.
> 
> I think it would be better to bump this to 8 and always update the
> events array, even though it will be unused for CCPI2. That's less
> surprising, needs fewer special-cases, and we can use the hrtimer
> function pointer alone to determine if we need to do any hrtimer work.

tbf, my complaint was actually about some macros applying to the whole
PMU whilst others refer only to DMC/L3C and this not being apparent from
the naming:

https://lkml.org/lkml/2019/6/27/250

so I'm fine having TX2_PMU_DMC_L3C_MAX_COUNTERS and
TX2_PMU_CCPI2_MAX_COUNTERS, but that sort of naming needs to be consistent
unless the macro/definition really applies to both. That fed the suggestion
that GET_EVENTID could be generic and switch on the event type internally
instead of at the caller.

Will