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

[Ganapatrao Kulkarni <gklkml16@gmail.com>]

Hi Mark,

On Mon, Aug 12, 2019 at 5:31 PM Mark Rutland <mark.rutland@arm.com> 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@marvell.com>
> > ---
> >  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.

>
> [...]
>
> >  /*
> > - * pmu on each socket has 2 uncore devices(dmc and l3c),
> > - * each device has 4 counters.
> > + * pmu on each socket has 3 uncore devices(dmc, l3ci and ccpi2),
> > + * dmc and l3c has 4 counters and ccpi2 8.
> >   */
>
> How about:
>
> /*
>  * Each socket has 3 uncore device associated with a PMU. The DMC and
>  * L3C have 4 32-bit counters, and the CCPI2 has 8 64-bit counters.
>  */

Thanks.
>
> >  struct tx2_uncore_pmu {
> >       struct hlist_node hpnode;
> > @@ -69,12 +86,14 @@ struct tx2_uncore_pmu {
> >       int node;
> >       int cpu;
> >       u32 max_counters;
> > +     u32 counters_mask;
> >       u32 prorate_factor;
> >       u32 max_events;
> > +     u32 events_mask;
> >       u64 hrtimer_interval;
> >       void __iomem *base;
> >       DECLARE_BITMAP(active_counters, TX2_PMU_MAX_COUNTERS);
>
> This bitmap isn't big enough for the 4 new counters.
>
> > -     struct perf_event *events[TX2_PMU_MAX_COUNTERS];
> > +     struct perf_event **events;
>
> As above, can't we bump TX2_PMU_MAX_COUNTERS to 8 rather than making
> this a dynamic allocation?

events is only relevant for L3 and DMC since they use timer callbacks.
This is done as per previous review comments.

>
> [...]
>
> >  static inline u32 reg_readl(unsigned long addr)
> >  {
> >       return readl((void __iomem *)addr);
> >  }
> >
> > +static inline u32 reg_readq(unsigned long addr)
> > +{
> > +     return readq((void __iomem *)addr);
> > +}
>
> Presumably reg_readq() should return a u64.

Yes,  My bad.

>
> [...]
>
> > +static void uncore_start_event_ccpi2(struct perf_event *event, int flags)
> > +{
> > +     u32 emask;
> > +     struct hw_perf_event *hwc = &event->hw;
> > +     struct tx2_uncore_pmu *tx2_pmu;
> > +
> > +     tx2_pmu = pmu_to_tx2_pmu(event->pmu);
> > +     emask = tx2_pmu->events_mask;
> > +
> > +     /* Bit [09:00] to set event id, set level and type to 1 */
> > +     reg_writel((3 << 10) |
>
> Do you mean that bits [11:10] are level and type?

Yes, i will change the comment.
>
> What exactly are 'level' and 'type'?

They are for other settings which are not relevant for software/kernel.
>
> Can we give those bits mnemonics?
>
> > +                     GET_EVENTID(event, emask), hwc->config_base);
> > +     /* reset[4], enable[0] and start[1] counters */
>
> Rather than using magic numbers everywhere, please give these mnemonics,
> e.g.
>
> #define CCPI2_PERF_CTL_ENABLE   BIT(0)
> #define CCPI2_PERF_CTL_START    BIT(1)
> #define CCPI2_PERF_CTL_RESET    BIT(4)

not used everywhere, only in this function.
I can add these macros.

>
> > +     reg_writel(0x13, hwc->event_base + CCPI2_PERF_CTL);
>
> ... and then you can OR them in here:

OK
>
>         ctl = CCPI2_PERF_CTL_ENABLE |
>               CCPI2_PERF_CTL_START |
>               CCPI2_PERF_CTL_RESET;
>         reg_writel(ctl, hwc->event_base + CCPI2_PERF_CTL);
>
> [...]
>
> > @@ -456,8 +603,9 @@ static void tx2_uncore_event_start(struct perf_event *event, int flags)
> >       tx2_pmu->start_event(event, flags);
> >       perf_event_update_userpage(event);
> >
> > -     /* Start timer for first event */
> > -     if (bitmap_weight(tx2_pmu->active_counters,
> > +     /* Start timer for first non ccpi2 event */
> > +     if (tx2_pmu->type != PMU_TYPE_CCPI2 &&
> > +                     bitmap_weight(tx2_pmu->active_counters,
> >                               tx2_pmu->max_counters) == 1) {
> >               hrtimer_start(&tx2_pmu->hrtimer,
> >                       ns_to_ktime(tx2_pmu->hrtimer_interval),
>
> This would be easier to read as two statements:
>
>         /* No hrtimer needed with 64-bit counters */
>         if (tx2_pmu->type == PMU_TYPE_CCPI2)
>                 return;
>
>         /* Start timer for first event */
>         if (bitmap_weight(tx2_pmu->active_counters,
>             tx2_pmu->max_counters) != 1) {
>                 ...
>         }
>

OK, makes sense.

> > @@ -495,7 +643,8 @@ static int tx2_uncore_event_add(struct perf_event *event, int flags)
> >       if (hwc->idx < 0)
> >               return -EAGAIN;
> >
> > -     tx2_pmu->events[hwc->idx] = event;
> > +     if (tx2_pmu->events)
> > +             tx2_pmu->events[hwc->idx] = event;
>
> So this is NULL for CCPI2?

Yes.
>
> I guess we don't strictly need the if we don't have a hrtimer to update
> event counts, but this makes the code more complicated than it needs to
> be.

Yes I am using tx2_pmu->events to differentiate the type, it is NULL for CCPI2.
I can extend same to tx2_uncore_event_start().
>
> [...]
>
> > @@ -580,8 +732,12 @@ static int tx2_uncore_pmu_add_dev(struct tx2_uncore_pmu *tx2_pmu)
> >                       cpu_online_mask);
> >
> >       tx2_pmu->cpu = cpu;
> > -     hrtimer_init(&tx2_pmu->hrtimer, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > -     tx2_pmu->hrtimer.function = tx2_hrtimer_callback;
> > +     /* CCPI2 counters are 64 bit counters, no overflow  */
> > +     if (tx2_pmu->type != PMU_TYPE_CCPI2) {
> > +             hrtimer_init(&tx2_pmu->hrtimer,
> > +                             CLOCK_MONOTONIC, HRTIMER_MODE_REL);
> > +             tx2_pmu->hrtimer.function = tx2_hrtimer_callback;
> > +     }
>
> Hmmm... this means that tx2_pmu->hrtimer.function is NULL for the CCPI2
> PMU. I think it would be best to check that when (re)programming the
> counters rather than the PMU type. For example, in
> tx2_uncore_event_start() we could have:
>
>         if (!tx2_pmu->hrtimer.function)
>                 return;
>         if (bitmap_weight(tx2_pmu->active_counters,
>             tx2_pmu->max_counters) != 1) {
>                 ...
>         }
>

Yes it is NULL for CCPI2.
Ok, I can use tx2_pmu->events instead(like other places).

> Thanks,
> Mark.

Thanks,
Ganapat

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel