Hi Mark,

On Mon, Aug 12, 2019 at 5:31 PM Mark Rutland [off-list ref] wrote:

On Tue, Jul 23, 2019 at 09:16:28AM +0000, Ganapatrao Kulkarni wrote:

quoted

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.

quoted

Signed-off-by: Ganapatrao Kulkarni <redacted>
---
 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.

[...]

quoted

 /*
- * 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.

quoted

 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.

quoted

-     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.

[...]

quoted

 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.

[...]

quoted

+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?

quoted

+                     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.

quoted

+     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);

[...]

quoted

@@ -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.

quoted

@@ -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().

[...]

quoted

@@ -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