lkml.org 
[lkml]   [2018]   [Jul]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH v6 2/2] ThunderX2: Add Cavium ThunderX2 SoC UNCORE PMU driver
Hi Ganapat,

On Thu, Jun 21, 2018 at 12:03:38PM +0530, Ganapatrao Kulkarni wrote:
> This patch adds a perf driver for the PMU UNCORE devices DDR4 Memory
> Controller(DMC) and Level 3 Cache(L3C).
>
> ThunderX2 has 8 independent DMC PMUs to capture performance events
> corresponding to 8 channels of DDR4 Memory Controller and 16 independent
> L3C PMUs to capture events corresponding to 16 tiles of L3 cache.
> Each PMU supports up to 4 counters. All counters lack overflow interrupt
> and are sampled periodically.
>
> Signed-off-by: Ganapatrao Kulkarni <ganapatrao.kulkarni@cavium.com>
> ---
> drivers/perf/Kconfig | 8 +
> drivers/perf/Makefile | 1 +
> drivers/perf/thunderx2_pmu.c | 949 +++++++++++++++++++++++++++++++++++++++++++
> include/linux/cpuhotplug.h | 1 +
> 4 files changed, 959 insertions(+)
> create mode 100644 drivers/perf/thunderx2_pmu.c
>
> diff --git a/drivers/perf/Kconfig b/drivers/perf/Kconfig
> index 08ebaf7..ecedb9e 100644
> --- a/drivers/perf/Kconfig
> +++ b/drivers/perf/Kconfig
> @@ -87,6 +87,14 @@ config QCOM_L3_PMU
> Adds the L3 cache PMU into the perf events subsystem for
> monitoring L3 cache events.
>
> +config THUNDERX2_PMU
> + bool "Cavium ThunderX2 SoC PMU UNCORE"
> + depends on ARCH_THUNDER2 && ARM64 && ACPI

Surely this depends on NUMA for the node id?

[...]

> +/*
> + * pmu on each socket has 2 uncore devices(dmc and l3),
> + * each uncore device has up to 16 channels, each channel can sample
> + * events independently with counters up to 4.
> + */
> +struct thunderx2_pmu_uncore_channel {
> + struct pmu pmu;
> + struct hlist_node node;
> + struct thunderx2_pmu_uncore_dev *uncore_dev;
> + int channel;
> + int cpu;
> + DECLARE_BITMAP(active_counters, UNCORE_MAX_COUNTERS);
> + struct perf_event *events[UNCORE_MAX_COUNTERS];
> + struct hrtimer hrtimer;
> + /* to sync counter alloc/release */
> + raw_spinlock_t lock;
> +};

This lock should not be necessary. The pmu::{add,del} callbacks are
strictly serialised w.r.t. one another per CPU because the core perf
code holds ctx->lock whenever it calls either.

Previously, you mentioned you'd seen scheduling while atomic when this
lock was removed. Could you please provide a backtrace?

[...]

It would be worth a comment here explaining which resources the channel
PMUs share, and why we need this thunderx2_pmu_uncore_dev. IIUC, it's
just that they share a register windows switched by FW?

> +struct thunderx2_pmu_uncore_dev {
> + char *name;
> + struct device *dev;
> + enum thunderx2_uncore_type type;
> + void __iomem *base;
> + int node;
> + u32 max_counters;
> + u32 max_channels;
> + u32 max_events;
> + u64 hrtimer_interval;
> + /* this lock synchronizes across channels */
> + raw_spinlock_t lock;
> + const struct attribute_group **attr_groups;
> + void (*init_cntr_base)(struct perf_event *event,
> + struct thunderx2_pmu_uncore_dev *uncore_dev);
> + void (*select_channel)(struct perf_event *event);
> + void (*stop_event)(struct perf_event *event);
> + void (*start_event)(struct perf_event *event, int flags);
> +};

As a general thing, the really long structure/enum/macro names make
things really hard to read.

Could we s/THUNDERX2/TX2/ and s/thunderx2/tx2/ for identifiers please?

Similarly:

* s/thunderx2_pmu_uncore_channel/tx2_pmu/

* s/thunderx2_pmu_uncore_dev/tx2_pmu_group/

... and consistently name those "pmu" and "pmu_group" respectively --
that mekas the relationship between the two much clearer for someone who
is not intimately familiar with the hardware.

That makes things far more legible, e.g.

> +static inline struct thunderx2_pmu_uncore_channel *
> +pmu_to_thunderx2_pmu_uncore(struct pmu *pmu)
> +{
> + return container_of(pmu, struct thunderx2_pmu_uncore_channel, pmu);
> +}

... becomes:

static struct tx2_pmu *pmu_to_tx2_pmu(struct pmu *pmu)
{
return container_of(pmu, struct tx2_pmu, pmu);
}

[...]

> +/*
> + * sysfs cpumask attributes
> + */
> +static ssize_t cpumask_show(struct device *dev,
> + struct device_attribute *attr, char *buf)
> +{
> + struct cpumask cpu_mask;
> + struct thunderx2_pmu_uncore_channel *pmu_uncore =
> + pmu_to_thunderx2_pmu_uncore(dev_get_drvdata(dev));
> +
> + cpumask_clear(&cpu_mask);
> + cpumask_set_cpu(pmu_uncore->cpu, &cpu_mask);
> + return cpumap_print_to_pagebuf(true, buf, &cpu_mask);
> +}
> +static DEVICE_ATTR_RO(cpumask);

This can be simplified to:

static ssize_t cpumask_show(struct device *dev, struct device_attribute
*attr, char *buf)
{
struct thunderx2_pmu_uncore_channel *pmu_uncore;
pmu_uncore = pmu_to_thunderx2_pmu_uncore(dev_get_drvdata(dev));

return cpumap_print_to_pagebuf(true, buf, cpumask_of(pmu_uncore->cpu));
}

[...]

> +static int alloc_counter(struct thunderx2_pmu_uncore_channel *pmu_uncore)
> +{
> + int counter;
> +
> + raw_spin_lock(&pmu_uncore->lock);
> + counter = find_first_zero_bit(pmu_uncore->active_counters,
> + pmu_uncore->uncore_dev->max_counters);
> + if (counter == pmu_uncore->uncore_dev->max_counters) {
> + raw_spin_unlock(&pmu_uncore->lock);
> + return -ENOSPC;
> + }
> + set_bit(counter, pmu_uncore->active_counters);
> + raw_spin_unlock(&pmu_uncore->lock);
> + return counter;
> +}
> +
> +static void free_counter(
> + struct thunderx2_pmu_uncore_channel *pmu_uncore, int counter)
> +{
> + raw_spin_lock(&pmu_uncore->lock);
> + clear_bit(counter, pmu_uncore->active_counters);
> + raw_spin_unlock(&pmu_uncore->lock);
> +}

As above, I still don't believe that these locks are necessary, unless
we have a bug elsewhere.

[...]

> +/*
> + * DMC and L3 counter interface is muxed across all channels.
> + * hence we need to select the channel before accessing counter
> + * data/control registers.
> + *
> + * L3 Tile and DMC channel selection is through SMC call
> + * SMC call arguments,
> + * x0 = THUNDERX2_SMC_CALL_ID (Vendor SMC call Id)
> + * x1 = THUNDERX2_SMC_SET_CHANNEL (Id to set DMC/L3C channel)
> + * x2 = Node id
> + * x3 = DMC(1)/L3C(0)
> + * x4 = channel Id
> + */

Please document which ID space the node id is in, e.g.

x2 = FW Node ID (matching SRAT/SLIT IDs)

... so that it's clear that this isn't a Linux logical node id, even if
those happen to be the same today.

[...]

> +static void thunderx2_uncore_start(struct perf_event *event, int flags)
> +{
> + struct hw_perf_event *hwc = &event->hw;
> + struct thunderx2_pmu_uncore_channel *pmu_uncore;
> + struct thunderx2_pmu_uncore_dev *uncore_dev;
> + unsigned long irqflags;
> +
> + hwc->state = 0;
> + pmu_uncore = pmu_to_thunderx2_pmu_uncore(event->pmu);
> + uncore_dev = pmu_uncore->uncore_dev;
> +
> + raw_spin_lock_irqsave(&uncore_dev->lock, irqflags);
> + uncore_dev->select_channel(event);
> + uncore_dev->start_event(event, flags);
> + raw_spin_unlock_irqrestore(&uncore_dev->lock, irqflags);
> + perf_event_update_userpage(event);
> +
> + if (!find_last_bit(pmu_uncore->active_counters,
> + pmu_uncore->uncore_dev->max_counters)) {

This would be clearer using !bitmap_empty().

> + hrtimer_start(&pmu_uncore->hrtimer,
> + ns_to_ktime(uncore_dev->hrtimer_interval),
> + HRTIMER_MODE_REL_PINNED);
> + }
> +}

[...]

> +static enum hrtimer_restart thunderx2_uncore_hrtimer_callback(
> + struct hrtimer *hrt)

s/hrt/timer/ please

> +{
> + struct thunderx2_pmu_uncore_channel *pmu_uncore;
> + unsigned long irqflags;
> + int idx;
> + bool restart_timer = false;
> +
> + pmu_uncore = container_of(hrt, struct thunderx2_pmu_uncore_channel,
> + hrtimer);
> +
> + raw_spin_lock_irqsave(&pmu_uncore->uncore_dev->lock, irqflags);
> + for_each_set_bit(idx, pmu_uncore->active_counters,
> + pmu_uncore->uncore_dev->max_counters) {
> + struct perf_event *event = pmu_uncore->events[idx];
> +
> + thunderx2_uncore_update(event);
> + restart_timer = true;
> + }
> + raw_spin_unlock_irqrestore(&pmu_uncore->uncore_dev->lock, irqflags);
> +
> + if (restart_timer)
> + hrtimer_forward_now(hrt,
> + ns_to_ktime(
> + pmu_uncore->uncore_dev->hrtimer_interval));
> +
> + return restart_timer ? HRTIMER_RESTART : HRTIMER_NORESTART;
> +}

You don't need to take the lock at all if there are no active events,
and we can avoid all the conditional logic that way. e.g. assuming the
renames I requested above:

static enum hrtimer_restart tx2_hrtimer_callback(struct hrtimer *timer)

{
struct tx2_pmu *pmu;
struct tx2_pmu_group *pmu_group;
unsigned long irqflags;
int max_counters;
int idx;

pmu = container_of(timer, struct tx2_pmu, hrtimer);
pmu_group = pmu->pmu_group;
max_counters = pmu_group->max_counters;

if (cpumask_empty(pmu->active_counters, max_counters))
return HRTIMER_NORESTART;

raw_spin_lock_irqsave(&pmu_group->lock, irqflags);
for_each_set_bit(idx, pmu->active_counters, max_counters) {
struct perf_event *event = pmu->events[idx];

tx2_uncore_update(event);
restart_timer = true;
}
raw_spin_unlock_irqrestore(&pmu_group->lock, irqflags);

hrtimer_forward_now(hrt, ns_to_ktime(pmu_group->hrtimer_interval));

return HRTIMER_RESTART;
}

[...]

> + switch (uncore_dev->type) {
> + case PMU_TYPE_L3C:
> + uncore_dev->max_counters = UNCORE_MAX_COUNTERS;
> + uncore_dev->max_channels = UNCORE_L3_MAX_TILES;
> + uncore_dev->max_events = L3_EVENT_MAX;
> + uncore_dev->hrtimer_interval = UNCORE_HRTIMER_INTERVAL;
> + uncore_dev->attr_groups = l3c_pmu_attr_groups;
> + uncore_dev->name = devm_kasprintf(dev, GFP_KERNEL,
> + "uncore_l3c_%d", uncore_dev->node);
> + uncore_dev->init_cntr_base = init_cntr_base_l3c;
> + uncore_dev->start_event = uncore_start_event_l3c;
> + uncore_dev->stop_event = uncore_stop_event_l3c;
> + uncore_dev->select_channel = uncore_select_channel;
> + break;
> + case PMU_TYPE_DMC:
> + uncore_dev->max_counters = UNCORE_MAX_COUNTERS;
> + uncore_dev->max_channels = UNCORE_DMC_MAX_CHANNELS;
> + uncore_dev->max_events = DMC_EVENT_MAX;
> + uncore_dev->hrtimer_interval = UNCORE_HRTIMER_INTERVAL;
> + uncore_dev->attr_groups = dmc_pmu_attr_groups;
> + uncore_dev->name = devm_kasprintf(dev, GFP_KERNEL,
> + "uncore_dmc_%d", uncore_dev->node);
> + uncore_dev->init_cntr_base = init_cntr_base_dmc;
> + uncore_dev->start_event = uncore_start_event_dmc;
> + uncore_dev->stop_event = uncore_stop_event_dmc;
> + uncore_dev->select_channel = uncore_select_channel;
> + break;

We should probably s/uncore/tx2/, or s/uncore/thunderx2/ to namespace
this.

> +static int thunderx2_uncore_pmu_offline_cpu(unsigned int cpu,
> + struct hlist_node *node)
> +{
> + int new_cpu;
> + struct thunderx2_pmu_uncore_channel *pmu_uncore;
> +
> + pmu_uncore = hlist_entry_safe(node,
> + struct thunderx2_pmu_uncore_channel, node);
> + if (cpu != pmu_uncore->cpu)
> + return 0;
> +
> + new_cpu = cpumask_any_and(
> + cpumask_of_node(pmu_uncore->uncore_dev->node),
> + cpu_online_mask);
> + if (new_cpu >= nr_cpu_ids)
> + return 0;
> +
> + pmu_uncore->cpu = new_cpu;
> + perf_pmu_migrate_context(&pmu_uncore->pmu, cpu, new_cpu);
> + return 0;
> +}

We'll also need a onlining callback. Consider if all CPUs in a NUMA node
were offlined, then we tried to online an arbitrary CPU from that node.

Thanks,
Mark.
\
 
 \ /
  Last update: 2018-07-15 22:06    [W:0.118 / U:0.088 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site