Add Cavium Coherent Processor Interconnect (CCPI2) PMU
support in ThunderX2 Uncore driver.
v3: Rebased to 5.3-rc1
v2: Updated with review comments [1]
[1] https://lkml.org/lkml/2019/6/14/965
v1: initial patch
Ganapatrao Kulkarni (2):
Documentation: perf: Update documentation for ThunderX2 PMU uncore
driver
drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
.../admin-guide/perf/thunderx2-pmu.rst | 20 +-
drivers/perf/thunderx2_pmu.c | 248 +++++++++++++++---
2 files changed, 225 insertions(+), 43 deletions(-)
--
2.17.1
CCPI2 is a low-latency high-bandwidth serial interface for connecting
ThunderX2 processors. This patch adds support to capture CCPI2 perf events.
Signed-off-by: Ganapatrao Kulkarni <[email protected]>
---
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
+
#define TX2_PMU_DMC_CHANNELS 8
#define TX2_PMU_L3_TILES 16
#define TX2_PMU_HRTIMER_INTERVAL (2 * NSEC_PER_SEC)
-#define GET_EVENTID(ev) ((ev->hw.config) & 0x1f)
-#define GET_COUNTERID(ev) ((ev->hw.idx) & 0x3)
+#define GET_EVENTID(ev, mask) ((ev->hw.config) & mask)
+#define GET_COUNTERID(ev, mask) ((ev->hw.idx) & mask)
/* 1 byte per counter(4 counters).
* Event id is encoded in bits [5:1] of a byte,
*/
#define DMC_EVENT_CFG(idx, val) ((val) << (((idx) * 8) + 1))
+/* bits[3:0] to select counters, are indexed from 8 to 15. */
+#define CCPI2_COUNTER_OFFSET 8
+
#define L3C_COUNTER_CTL 0xA8
#define L3C_COUNTER_DATA 0xAC
#define DMC_COUNTER_CTL 0x234
#define DMC_COUNTER_DATA 0x240
+#define CCPI2_PERF_CTL 0x108
+#define CCPI2_COUNTER_CTL 0x10C
+#define CCPI2_COUNTER_SEL 0x12c
+#define CCPI2_COUNTER_DATA_L 0x130
+
/* L3C event IDs */
#define L3_EVENT_READ_REQ 0xD
#define L3_EVENT_WRITEBACK_REQ 0xE
@@ -51,15 +60,23 @@
#define DMC_EVENT_READ_TXNS 0xF
#define DMC_EVENT_MAX 0x10
+#define CCPI2_EVENT_REQ_PKT_SENT 0x3D
+#define CCPI2_EVENT_SNOOP_PKT_SENT 0x65
+#define CCPI2_EVENT_DATA_PKT_SENT 0x105
+#define CCPI2_EVENT_GIC_PKT_SENT 0x12D
+#define CCPI2_EVENT_MAX 0x200
+
enum tx2_uncore_type {
PMU_TYPE_L3C,
PMU_TYPE_DMC,
+ PMU_TYPE_CCPI2,
PMU_TYPE_INVALID,
};
+
/*
- * 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.
*/
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);
- struct perf_event *events[TX2_PMU_MAX_COUNTERS];
+ struct perf_event **events;
struct device *dev;
struct hrtimer hrtimer;
const struct attribute_group **attr_groups;
@@ -92,7 +111,21 @@ static inline struct tx2_uncore_pmu *pmu_to_tx2_pmu(struct pmu *pmu)
return container_of(pmu, struct tx2_uncore_pmu, pmu);
}
-PMU_FORMAT_ATTR(event, "config:0-4");
+#define TX2_PMU_FORMAT_ATTR(_var, _name, _format) \
+static ssize_t \
+__tx2_pmu_##_var##_show(struct device *dev, \
+ struct device_attribute *attr, \
+ char *page) \
+{ \
+ BUILD_BUG_ON(sizeof(_format) >= PAGE_SIZE); \
+ return sprintf(page, _format "\n"); \
+} \
+ \
+static struct device_attribute format_attr_##_var = \
+ __ATTR(_name, 0444, __tx2_pmu_##_var##_show, NULL)
+
+TX2_PMU_FORMAT_ATTR(event, event, "config:0-4");
+TX2_PMU_FORMAT_ATTR(event_ccpi2, event, "config:0-9");
static struct attribute *l3c_pmu_format_attrs[] = {
&format_attr_event.attr,
@@ -104,6 +137,11 @@ static struct attribute *dmc_pmu_format_attrs[] = {
NULL,
};
+static struct attribute *ccpi2_pmu_format_attrs[] = {
+ &format_attr_event_ccpi2.attr,
+ NULL,
+};
+
static const struct attribute_group l3c_pmu_format_attr_group = {
.name = "format",
.attrs = l3c_pmu_format_attrs,
@@ -114,6 +152,11 @@ static const struct attribute_group dmc_pmu_format_attr_group = {
.attrs = dmc_pmu_format_attrs,
};
+static const struct attribute_group ccpi2_pmu_format_attr_group = {
+ .name = "format",
+ .attrs = ccpi2_pmu_format_attrs,
+};
+
/*
* sysfs event attributes
*/
@@ -164,6 +207,19 @@ static struct attribute *dmc_pmu_events_attrs[] = {
NULL,
};
+TX2_EVENT_ATTR(req_pktsent, CCPI2_EVENT_REQ_PKT_SENT);
+TX2_EVENT_ATTR(snoop_pktsent, CCPI2_EVENT_SNOOP_PKT_SENT);
+TX2_EVENT_ATTR(data_pktsent, CCPI2_EVENT_DATA_PKT_SENT);
+TX2_EVENT_ATTR(gic_pktsent, CCPI2_EVENT_GIC_PKT_SENT);
+
+static struct attribute *ccpi2_pmu_events_attrs[] = {
+ &tx2_pmu_event_attr_req_pktsent.attr.attr,
+ &tx2_pmu_event_attr_snoop_pktsent.attr.attr,
+ &tx2_pmu_event_attr_data_pktsent.attr.attr,
+ &tx2_pmu_event_attr_gic_pktsent.attr.attr,
+ NULL,
+};
+
static const struct attribute_group l3c_pmu_events_attr_group = {
.name = "events",
.attrs = l3c_pmu_events_attrs,
@@ -174,6 +230,11 @@ static const struct attribute_group dmc_pmu_events_attr_group = {
.attrs = dmc_pmu_events_attrs,
};
+static const struct attribute_group ccpi2_pmu_events_attr_group = {
+ .name = "events",
+ .attrs = ccpi2_pmu_events_attrs,
+};
+
/*
* sysfs cpumask attributes
*/
@@ -213,11 +274,23 @@ static const struct attribute_group *dmc_pmu_attr_groups[] = {
NULL
};
+static const struct attribute_group *ccpi2_pmu_attr_groups[] = {
+ &ccpi2_pmu_format_attr_group,
+ &pmu_cpumask_attr_group,
+ &ccpi2_pmu_events_attr_group,
+ NULL
+};
+
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);
+}
+
static inline void reg_writel(u32 val, unsigned long addr)
{
writel(val, (void __iomem *)addr);
@@ -245,33 +318,58 @@ static void init_cntr_base_l3c(struct perf_event *event,
struct tx2_uncore_pmu *tx2_pmu)
{
struct hw_perf_event *hwc = &event->hw;
+ u32 cmask;
+
+ tx2_pmu = pmu_to_tx2_pmu(event->pmu);
+ cmask = tx2_pmu->counters_mask;
/* counter ctrl/data reg offset at 8 */
hwc->config_base = (unsigned long)tx2_pmu->base
- + L3C_COUNTER_CTL + (8 * GET_COUNTERID(event));
+ + L3C_COUNTER_CTL + (8 * GET_COUNTERID(event, cmask));
hwc->event_base = (unsigned long)tx2_pmu->base
- + L3C_COUNTER_DATA + (8 * GET_COUNTERID(event));
+ + L3C_COUNTER_DATA + (8 * GET_COUNTERID(event, cmask));
}
static void init_cntr_base_dmc(struct perf_event *event,
struct tx2_uncore_pmu *tx2_pmu)
{
struct hw_perf_event *hwc = &event->hw;
+ u32 cmask;
+
+ tx2_pmu = pmu_to_tx2_pmu(event->pmu);
+ cmask = tx2_pmu->counters_mask;
hwc->config_base = (unsigned long)tx2_pmu->base
+ DMC_COUNTER_CTL;
/* counter data reg offset at 0xc */
hwc->event_base = (unsigned long)tx2_pmu->base
- + DMC_COUNTER_DATA + (0xc * GET_COUNTERID(event));
+ + DMC_COUNTER_DATA + (0xc * GET_COUNTERID(event, cmask));
+}
+
+static void init_cntr_base_ccpi2(struct perf_event *event,
+ struct tx2_uncore_pmu *tx2_pmu)
+{
+ struct hw_perf_event *hwc = &event->hw;
+ u32 cmask;
+
+ cmask = tx2_pmu->counters_mask;
+
+ hwc->config_base = (unsigned long)tx2_pmu->base
+ + CCPI2_COUNTER_CTL + (4 * GET_COUNTERID(event, cmask));
+ hwc->event_base = (unsigned long)tx2_pmu->base;
}
static void uncore_start_event_l3c(struct perf_event *event, int flags)
{
- u32 val;
+ u32 val, 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;
/* event id encoded in bits [07:03] */
- val = GET_EVENTID(event) << 3;
+ val = GET_EVENTID(event, emask) << 3;
reg_writel(val, hwc->config_base);
local64_set(&hwc->prev_count, 0);
reg_writel(0, hwc->event_base);
@@ -284,10 +382,17 @@ static inline void uncore_stop_event_l3c(struct perf_event *event)
static void uncore_start_event_dmc(struct perf_event *event, int flags)
{
- u32 val;
+ u32 val, cmask, emask;
struct hw_perf_event *hwc = &event->hw;
- int idx = GET_COUNTERID(event);
- int event_id = GET_EVENTID(event);
+ struct tx2_uncore_pmu *tx2_pmu;
+ int idx, event_id;
+
+ tx2_pmu = pmu_to_tx2_pmu(event->pmu);
+ cmask = tx2_pmu->counters_mask;
+ emask = tx2_pmu->events_mask;
+
+ idx = GET_COUNTERID(event, cmask);
+ event_id = GET_EVENTID(event, emask);
/* enable and start counters.
* 8 bits for each counter, bits[05:01] of a counter to set event type.
@@ -302,9 +407,14 @@ static void uncore_start_event_dmc(struct perf_event *event, int flags)
static void uncore_stop_event_dmc(struct perf_event *event)
{
- u32 val;
+ u32 val, cmask;
struct hw_perf_event *hwc = &event->hw;
- int idx = GET_COUNTERID(event);
+ struct tx2_uncore_pmu *tx2_pmu;
+ int idx;
+
+ tx2_pmu = pmu_to_tx2_pmu(event->pmu);
+ cmask = tx2_pmu->counters_mask;
+ idx = GET_COUNTERID(event, cmask);
/* clear event type(bits[05:01]) to stop counter */
val = reg_readl(hwc->config_base);
@@ -312,6 +422,31 @@ static void uncore_stop_event_dmc(struct perf_event *event)
reg_writel(val, hwc->config_base);
}
+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) |
+ GET_EVENTID(event, emask), hwc->config_base);
+ /* reset[4], enable[0] and start[1] counters */
+ reg_writel(0x13, hwc->event_base + CCPI2_PERF_CTL);
+ local64_set(&event->hw.prev_count, 0ULL);
+}
+
+static void uncore_stop_event_ccpi2(struct perf_event *event)
+{
+ struct hw_perf_event *hwc = &event->hw;
+
+ /* disable and stop counter */
+ reg_writel(0, hwc->event_base + CCPI2_PERF_CTL);
+}
+
static void tx2_uncore_event_update(struct perf_event *event)
{
s64 prev, delta, new = 0;
@@ -319,20 +454,30 @@ static void tx2_uncore_event_update(struct perf_event *event)
struct tx2_uncore_pmu *tx2_pmu;
enum tx2_uncore_type type;
u32 prorate_factor;
+ u32 cmask, emask;
tx2_pmu = pmu_to_tx2_pmu(event->pmu);
type = tx2_pmu->type;
+ cmask = tx2_pmu->counters_mask;
+ emask = tx2_pmu->events_mask;
prorate_factor = tx2_pmu->prorate_factor;
-
- new = reg_readl(hwc->event_base);
- prev = local64_xchg(&hwc->prev_count, new);
-
- /* handles rollover of 32 bit counter */
- delta = (u32)(((1UL << 32) - prev) + new);
+ if (type == PMU_TYPE_CCPI2) {
+ reg_writel(CCPI2_COUNTER_OFFSET +
+ GET_COUNTERID(event, cmask),
+ hwc->event_base + CCPI2_COUNTER_SEL);
+ new = reg_readq(hwc->event_base + CCPI2_COUNTER_DATA_L);
+ prev = local64_xchg(&hwc->prev_count, new);
+ delta = new - prev;
+ } else {
+ new = reg_readl(hwc->event_base);
+ prev = local64_xchg(&hwc->prev_count, new);
+ /* handles rollover of 32 bit counter */
+ delta = (u32)(((1UL << 32) - prev) + new);
+ }
/* DMC event data_transfers granularity is 16 Bytes, convert it to 64 */
if (type == PMU_TYPE_DMC &&
- GET_EVENTID(event) == DMC_EVENT_DATA_TRANSFERS)
+ GET_EVENTID(event, emask) == DMC_EVENT_DATA_TRANSFERS)
delta = delta/4;
/* L3C and DMC has 16 and 8 interleave channels respectively.
@@ -351,6 +496,7 @@ static enum tx2_uncore_type get_tx2_pmu_type(struct acpi_device *adev)
} devices[] = {
{"CAV901D", PMU_TYPE_L3C},
{"CAV901F", PMU_TYPE_DMC},
+ {"CAV901E", PMU_TYPE_CCPI2},
{"", PMU_TYPE_INVALID}
};
@@ -380,7 +526,8 @@ static bool tx2_uncore_validate_event(struct pmu *pmu,
* Make sure the group of events can be scheduled at once
* on the PMU.
*/
-static bool tx2_uncore_validate_event_group(struct perf_event *event)
+static bool tx2_uncore_validate_event_group(struct perf_event *event,
+ int max_counters)
{
struct perf_event *sibling, *leader = event->group_leader;
int counters = 0;
@@ -403,7 +550,7 @@ static bool tx2_uncore_validate_event_group(struct perf_event *event)
* If the group requires more counters than the HW has,
* it cannot ever be scheduled.
*/
- return counters <= TX2_PMU_MAX_COUNTERS;
+ return counters <= max_counters;
}
@@ -439,7 +586,7 @@ static int tx2_uncore_event_init(struct perf_event *event)
hwc->config = event->attr.config;
/* Validate the group */
- if (!tx2_uncore_validate_event_group(event))
+ if (!tx2_uncore_validate_event_group(event, tx2_pmu->max_counters))
return -EINVAL;
return 0;
@@ -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),
@@ -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;
/* set counter control and data registers base address */
tx2_pmu->init_cntr_base(event, tx2_pmu);
@@ -510,14 +659,17 @@ static void tx2_uncore_event_del(struct perf_event *event, int flags)
{
struct tx2_uncore_pmu *tx2_pmu = pmu_to_tx2_pmu(event->pmu);
struct hw_perf_event *hwc = &event->hw;
+ u32 cmask;
+ cmask = tx2_pmu->counters_mask;
tx2_uncore_event_stop(event, PERF_EF_UPDATE);
/* clear the assigned counter */
- free_counter(tx2_pmu, GET_COUNTERID(event));
+ free_counter(tx2_pmu, GET_COUNTERID(event, cmask));
perf_event_update_userpage(event);
- tx2_pmu->events[hwc->idx] = NULL;
+ if (tx2_pmu->events)
+ tx2_pmu->events[hwc->idx] = NULL;
hwc->idx = -1;
}
@@ -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;
+ }
ret = tx2_uncore_pmu_register(tx2_pmu);
if (ret) {
@@ -654,8 +810,10 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
switch (tx2_pmu->type) {
case PMU_TYPE_L3C:
tx2_pmu->max_counters = TX2_PMU_MAX_COUNTERS;
+ tx2_pmu->counters_mask = 0x3;
tx2_pmu->prorate_factor = TX2_PMU_L3_TILES;
tx2_pmu->max_events = L3_EVENT_MAX;
+ tx2_pmu->events_mask = 0x1f;
tx2_pmu->hrtimer_interval = TX2_PMU_HRTIMER_INTERVAL;
tx2_pmu->attr_groups = l3c_pmu_attr_groups;
tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
@@ -663,11 +821,15 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
tx2_pmu->init_cntr_base = init_cntr_base_l3c;
tx2_pmu->start_event = uncore_start_event_l3c;
tx2_pmu->stop_event = uncore_stop_event_l3c;
+ tx2_pmu->events = devm_kzalloc(dev, tx2_pmu->max_counters *
+ sizeof(*tx2_pmu->events), GFP_KERNEL);
break;
case PMU_TYPE_DMC:
tx2_pmu->max_counters = TX2_PMU_MAX_COUNTERS;
+ tx2_pmu->counters_mask = 0x3;
tx2_pmu->prorate_factor = TX2_PMU_DMC_CHANNELS;
tx2_pmu->max_events = DMC_EVENT_MAX;
+ tx2_pmu->events_mask = 0x1f;
tx2_pmu->hrtimer_interval = TX2_PMU_HRTIMER_INTERVAL;
tx2_pmu->attr_groups = dmc_pmu_attr_groups;
tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
@@ -675,6 +837,23 @@ static struct tx2_uncore_pmu *tx2_uncore_pmu_init_dev(struct device *dev,
tx2_pmu->init_cntr_base = init_cntr_base_dmc;
tx2_pmu->start_event = uncore_start_event_dmc;
tx2_pmu->stop_event = uncore_stop_event_dmc;
+ tx2_pmu->events = devm_kzalloc(dev, tx2_pmu->max_counters *
+ sizeof(*tx2_pmu->events), GFP_KERNEL);
+ break;
+ case PMU_TYPE_CCPI2:
+ /* CCPI2 has 8 counters */
+ tx2_pmu->max_counters = 8;
+ tx2_pmu->counters_mask = 0x7;
+ tx2_pmu->prorate_factor = 1;
+ tx2_pmu->max_events = CCPI2_EVENT_MAX;
+ tx2_pmu->events_mask = 0x1ff;
+ tx2_pmu->attr_groups = ccpi2_pmu_attr_groups;
+ tx2_pmu->name = devm_kasprintf(dev, GFP_KERNEL,
+ "uncore_ccpi2_%d", tx2_pmu->node);
+ tx2_pmu->init_cntr_base = init_cntr_base_ccpi2;
+ tx2_pmu->start_event = uncore_start_event_ccpi2;
+ tx2_pmu->stop_event = uncore_stop_event_ccpi2;
+ tx2_pmu->events = NULL;
break;
case PMU_TYPE_INVALID:
devm_kfree(dev, tx2_pmu);
@@ -744,7 +923,8 @@ static int tx2_uncore_pmu_offline_cpu(unsigned int cpu,
if (cpu != tx2_pmu->cpu)
return 0;
- hrtimer_cancel(&tx2_pmu->hrtimer);
+ if (tx2_pmu->type != PMU_TYPE_CCPI2)
+ hrtimer_cancel(&tx2_pmu->hrtimer);
cpumask_copy(&cpu_online_mask_temp, cpu_online_mask);
cpumask_clear_cpu(cpu, &cpu_online_mask_temp);
new_cpu = cpumask_any_and(
--
2.17.1
Hi Will,
Any comments to this patchset?
On Tue, Jul 23, 2019 at 2:46 PM Ganapatrao Kulkarni
<[email protected]> wrote:
>
> Add Cavium Coherent Processor Interconnect (CCPI2) PMU
> support in ThunderX2 Uncore driver.
>
> v3: Rebased to 5.3-rc1
>
> v2: Updated with review comments [1]
>
> [1] https://lkml.org/lkml/2019/6/14/965
>
> v1: initial patch
>
> Ganapatrao Kulkarni (2):
> Documentation: perf: Update documentation for ThunderX2 PMU uncore
> driver
> drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
>
> .../admin-guide/perf/thunderx2-pmu.rst | 20 +-
> drivers/perf/thunderx2_pmu.c | 248 +++++++++++++++---
> 2 files changed, 225 insertions(+), 43 deletions(-)
>
> --
> 2.17.1
>
Thanks,
Ganapat
On Mon, Jul 29, 2019 at 4:24 PM Ganapatrao Kulkarni <[email protected]> wrote:
>
> Hi Will,
>
> Any comments to this patchset?
If no further comments, can it be queued please?
>
> On Tue, Jul 23, 2019 at 2:46 PM Ganapatrao Kulkarni
> <[email protected]> wrote:
> >
> > Add Cavium Coherent Processor Interconnect (CCPI2) PMU
> > support in ThunderX2 Uncore driver.
> >
> > v3: Rebased to 5.3-rc1
> >
> > v2: Updated with review comments [1]
> >
> > [1] https://lkml.org/lkml/2019/6/14/965
> >
> > v1: initial patch
> >
> > Ganapatrao Kulkarni (2):
> > Documentation: perf: Update documentation for ThunderX2 PMU uncore
> > driver
> > drivers/perf: Add CCPI2 PMU support in ThunderX2 UNCORE driver.
> >
> > .../admin-guide/perf/thunderx2-pmu.rst | 20 +-
> > drivers/perf/thunderx2_pmu.c | 248 +++++++++++++++---
> > 2 files changed, 225 insertions(+), 43 deletions(-)
> >
> > --
> > 2.17.1
> >
>
> Thanks,
> Ganapat
Thanks,
Ganapat
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 <[email protected]>
> ---
> 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.
Hi Mark,
On Mon, Aug 12, 2019 at 5:31 PM Mark Rutland <[email protected]> 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 <[email protected]>
> > ---
> > 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
On Tue, Aug 13, 2019 at 04:25:15PM +0530, Ganapatrao Kulkarni wrote:
> Hi Mark,
>
> On Mon, Aug 12, 2019 at 5:31 PM Mark Rutland <[email protected]> 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 <[email protected]>
> > > ---
> > > 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.
Thanks,
Mark.
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 <[email protected]> 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 <[email protected]>
> > > > ---
> > > > 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
On Wed, Aug 21, 2019 at 10:23 PM Will Deacon <[email protected]> wrote:
>
> 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 <[email protected]> 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 <[email protected]>
> > > > > ---
> > > > > 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.
Thanks Will for the clarification.
I will send new version with changes as suggested.
>
> Will
Thanks,
Ganapat