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...
>
> 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?
[...]
> /*
> - * 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.
*/
> 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?
[...]
> 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.
[...]
> +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?
What exactly are 'level' and 'type'?
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)
> + reg_writel(0x13, hwc->event_base + CCPI2_PERF_CTL);
... and then you can OR them in here:
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) {
...
}
> @@ -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?
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.
[...]
> @@ -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) {
...
}
Thanks,
Mark.
