Hi Ganapat,

On Thu, Jun 21, 2018 at 12:03:38PM +0530, Ganapatrao Kulkarni wrote:

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-doc" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html