Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1399592 > unrolled thread

[PATCH v2 00/32] 2nd Iteration of Cache QoS Monitoring support.

Back to article view | Back to linux.kernel

Contents

  [PATCH v2 00/32] 2nd Iteration of Cache QoS Monitoring support. David Carrillo-Cisneros <davidcc@google.com> - 2016-05-12 01:10 +0200
    [PATCH v2 16/32] perf/x86/intel/cqm: add cgroup support David Carrillo-Cisneros <davidcc@google.com> - 2016-05-12 01:10 +0200
    [PATCH v2 11/32] perf/x86/intel/cqm: add per-package RMID rotation David Carrillo-Cisneros <davidcc@google.com> - 2016-05-12 01:10 +0200
      Re: [PATCH v2 11/32] perf/x86/intel/cqm: add per-package RMID  rotation Thomas Gleixner <tglx@linutronix.de> - 2016-05-18 23:40 +0200
        Re: [PATCH v2 11/32] perf/x86/intel/cqm: add per-package RMID rotation David Carrillo-Cisneros <davidcc@google.com> - 2016-05-24 23:10 +0200
    [PATCH v2 07/32] perf/x86/intel/cqm: add helpers for per-package locking David Carrillo-Cisneros <davidcc@google.com> - 2016-05-12 01:20 +0200
      Re: [PATCH v2 07/32] perf/x86/intel/cqm: add helpers for per-package  locking Thomas Gleixner <tglx@linutronix.de> - 2016-05-18 19:40 +0200
        Re: [PATCH v2 07/32] perf/x86/intel/cqm: add helpers for per-package  locking Thomas Gleixner <tglx@linutronix.de> - 2016-05-18 21:20 +0200
    [PATCH v2 04/32] perf/x86/intel/cqm: add constants for CQM David Carrillo-Cisneros <davidcc@google.com> - 2016-05-12 01:20 +0200
    [PATCH v2 02/32] perf/x86/intel/cqm: software cache for MSR_IA32_PQR_ASSOC David Carrillo-Cisneros <davidcc@google.com> - 2016-05-12 01:20 +0200
    [PATCH v2 03/32] x86/intel,cqm: add CONFIG_INTEL_RDT configuration flag David Carrillo-Cisneros <davidcc@google.com> - 2016-05-12 01:20 +0200
      Re: [PATCH v2 03/32] x86/intel,cqm: add CONFIG_INTEL_RDT configuration  flag Thomas Gleixner <tglx@linutronix.de> - 2016-05-18 19:40 +0200
    [PATCH v2 06/32] perf/x86/intel/cqm: add per-package RMIDs, data and locks David Carrillo-Cisneros <davidcc@google.com> - 2016-05-12 01:20 +0200
      Re: [PATCH v2 06/32] perf/x86/intel/cqm: add per-package RMIDs, data  and locks Thomas Gleixner <tglx@linutronix.de> - 2016-05-18 18:20 +0200

#1399592 — [PATCH v2 00/32] 2nd Iteration of Cache QoS Monitoring support.

This series introduces the next iteration of kernel support for the
Cache QoS Monitoring (CQM) technology available in Intel Xeon processors.

One of the main limitations of the previous version is the inability
to simultaneously monitor:
  1) cpu event and any other event in that cpu.
  2) cgroup events for cgroups in same descendancy line.
  3) cgroup events and any thread event of a cgroup in the same
     descendancy line.

Another limitation is that monitoring for a cgroup was enabled/disabled by
the existence of a perf event for that cgroup. Since the event
llc_occupancy measures changes in occupancy rather than total occupancy,
in order to read meaningful llc_occupancy values, an event should be
enabled for a long enough period of time. The overhead in context switches
caused by the perf events is undesired in some sensitive scenarios.

This series of patches addresses the shortcomings mentioned above and,
add some other improvements. The main changes are:
	- No more potential conflicts between different events. New
	version	builds a hierarchy of RMIDs that captures the dependency
	between	monitored cgroups. llc_occupancy for cgroup is the sum of
	llc_occupancies for that cgroup RMID and all other RMIDs in the
	cgroups subtree (both monitored cgroups and threads).

	- A cgroup integration that allows to monitor the a cgroup without
	creating a perf event, decreasing the context switch overhead.
	Monitoring is controlled by a boolean cgroup subsystem attribute
	in each perf cgroup, this is:

		echo 1 > cgroup_path/perf_event.cqm_cont_monitoring

	starts CQM monitoring whether or not there is a perf_event
	attached to the cgroup. Setting the attribute to 0 makes
	monitoring dependent on the existence of a perf_event.
	A perf_event is always required in order to read llc_occupancy.
	This cgroup integration uses Intel's PQR code and is intended to
	be used by upcoming versions of Intel's CAT.
	
	- A more stable rotation algorithm: New algorithm uses SLOs that
	guarantee:
		- A minimum of enabled time for monitored cgroups and
		threads.
		- A maximum time disabled before error is introduced by
		reusing dirty RMIDs.
		- A minimum rate at which RMIDs recycling must progress.

	- Reduced impact of stealing/rotation of RMIDs: The new algorithm
	accounts the residual occupancy held by limbo RMIDs towards the
	former owner of the limbo RMID, decreasing the error introduced
	by RMID rotation.
	It also allows a limbo RMID to be reused by its former owner when
	appropriate, decreasing the potential error of reusing dirty RMIDs
	and allowing to make progress even if most limbo RMIDs do not
	drop occupancy fast enough.

	- Elimination of pmu::count: perf generic's perf_event_count()
	perform a quick add of atomic types. The introduction of
	pmu::count in the previous CQM series to read occupancy for thread
	events changed the behavior of perf_event_count() by performing a
	potentially slow IPI and write/read to MSR. It also made pmu::read
	to have different behaviors depending on whether the event was a
	cpu/cgroup event or a thread. This patches serie removes the custom
	pmu::count from CQM and provides a consistent behavior for all
	calls of perf_event_read .

	- Added error return for pmu::read: Reads to CQM events may fail
	due to stealing of RMIDs, even after successfully adding an event
	to a PMU. This patch series expands pmu::read with an int return
	value and propagates the error to callers that can fail
	(ie. perf_read).
	The ability to fail of pmu::read is consistent with the recent
	changes	that allow perf_event_read to fail for transactional
	reading of event groups.

	- Introduces the field pmu_event_flags that contain flags set by
	the PMU to signal variations on the default behavior to perf's
	generic	code. In this series, three flags are introduced:
		- PERF_CGROUP_NO_RECURSION : Signals generic code to add
		events of the cgroup ancestors of a cgroup.
		- PERF_INACTIVE_CPU_READ_PKG: Signals generic coda that
		this CPU event can be read in any CPU in its event::cpu's
		package, even if the event is not active.
		- PERF_INACTIVE_EV_READ_ANY_CPU: Signals generic code that
		this event can be read in any CPU in any package in the
		system even if the event is not active.
	Using the above flags takes advantage of the CQM's hw ability to
	read llc_occupancy even when the associated perf event is not
	running in a CPU.

This patch series also updates the perf tool to fix error handling and to
better handle the idiosyncrasies of snapshot and per-pkg events.


Changes in 2nd version:
  - As requested by Peter Z., redo commit history to completely remove
    old version of CQM in a single patch.
  - Use topology_max_packages and fix build errors reported by
  Vikas Shivappa.
  - Split largest patches, clean up.
  - Rebased to peterz/queue perf/core .


David Carrillo-Cisneros (31):
  perf/x86/intel/cqm: remove previous version of CQM and MBM
  perf/x86/intel/cqm: software cache for MSR_IA32_PQR_ASSOC
  x86/intel,cqm: add CONFIG_INTEL_RDT configuration flag
  perf/x86/intel/cqm: add constants for CQM
  perf/x86/intel/cqm: encapsulate per-package RMIDs
  perf/x86/intel/cqm: add per-package RMIDs, data and locks
  perf/x86/intel/cqm: add helpers for per-package locking
  perf/x86/intel/cqm: add pmu sysfs attribute
  perf/x86/intel/cqm: basic RMID hierarchy with per package RMIDs
  perf/x86/intel/cqm: introduce (I)state and limbo prmids
  perf/x86/intel/cqm: add per-package RMID rotation
  perf/x86/intel/cqm: schedule work for rotation task
  perf/x86/intel/cqm: add polled update of RMID's llc_occupancy
  perf/x86/intel/cqm: add preallocation of anodes
  perf/core: add hooks to expose architecture specific features in
    perf_cgroup
  perf/x86/intel/cqm: add cgroup support
  perf/core,perf/x86/intel/cqm: add pmu::event_terminate
  perf/core: introduce PMU event flag PERF_CGROUP_NO_RECURSION
  x86/intel/cqm: use PERF_CGROUP_NO_RECURSION in CQM
  perf/x86/intel/cqm: handle inherit event and inherit_stat flag
  perf/x86/intel/cqm: introduce read_subtree
  perf/core: introduce PERF_INACTIVE_*_READ_* flags
  perf/x86/intel/cqm: use PERF_INACTIVE_*_READ_* flags in CQM
  sched: introduce the finish_arch_pre_lock_switch() scheduler hook
  perf/x86/intel/cqm: integrate CQM cgroups with scheduler
  perf/x86/intel/cqm: make one write of PQR_ASSOC per ctx switch
  perf/core: add perf_event cgroup hooks for subsystem attributes
  perf/x86/intel/cqm: add CQM attributes to perf_event cgroup
  perf,perf/x86,perf/powerpc,perf/arm,perf/*: add int error return to
    pmu::read
  perf,perf/x86: add hook perf_event_arch_exec
  perf/stat: revamp read error handling, snapshot and per_pkg events

Stephane Eranian (1):
  perf/stat: fix bug in handling events in error state

 arch/alpha/kernel/perf_event.c           |    3 +-
 arch/arc/kernel/perf_event.c             |    3 +-
 arch/arm64/include/asm/hw_breakpoint.h   |    2 +-
 arch/arm64/kernel/hw_breakpoint.c        |    3 +-
 arch/metag/kernel/perf/perf_event.c      |    5 +-
 arch/mips/kernel/perf_event_mipsxx.c     |    3 +-
 arch/powerpc/include/asm/hw_breakpoint.h |    2 +-
 arch/powerpc/kernel/hw_breakpoint.c      |    3 +-
 arch/powerpc/perf/core-book3s.c          |   11 +-
 arch/powerpc/perf/core-fsl-emb.c         |    5 +-
 arch/powerpc/perf/hv-24x7.c              |    5 +-
 arch/powerpc/perf/hv-gpci.c              |    3 +-
 arch/s390/kernel/perf_cpum_cf.c          |    5 +-
 arch/s390/kernel/perf_cpum_sf.c          |    3 +-
 arch/sh/include/asm/hw_breakpoint.h      |    2 +-
 arch/sh/kernel/hw_breakpoint.c           |    3 +-
 arch/sparc/kernel/perf_event.c           |    2 +-
 arch/tile/kernel/perf_event.c            |    3 +-
 arch/x86/Kconfig                         |    7 +
 arch/x86/events/amd/ibs.c                |    2 +-
 arch/x86/events/amd/iommu.c              |    5 +-
 arch/x86/events/amd/uncore.c             |    3 +-
 arch/x86/events/core.c                   |    3 +-
 arch/x86/events/intel/Makefile           |    3 +-
 arch/x86/events/intel/bts.c              |    3 +-
 arch/x86/events/intel/cqm.c              | 3842 +++++++++++++++++++++---------
 arch/x86/events/intel/cqm.h              |  532 +++++
 arch/x86/events/intel/cstate.c           |    3 +-
 arch/x86/events/intel/pt.c               |    3 +-
 arch/x86/events/intel/rapl.c             |    3 +-
 arch/x86/events/intel/uncore.c           |    3 +-
 arch/x86/events/intel/uncore.h           |    2 +-
 arch/x86/events/msr.c                    |    3 +-
 arch/x86/include/asm/hw_breakpoint.h     |    2 +-
 arch/x86/include/asm/perf_event.h        |   44 +
 arch/x86/include/asm/pqr_common.h        |   84 +
 arch/x86/include/asm/processor.h         |    4 +
 arch/x86/kernel/cpu/Makefile             |    4 +
 arch/x86/kernel/cpu/pqr_common.c         |   33 +
 arch/x86/kernel/hw_breakpoint.c          |    3 +-
 arch/x86/kvm/pmu.h                       |   10 +-
 drivers/bus/arm-cci.c                    |    3 +-
 drivers/bus/arm-ccn.c                    |    3 +-
 drivers/perf/arm_pmu.c                   |    3 +-
 include/linux/perf_event.h               |   92 +-
 kernel/events/core.c                     |  160 +-
 kernel/sched/core.c                      |    1 +
 kernel/sched/sched.h                     |    3 +
 kernel/trace/bpf_trace.c                 |    5 +-
 tools/perf/builtin-stat.c                |   43 +-
 tools/perf/util/counts.h                 |   19 +
 tools/perf/util/evsel.c                  |   44 +-
 tools/perf/util/evsel.h                  |    8 +-
 tools/perf/util/stat.c                   |   35 +-
 54 files changed, 3760 insertions(+), 1326 deletions(-)
 create mode 100644 arch/x86/events/intel/cqm.h
 create mode 100644 arch/x86/include/asm/pqr_common.h
 create mode 100644 arch/x86/kernel/cpu/pqr_common.c

-- 
2.8.0.rc3.226.g39d4020

[toc] | [next] | [standalone]

#1399593 — [PATCH v2 16/32] perf/x86/intel/cqm: add cgroup support

Create a monr per monitored cgroup. Inserts monrs in the monr hierarchy.
Task events are leaves of the lowest monitored ancestor cgroup (the lowest
cgroup ancestor with a monr).

CQM starts after the cgroup subsystem, and uses the cqm_initialized_key
static key to avoid interfering with the perf cgroup logic until
propertly initialized. The cgroup_init_mutex protects the initialization.

Reviewed-by: Stephane Eranian <eranian@google.com>
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 arch/x86/events/intel/cqm.c       | 595 +++++++++++++++++++++++++++++++++++++-
 arch/x86/events/intel/cqm.h       |  16 +
 arch/x86/include/asm/perf_event.h |  32 ++
 3 files changed, 639 insertions(+), 4 deletions(-)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index 0771154..fb62bac 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -89,6 +89,13 @@ struct monr *monr_hrchy_root;
 
 struct pkg_data **cqm_pkgs_data;
 
+/*
+ * Synchronizes initialization of cqm with cgroups.
+ */
+static DEFINE_MUTEX(cqm_init_mutex);
+
+DEFINE_STATIC_KEY_FALSE(cqm_initialized_key);
+
 static inline bool __pmonr__in_astate(struct pmonr *pmonr)
 {
 	lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
@@ -119,6 +126,9 @@ static inline bool __pmonr__in_instate(struct pmonr *pmonr)
 	return __pmonr__in_istate(pmonr) && !__pmonr__in_ilstate(pmonr);
 }
 
+/* Whether the monr is root. Recall that the cgroups can not be root and yet
+ * point to a root monr.
+ */
 static inline bool monr__is_root(struct monr *monr)
 {
 	return monr_hrchy_root == monr;
@@ -165,6 +175,19 @@ static inline void __monr__clear_mon_active(struct monr *monr)
 	monr->flags &= ~MONR_MON_ACTIVE;
 }
 
+static inline bool monr_is_event_type(struct monr *monr)
+{
+	return !monr->mon_cgrp && monr->mon_event_group;
+}
+
+#ifdef CONFIG_CGROUP_PERF
+static inline struct cgroup_subsys_state *get_root_perf_css(void)
+{
+	/* Get css for root cgroup */
+	return  init_css_set.subsys[perf_event_cgrp_id];
+}
+#endif
+
 static inline bool __valid_pkg_id(u16 pkg_id)
 {
 	return pkg_id < topology_max_packages();
@@ -706,6 +729,7 @@ static struct monr *monr_alloc(void)
 	monr->parent = NULL;
 	INIT_LIST_HEAD(&monr->children);
 	INIT_LIST_HEAD(&monr->parent_entry);
+	monr->mon_cgrp = NULL;
 	monr->mon_event_group = NULL;
 
 	monr->pmonrs = kmalloc(
@@ -934,7 +958,7 @@ retry:
 }
 
 /*
- * Wrappers for monr manipulation in events.
+ * Wrappers for monr manipulation in events and cgroups.
  *
  */
 static inline struct monr *monr_from_event(struct perf_event *event)
@@ -947,6 +971,100 @@ static inline void event_set_monr(struct perf_event *event, struct monr *monr)
 	WRITE_ONCE(event->hw.cqm_monr, monr);
 }
 
+#ifdef CONFIG_CGROUP_PERF
+static inline struct monr *monr_from_perf_cgroup(struct perf_cgroup *cgrp)
+{
+	struct monr *monr;
+	struct cgrp_cqm_info *cqm_info;
+
+	cqm_info = (struct cgrp_cqm_info *)READ_ONCE(cgrp->arch_info);
+	WARN_ON_ONCE(!cqm_info);
+	monr = READ_ONCE(cqm_info->monr);
+	return monr;
+}
+
+static inline struct perf_cgroup *monr__get_mon_cgrp(struct monr *monr)
+{
+	WARN_ON_ONCE(!monr);
+	return READ_ONCE(monr->mon_cgrp);
+}
+
+static inline void
+monr__set_mon_cgrp(struct monr *monr, struct perf_cgroup *cgrp)
+{
+	WRITE_ONCE(monr->mon_cgrp, cgrp);
+}
+
+static inline void
+perf_cgroup_set_monr(struct perf_cgroup *cgrp, struct monr *monr)
+{
+	WRITE_ONCE(cgrp_to_cqm_info(cgrp)->monr, monr);
+}
+
+/*
+ * A perf_cgroup is monitored when it's set in a monr->mon_cgrp.
+ * There is a many-to-one relationship between perf_cgroup's monrs
+ * and monrs' mon_cgrp. A monitored cgroup is necesarily referenced
+ * back by its monr's mon_cgrp.
+ */
+static inline bool perf_cgroup_is_monitored(struct perf_cgroup *cgrp)
+{
+	struct monr *monr;
+	struct perf_cgroup *monr_cgrp;
+
+	/* monr can be referenced by a cgroup other than the one in its
+	 * mon_cgrp, be careful.
+	 */
+	monr = monr_from_perf_cgroup(cgrp);
+
+	monr_cgrp = monr__get_mon_cgrp(monr);
+	/* Root monr do not have a cgroup associated before initialization.
+	 * mon_cgrp and mon_event_group are union, so the pointer must be set
+	 * for all non-root monrs.
+	 */
+	return  monr_cgrp && monr__get_mon_cgrp(monr) == cgrp;
+}
+
+/* Set css's monr to the monr of its lowest monitored ancestor. */
+static inline void __css_set_monr_to_lma(struct cgroup_subsys_state *css)
+{
+	lockdep_assert_held(&cqm_mutex);
+	if (!css->parent) {
+		perf_cgroup_set_monr(css_to_perf_cgroup(css), monr_hrchy_root);
+		return;
+	}
+	perf_cgroup_set_monr(
+		css_to_perf_cgroup(css),
+		monr_from_perf_cgroup(css_to_perf_cgroup(css->parent)));
+}
+
+static inline void
+perf_cgroup_make_monitored(struct perf_cgroup *cgrp, struct monr *monr)
+{
+	monr_hrchy_assert_held_mutexes();
+	perf_cgroup_set_monr(cgrp, monr);
+	/* Make sure that monr is a valid monr for css before it's visible
+	 * to any reader of css.
+	 */
+	smp_wmb();
+	monr__set_mon_cgrp(monr, cgrp);
+}
+
+static inline void
+perf_cgroup_make_unmonitored(struct perf_cgroup *cgrp)
+{
+	struct monr *monr = monr_from_perf_cgroup(cgrp);
+
+	monr_hrchy_assert_held_mutexes();
+	__css_set_monr_to_lma(&cgrp->css);
+	/* Make sure that all readers of css'monr see lma css before
+	 * monr stops being a valid monr for css.
+	 */
+	smp_wmb();
+	monr__set_mon_cgrp(monr, NULL);
+}
+#endif
+
 /*
  * Always finds a rmid_entry to schedule. To be called during scheduler.
  * A fast path that only uses read_lock for common case when rmid for current
@@ -1055,6 +1173,286 @@ __monr_hrchy_remove_leaf(struct monr *monr)
 	monr->parent = NULL;
 }
 
+#ifdef CONFIG_CGROUP_PERF
+static struct perf_cgroup *__perf_cgroup_parent(struct perf_cgroup *cgrp)
+{
+	struct cgroup_subsys_state *parent_css = cgrp->css.parent;
+
+	if (parent_css)
+		return css_to_perf_cgroup(parent_css);
+	return NULL;
+}
+
+/* Get cgroup for both task and cgroup event. */
+static inline struct perf_cgroup *
+perf_cgroup_from_event(struct perf_event *event)
+{
+#ifdef CONFIG_LOCKDEP
+	u16 pkg_id = topology_physical_package_id(smp_processor_id());
+	bool rcu_safe = lockdep_is_held(
+		&cqm_pkgs_data[pkg_id]->pkg_data_lock);
+#endif
+
+	if (!(event->attach_state & PERF_ATTACH_TASK))
+		return event->cgrp;
+
+	return container_of(
+		task_css_check(event->hw.target, perf_event_cgrp_id, rcu_safe),
+		struct perf_cgroup, css);
+}
+
+/* Find lowest ancestor that is monitored, not including this cgrp.
+ * Return NULL if no ancestor is monitored.
+ */
+struct perf_cgroup *__cgroup_find_lma(struct perf_cgroup *cgrp)
+{
+	do {
+		cgrp = __perf_cgroup_parent(cgrp);
+	} while (cgrp && !perf_cgroup_is_monitored(cgrp));
+	return cgrp;
+}
+
+/* Similar to css_next_descendant_pre but skips the subtree rooted by pos. */
+struct cgroup_subsys_state *
+css_skip_subtree_pre(struct cgroup_subsys_state *pos,
+		     struct cgroup_subsys_state *root)
+{
+	struct cgroup_subsys_state *next;
+
+	WARN_ON_ONCE(!pos);
+	while (pos != root) {
+		next = css_next_child(pos, pos->parent);
+		if (next)
+			return next;
+		pos = pos->parent;
+	}
+	return NULL;
+}
+
+/* Make all monrs of css descendants of css to depend on new_monr. */
+inline void __css_subtree_update_monrs(struct cgroup_subsys_state *css,
+				       struct monr *new_monr)
+{
+	struct cgroup_subsys_state *pos_css;
+	int i;
+	unsigned long flags;
+
+	lockdep_assert_held(&cqm_mutex);
+	monr_hrchy_assert_held_mutexes();
+
+	rcu_read_lock();
+
+	/* Iterate over descendants of css in pre-order, in a way
+	 * similar to css_for_each_descendant_pre, but skipping the subtrees
+	 * rooted by css's with a monitored cgroup, since the elements
+	 * in those subtrees do not need to be updated.
+	 */
+	pos_css = css_next_descendant_pre(css, css);
+	while (pos_css) {
+		struct perf_cgroup *pos_cgrp = css_to_perf_cgroup(pos_css);
+		struct monr *pos_monr = monr_from_perf_cgroup(pos_cgrp);
+
+		/* Skip css that are not online, sync'ed with cqm_mutex. */
+		if (!(pos_css->flags & CSS_ONLINE)) {
+			pos_css = css_next_descendant_pre(pos_css, css);
+			continue;
+		}
+		/* Update descendant pos's mnor pointers to monr_parent. */
+		if (!perf_cgroup_is_monitored(pos_cgrp)) {
+			perf_cgroup_set_monr(pos_cgrp, new_monr);
+			pos_css = css_next_descendant_pre(pos_css, css);
+			continue;
+		}
+		monr_hrchy_acquire_raw_spin_locks_irq_save(flags, i);
+		pos_monr->parent = new_monr;
+		list_move_tail(&pos_monr->parent_entry, &new_monr->children);
+		monr_hrchy_release_raw_spin_locks_irq_restore(flags, i);
+		/* Dont go down the subtree in pos_css since pos_monr is the
+		 * lma for all its descendants.
+		 */
+		pos_css = css_skip_subtree_pre(pos_css, css);
+	}
+	rcu_read_unlock();
+}
+
+static inline int __css_start_monitoring(struct cgroup_subsys_state *css)
+{
+	struct perf_cgroup *cgrp, *cgrp_lma, *pos_cgrp;
+	struct monr *monr, *monr_parent, *pos_monr, *tmp_monr;
+	unsigned long flags;
+	int i;
+
+	lockdep_assert_held(&cqm_mutex);
+
+	/* Hold mutexes to prevent all rotation threads in all packages from
+	 * messing with this.
+	 */
+	monr_hrchy_acquire_mutexes();
+	cgrp = css_to_perf_cgroup(css);
+	if (WARN_ON_ONCE(perf_cgroup_is_monitored(cgrp)))
+		return -1;
+
+	/* When css is root cgroup's css, attach to the pre-existing
+	 * and active root monr.
+	 */
+	cgrp_lma = __cgroup_find_lma(cgrp);
+	if (!cgrp_lma) {
+		/* monr of root cgrp must be monr_hrchy_root. */
+		WARN_ON_ONCE(!monr__is_root(monr_from_perf_cgroup(cgrp)));
+		perf_cgroup_make_monitored(cgrp, monr_hrchy_root);
+		monr_hrchy_release_mutexes();
+		return 0;
+	}
+	/* The monr for the lowest monitored ancestor is direct ancestor
+	 * of monr in the monr hierarchy.
+	 */
+	monr_parent = monr_from_perf_cgroup(cgrp_lma);
+
+	/* Create new monr. */
+	monr = monr_alloc();
+	if (IS_ERR(monr)) {
+		monr_hrchy_release_mutexes();
+		return PTR_ERR(monr);
+	}
+
+	/* monr has no children yet so it is to be inserted in hierarchy with
+	 * all its pmors in (U)state.
+	 * We hold locks until monr_hrchy changes are complete, to prevent
+	 * possible state transition for the pmonrs in monr while still
+	 * allowing to read the prmid_summary in the scheduler path.
+	 */
+	monr_hrchy_acquire_raw_spin_locks_irq_save(flags, i);
+	__monr_hrchy_insert_leaf(monr, monr_parent);
+	monr_hrchy_release_raw_spin_locks_irq_restore(flags, i);
+
+	/* Make sure monr is in hierarchy before attaching monr to cgroup. */
+	barrier();
+
+	perf_cgroup_make_monitored(cgrp, monr);
+	__css_subtree_update_monrs(css, monr);
+
+	monr_hrchy_acquire_raw_spin_locks_irq_save(flags, i);
+	/* Move task-event monrs that are descendant from css's cgroup. */
+	list_for_each_entry_safe(pos_monr, tmp_monr,
+				 &monr_parent->children, parent_entry) {
+		if (!monr_is_event_type(pos_monr))
+			continue;
+		/* all events in event group must have the same cgroup.
+		 * No RCU read lock necessary for task_css_check since calling
+		 * inside critical section.
+		 */
+		pos_cgrp = perf_cgroup_from_event(pos_monr->mon_event_group);
+		if (!cgroup_is_descendant(pos_cgrp->css.cgroup,
+					  cgrp->css.cgroup))
+			continue;
+		pos_monr->parent = monr;
+		list_move_tail(&pos_monr->parent_entry, &monr->children);
+	}
+	/* Make sure monitoring starts after all monrs have moved. */
+	barrier();
+
+	__monr__set_mon_active(monr);
+	monr_hrchy_release_raw_spin_locks_irq_restore(flags, i);
+
+	monr_hrchy_release_mutexes();
+	return 0;
+}
+
+static inline int __css_stop_monitoring(struct cgroup_subsys_state *css)
+{
+	struct perf_cgroup *cgrp, *cgrp_lma;
+	struct monr *monr, *monr_parent, *pos_monr;
+	unsigned long flags;
+	int i;
+
+	lockdep_assert_held(&cqm_mutex);
+
+	monr_hrchy_acquire_mutexes();
+	cgrp = css_to_perf_cgroup(css);
+	if (WARN_ON_ONCE(!perf_cgroup_is_monitored(cgrp)))
+		return -1;
+
+	monr = monr_from_perf_cgroup(cgrp);
+
+	/* When css is root cgroup's css, detach cgroup but do not
+	 * destroy monr.
+	 */
+	cgrp_lma = __cgroup_find_lma(cgrp);
+	if (!cgrp_lma) {
+		/* monr of root cgrp must be monr_hrchy_root. */
+		WARN_ON_ONCE(!monr__is_root(monr_from_perf_cgroup(cgrp)));
+		perf_cgroup_make_unmonitored(cgrp);
+		monr_hrchy_release_mutexes();
+		return 0;
+	}
+	/* The monr for the lowest monitored ancestor is direct ancestor
+	 * of monr in the monr hierarchy.
+	 */
+	monr_parent = monr_from_perf_cgroup(cgrp_lma);
+
+	/* Lock together the transition to (U)state and clearing
+	 * MONR_MON_ACTIVE to prevent prmids to return to (A)state
+	 * or (I)state in between.
+	 */
+	monr_hrchy_acquire_raw_spin_locks_irq_save(flags, i);
+	cqm_pkg_id_for_each_online(i)
+		__pmonr__to_ustate(monr->pmonrs[i]);
+	barrier();
+	__monr__clear_mon_active(monr);
+	monr_hrchy_release_raw_spin_locks_irq_restore(flags, i);
+
+	__css_subtree_update_monrs(css, monr_parent);
+
+
+	/*
+	 * Move the children monrs that are no cgroups.
+	 */
+	monr_hrchy_acquire_raw_spin_locks_irq_save(flags, i);
+
+	list_for_each_entry(pos_monr, &monr->children, parent_entry)
+		pos_monr->parent = monr_parent;
+	list_splice_tail_init(&monr->children, &monr_parent->children);
+	perf_cgroup_make_unmonitored(cgrp);
+	__monr_hrchy_remove_leaf(monr);
+
+	monr_hrchy_release_raw_spin_locks_irq_restore(flags, i);
+
+	monr_hrchy_release_mutexes();
+	monr_dealloc(monr);
+	return 0;
+}
+
+/* Attaching an event to a cgroup starts monitoring in the cgroup.
+ * If the cgroup is already monitoring, just use its pre-existing mnor.
+ */
+static int __monr_hrchy_attach_cgroup_event(struct perf_event *event,
+					    struct perf_cgroup *perf_cgrp)
+{
+	struct monr *monr;
+	int ret;
+
+	lockdep_assert_held(&cqm_mutex);
+	WARN_ON_ONCE(event->attach_state & PERF_ATTACH_TASK);
+	WARN_ON_ONCE(monr_from_event(event));
+	WARN_ON_ONCE(!perf_cgrp);
+
+	if (!perf_cgroup_is_monitored(perf_cgrp)) {
+		css_get(&perf_cgrp->css);
+		ret = __css_start_monitoring(&perf_cgrp->css);
+		css_put(&perf_cgrp->css);
+		if (ret)
+			return ret;
+	}
+
+	/* At this point, cgrp is always monitored, use its monr. */
+	monr = monr_from_perf_cgroup(perf_cgrp);
+
+	event_set_monr(event, monr);
+	monr->mon_event_group = event;
+	return 0;
+}
+#endif
+
 static int __monr_hrchy_attach_cpu_event(struct perf_event *event)
 {
 	lockdep_assert_held(&cqm_mutex);
@@ -1096,12 +1494,30 @@ static int __monr_hrchy_attach_task_event(struct perf_event *event,
 static int monr_hrchy_attach_event(struct perf_event *event)
 {
 	struct monr *monr_parent;
+	bool has_cgrp = false;
+#ifdef CONFIG_CGROUP_PERF
+	struct perf_cgroup *perf_cgrp;
+
+	has_cgrp = event->cgrp;
+#endif
 
-	if (!event->cgrp && !(event->attach_state & PERF_ATTACH_TASK))
+	if (!has_cgrp && !(event->attach_state & PERF_ATTACH_TASK))
 		return __monr_hrchy_attach_cpu_event(event);
 
+#ifdef CONFIG_CGROUP_PERF
+	/* Task events become leaves, cgroup events reuse the cgroup's monr */
+	if (event->cgrp)
+		return __monr_hrchy_attach_cgroup_event(event, event->cgrp);
+
+	rcu_read_lock();
+	perf_cgrp = perf_cgroup_from_event(event);
+	rcu_read_unlock();
+
+	monr_parent = monr_from_perf_cgroup(perf_cgrp);
+#else
 	/* Two-levels hierarchy: Root and all event monr underneath it. */
 	monr_parent = monr_hrchy_root;
+#endif
 	return __monr_hrchy_attach_task_event(event, monr_parent);
 }
 
@@ -1113,7 +1529,7 @@ static int monr_hrchy_attach_event(struct perf_event *event)
  */
 static bool __match_event(struct perf_event *a, struct perf_event *b)
 {
-	/* Per-cpu and task events don't mix */
+	/* Cgroup/non-task per-cpu and task events don't mix */
 	if ((a->attach_state & PERF_ATTACH_TASK) !=
 	    (b->attach_state & PERF_ATTACH_TASK))
 		return false;
@@ -2171,6 +2587,129 @@ static struct pmu intel_cqm_pmu = {
 	.read		     = intel_cqm_event_read,
 };
 
+#ifdef CONFIG_CGROUP_PERF
+/* XXX: Add hooks for attach dettach task with monr to a cgroup. */
+int perf_cgroup_arch_css_alloc(struct cgroup_subsys_state *parent_css,
+				      struct cgroup_subsys_state *new_css)
+{
+	struct perf_cgroup *new_cgrp;
+	struct cgrp_cqm_info *cqm_info;
+
+	new_cgrp = css_to_perf_cgroup(new_css);
+	cqm_info = kmalloc(sizeof(struct cgrp_cqm_info), GFP_KERNEL);
+	if (!cqm_info)
+		return -ENOMEM;
+	cqm_info->cont_monitoring = false;
+	cqm_info->monr = NULL;
+	new_cgrp->arch_info = cqm_info;
+
+	return 0;
+}
+
+void perf_cgroup_arch_css_free(struct cgroup_subsys_state *css)
+{
+	struct perf_cgroup *cgrp = css_to_perf_cgroup(css);
+
+	kfree(cgrp_to_cqm_info(cgrp));
+	cgrp->arch_info = NULL;
+}
+
+/* Do the bulk of arch_css_online. To be called when CQM starts after
+ * css has gone online.
+ */
+static inline int __css_go_online(struct cgroup_subsys_state *css)
+{
+	lockdep_assert_held(&cqm_mutex);
+
+	/* css must not be used in monr hierarchy before having
+	 * set its monr in this step.
+	 */
+	__css_set_monr_to_lma(css);
+	/* Root monr is always monitoring. */
+	if (!css->parent)
+		css_to_cqm_info(css)->cont_monitoring = true;
+
+	if (css_to_cqm_info(css)->cont_monitoring)
+		return __css_start_monitoring(css);
+	return 0;
+}
+
+int perf_cgroup_arch_css_online(struct cgroup_subsys_state *css)
+{
+	int ret = 0;
+
+	/* use cqm_init_mutex to synchronize with
+	 * __start_monitoring_all_cgroups.
+	 */
+	mutex_lock(&cqm_init_mutex);
+
+	if (static_branch_unlikely(&cqm_initialized_key)) {
+		mutex_lock(&cqm_mutex);
+		ret = __css_go_online(css);
+		mutex_unlock(&cqm_mutex);
+		WARN_ON_ONCE(ret);
+	}
+
+	mutex_unlock(&cqm_init_mutex);
+	return ret;
+}
+
+void perf_cgroup_arch_css_offline(struct cgroup_subsys_state *css)
+{
+	int ret = 0;
+	struct monr *monr;
+	struct perf_cgroup *cgrp = css_to_perf_cgroup(css);
+
+	mutex_lock(&cqm_init_mutex);
+
+	if (!static_branch_unlikely(&cqm_initialized_key))
+		goto out;
+
+	mutex_lock(&cqm_mutex);
+
+	monr = monr_from_perf_cgroup(cgrp);
+	if (!perf_cgroup_is_monitored(cgrp))
+		goto out_cqm;
+
+	/* Stop monitoring for the css's monr only if no more events need it.
+	 * If events need the monr, it will be destroyed when the events that
+	 * use it are destroyed.
+	 */
+	if (monr->mon_event_group) {
+		monr_hrchy_acquire_mutexes();
+		perf_cgroup_make_unmonitored(cgrp);
+		monr_hrchy_release_mutexes();
+	} else {
+		ret = __css_stop_monitoring(css);
+		WARN_ON_ONCE(ret);
+	}
+
+out_cqm:
+	mutex_unlock(&cqm_mutex);
+out:
+	mutex_unlock(&cqm_init_mutex);
+	WARN_ON_ONCE(ret);
+}
+
+void perf_cgroup_arch_css_released(struct cgroup_subsys_state *css)
+{
+	mutex_lock(&cqm_init_mutex);
+
+	if (static_branch_unlikely(&cqm_initialized_key)) {
+		mutex_lock(&cqm_mutex);
+		/*
+		 * Remove css from monr hierarchy now that css is about to
+		 * leave the cgroup hierarchy.
+		 */
+		perf_cgroup_set_monr(css_to_perf_cgroup(css), NULL);
+		mutex_unlock(&cqm_mutex);
+	}
+
+	mutex_unlock(&cqm_init_mutex);
+}
+
+#endif
+
 static inline void cqm_pick_event_reader(int cpu)
 {
 	u16 pkg_id = topology_physical_package_id(cpu);
@@ -2235,6 +2774,39 @@ static const struct x86_cpu_id intel_cqm_match[] = {
 	{}
 };
 
+#ifdef CONFIG_CGROUP_PERF
+/* Start monitoring for all cgroups in cgroup hierarchy. */
+static int __start_monitoring_all_cgroups(void)
+{
+	int ret;
+	struct cgroup_subsys_state *css, *css_root;
+
+	lockdep_assert_held(&cqm_init_mutex);
+
+	rcu_read_lock();
+	/* Get css for root cgroup */
+	css_root =  get_root_perf_css();
+
+	css_for_each_descendant_pre(css, css_root) {
+		if (!css_tryget_online(css))
+			continue;
+
+		rcu_read_unlock();
+		mutex_lock(&cqm_mutex);
+		ret = __css_go_online(css);
+		mutex_unlock(&cqm_mutex);
+
+		css_put(css);
+		if (ret)
+			return ret;
+
+		rcu_read_lock();
+	}
+	rcu_read_unlock();
+	return 0;
+}
+#endif
+
 static int __init intel_cqm_init(void)
 {
 	char *str, scale[20];
@@ -2316,17 +2888,32 @@ static int __init intel_cqm_init(void)
 
 	__perf_cpu_notifier(intel_cqm_cpu_notifier);
 
+	/* Use cqm_init_mutex to synchronize with css's online/offline. */
+	mutex_lock(&cqm_init_mutex);
+
+#ifdef CONFIG_CGROUP_PERF
+	ret = __start_monitoring_all_cgroups();
+	if (ret)
+		goto error_init_mutex;
+#endif
+
 	ret = perf_pmu_register(&intel_cqm_pmu, "intel_cqm", -1);
 	if (ret)
-		goto error;
+		goto error_init_mutex;
 
 	cpu_notifier_register_done();
 
+	static_branch_enable(&cqm_initialized_key);
+
+	mutex_unlock(&cqm_init_mutex);
+
 	pr_info("Intel CQM monitoring enabled with at least %u rmids per package.\n",
 		min_max_rmid + 1);
 
 	return ret;
 
+error_init_mutex:
+	mutex_unlock(&cqm_init_mutex);
 error:
 	pr_err("Intel CQM perf registration failed: %d\n", ret);
 	cpu_notifier_register_done();
diff --git a/arch/x86/events/intel/cqm.h b/arch/x86/events/intel/cqm.h
index 0467c52..a66fe02 100644
--- a/arch/x86/events/intel/cqm.h
+++ b/arch/x86/events/intel/cqm.h
@@ -316,6 +316,7 @@ struct pkg_data {
  * struct monr: MONitored Resource.
  * @flags:		Flags field for monr (XXX: More flags will be added
  *			with MBM).
+ * @mon_cgrp:		The cgroup associated with this monr, if any
  * @mon_event_group:	The head of event's group that use this monr, if any.
  * @parent:		Parent in monr hierarchy.
  * @children:		List of children in monr hierarchy.
@@ -336,6 +337,7 @@ struct pkg_data {
 struct monr {
 	u16				flags;
 	/* Back reference pointers */
+	struct perf_cgroup		*mon_cgrp;
 	struct perf_event		*mon_event_group;
 
 	struct monr			*parent;
@@ -514,3 +516,17 @@ static unsigned int __cqm_min_progress_rate = CQM_DEFAULT_MIN_PROGRESS_RATE;
  * It's units are bytes must be scaled by cqm_l3_scale to obtain cache lines.
  */
 static unsigned int __intel_cqm_max_threshold;
+
+#ifdef CONFIG_CGROUP_PERF
+
+struct cgrp_cqm_info {
+	/* Should the cgroup be continuously monitored? */
+	bool		cont_monitoring;
+	struct monr	*monr;
+};
+
+# define css_to_perf_cgroup(css_) container_of(css_, struct perf_cgroup, css)
+# define cgrp_to_cqm_info(cgrp_) ((struct cgrp_cqm_info *)cgrp_->arch_info)
+# define css_to_cqm_info(css_) cgrp_to_cqm_info(css_to_perf_cgroup(css_))
+
+#endif
diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index f353061..2246443 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -299,4 +299,36 @@ static inline void perf_check_microcode(void) { }
 
 #define arch_perf_out_copy_user copy_from_user_nmi
 
+
+/*
+ * Hooks for architecture specific features of perf_event cgroup.
+ * Currently used by Intel's CQM.
+ */
+#ifdef CONFIG_CGROUP_PERF
+#ifdef CONFIG_INTEL_RDT
+
+#define perf_cgroup_arch_css_alloc \
+	perf_cgroup_arch_css_alloc
+int perf_cgroup_arch_css_alloc(struct cgroup_subsys_state *parent_css,
+				      struct cgroup_subsys_state *new_css);
+
+#define perf_cgroup_arch_css_online \
+	perf_cgroup_arch_css_online
+int perf_cgroup_arch_css_online(struct cgroup_subsys_state *css);
+
+#define perf_cgroup_arch_css_offline \
+	perf_cgroup_arch_css_offline
+void perf_cgroup_arch_css_offline(struct cgroup_subsys_state *css);
+
+#define perf_cgroup_arch_css_released \
+	perf_cgroup_arch_css_released
+void perf_cgroup_arch_css_released(struct cgroup_subsys_state *css);
+
+#define perf_cgroup_arch_css_free \
+	perf_cgroup_arch_css_free
+void perf_cgroup_arch_css_free(struct cgroup_subsys_state *css);
+
+#endif
+#endif
+
 #endif /* _ASM_X86_PERF_EVENT_H */
-- 
2.8.0.rc3.226.g39d4020

[toc] | [prev] | [next] | [standalone]

#1399594 — [PATCH v2 11/32] perf/x86/intel/cqm: add per-package RMID rotation

This version of RMID rotation improves over original one by:
  1. Being per-package. No need for IPIs to test for occupancy.
  2. Since the monr hierarchy removed the potential conflicts between
     events, the new RMID rotation logic does not need to check and
     resolve conflicts.
  3. No need to mantain an unused RMID as rotation_rmid, effectively
     freeing one RMID per package.
  4. Guarantee that monitored events and cgroups with a valid RMID keep
     the RMID for an user configurable time: __cqm_min_mon_slice ms.
     Previously, it was likely to receive a RMID in one execution of the
     rotation logic just to have it removed in the next. That was
     specially problematic in the presence of events conflict
     (ie. cgroup events and thread events in a descendant cgroup).
  5. Do not increase the dirty threshold unless strictly necessary to make
     progress. Previous version simultaneously stole RMIDs and increased
     the dirty threshold (the maximum number of cache lines with spurious
     occupancy associated with a "clean" RMID). This version makes sure
     that increasing the dirty threshold is the only way to make progress
     in the RMID rotation (the case when too many RMID in limbo do not
     drop occupancy despite having spent enough time in limbo) before
     increasing the threshold.
     This change reduces spurious occupancy as a source of error.
  6. Do not steal RMIDs unnecesarily. Thanks to a more detailed
     bookeeping, this patch guarantees that the number of RMIDs in limbo
     do not exceed the number of RMIDs needed by pmonrs currently waiting
     for an RMID.
  7. Reuse dirty limbo RMIDs when appropriate. In this new version, a
     stolen RMID remains referenced by its former pmonr owner until it is
     reutilized by another pmonr or it is moved from limbo into the pool
     of free RMIDs.
     These RMIDs that are referenced and in limbo are not written into the
     MSR_IA32_PQR_ASSOC msr, therefore, they have the chance to drop
     occupancy as any other limbo RMID. If the pmonr with a limbo RMID is
     to be activated, then it reuses its former RMID even if its still
     dirty. The occupancy attributed to that RMID is part of the pmonr
     occupancy and therefore reusing the RMID even when dirty decreases
     the error of the read.
     This feature decreases the negative impact of RMIDs that do not drop
     occupancy in the efficiency of the rotation logic.

For an user perspective, the behavior of the new rotation logic is
controlled by SLO type parameters:
   __cqm_min_mon_slice : 	Minimum time a monr is to be monitored
	before being eligible by rotation logic to loss any of its RMIDs.
  __cqm_max_wait_mon :		Maximum time a monr can be deactivated
	before forcing rotation logic to be more aggresive (stealing more
	RMIDs per iteration).
  __cqm_min_progress_rate:	Minimum number of pmonrs that must be
	activated per second to consider that rotation logic's progress
	is acceptable.

Since the minimum progress rate is a SLO, the magnitude of the rotation
period (the rtimer_interval_ms) do not control the speed of RMID rotation,
it only controls the frequency at which rotation logic is executed.

Reviewed-by: Stephane Eranian <eranian@google.com>
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 arch/x86/events/intel/cqm.c | 659 ++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/events/intel/cqm.h |  40 +++
 2 files changed, 699 insertions(+)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index 62fc7b1..203fc66 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -216,6 +216,9 @@ static int pkg_data_init_cpu(int cpu)
 	INIT_LIST_HEAD(&pkg_data->istate_pmonrs_lru);
 	INIT_LIST_HEAD(&pkg_data->ilstate_pmonrs_lru);
 
+	pkg_data->nr_instate_pmonrs = 0;
+	pkg_data->nr_ilstate_pmonrs = 0;
+
 	mutex_init(&pkg_data->pkg_data_mutex);
 	raw_spin_lock_init(&pkg_data->pkg_data_lock);
 
@@ -276,6 +279,10 @@ static struct pmonr *pmonr_alloc(int cpu)
 	pmonr->monr = NULL;
 	INIT_LIST_HEAD(&pmonr->rotation_entry);
 
+	pmonr->last_enter_istate = 0;
+	pmonr->last_enter_astate = 0;
+	pmonr->nr_enter_istate = 0;
+
 	pmonr->pkg_id = topology_physical_package_id(cpu);
 	summary.sched_rmid = INVALID_RMID;
 	summary.read_rmid = INVALID_RMID;
@@ -327,6 +334,8 @@ __pmonr__finish_to_astate(struct pmonr *pmonr, struct prmid *prmid)
 
 	pmonr->prmid = prmid;
 
+	pmonr->last_enter_astate = jiffies;
+
 	list_move_tail(
 		&prmid->pool_entry, &__pkg_data(pmonr, active_prmids_pool));
 	list_move_tail(
@@ -354,6 +363,8 @@ __pmonr__instate_to_astate(struct pmonr *pmonr, struct prmid *prmid)
 	 */
 	WARN_ON_ONCE(pmonr->limbo_prmid);
 
+	__pkg_data(pmonr, nr_instate_pmonrs)--;
+
 	/* Do not depend on ancestor_pmonr anymore. Make it (A)state. */
 	ancestor = pmonr->ancestor_pmonr;
 	list_del_init(&pmonr->pmonr_deps_entry);
@@ -375,6 +386,28 @@ __pmonr__instate_to_astate(struct pmonr *pmonr, struct prmid *prmid)
 	}
 }
 
+/*
+ * Transition from (IL)state  to (A)state.
+ */
+static inline void
+__pmonr__ilstate_to_astate(struct pmonr *pmonr)
+{
+	struct prmid *prmid;
+
+	lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
+	WARN_ON_ONCE(!pmonr->limbo_prmid);
+
+	prmid = pmonr->limbo_prmid;
+	pmonr->limbo_prmid = NULL;
+	list_del_init(&pmonr->limbo_rotation_entry);
+
+	__pkg_data(pmonr, nr_ilstate_pmonrs)--;
+	__pkg_data(pmonr, nr_instate_pmonrs)++;
+	list_del_init(&prmid->pool_entry);
+
+	__pmonr__instate_to_astate(pmonr, prmid);
+}
+
 static inline void
 __pmonr__ustate_to_astate(struct pmonr *pmonr, struct prmid *prmid)
 {
@@ -466,7 +499,9 @@ __pmonr__to_ustate(struct pmonr *pmonr)
 
 			pmonr->limbo_prmid = NULL;
 			list_del_init(&pmonr->limbo_rotation_entry);
+			__pkg_data(pmonr, nr_ilstate_pmonrs)--;
 		} else {
+			__pkg_data(pmonr, nr_instate_pmonrs)--;
 		}
 		pmonr->ancestor_pmonr = NULL;
 	} else {
@@ -523,6 +558,9 @@ __pmonr__to_istate(struct pmonr *pmonr)
 		__pmonr__move_dependants(pmonr, ancestor);
 		list_move_tail(&pmonr->limbo_prmid->pool_entry,
 			       &__pkg_data(pmonr, pmonr_limbo_prmids_pool));
+		__pkg_data(pmonr, nr_ilstate_pmonrs)++;
+	} else {
+		__pkg_data(pmonr, nr_instate_pmonrs)++;
 	}
 
 	pmonr->ancestor_pmonr = ancestor;
@@ -535,10 +573,51 @@ __pmonr__to_istate(struct pmonr *pmonr)
 		list_move_tail(&pmonr->limbo_rotation_entry,
 			       &__pkg_data(pmonr, ilstate_pmonrs_lru));
 
+	pmonr->last_enter_istate = jiffies;
+	pmonr->nr_enter_istate++;
+
 	__pmonr__set_istate_summary(pmonr);
 
 }
 
+static inline void
+__pmonr__ilstate_to_instate(struct pmonr *pmonr)
+{
+	lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
+
+	list_move_tail(&pmonr->limbo_prmid->pool_entry,
+		       &__pkg_data(pmonr, free_prmids_pool));
+	pmonr->limbo_prmid = NULL;
+
+	__pkg_data(pmonr, nr_ilstate_pmonrs)--;
+	__pkg_data(pmonr, nr_instate_pmonrs)++;
+
+	list_del_init(&pmonr->limbo_rotation_entry);
+	__pmonr__set_istate_summary(pmonr);
+}
+
+/* Count all limbo prmids, including the ones still attached to pmonrs.
+ * Maximum number of prmids is fixed by hw and generally small.
+ */
+static int count_limbo_prmids(struct pkg_data *pkg_data)
+{
+	unsigned int c = 0;
+	struct prmid *prmid;
+
+	lockdep_assert_held(&pkg_data->pkg_data_mutex);
+
+	list_for_each_entry(
+		prmid, &pkg_data->pmonr_limbo_prmids_pool, pool_entry) {
+		c++;
+	}
+	list_for_each_entry(
+		prmid, &pkg_data->nopmonr_limbo_prmids_pool, pool_entry) {
+		c++;
+	}
+
+	return c;
+}
+
 static int intel_cqm_setup_pkg_prmid_pools(u16 pkg_id)
 {
 	int r;
@@ -858,6 +937,586 @@ static bool __match_event(struct perf_event *a, struct perf_event *b)
 	return false;
 }
 
+/*
+ * Try to reuse limbo prmid's for pmonrs at the front of  ilstate_pmonrs_lru.
+ */
+static int __try_reuse_ilstate_pmonrs(struct pkg_data *pkg_data)
+{
+	int reused = 0;
+	struct pmonr *pmonr;
+
+	lockdep_assert_held(&pkg_data->pkg_data_mutex);
+	lockdep_assert_held(&pkg_data->pkg_data_lock);
+
+	while ((pmonr = list_first_entry_or_null(
+		&pkg_data->istate_pmonrs_lru, struct pmonr, rotation_entry))) {
+
+		if (__pmonr__in_instate(pmonr))
+			break;
+		__pmonr__ilstate_to_astate(pmonr);
+		reused++;
+	}
+	return reused;
+}
+
+static int try_reuse_ilstate_pmonrs(struct pkg_data *pkg_data)
+{
+	int reused;
+	unsigned long flags;
+#ifdef CONFIG_LOCKDEP
+	u16 pkg_id = topology_physical_package_id(smp_processor_id());
+#endif
+
+	lockdep_assert_held(&pkg_data->pkg_data_mutex);
+
+	raw_spin_lock_irqsave_nested(&pkg_data->pkg_data_lock, flags, pkg_id);
+	reused = __try_reuse_ilstate_pmonrs(pkg_data);
+	raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
+	return reused;
+}
+
+
+/*
+ * A monr is only readable when all it's used pmonrs have a RMID.
+ * Therefore, the time a monr entered (A)state is the maximum of the
+ * last_enter_astate times for all (A)state pmonrs if no pmonr is in (I)state.
+ * A monr with any pmonr in (I)state has no entered (A)state.
+ * Returns monr_enter_astate time if available, otherwise min_inh_pkg is
+ * set to the smallest pkg_id where the monr's pmnor is in (I)state and
+ * the return value is undefined.
+ */
+static unsigned long
+__monr__last_enter_astate(struct monr *monr, int *min_inh_pkg)
+{
+	struct pkg_data *pkg_data;
+	u16 pkg_id;
+	unsigned long flags, astate_time = 0;
+
+	*min_inh_pkg = -1;
+	cqm_pkg_id_for_each_online(pkg_id) {
+		struct pmonr *pmonr;
+
+		if (min_inh_pkg >= 0)
+			break;
+
+		raw_spin_lock_irqsave_nested(
+				&pkg_data->pkg_data_lock, flags, pkg_id);
+
+		pmonr = monr->pmonrs[pkg_id];
+		if (__pmonr__in_istate(pmonr) && min_inh_pkg < 0)
+			*min_inh_pkg = pkg_id;
+		else if (__pmonr__in_astate(pmonr) &&
+				astate_time < pmonr->last_enter_astate)
+			astate_time = pmonr->last_enter_astate;
+
+		raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
+	}
+	return astate_time;
+}
+
+/*
+ * Steal as many rmids as possible.
+ * Transition pmonrs that have stayed at least __cqm_min_mon_slice in
+ * (A)state to (I)state.
+ */
+static inline int
+__try_steal_active_pmonrs(
+	struct pkg_data *pkg_data, unsigned int max_to_steal)
+{
+	struct pmonr *pmonr, *tmp;
+	int nr_stolen = 0, min_inh_pkg;
+	u16 pkg_id = topology_physical_package_id(smp_processor_id());
+	unsigned long flags, monr_astate_end_time, now = jiffies;
+	struct list_head *alist = &pkg_data->astate_pmonrs_lru;
+
+	lockdep_assert_held(&pkg_data->pkg_data_mutex);
+
+	/* pmonrs don't leave astate outside of rotation logic.
+	 * The pkg mutex protects against the pmonr leaving
+	 * astate_pmonrs_lru. The raw_spin_lock protects these list
+	 * operations from list insertions at tail coming from the
+	 * sched logic ( (U)state -> (A)state )
+	 */
+	raw_spin_lock_irqsave_nested(&pkg_data->pkg_data_lock, flags, pkg_id);
+
+	pmonr = list_first_entry(alist, struct pmonr, rotation_entry);
+	WARN_ON_ONCE(pmonr != monr_hrchy_root->pmonrs[pkg_id]);
+	WARN_ON_ONCE(pmonr->pkg_id != pkg_id);
+
+	list_for_each_entry_safe_continue(pmonr, tmp, alist, rotation_entry) {
+		bool steal_rmid = false;
+
+		WARN_ON_ONCE(!__pmonr__in_astate(pmonr));
+		WARN_ON_ONCE(pmonr->pkg_id != pkg_id);
+
+		raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
+
+		monr_astate_end_time =
+			__monr__last_enter_astate(pmonr->monr, &min_inh_pkg) +
+			__cqm_min_mon_slice;
+
+		/* pmonr in this pkg is supposed to be in (A)state. */
+		WARN_ON_ONCE(min_inh_pkg == pkg_id);
+
+		/* Steal a pmonr if:
+		 *   1) Any pmonr in a pkg with pkg_id < local pkg_id is
+		 *	in (I)state.
+		 *   2) It's monr has been active for enough time.
+		 * Note that since the min_inh_pkg for a monr cannot decrease
+		 * while the monr is not active, then the monr eventually will
+		 * become active again despite the stealing of pmonrs in pkgs
+		 * with id larger than min_inh_pkg.
+		 */
+		if (min_inh_pkg >= 0 && min_inh_pkg < pkg_id)
+			steal_rmid = true;
+		if (min_inh_pkg < 0 && monr_astate_end_time <= now)
+			steal_rmid = true;
+
+		raw_spin_lock_irqsave_nested(
+			&pkg_data->pkg_data_lock, flags, pkg_id);
+		if (!steal_rmid)
+			continue;
+
+		__pmonr__to_istate(pmonr);
+		nr_stolen++;
+		if (nr_stolen == max_to_steal)
+			break;
+	}
+
+	raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
+
+	return nr_stolen;
+}
+
+/* It will remove the prmid from the list its attached, if used. */
+static inline int __try_use_free_prmid(struct pkg_data *pkg_data,
+				       struct prmid *prmid, bool *succeed)
+{
+	struct pmonr *pmonr;
+	int nr_activated = 0;
+
+	lockdep_assert_held(&pkg_data->pkg_data_mutex);
+	lockdep_assert_held(&pkg_data->pkg_data_lock);
+
+	*succeed = false;
+	nr_activated += __try_reuse_ilstate_pmonrs(pkg_data);
+	pmonr = list_first_entry_or_null(&pkg_data->istate_pmonrs_lru,
+					 struct pmonr, rotation_entry);
+	if (!pmonr)
+		return nr_activated;
+	WARN_ON_ONCE(__pmonr__in_ilstate(pmonr));
+	WARN_ON_ONCE(!__pmonr__in_instate(pmonr));
+
+	/* the state transition function will move the prmid to
+	 * the active lru list.
+	 */
+	__pmonr__instate_to_astate(pmonr, prmid);
+	nr_activated++;
+	*succeed = true;
+	return nr_activated;
+}
+
+static inline int __try_use_free_prmids(struct pkg_data *pkg_data)
+{
+	struct prmid *prmid, *tmp_prmid;
+	unsigned long flags;
+	int nr_activated = 0;
+	bool succeed;
+#ifdef CONFIG_DEBUG_SPINLOCK
+	u16 pkg_id = topology_physical_package_id(smp_processor_id());
+#endif
+
+	lockdep_assert_held(&pkg_data->pkg_data_mutex);
+	/* Lock protects free_prmids_pool, istate_pmonrs_lru and
+	 * the monr hrchy.
+	 */
+	raw_spin_lock_irqsave_nested(&pkg_data->pkg_data_lock, flags, pkg_id);
+
+	list_for_each_entry_safe(prmid, tmp_prmid,
+				 &pkg_data->free_prmids_pool, pool_entry) {
+
+		/* Removes the free prmid if used. */
+		nr_activated += __try_use_free_prmid(pkg_data,
+						     prmid, &succeed);
+	}
+
+	nr_activated += __try_reuse_ilstate_pmonrs(pkg_data);
+	raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
+
+	return nr_activated;
+}
+
+/* Update prmid's of pmonrs in ilstate. To mantain fairness of rotation
+ * logic, Try to activate (IN)state pmonrs with recovered prmids when
+ * possible rather than simply adding them to free rmids list. This prevents,
+ * ustate pmonrs (pmonrs that haven't wait in istate_pmonrs_lru) to obtain
+ * the newly available RMIDs before those waiting in queue.
+ */
+static inline int
+__try_free_ilstate_prmids(struct pkg_data *pkg_data,
+			  unsigned int cqm_threshold,
+			  unsigned int *min_occupancy_dirty)
+{
+	struct pmonr *pmonr, *tmp_pmonr, *istate_pmonr;
+	struct prmid *prmid;
+	unsigned long flags;
+	u64 val;
+	bool succeed;
+	int ret, nr_activated = 0;
+#ifdef CONFIG_LOCKDEP
+	u16 pkg_id = topology_physical_package_id(smp_processor_id());
+#endif
+
+	lockdep_assert_held(&pkg_data->pkg_data_mutex);
+
+	WARN_ON_ONCE(try_reuse_ilstate_pmonrs(pkg_data));
+
+	/* No need to acquire pkg lock to iterate over ilstate_pmonrs_lru
+	 * since only rotation logic modifies it.
+	 */
+	list_for_each_entry_safe(
+		pmonr, tmp_pmonr,
+		&pkg_data->ilstate_pmonrs_lru, limbo_rotation_entry) {
+
+		if (WARN_ON_ONCE(list_empty(&pkg_data->istate_pmonrs_lru)))
+			return nr_activated;
+
+		istate_pmonr = list_first_entry(&pkg_data->istate_pmonrs_lru,
+						struct pmonr, rotation_entry);
+
+		if (pmonr == istate_pmonr) {
+			raw_spin_lock_irqsave_nested(
+				&pkg_data->pkg_data_lock, flags, pkg_id);
+
+			nr_activated++;
+			__pmonr__ilstate_to_astate(pmonr);
+
+			raw_spin_unlock_irqrestore(
+				&pkg_data->pkg_data_lock, flags);
+			continue;
+		}
+
+		ret = __cqm_prmid_update(pmonr->limbo_prmid,
+					  __rmid_min_update_time);
+		if (WARN_ON_ONCE(ret < 0))
+			continue;
+
+		val = atomic64_read(&pmonr->limbo_prmid->last_read_value);
+		if (val > cqm_threshold) {
+			if (val < *min_occupancy_dirty)
+				*min_occupancy_dirty = val;
+			continue;
+		}
+
+		raw_spin_lock_irqsave_nested(
+			&pkg_data->pkg_data_lock, flags, pkg_id);
+
+		prmid = pmonr->limbo_prmid;
+
+		/* moves the prmid to free_prmids_pool. */
+		__pmonr__ilstate_to_instate(pmonr);
+
+		/* Do not affect ilstate_pmonrs_lru.
+		 * If succeeds, prmid will end in active_prmids_pool,
+		 * otherwise, stays in free_prmids_pool where the
+		 * ilstate_to_instate transition left it.
+		 */
+		nr_activated += __try_use_free_prmid(pkg_data,
+						     prmid, &succeed);
+
+		raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
+	}
+	return nr_activated;
+}
+
+/* Update limbo prmid's no associated to a pmonr. To mantain fairness of
+ * rotation logic, Try to activate (IN)state pmonrs with recovered prmids when
+ * possible rather than simply adding them to free rmids list. This prevents,
+ * ustate pmonrs (pmonrs that haven't wait in istate_pmonrs_lru) to obtain
+ * the newly available RMIDs before those waiting in queue.
+ */
+static inline int
+__try_free_limbo_prmids(struct pkg_data *pkg_data,
+			unsigned int cqm_threshold,
+			unsigned int *min_occupancy_dirty)
+{
+	struct prmid *prmid, *tmp_prmid;
+	unsigned long flags;
+	bool succeed;
+	int ret, nr_activated = 0;
+
+#ifdef CONFIG_LOCKDEP
+	u16 pkg_id = topology_physical_package_id(smp_processor_id());
+#endif
+	u64 val;
+
+	lockdep_assert_held(&pkg_data->pkg_data_mutex);
+
+	list_for_each_entry_safe(
+		prmid, tmp_prmid,
+		&pkg_data->nopmonr_limbo_prmids_pool, pool_entry) {
+
+		/* If min update time is good enough for user, it is good
+		 * enough for rotation.
+		 */
+		ret = __cqm_prmid_update(prmid, __rmid_min_update_time);
+		if (WARN_ON_ONCE(ret < 0))
+			continue;
+
+		val = atomic64_read(&prmid->last_read_value);
+		if (val > cqm_threshold) {
+			if (val < *min_occupancy_dirty)
+				*min_occupancy_dirty = val;
+			continue;
+		}
+		raw_spin_lock_irqsave_nested(
+			&pkg_data->pkg_data_lock, flags, pkg_id);
+
+		nr_activated = __try_use_free_prmid(pkg_data, prmid, &succeed);
+		if (!succeed)
+			list_move_tail(&prmid->pool_entry,
+				       &pkg_data->free_prmids_pool);
+
+		raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
+	}
+	return nr_activated;
+}
+
+/*
+ * Activate (I)state pmonrs.
+ *
+ * @min_occupancy_dirty: pointer to store the minimum occupancy of any
+ *   dirty prmid.
+ *
+ * Try to activate as many pmonrs as possible before utilizing limbo prmids
+ * pointed by ilstate pmonrs in order to minimize the number of dirty rmids
+ * that move to other pmonr when cqm_threshold > 0.
+ */
+static int __try_activate_istate_pmonrs(
+	struct pkg_data *pkg_data, unsigned int cqm_threshold,
+	unsigned int *min_occupancy_dirty)
+{
+	int nr_activated = 0;
+
+	lockdep_assert_held(&pkg_data->pkg_data_mutex);
+
+	/* Start reusing limbo prmids no pointed by any ilstate pmonr. */
+	nr_activated += __try_free_limbo_prmids(pkg_data, cqm_threshold,
+						min_occupancy_dirty);
+
+	/* Try to use newly available free prmids */
+	nr_activated += __try_use_free_prmids(pkg_data);
+
+	/* Continue reusing limbo prmids pointed by a ilstate pmonr. */
+	nr_activated += __try_free_ilstate_prmids(pkg_data, cqm_threshold,
+						  min_occupancy_dirty);
+	/* Try to use newly available free prmids */
+	nr_activated += __try_use_free_prmids(pkg_data);
+
+	WARN_ON_ONCE(try_reuse_ilstate_pmonrs(pkg_data));
+	return nr_activated;
+}
+
+/* Number of pmonrs that have been in (I)state for at least min_wait_jiffies.
+ * XXX: Use rcu to access to istate_pmonrs_lru.
+ */
+static int
+count_istate_pmonrs(struct pkg_data *pkg_data,
+		    unsigned int min_wait_jiffies, bool exclude_limbo)
+{
+	unsigned long flags;
+	unsigned int c = 0;
+	struct pmonr *pmonr;
+#ifdef CONFIG_DEBUG_SPINLOCK
+	u16 pkg_id = topology_physical_package_id(smp_processor_id());
+#endif
+
+	lockdep_assert_held(&pkg_data->pkg_data_mutex);
+
+	raw_spin_lock_irqsave_nested(&pkg_data->pkg_data_lock, flags, pkg_id);
+	list_for_each_entry(
+		pmonr, &pkg_data->istate_pmonrs_lru, rotation_entry) {
+
+		if (jiffies - pmonr->last_enter_istate < min_wait_jiffies)
+			break;
+
+		WARN_ON_ONCE(!__pmonr__in_istate(pmonr));
+		if (exclude_limbo && __pmonr__in_ilstate(pmonr))
+			continue;
+		c++;
+	}
+	raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
+
+	return c;
+}
+
+static inline int
+read_nr_instate_pmonrs(struct pkg_data *pkg_data, u16 pkg_id) {
+	unsigned long flags;
+	int n;
+
+	raw_spin_lock_irqsave_nested(&pkg_data->pkg_data_lock, flags, pkg_id);
+	n = READ_ONCE(cqm_pkgs_data[pkg_id]->nr_instate_pmonrs);
+	raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
+	WARN_ON_ONCE(n < 0);
+	return n;
+}
+
+/*
+ * Rotate RMIDs among rpgks.
+ *
+ * For reads to be meaningful valid rmids had to be programmed for
+ * enough time to capture enough instances of cache allocation/retirement
+ * to yield useful occupancy values. The approach to handle that problem
+ * is to guarantee that every pmonr will spend at least T time in (A)state
+ * when such transition has occurred and hope that T is long enough.
+ *
+ * The hardware retains occupancy for 'old' tags, even after changing rmid
+ * for a task/cgroup. To workaround this problem, we keep retired rmids
+ * as limbo in each pmonr and use their occupancy. Also we prefer reusing
+ * such limbo rmids rather than free ones since their residual occupancy
+ * is valid occupancy for the task/cgroup.
+ *
+ * Rotation works by taking away an RMID from a group (the old RMID),
+ * and assigning the free RMID to another group (the new RMID). We must
+ * then wait for the old RMID to not be used (no cachelines tagged).
+ * This ensure that all cachelines are tagged with 'active' RMIDs. At
+ * this point we can start reading values for the new RMID and treat the
+ * old RMID as the free RMID for the next rotation.
+ */
+void
+__intel_cqm_rmid_rotate(struct pkg_data *pkg_data,
+			unsigned int nr_max_limbo,
+			unsigned int nr_min_activated)
+{
+	int nr_instate, nr_to_steal, nr_stolen, nr_slo_violated;
+	int limbo_cushion = 0;
+	unsigned int cqm_threshold = 0, min_occupancy_dirty;
+	u16 pkg_id = topology_physical_package_id(smp_processor_id());
+
+	/*
+	 * To avoid locking the process, keep track of pmonrs that
+	 * are activated during this execution of rotaton logic, so
+	 * we don't have to rely on the state of the pmonrs lists
+	 * to estimate progress, that can be modified during
+	 * creation and destruction of events and cgroups.
+	 */
+	int nr_activated = 0;
+
+	mutex_lock_nested(&pkg_data->pkg_data_mutex, pkg_id);
+
+	/*
+	 * Since ilstates are created only during stealing or destroying pmonrs,
+	 * but destroy requires pkg_data_mutex, then it is only necessary to
+	 * try to reuse ilstate once per call. Furthermore, new ilstates during
+	 * iteration in rotation logic is an error.
+	*/
+	nr_activated += try_reuse_ilstate_pmonrs(pkg_data);
+
+again:
+	nr_stolen = 0;
+	min_occupancy_dirty = UINT_MAX;
+	/*
+	 * Three types of actions are taken in rotation logic:
+	 *   1) Try to activate pmonrs using limbo RMIDs.
+	 *   2) Steal more RMIDs. Ideally the number of RMIDs in limbo equals
+	 *   the number of pmonrs in (I)state plus the limbo_cushion aimed to
+	 *   compensate for limbo RMIDs that do no drop occupancy fast enough.
+	 *   The actual number stolen is constrained
+	 *   prevent having more than nr_max_limbo RMIDs in limbo.
+	 *   3) Increase cqm_threshold so even RMIDs with residual occupancy
+	 *   are utilized to activate (I)state primds. Doing so increases the
+	 *   error in the reported value in a way undetectable to the user, so
+	 *   it is left as a last resource.
+	 */
+
+	/* Verify all available ilimbo where activated where they
+	 * were supposed to.
+	 */
+	WARN_ON_ONCE(try_reuse_ilstate_pmonrs(pkg_data) > 0);
+
+	/* Activate all pmonrs that we can by recycling rmids in limbo */
+	nr_activated += __try_activate_istate_pmonrs(
+		pkg_data, cqm_threshold, &min_occupancy_dirty);
+
+	/* Count nr of pmonrs that are inherited and do not have limbo_prmid */
+	nr_instate = read_nr_instate_pmonrs(pkg_data, pkg_id);
+	WARN_ON_ONCE(nr_instate < 0);
+	/*
+	 * If no pmonr needs rmid, then it's time to let go. pmonrs in ilimbo
+	 * are not counted since the limbo_prmid can be reused, once its time
+	 * to activate them.
+	 */
+	if (nr_instate == 0)
+		goto exit;
+
+	WARN_ON_ONCE(!list_empty(&pkg_data->free_prmids_pool));
+	WARN_ON_ONCE(try_reuse_ilstate_pmonrs(pkg_data) > 0);
+
+	/* There are still pmonrs waiting for RMID, check if the SLO about
+	 * _cqm_max_wait_mon has been violated. If so, use a more
+	 * aggresive version of RMID stealing and reutilization.
+	 */
+	nr_slo_violated = count_istate_pmonrs(
+		pkg_data, msecs_to_jiffies(__cqm_max_wait_mon), false);
+
+	/* First measure against SLO violation is to increase number of stolen
+	 * RMIDs beyond the number of pmonrs waiting for RMID. The magnitud of
+	 * the limbo_cushion is proportional to nr_slo_violated (but
+	 * arbitarily weighthed).
+	 */
+	if (nr_slo_violated)
+		limbo_cushion = (nr_slo_violated + 1) / 2;
+
+	/*
+	 * Need more free rmids. Steal RMIDs from active pmonrs and place them
+	 * into limbo lru. Steal enough to have high chances that eventually
+	 * occupancy of enough RMIDs in limbo will drop enough to be reused
+	 * (the limbo_cushion).
+	 */
+	nr_to_steal = min(nr_instate + limbo_cushion,
+			  max(0, (int)nr_max_limbo -
+				 count_limbo_prmids(pkg_data)));
+
+	if (nr_to_steal)
+		nr_stolen = __try_steal_active_pmonrs(pkg_data, nr_to_steal);
+
+	/* Already stole as many as possible, finish if no SLO violations. */
+	if (!nr_slo_violated)
+		goto exit;
+
+	/*
+	 * There are SLO violations due to recycling RMIDs not progressing
+	 * fast enough. Possible (non-exclusive) causal factors are:
+	 *   1) Too many RMIDs in limbo do not drop occupancy despite having
+	 *      spent a "reasonable" time in limbo lru.
+	 *   2) RMIDs in limbo have not been for long enough to have drop
+	 *	occupancy, but they will within "reasonable" time.
+	 *
+	 * If (2) only, it is ok to wait, since eventually the rmids
+	 * will rotate. If (1), there is a danger of being stuck, in that case
+	 * the dirty threshold, cqm_threshold, must be increased.
+	 * The notion of "reasonable" time is ambiguous since the more SLOs
+	 * violations, the more urgent it is to rotate. For now just try
+	 * to guarantee any progress is made (activate at least one prmid
+	 * with SLO violated).
+	 */
+
+	/* Using the minimum observed occupancy in dirty rmids guarantees to
+	 * to recover at least one rmid per iteration. Check if constrainst
+	 * would allow to use such threshold, otherwise makes no sense to
+	 * retry.
+	 */
+	if (nr_activated < nr_min_activated && min_occupancy_dirty <=
+		READ_ONCE(__intel_cqm_max_threshold) / cqm_l3_scale) {
+
+		cqm_threshold = min_occupancy_dirty;
+		goto again;
+	}
+exit:
+	mutex_unlock(&pkg_data->pkg_data_mutex);
+}
+
 static struct pmu intel_cqm_pmu;
 
 /*
diff --git a/arch/x86/events/intel/cqm.h b/arch/x86/events/intel/cqm.h
index 28011a8..12f4156 100644
--- a/arch/x86/events/intel/cqm.h
+++ b/arch/x86/events/intel/cqm.h
@@ -125,9 +125,16 @@ struct monr;
  *				prmids.
  * @limbo_rotation_entry:	List entry to attach to ilstate_pmonrs_lru when
  *				this pmonr is in (IL)state.
+ * @last_enter_istate:		Time last enter (I)state.
+ * @last_enter_astate:		Time last enter (A)state. Used in rotation logic
+ *				to guarantee that each pmonr gets a minimum
+ *				time in	(A)state.
  * @rotation_entry:		List entry to attach to pmonr rotation lists in
  *				pkg_data.
  * @monr:			The monr that contains this pmonr.
+ * @nr_enter_istate:		Track number of times entered (I)state. Useful
+ *				signal to diagnose excessive contention for
+ *				rmids in this package.
  * @pkg_id:			Auxiliar variable with pkg id for this pmonr.
  * @prmid_summary_atomic:	Atomic accesor to store a union prmid_summary
  *				that represent the state of this pmonr.
@@ -196,6 +203,10 @@ struct pmonr {
 	struct monr				*monr;
 	struct list_head			rotation_entry;
 
+	unsigned long				last_enter_istate;
+	unsigned long				last_enter_astate;
+	unsigned int				nr_enter_istate;
+
 	u16					pkg_id;
 
 	/* all writers are sync'ed by package's lock. */
@@ -220,6 +231,8 @@ struct pmonr {
  * @ilsate_pmonrs_lru:		pmonrs in (IL)state, these pmonrs have a valid
  *				limbo_prmid. It's a subset of istate_pmonrs_lru.
  *				Sorted increasingly by pmonr.last_enter_istate.
+ * @nr_instate_pmonrs		nr of pmonrs in (IN)state.
+ * @nr_ilstate_pmonrs		nr of pmonrs in (IL)state.
  * @pkg_data_mutex:		Hold for stability when modifying pmonrs
  *				hierarchy.
  * @pkg_data_lock:		Hold to protect variables that may be accessed
@@ -249,6 +262,9 @@ struct pkg_data {
 	struct list_head	istate_pmonrs_lru;
 	struct list_head	ilstate_pmonrs_lru;
 
+	int			nr_instate_pmonrs;
+	int			nr_ilstate_pmonrs;
+
 	struct mutex		pkg_data_mutex;
 	raw_spinlock_t		pkg_data_lock;
 
@@ -412,6 +428,30 @@ static inline int monr_hrchy_count_held_raw_spin_locks(void)
 #define CQM_DEFAULT_ROTATION_PERIOD 1200	/* ms */
 
 /*
+ * Service Level Objectives (SLO) for the rotation logic.
+ *
+ * @__cqm_min_duration_mon_slice: Minimum duration of a monitored slice.
+ * @__cqm_max_wait_monitor: Maximum time that a pmonr can pass waiting for an
+ *   RMID without rotation logic making any progress. Once elapsed for any
+ *  prmid, the reusing threshold (__intel_cqm_max_threshold) can be increased,
+ *  potentially increasing the speed at which RMIDs are reused, but potentially
+ *  introducing measurement error.
+ */
+#define CQM_DEFAULT_MIN_MON_SLICE 2000	/* ms */
+static unsigned int __cqm_min_mon_slice = CQM_DEFAULT_MIN_MON_SLICE;
+
+#define CQM_DEFAULT_MAX_WAIT_MON 20000 /* ms */
+static unsigned int __cqm_max_wait_mon = CQM_DEFAULT_MAX_WAIT_MON;
+
+/*
+ * If we fail to assign any RMID for intel_cqm_rotation because cachelines are
+ * still tagged with RMIDs in limbo even after having stolen enough rmids (a
+ * maximum number of rmids in limbo at any time), then we increment the dirty
+ * threshold to allow at least one RMID to be recycled. This mitigates the
+ * problem caused when cachelines tagged with a RMID are not evicted but
+ * it introduces error in the occupancy reads but allows the rotation of rmids
+ * to proceed.
+ *
  * __intel_cqm_max_threshold provides an upper bound on the threshold,
  * and is measured in bytes because it's exposed to userland.
  * It's units are bytes must be scaled by cqm_l3_scale to obtain cache lines.
-- 
2.8.0.rc3.226.g39d4020

[toc] | [prev] | [next] | [standalone]

#1403314 — Re: [PATCH v2 11/32] perf/x86/intel/cqm: add per-package RMID rotation

On Wed, 11 May 2016, David Carrillo-Cisneros wrote:
> @@ -216,6 +216,9 @@ static int pkg_data_init_cpu(int cpu)
>  	INIT_LIST_HEAD(&pkg_data->istate_pmonrs_lru);
>  	INIT_LIST_HEAD(&pkg_data->ilstate_pmonrs_lru);
>  
> +	pkg_data->nr_instate_pmonrs = 0;
> +	pkg_data->nr_ilstate_pmonrs = 0;

with kzalloc you can avoid all this 0 initialization mess.

>  	mutex_init(&pkg_data->pkg_data_mutex);
>  	raw_spin_lock_init(&pkg_data->pkg_data_lock);
>  
> @@ -276,6 +279,10 @@ static struct pmonr *pmonr_alloc(int cpu)
>  	pmonr->monr = NULL;
>  	INIT_LIST_HEAD(&pmonr->rotation_entry);
>  
> +	pmonr->last_enter_istate = 0;
> +	pmonr->last_enter_astate = 0;
> +	pmonr->nr_enter_istate = 0;

Ditto.

> +/*
> + * Transition from (IL)state  to (A)state.
> + */
> +static inline void 
> +__pmonr__ilstate_to_astate(struct pmonr *pmonr)

That nicely fits in a single line

> +{
> +	struct prmid *prmid;
> +
> +	lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
> +	WARN_ON_ONCE(!pmonr->limbo_prmid);

I'm giving up on that.

> +	prmid = pmonr->limbo_prmid;
> +	pmonr->limbo_prmid = NULL;
> +	list_del_init(&pmonr->limbo_rotation_entry);
> +
> +	__pkg_data(pmonr, nr_ilstate_pmonrs)--;
> +	__pkg_data(pmonr, nr_instate_pmonrs)++;

Just once more before stop reading.

     struct pkg_data *pkd = pmonr->pkg_data;

Will get rid of all this __pkg_data() nonsense. 

In this function the compiler will be able to figure out that its always the
same thing on its own. But there is enough code where it will simply
reevaluate.

> +static inline void
> +__pmonr__ilstate_to_instate(struct pmonr *pmonr)
> +{
> +	lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
> +
> +	list_move_tail(&pmonr->limbo_prmid->pool_entry,
> +		       &__pkg_data(pmonr, free_prmids_pool));
> +	pmonr->limbo_prmid = NULL;
> +
> +	__pkg_data(pmonr, nr_ilstate_pmonrs)--;
> +	__pkg_data(pmonr, nr_instate_pmonrs)++;
> +
> +	list_del_init(&pmonr->limbo_rotation_entry);
> +	__pmonr__set_istate_summary(pmonr);
> +}
> +
> +/* Count all limbo prmids, including the ones still attached to pmonrs.
> + * Maximum number of prmids is fixed by hw and generally small.
> + */
> +static int count_limbo_prmids(struct pkg_data *pkg_data)
> +{
> +	unsigned int c = 0;
> +	struct prmid *prmid;
> +
> +	lockdep_assert_held(&pkg_data->pkg_data_mutex);
> +
> +	list_for_each_entry(
> +		prmid, &pkg_data->pmonr_limbo_prmids_pool, pool_entry) {
> +		c++;
> +	}
> +	list_for_each_entry(
> +		prmid, &pkg_data->nopmonr_limbo_prmids_pool, pool_entry) {
> +		c++;
> +	}

And why can't you track the number of queued entries at list_add/del time and spare these loops?

> +	return c;
> +}
> +
>  static int intel_cqm_setup_pkg_prmid_pools(u16 pkg_id)
>  {
>  	int r;
> @@ -858,6 +937,586 @@ static bool __match_event(struct perf_event *a, struct perf_event *b)
>  	return false;
>  }
>  
> +/*
> + * Try to reuse limbo prmid's for pmonrs at the front of  ilstate_pmonrs_lru.
> + */
> +static int __try_reuse_ilstate_pmonrs(struct pkg_data *pkg_data)
> +{
> +	int reused = 0;
> +	struct pmonr *pmonr;
> +
> +	lockdep_assert_held(&pkg_data->pkg_data_mutex);
> +	lockdep_assert_held(&pkg_data->pkg_data_lock);
> +
> +	while ((pmonr = list_first_entry_or_null(
> +		&pkg_data->istate_pmonrs_lru, struct pmonr, rotation_entry))) {
> +
> +		if (__pmonr__in_instate(pmonr))
> +			break;

That really deserves a comment that pmonr is removed from the rotation
list. It's non obvious that the function below will do this.

> +		__pmonr__ilstate_to_astate(pmonr);
> +		reused++;
> +	}
> +	return reused;
> +}
> +
> +static int try_reuse_ilstate_pmonrs(struct pkg_data *pkg_data)
> +{
> +	int reused;
> +	unsigned long flags;
> +#ifdef CONFIG_LOCKDEP
> +	u16 pkg_id = topology_physical_package_id(smp_processor_id());
> +#endif
> +
> +	lockdep_assert_held(&pkg_data->pkg_data_mutex);
> +
> +	raw_spin_lock_irqsave_nested(&pkg_data->pkg_data_lock, flags, pkg_id);

Why do you need nested here? You are not nesting pkd->lock at all.

> +	reused = __try_reuse_ilstate_pmonrs(pkg_data);
> +	raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
> +	return reused;
> +}
> +
> +
> +/*
> + * A monr is only readable when all it's used pmonrs have a RMID.
> + * Therefore, the time a monr entered (A)state is the maximum of the
> + * last_enter_astate times for all (A)state pmonrs if no pmonr is in (I)state.
> + * A monr with any pmonr in (I)state has no entered (A)state.
> + * Returns monr_enter_astate time if available, otherwise min_inh_pkg is
> + * set to the smallest pkg_id where the monr's pmnor is in (I)state and
> + * the return value is undefined.
> + */
> +static unsigned long
> +__monr__last_enter_astate(struct monr *monr, int *min_inh_pkg)
> +{
> +	struct pkg_data *pkg_data;
> +	u16 pkg_id;
> +	unsigned long flags, astate_time = 0;
> +
> +	*min_inh_pkg = -1;
> +	cqm_pkg_id_for_each_online(pkg_id) {
> +		struct pmonr *pmonr;
> +
> +		if (min_inh_pkg >= 0)
> +			break;
> +
> +		raw_spin_lock_irqsave_nested(
> +				&pkg_data->pkg_data_lock, flags, pkg_id);

Ditto.

> +
> +		pmonr = monr->pmonrs[pkg_id];
> +		if (__pmonr__in_istate(pmonr) && min_inh_pkg < 0)
> +			*min_inh_pkg = pkg_id;
> +		else if (__pmonr__in_astate(pmonr) &&
> +				astate_time < pmonr->last_enter_astate)
> +			astate_time = pmonr->last_enter_astate;
> +
> +		raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
> +	}
> +	return astate_time;
> +}

> +/* It will remove the prmid from the list its attached, if used. */
> +static inline int __try_use_free_prmid(struct pkg_data *pkg_data,
> +				       struct prmid *prmid, bool *succeed)
> +{
> +	struct pmonr *pmonr;
> +	int nr_activated = 0;
> +
> +	lockdep_assert_held(&pkg_data->pkg_data_mutex);
> +	lockdep_assert_held(&pkg_data->pkg_data_lock);
> +
> +	*succeed = false;
> +	nr_activated += __try_reuse_ilstate_pmonrs(pkg_data);
> +	pmonr = list_first_entry_or_null(&pkg_data->istate_pmonrs_lru,
> +					 struct pmonr, rotation_entry);
> +	if (!pmonr)
> +		return nr_activated;
> +	WARN_ON_ONCE(__pmonr__in_ilstate(pmonr));
> +	WARN_ON_ONCE(!__pmonr__in_instate(pmonr));
> +
> +	/* the state transition function will move the prmid to
> +	 * the active lru list.
> +	 */
> +	__pmonr__instate_to_astate(pmonr, prmid);
> +	nr_activated++;
> +	*succeed = true;
> +	return nr_activated;

I really give up on this now. I have no idea what that code has to do with the
comment above the function and I really can't figure out what this succeed
flag is for.

And to be honest, I'm too tired to figure this out now, but I'm also tired of
looking at this maze in general. I think I gave you enough hints for now and I
won't look at the rest of this before the next iteration comes along.

Here is a summary of observations which I like to be addressed:

 - I like the general idea of splitting this apart in bits and pieces. But
   some of these splits are just mechanical or by throwing a dice. Please make
   this understandable

 - Don't do variable definitions in header files

 - Do not forward declare inlines or static function in headers.

 - Add proper error handling and cleanups right from the very beginning

 - Make this code modular

 - Rethink the naming conventions

 - Rethink the state tracking

 - Avoid these gazillion of macros/inlines which just obfuscate the code

 - Reduce the number of asserts and WARN_ONs to a useful set.

 - Handle unexpected conditions gracefully instead of emitting a useles
   warning followed by an oops. If there is no way to handle it gracefully get
   rid of the WARN_ONs as they are pointless.

 - Use kzalloc instead of zeroing/NULLing each newly added struct member.

 - Rethink the use of your unions. If they make sense, explain why.

 - Use consistent formatting for comments and functions, proper kerneldoc and
   use a consistent ordering of your local variables.

 - Avoid pointless long struct member names which mostly provide redundant
   information

 - Use intermediate variables instead of annoying line breaks.

 - Cleanup the lock_nested() abuse.

 - Please provide locking rules so there is a single place which explains
   which data is protected by which lock and explain the nesting rules.

 - Rethink the lock nesting. More than 8 packages in a system are reality
   today.

 - Please provide a proper overview how this stuff works. Your changelogs talk
   about improvements over the previous code and focus on implementation
   details, but the big picture is missing completely, unless I failed to find
   it.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1406455 — Re: [PATCH v2 11/32] perf/x86/intel/cqm: add per-package RMID rotation

On Wed, May 18, 2016 at 2:39 PM Thomas Gleixner <tglx@linutronix.de> wrote:
>
> On Wed, 11 May 2016, David Carrillo-Cisneros wrote:
> > @@ -216,6 +216,9 @@ static int pkg_data_init_cpu(int cpu)
> >       INIT_LIST_HEAD(&pkg_data->istate_pmonrs_lru);
> >       INIT_LIST_HEAD(&pkg_data->ilstate_pmonrs_lru);
> >
> > +     pkg_data->nr_instate_pmonrs = 0;
> > +     pkg_data->nr_ilstate_pmonrs = 0;
>
> with kzalloc you can avoid all this 0 initialization mess.
>
> >       mutex_init(&pkg_data->pkg_data_mutex);
> >       raw_spin_lock_init(&pkg_data->pkg_data_lock);
> >
> > @@ -276,6 +279,10 @@ static struct pmonr *pmonr_alloc(int cpu)
> >       pmonr->monr = NULL;
> >       INIT_LIST_HEAD(&pmonr->rotation_entry);
> >
> > +     pmonr->last_enter_istate = 0;
> > +     pmonr->last_enter_astate = 0;
> > +     pmonr->nr_enter_istate = 0;
>
> Ditto.
>
> > +/*
> > + * Transition from (IL)state  to (A)state.
> > + */
> > +static inline void
> > +__pmonr__ilstate_to_astate(struct pmonr *pmonr)
>
> That nicely fits in a single line
>
> > +{
> > +     struct prmid *prmid;
> > +
> > +     lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
> > +     WARN_ON_ONCE(!pmonr->limbo_prmid);
>
> I'm giving up on that.
>
> > +     prmid = pmonr->limbo_prmid;
> > +     pmonr->limbo_prmid = NULL;
> > +     list_del_init(&pmonr->limbo_rotation_entry);
> > +
> > +     __pkg_data(pmonr, nr_ilstate_pmonrs)--;
> > +     __pkg_data(pmonr, nr_instate_pmonrs)++;
>
> Just once more before stop reading.
>
>      struct pkg_data *pkd = pmonr->pkg_data;
>
> Will get rid of all this __pkg_data() nonsense.
>
> In this function the compiler will be able to figure out that its always the
> same thing on its own. But there is enough code where it will simply
> reevaluate.
>
> > +static inline void
> > +__pmonr__ilstate_to_instate(struct pmonr *pmonr)
> > +{
> > +     lockdep_assert_held(&__pkg_data(pmonr, pkg_data_lock));
> > +
> > +     list_move_tail(&pmonr->limbo_prmid->pool_entry,
> > +                    &__pkg_data(pmonr, free_prmids_pool));
> > +     pmonr->limbo_prmid = NULL;
> > +
> > +     __pkg_data(pmonr, nr_ilstate_pmonrs)--;
> > +     __pkg_data(pmonr, nr_instate_pmonrs)++;
> > +
> > +     list_del_init(&pmonr->limbo_rotation_entry);
> > +     __pmonr__set_istate_summary(pmonr);
> > +}
> > +
> > +/* Count all limbo prmids, including the ones still attached to pmonrs.
> > + * Maximum number of prmids is fixed by hw and generally small.
> > + */
> > +static int count_limbo_prmids(struct pkg_data *pkg_data)
> > +{
> > +     unsigned int c = 0;
> > +     struct prmid *prmid;
> > +
> > +     lockdep_assert_held(&pkg_data->pkg_data_mutex);
> > +
> > +     list_for_each_entry(
> > +             prmid, &pkg_data->pmonr_limbo_prmids_pool, pool_entry) {
> > +             c++;
> > +     }
> > +     list_for_each_entry(
> > +             prmid, &pkg_data->nopmonr_limbo_prmids_pool, pool_entry) {
> > +             c++;
> > +     }
>
> And why can't you track the number of queued entries at list_add/del time and spare these loops?
>
> > +     return c;
> > +}
> > +
> >  static int intel_cqm_setup_pkg_prmid_pools(u16 pkg_id)
> >  {
> >       int r;
> > @@ -858,6 +937,586 @@ static bool __match_event(struct perf_event *a, struct perf_event *b)
> >       return false;
> >  }
> >
> > +/*
> > + * Try to reuse limbo prmid's for pmonrs at the front of  ilstate_pmonrs_lru.
> > + */
> > +static int __try_reuse_ilstate_pmonrs(struct pkg_data *pkg_data)
> > +{
> > +     int reused = 0;
> > +     struct pmonr *pmonr;
> > +
> > +     lockdep_assert_held(&pkg_data->pkg_data_mutex);
> > +     lockdep_assert_held(&pkg_data->pkg_data_lock);
> > +
> > +     while ((pmonr = list_first_entry_or_null(
> > +             &pkg_data->istate_pmonrs_lru, struct pmonr, rotation_entry))) {
> > +
> > +             if (__pmonr__in_instate(pmonr))
> > +                     break;
>
> That really deserves a comment that pmonr is removed from the rotation
> list. It's non obvious that the function below will do this.
>
> > +             __pmonr__ilstate_to_astate(pmonr);
> > +             reused++;
> > +     }
> > +     return reused;
> > +}
> > +
> > +static int try_reuse_ilstate_pmonrs(struct pkg_data *pkg_data)
> > +{
> > +     int reused;
> > +     unsigned long flags;
> > +#ifdef CONFIG_LOCKDEP
> > +     u16 pkg_id = topology_physical_package_id(smp_processor_id());
> > +#endif
> > +
> > +     lockdep_assert_held(&pkg_data->pkg_data_mutex);
> > +
> > +     raw_spin_lock_irqsave_nested(&pkg_data->pkg_data_lock, flags, pkg_id);
>
> Why do you need nested here? You are not nesting pkd->lock at all.
>
> > +     reused = __try_reuse_ilstate_pmonrs(pkg_data);
> > +     raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
> > +     return reused;
> > +}
> > +
> > +
> > +/*
> > + * A monr is only readable when all it's used pmonrs have a RMID.
> > + * Therefore, the time a monr entered (A)state is the maximum of the
> > + * last_enter_astate times for all (A)state pmonrs if no pmonr is in (I)state.
> > + * A monr with any pmonr in (I)state has no entered (A)state.
> > + * Returns monr_enter_astate time if available, otherwise min_inh_pkg is
> > + * set to the smallest pkg_id where the monr's pmnor is in (I)state and
> > + * the return value is undefined.
> > + */
> > +static unsigned long
> > +__monr__last_enter_astate(struct monr *monr, int *min_inh_pkg)
> > +{
> > +     struct pkg_data *pkg_data;
> > +     u16 pkg_id;
> > +     unsigned long flags, astate_time = 0;
> > +
> > +     *min_inh_pkg = -1;
> > +     cqm_pkg_id_for_each_online(pkg_id) {
> > +             struct pmonr *pmonr;
> > +
> > +             if (min_inh_pkg >= 0)
> > +                     break;
> > +
> > +             raw_spin_lock_irqsave_nested(
> > +                             &pkg_data->pkg_data_lock, flags, pkg_id);
>
> Ditto.
>
> > +
> > +             pmonr = monr->pmonrs[pkg_id];
> > +             if (__pmonr__in_istate(pmonr) && min_inh_pkg < 0)
> > +                     *min_inh_pkg = pkg_id;
> > +             else if (__pmonr__in_astate(pmonr) &&
> > +                             astate_time < pmonr->last_enter_astate)
> > +                     astate_time = pmonr->last_enter_astate;
> > +
> > +             raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
> > +     }
> > +     return astate_time;
> > +}
>
> > +/* It will remove the prmid from the list its attached, if used. */
> > +static inline int __try_use_free_prmid(struct pkg_data *pkg_data,
> > +                                    struct prmid *prmid, bool *succeed)
> > +{
> > +     struct pmonr *pmonr;
> > +     int nr_activated = 0;
> > +
> > +     lockdep_assert_held(&pkg_data->pkg_data_mutex);
> > +     lockdep_assert_held(&pkg_data->pkg_data_lock);
> > +
> > +     *succeed = false;
> > +     nr_activated += __try_reuse_ilstate_pmonrs(pkg_data);
> > +     pmonr = list_first_entry_or_null(&pkg_data->istate_pmonrs_lru,
> > +                                      struct pmonr, rotation_entry);
> > +     if (!pmonr)
> > +             return nr_activated;
> > +     WARN_ON_ONCE(__pmonr__in_ilstate(pmonr));
> > +     WARN_ON_ONCE(!__pmonr__in_instate(pmonr));
> > +
> > +     /* the state transition function will move the prmid to
> > +      * the active lru list.
> > +      */
> > +     __pmonr__instate_to_astate(pmonr, prmid);
> > +     nr_activated++;
> > +     *succeed = true;
> > +     return nr_activated;
>
> I really give up on this now. I have no idea what that code has to do with the
> comment above the function and I really can't figure out what this succeed
> flag is for.
>
> And to be honest, I'm too tired to figure this out now, but I'm also tired of
> looking at this maze in general. I think I gave you enough hints for now and I
> won't look at the rest of this before the next iteration comes along.
>
> Here is a summary of observations which I like to be addressed:
>
>  - I like the general idea of splitting this apart in bits and pieces. But
>    some of these splits are just mechanical or by throwing a dice. Please make
>    this understandable
>
>  - Don't do variable definitions in header files
>
>  - Do not forward declare inlines or static function in headers.
>
>  - Add proper error handling and cleanups right from the very beginning
>
>  - Make this code modular
>
>  - Rethink the naming conventions
>
>  - Rethink the state tracking
>
>  - Avoid these gazillion of macros/inlines which just obfuscate the code
>
>  - Reduce the number of asserts and WARN_ONs to a useful set.
>
>  - Handle unexpected conditions gracefully instead of emitting a useles
>    warning followed by an oops. If there is no way to handle it gracefully get
>    rid of the WARN_ONs as they are pointless.
>
>  - Use kzalloc instead of zeroing/NULLing each newly added struct member.
>
>  - Rethink the use of your unions. If they make sense, explain why.
>
>  - Use consistent formatting for comments and functions, proper kerneldoc and
>    use a consistent ordering of your local variables.
>
>  - Avoid pointless long struct member names which mostly provide redundant
>    information
>
>  - Use intermediate variables instead of annoying line breaks.
>
>  - Cleanup the lock_nested() abuse.
>
>  - Please provide locking rules so there is a single place which explains
>    which data is protected by which lock and explain the nesting rules.
>
>  - Rethink the lock nesting. More than 8 packages in a system are reality
>    today.
>
>  - Please provide a proper overview how this stuff works. Your changelogs talk
>    about improvements over the previous code and focus on implementation
>    details, but the big picture is missing completely, unless I failed to find
>    it.
>
> Thanks,
>
>         tglx
>
>

Thanks for the review and the comments, I will address them. I have
plenty to fix for the next iteration :)

[toc] | [prev] | [next] | [standalone]

#1399595 — [PATCH v2 07/32] perf/x86/intel/cqm: add helpers for per-package locking

Add helper macros and functions to acquire and release the locks
and mutexes in pkg_data.

Reviewed-by: Stephane Eranian <eranian@google.com>
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 arch/x86/events/intel/cqm.h | 78 +++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 78 insertions(+)

diff --git a/arch/x86/events/intel/cqm.h b/arch/x86/events/intel/cqm.h
index 08623b5..7837db0 100644
--- a/arch/x86/events/intel/cqm.h
+++ b/arch/x86/events/intel/cqm.h
@@ -96,6 +96,84 @@ static inline u16 __cqm_pkgs_data_first_online(void)
 #define __pkg_data(pmonr, member) cqm_pkgs_data[pmonr->pkg_id]->member
 
 /*
+ * Utility function and macros to manage per-package locks.
+ * Use macros to keep flags in caller's stace.
+ * Hold lock in all the packages, required to alter the monr hierarchy
+ */
+static inline void monr_hrchy_acquire_mutexes(void)
+{
+	int i;
+
+	cqm_pkg_id_for_each_online(i)
+		mutex_lock_nested(&cqm_pkgs_data[i]->pkg_data_mutex, i);
+}
+
+# define monr_hrchy_acquire_raw_spin_locks_irq_save(flags, i) \
+	do { \
+		raw_local_irq_save(flags); \
+		cqm_pkg_id_for_each_online(i) {\
+			raw_spin_lock_nested( \
+				&cqm_pkgs_data[i]->pkg_data_lock, i); \
+		} \
+	} while (0)
+
+#define monr_hrchy_acquire_locks(flags, i) \
+	do {\
+		monr_hrchy_acquire_mutexes(); \
+		monr_hrchy_acquire_raw_spin_locks_irq_save(flags, i); \
+	} while (0)
+
+static inline void monr_hrchy_release_mutexes(void)
+{
+	int i;
+
+	cqm_pkg_id_for_each_online(i)
+		mutex_unlock(&cqm_pkgs_data[i]->pkg_data_mutex);
+}
+
+# define monr_hrchy_release_raw_spin_locks_irq_restore(flags, i) \
+	do { \
+		cqm_pkg_id_for_each_online(i) {\
+			raw_spin_unlock(&cqm_pkgs_data[i]->pkg_data_lock); \
+		} \
+		raw_local_irq_restore(flags); \
+	} while (0)
+
+#define monr_hrchy_release_locks(flags, i) \
+	do {\
+		monr_hrchy_release_raw_spin_locks_irq_restore(flags, i); \
+		monr_hrchy_release_mutexes(); \
+	} while (0)
+
+static inline void monr_hrchy_assert_held_mutexes(void)
+{
+	int i;
+
+	cqm_pkg_id_for_each_online(i)
+		lockdep_assert_held(&cqm_pkgs_data[i]->pkg_data_mutex);
+}
+
+static inline void monr_hrchy_assert_held_raw_spin_locks(void)
+{
+	int i;
+
+	cqm_pkg_id_for_each_online(i)
+		lockdep_assert_held(&cqm_pkgs_data[i]->pkg_data_lock);
+}
+#ifdef CONFIG_LOCKDEP
+static inline int monr_hrchy_count_held_raw_spin_locks(void)
+{
+	int i, nr_held = 0;
+
+	cqm_pkg_id_for_each_online(i) {
+		if (lockdep_is_held(&cqm_pkgs_data[i]->pkg_data_lock))
+			nr_held++;
+	}
+	return nr_held;
+}
+#endif
+
+/*
  * Time between execution of rotation logic. The frequency of execution does
  * not affect the rate at which RMIDs are recycled, except by the delay by the
  * delay updating the prmid's and their pools.
-- 
2.8.0.rc3.226.g39d4020

[toc] | [prev] | [next] | [standalone]

#1403183 — Re: [PATCH v2 07/32] perf/x86/intel/cqm: add helpers for per-package locking

On Wed, 11 May 2016, David Carrillo-Cisneros wrote:
> Add helper macros and functions to acquire and release the locks
> and mutexes in pkg_data.

Why? Whatfor do we need nested locks?

> Reviewed-by: Stephane Eranian <eranian@google.com>
> Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
> ---
>  arch/x86/events/intel/cqm.h | 78 +++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 78 insertions(+)
> 
> diff --git a/arch/x86/events/intel/cqm.h b/arch/x86/events/intel/cqm.h
> index 08623b5..7837db0 100644
> --- a/arch/x86/events/intel/cqm.h
> +++ b/arch/x86/events/intel/cqm.h
> @@ -96,6 +96,84 @@ static inline u16 __cqm_pkgs_data_first_online(void)
>  #define __pkg_data(pmonr, member) cqm_pkgs_data[pmonr->pkg_id]->member
>  
>  /*
> + * Utility function and macros to manage per-package locks.
> + * Use macros to keep flags in caller's stace.

stace? 

You can do that with inlines as well.

> + * Hold lock in all the packages, required to alter the monr hierarchy

And what's monr? 

> + */
> +static inline void monr_hrchy_acquire_mutexes(void)
> +{
> +	int i;
> +
> +	cqm_pkg_id_for_each_online(i)
> +		mutex_lock_nested(&cqm_pkgs_data[i]->pkg_data_mutex, i);
> +}
> +
> +# define monr_hrchy_acquire_raw_spin_locks_irq_save(flags, i) \

Why on earth do you want to hand in 'i' ?

> +	do { \
> +		raw_local_irq_save(flags); \
> +		cqm_pkg_id_for_each_online(i) {\
> +			raw_spin_lock_nested( \
> +				&cqm_pkgs_data[i]->pkg_data_lock, i); \
> +		} \
> +	} while (0)

All of this can be done with readable inlines.

> +#ifdef CONFIG_LOCKDEP
> +static inline int monr_hrchy_count_held_raw_spin_locks(void)
> +{
> +	int i, nr_held = 0;
> +
> +	cqm_pkg_id_for_each_online(i) {
> +		if (lockdep_is_held(&cqm_pkgs_data[i]->pkg_data_lock))
> +			nr_held++;
> +	}
> +	return nr_held;
> +}

And we need this because it looks neat?

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1403243 — Re: [PATCH v2 07/32] perf/x86/intel/cqm: add helpers for per-package locking

On Wed, 18 May 2016, Thomas Gleixner wrote:
> On Wed, 11 May 2016, David Carrillo-Cisneros wrote:

> > +	cqm_pkg_id_for_each_online(i)
> > +		mutex_lock_nested(&cqm_pkgs_data[i]->pkg_data_mutex, i);

Peter just pointed out that this will fail when the number of nest levels
exceeds 8. So any system with more than 8 packages will make lockdep explode.

Thanks,

	tglx

[toc] | [prev] | [next] | [standalone]

#1399596 — [PATCH v2 04/32] perf/x86/intel/cqm: add constants for CQM

Add initial constants and comments.

Reviewed-by: Stephane Eranian <eranian@google.com>
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 arch/x86/events/intel/cqm.c | 20 ++++++++++++++++++++
 arch/x86/events/intel/cqm.h | 31 +++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+)
 create mode 100644 arch/x86/events/intel/cqm.h

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index e69de29..0c4f3fe 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -0,0 +1,20 @@
+/*
+ * Intel Cache Quality-of-Service Monitoring (CQM) support.
+ *
+ * Based very, very heavily on work by Peter Zijlstra.
+ */
+
+#include <linux/slab.h>
+#include <asm/cpu_device_id.h>
+#include "cqm.h"
+#include "../perf_event.h"
+
+#define MSR_IA32_QM_CTR		0x0c8e
+#define MSR_IA32_QM_EVTSEL	0x0c8d
+
+#define RMID_VAL_ERROR		(1ULL << 63)
+#define RMID_VAL_UNAVAIL	(1ULL << 62)
+
+#define QOS_L3_OCCUP_EVENT_ID	(1 << 0)
+
+#define QOS_EVENT_MASK		QOS_L3_OCCUP_EVENT_ID
diff --git a/arch/x86/events/intel/cqm.h b/arch/x86/events/intel/cqm.h
new file mode 100644
index 0000000..bb906c7
--- /dev/null
+++ b/arch/x86/events/intel/cqm.h
@@ -0,0 +1,31 @@
+/*
+ * Intel Cache Quality-of-Service Monitoring (CQM) support.
+ *
+ * A Resource Manager ID (RMID) is a u32 value that, when programmed in a
+ * logical CPU, will allow the LLC cache to associate the changes in occupancy
+ * generated by that cpu (cache lines allocations - deallocations) to the RMID.
+ * If a RMID has been assigned to a thread T long enough for all cache lines
+ * used by T to be allocated, then the occupancy reported by the hardware
+ * equals the total cache occupancy for T.
+ *
+ * Groups of threads that are to be monitored together (such as cgroups
+ * or processes) can shared a RMID.
+ *
+ * This driver implements a tree hierarchy of Monitored Resources (monr). Each
+ * monr is a cgroup, a process or a thread that needs one single RMID.
+ *
+ * Since the number of RMIDs is relatively small to the number of potential
+ * monitored elements, RMIDs must be "rotated" among all monitored elements.
+ */
+
+#include <linux/perf_event.h>
+#include <asm/pqr_common.h>
+#include <asm/topology.h>
+
+/*
+ * Time between execution of rotation logic. The frequency of execution does
+ * not affect the rate at which RMIDs are recycled, except by the delay by the
+ * delay updating the prmid's and their pools.
+ * The rotation period is stored in pmu->hrtimer_interval_ms.
+ */
+#define CQM_DEFAULT_ROTATION_PERIOD 1200	/* ms */
-- 
2.8.0.rc3.226.g39d4020

[toc] | [prev] | [next] | [standalone]

#1399597 — [PATCH v2 02/32] perf/x86/intel/cqm: software cache for MSR_IA32_PQR_ASSOC

The msr MSR_IA32_PQR_ASSOC is shared by CQM and the upcoming CAT. Since
writes to this msr are slow (more than 1000 cycles) and mostly occur in
context switch, this patch introduces a software cache that avoids wrsmr
that wont change the msr's value.

Reviewed-by: Stephane Eranian <eranian@google.com>
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 arch/x86/include/asm/pqr_common.h | 44 +++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/cpu/pqr_common.c  |  8 +++++++
 2 files changed, 52 insertions(+)
 create mode 100644 arch/x86/include/asm/pqr_common.h
 create mode 100644 arch/x86/kernel/cpu/pqr_common.c

diff --git a/arch/x86/include/asm/pqr_common.h b/arch/x86/include/asm/pqr_common.h
new file mode 100644
index 0000000..854febe
--- /dev/null
+++ b/arch/x86/include/asm/pqr_common.h
@@ -0,0 +1,44 @@
+#ifndef _X86_PQR_COMMON_H_
+#define _X86_PQR_COMMON_H_
+
+#if defined(CONFIG_INTEL_RDT)
+
+#include <linux/types.h>
+#include <asm/percpu.h>
+#include <asm/msr.h>
+
+#define MSR_IA32_PQR_ASSOC	0x0c8f
+
+#define INVALID_RMID		(-1)
+
+/**
+ * struct intel_pqr_state - State cache for the PQR MSR
+ * @rmid:		The cached Resource Monitoring ID
+ * @closid:		The cached Class Of Service ID
+ *
+ * The upper 32 bits of MSR_IA32_PQR_ASSOC contain closid and the
+ * lower 10 bits rmid. The update to MSR_IA32_PQR_ASSOC always
+ * contains both parts, so we need to cache them.
+ *
+ * The cache also helps to avoid pointless updates if the value does
+ * not change.
+ */
+struct intel_pqr_state {
+	u32			rmid;
+	u32			closid;
+};
+
+DECLARE_PER_CPU(struct intel_pqr_state, pqr_state);
+
+static inline void pqr_update_rmid(u32 rmid)
+{
+	struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
+
+	if (state->rmid == rmid)
+		return;
+	state->rmid = rmid;
+	wrmsr(MSR_IA32_PQR_ASSOC, rmid, state->closid);
+}
+
+#endif
+#endif
diff --git a/arch/x86/kernel/cpu/pqr_common.c b/arch/x86/kernel/cpu/pqr_common.c
new file mode 100644
index 0000000..dc6debc
--- /dev/null
+++ b/arch/x86/kernel/cpu/pqr_common.c
@@ -0,0 +1,8 @@
+#include <asm/pqr_common.h>
+
+/*
+ * The cached intel_pqr_state is strictly per CPU and can never be
+ * updated from a remote CPU. Functions that modify pqr_state
+ * must ensure interruptions are properly handled.
+ */
+DEFINE_PER_CPU(struct intel_pqr_state, pqr_state);
-- 
2.8.0.rc3.226.g39d4020

[toc] | [prev] | [next] | [standalone]

#1399598 — [PATCH v2 03/32] x86/intel,cqm: add CONFIG_INTEL_RDT configuration flag

Add Intel's PQR as its own build target with no build dependency
on CQM. Add CONFIG_INTEL_RDT as a configuration flag that builds PQR
and related drivers (currently CQM, future: MBM, CAT, CDP).

Reviewed-by: Stephane Eranian <eranian@google.com>
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 arch/x86/Kconfig               | 7 +++++++
 arch/x86/events/intel/Makefile | 3 ++-
 arch/x86/kernel/cpu/Makefile   | 4 ++++
 3 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index a494fa3..52a7edc 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -160,6 +160,13 @@ config X86
 	select ARCH_USES_HIGH_VMA_FLAGS		if X86_INTEL_MEMORY_PROTECTION_KEYS
 	select ARCH_HAS_PKEYS			if X86_INTEL_MEMORY_PROTECTION_KEYS
 
+config INTEL_RDT
+	def_bool y
+	depends on PERF_EVENTS && CPU_SUP_INTEL
+	---help---
+	Enable Resource Director Technology (RDT) for Intel Xeon Microprocessors.
+	RDT includes Cache Monitoring Technology (CMT aka CQM).
+
 config INSTRUCTION_DECODER
 	def_bool y
 	depends on KPROBES || PERF_EVENTS || UPROBES
diff --git a/arch/x86/events/intel/Makefile b/arch/x86/events/intel/Makefile
index 3660b2c..7e610bf 100644
--- a/arch/x86/events/intel/Makefile
+++ b/arch/x86/events/intel/Makefile
@@ -1,4 +1,4 @@
-obj-$(CONFIG_CPU_SUP_INTEL)		+= core.o bts.o cqm.o
+obj-$(CONFIG_CPU_SUP_INTEL)		+= core.o bts.o
 obj-$(CONFIG_CPU_SUP_INTEL)		+= ds.o knc.o
 obj-$(CONFIG_CPU_SUP_INTEL)		+= lbr.o p4.o p6.o pt.o
 obj-$(CONFIG_PERF_EVENTS_INTEL_RAPL)	+= intel-rapl.o
@@ -7,3 +7,4 @@ obj-$(CONFIG_PERF_EVENTS_INTEL_UNCORE)	+= intel-uncore.o
 intel-uncore-objs			:= uncore.o uncore_nhmex.o uncore_snb.o uncore_snbep.o
 obj-$(CONFIG_PERF_EVENTS_INTEL_CSTATE)	+= intel-cstate.o
 intel-cstate-objs			:= cstate.o
+obj-$(CONFIG_INTEL_RDT)			+= cqm.o
diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
index 4a8697f..87e6279 100644
--- a/arch/x86/kernel/cpu/Makefile
+++ b/arch/x86/kernel/cpu/Makefile
@@ -34,6 +34,10 @@ obj-$(CONFIG_CPU_SUP_CENTAUR)		+= centaur.o
 obj-$(CONFIG_CPU_SUP_TRANSMETA_32)	+= transmeta.o
 obj-$(CONFIG_CPU_SUP_UMC_32)		+= umc.o
 
+ifdef CONFIG_CPU_SUP_INTEL
+obj-$(CONFIG_INTEL_RDT)			+= pqr_common.o
+endif
+
 obj-$(CONFIG_X86_MCE)			+= mcheck/
 obj-$(CONFIG_MTRR)			+= mtrr/
 obj-$(CONFIG_MICROCODE)			+= microcode/
-- 
2.8.0.rc3.226.g39d4020

[toc] | [prev] | [next] | [standalone]

#1403182 — Re: [PATCH v2 03/32] x86/intel,cqm: add CONFIG_INTEL_RDT configuration flag

On Wed, 11 May 2016, David Carrillo-Cisneros wrote:

> Add Intel's PQR as its own build target with no build dependency
> on CQM. Add CONFIG_INTEL_RDT as a configuration flag that builds PQR
> and related drivers (currently CQM, future: MBM, CAT, CDP).
> 
> Reviewed-by: Stephane Eranian <eranian@google.com>
> Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
> ---
>  arch/x86/Kconfig               | 7 +++++++
>  arch/x86/events/intel/Makefile | 3 ++-
>  arch/x86/kernel/cpu/Makefile   | 4 ++++
>  3 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index a494fa3..52a7edc 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -160,6 +160,13 @@ config X86
>  	select ARCH_USES_HIGH_VMA_FLAGS		if X86_INTEL_MEMORY_PROTECTION_KEYS
>  	select ARCH_HAS_PKEYS			if X86_INTEL_MEMORY_PROTECTION_KEYS
>  
> +config INTEL_RDT
> +	def_bool y
> +	depends on PERF_EVENTS && CPU_SUP_INTEL
> +	---help---
> +	Enable Resource Director Technology (RDT) for Intel Xeon Microprocessors.
> +	RDT includes Cache Monitoring Technology (CMT aka CQM).
> +

First this wnats to be a module. Second this patch does not compile stand
alone.

>  config INSTRUCTION_DECODER
>  	def_bool y
>  	depends on KPROBES || PERF_EVENTS || UPROBES
> diff --git a/arch/x86/events/intel/Makefile b/arch/x86/events/intel/Makefile
> index 3660b2c..7e610bf 100644
> --- a/arch/x86/events/intel/Makefile
> +++ b/arch/x86/events/intel/Makefile
> @@ -1,4 +1,4 @@
> -obj-$(CONFIG_CPU_SUP_INTEL)		+= core.o bts.o cqm.o

This wants to go with the patch which removes cqm.c

> +obj-$(CONFIG_CPU_SUP_INTEL)		+= core.o bts.o
>  obj-$(CONFIG_CPU_SUP_INTEL)		+= ds.o knc.o
>  obj-$(CONFIG_CPU_SUP_INTEL)		+= lbr.o p4.o p6.o pt.o
>  obj-$(CONFIG_PERF_EVENTS_INTEL_RAPL)	+= intel-rapl.o
> @@ -7,3 +7,4 @@ obj-$(CONFIG_PERF_EVENTS_INTEL_UNCORE)	+= intel-uncore.o
>  intel-uncore-objs			:= uncore.o uncore_nhmex.o uncore_snb.o uncore_snbep.o
>  obj-$(CONFIG_PERF_EVENTS_INTEL_CSTATE)	+= intel-cstate.o
>  intel-cstate-objs			:= cstate.o
> +obj-$(CONFIG_INTEL_RDT)			+= cqm.o
> diff --git a/arch/x86/kernel/cpu/Makefile b/arch/x86/kernel/cpu/Makefile
> index 4a8697f..87e6279 100644
> --- a/arch/x86/kernel/cpu/Makefile
> +++ b/arch/x86/kernel/cpu/Makefile
> @@ -34,6 +34,10 @@ obj-$(CONFIG_CPU_SUP_CENTAUR)		+= centaur.o
>  obj-$(CONFIG_CPU_SUP_TRANSMETA_32)	+= transmeta.o
>  obj-$(CONFIG_CPU_SUP_UMC_32)		+= umc.o
>  
> +ifdef CONFIG_CPU_SUP_INTEL

INTEL_RDT already depends on CPU_SUP_INTEL ...

> +obj-$(CONFIG_INTEL_RDT)			+= pqr_common.o
> +endif

Thanks

	tglx

[toc] | [prev] | [next] | [standalone]

#1399599 — [PATCH v2 06/32] perf/x86/intel/cqm: add per-package RMIDs, data and locks

Introduce struct pkg_data that contains all per-package CQM data for new
CQM driver. The per-package data is:
  1) A pool of free prmids (per-package per RMID). Each package may have
  different number of prmids (different hw max_rmid_index).
  2) lock and mutex that protect the prmids pools, changes to the pmonr
  state, and the rotation logic.
  The per-package separation of locks reduces the contention for each
 lock and mutex compared with the previous version that had system-wide
 mutex and lock.

More per-package data will be added in future patches is this series.

Reviewed-by: Stephane Eranian <eranian@google.com>
Signed-off-by: David Carrillo-Cisneros <davidcc@google.com>
---
 arch/x86/events/intel/cqm.c | 499 ++++++++++++++++++++++++++++++++++++++++++++
 arch/x86/events/intel/cqm.h |  62 ++++++
 include/linux/perf_event.h  |   7 +
 3 files changed, 568 insertions(+)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index 2daee37..54f219f 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -12,6 +12,8 @@
 #define MSR_IA32_QM_CTR		0x0c8e
 #define MSR_IA32_QM_EVTSEL	0x0c8d
 
+static unsigned int cqm_l3_scale; /* supposedly cacheline size */
+
 #define RMID_VAL_ERROR		(1ULL << 63)
 #define RMID_VAL_UNAVAIL	(1ULL << 62)
 
@@ -69,3 +71,500 @@ static inline int __cqm_prmid_update(struct prmid *prmid,
 
 	return 1;
 }
+
+/*
+ * A cache groups is a group of perf_events with the same target (thread,
+ * cgroup, CPU or system-wide). Each cache group receives has one RMID.
+ * Cache groups are protected by cqm_mutex.
+ */
+static LIST_HEAD(cache_groups);
+static DEFINE_MUTEX(cqm_mutex);
+
+struct pkg_data **cqm_pkgs_data;
+
+static inline bool __valid_pkg_id(u16 pkg_id)
+{
+	return pkg_id < topology_max_packages();
+}
+
+/* Init cqm pkg_data for @cpu 's package. */
+static int pkg_data_init_cpu(int cpu)
+{
+	struct pkg_data *pkg_data;
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	u16 pkg_id = topology_physical_package_id(cpu);
+
+	if (cqm_pkgs_data[pkg_id])
+		return 0;
+
+
+	pkg_data = kmalloc_node(sizeof(struct pkg_data),
+				GFP_KERNEL, cpu_to_node(cpu));
+	if (!pkg_data)
+		return -ENOMEM;
+
+	pkg_data->max_rmid = c->x86_cache_max_rmid;
+
+	/* Does hardware has more rmids than this driver can handle? */
+	if (WARN_ON(pkg_data->max_rmid >= INVALID_RMID))
+		pkg_data->max_rmid = INVALID_RMID - 1;
+
+	if (c->x86_cache_occ_scale != cqm_l3_scale) {
+		pr_err("Multiple LLC scale values, disabling\n");
+		kfree(pkg_data);
+		return -EINVAL;
+	}
+
+	pkg_data->prmids_by_rmid = kmalloc_node(
+		sizeof(struct prmid *) * (1 + pkg_data->max_rmid),
+		GFP_KERNEL, cpu_to_node(cpu));
+
+	if (!pkg_data) {
+		kfree(pkg_data);
+		return -ENOMEM;
+	}
+
+	INIT_LIST_HEAD(&pkg_data->free_prmids_pool);
+
+	mutex_init(&pkg_data->pkg_data_mutex);
+	raw_spin_lock_init(&pkg_data->pkg_data_lock);
+
+	/* XXX: Chose randomly*/
+	pkg_data->rotation_cpu = cpu;
+
+	cqm_pkgs_data[pkg_id] = pkg_data;
+	return 0;
+}
+
+static int intel_cqm_setup_pkg_prmid_pools(u16 pkg_id)
+{
+	int r;
+	unsigned long flags;
+	struct prmid *prmid;
+	struct pkg_data *pkg_data = cqm_pkgs_data[pkg_id];
+
+	if (!__valid_pkg_id(pkg_id))
+		return -EINVAL;
+
+	for (r = 0; r <= pkg_data->max_rmid; r++) {
+
+		prmid = kmalloc_node(sizeof(struct prmid), GFP_KERNEL,
+				     cpu_to_node(pkg_data->rotation_cpu));
+		if (!prmid)
+			goto fail;
+
+		atomic64_set(&prmid->last_read_value, 0L);
+		atomic64_set(&prmid->last_read_time, 0L);
+		INIT_LIST_HEAD(&prmid->pool_entry);
+		prmid->rmid = r;
+
+		/* Lock needed if called during CPU hotplug. */
+		raw_spin_lock_irqsave_nested(
+			&pkg_data->pkg_data_lock, flags, pkg_id);
+		pkg_data->prmids_by_rmid[r] = prmid;
+
+
+		/* RMID 0 is special and makes the root of rmid hierarchy. */
+		raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
+	}
+	return 0;
+fail:
+	while (!list_empty(&pkg_data->free_prmids_pool)) {
+		prmid = list_first_entry(&pkg_data->free_prmids_pool,
+					 struct prmid, pool_entry);
+		list_del(&prmid->pool_entry);
+		kfree(pkg_data->prmids_by_rmid[prmid->rmid]);
+		kfree(prmid);
+	}
+	return -ENOMEM;
+}
+
+
+/*
+ * Determine if @a and @b measure the same set of tasks.
+ *
+ * If @a and @b measure the same set of tasks then we want to share a
+ * single RMID.
+ */
+static bool __match_event(struct perf_event *a, struct perf_event *b)
+{
+	/* Per-cpu and task events don't mix */
+	if ((a->attach_state & PERF_ATTACH_TASK) !=
+	    (b->attach_state & PERF_ATTACH_TASK))
+		return false;
+
+#ifdef CONFIG_CGROUP_PERF
+	if (a->cgrp != b->cgrp)
+		return false;
+#endif
+
+	/* If not task event, it's a a cgroup or a non-task cpu event. */
+	if (!(b->attach_state & PERF_ATTACH_TASK))
+		return true;
+
+	/*
+	 * Events that target same task are placed into the same cache group.
+	 */
+	if (a->hw.target == b->hw.target)
+		return true;
+
+	/*
+	 * Are we an inherited event?
+	 */
+	if (b->parent == a)
+		return true;
+
+	return false;
+}
+
+static struct pmu intel_cqm_pmu;
+
+/*
+ * Find a group and setup RMID.
+ *
+ * If we're part of a group, we use the group's monr.
+ */
+static int
+intel_cqm_setup_event(struct perf_event *event, struct perf_event **group)
+{
+	struct perf_event *iter;
+
+
+	list_for_each_entry(iter, &cache_groups, hw.cqm_event_groups_entry) {
+		if (__match_event(iter, event)) {
+			*group = iter;
+			return 0;
+		}
+	}
+	return 0;
+}
+
+/* Read current package immediately and remote pkg (if any) from cache. */
+static void intel_cqm_event_read(struct perf_event *event)
+{
+}
+
+static void intel_cqm_event_start(struct perf_event *event, int mode)
+{
+	if (!(event->hw.state & PERF_HES_STOPPED))
+		return;
+
+	event->hw.state &= ~PERF_HES_STOPPED;
+}
+
+static void intel_cqm_event_stop(struct perf_event *event, int mode)
+{
+	if (event->hw.state & PERF_HES_STOPPED)
+		return;
+
+	event->hw.state |= PERF_HES_STOPPED;
+}
+
+static int intel_cqm_event_add(struct perf_event *event, int mode)
+{
+	event->hw.state = PERF_HES_STOPPED;
+
+	return 0;
+}
+
+static inline bool cqm_group_leader(struct perf_event *event)
+{
+	return !list_empty(&event->hw.cqm_event_groups_entry);
+}
+
+static void intel_cqm_event_destroy(struct perf_event *event)
+{
+	struct perf_event *group_other = NULL;
+
+	mutex_lock(&cqm_mutex);
+	/*
+	 * If there's another event in this group...
+	 */
+	if (!list_empty(&event->hw.cqm_event_group_entry)) {
+		group_other = list_first_entry(&event->hw.cqm_event_group_entry,
+					       struct perf_event,
+					       hw.cqm_event_group_entry);
+		list_del(&event->hw.cqm_event_group_entry);
+	}
+	/*
+	 * And we're the group leader..
+	 */
+	if (!cqm_group_leader(event))
+		goto exit;
+
+	/*
+	 * If there was a group_other, make that leader, otherwise
+	 * destroy the group and return the RMID.
+	 */
+	if (group_other) {
+		/* Update monr reference to group head. */
+		list_replace(&event->hw.cqm_event_groups_entry,
+			     &group_other->hw.cqm_event_groups_entry);
+		goto exit;
+	}
+
+	/*
+	 * Event is the only event in cache group.
+	 */
+
+	list_del(&event->hw.cqm_event_groups_entry);
+
+exit:
+	mutex_unlock(&cqm_mutex);
+}
+
+static int intel_cqm_event_init(struct perf_event *event)
+{
+	struct perf_event *group = NULL;
+	int ret;
+
+	if (event->attr.type != intel_cqm_pmu.type)
+		return -ENOENT;
+
+	if (event->attr.config & ~QOS_EVENT_MASK)
+		return -EINVAL;
+
+	/* unsupported modes and filters */
+	if (event->attr.exclude_user   ||
+	    event->attr.exclude_kernel ||
+	    event->attr.exclude_hv     ||
+	    event->attr.exclude_idle   ||
+	    event->attr.exclude_host   ||
+	    event->attr.exclude_guest  ||
+	    event->attr.sample_period) /* no sampling */
+		return -EINVAL;
+
+	INIT_LIST_HEAD(&event->hw.cqm_event_groups_entry);
+	INIT_LIST_HEAD(&event->hw.cqm_event_group_entry);
+
+	event->destroy = intel_cqm_event_destroy;
+
+	mutex_lock(&cqm_mutex);
+
+
+	/* Will also set rmid */
+	ret = intel_cqm_setup_event(event, &group);
+	if (ret) {
+		mutex_unlock(&cqm_mutex);
+		return ret;
+	}
+
+	if (group) {
+		list_add_tail(&event->hw.cqm_event_group_entry,
+				&group->hw.cqm_event_group_entry);
+	} else {
+		list_add_tail(&event->hw.cqm_event_groups_entry,
+				&cache_groups);
+	}
+
+	mutex_unlock(&cqm_mutex);
+
+	return 0;
+}
+
+EVENT_ATTR_STR(llc_occupancy, intel_cqm_llc, "event=0x01");
+EVENT_ATTR_STR(llc_occupancy.per-pkg, intel_cqm_llc_pkg, "1");
+EVENT_ATTR_STR(llc_occupancy.unit, intel_cqm_llc_unit, "Bytes");
+EVENT_ATTR_STR(llc_occupancy.scale, intel_cqm_llc_scale, NULL);
+EVENT_ATTR_STR(llc_occupancy.snapshot, intel_cqm_llc_snapshot, "1");
+
+static struct attribute *intel_cqm_events_attr[] = {
+	EVENT_PTR(intel_cqm_llc),
+	EVENT_PTR(intel_cqm_llc_pkg),
+	EVENT_PTR(intel_cqm_llc_unit),
+	EVENT_PTR(intel_cqm_llc_scale),
+	EVENT_PTR(intel_cqm_llc_snapshot),
+	NULL,
+};
+
+static struct attribute_group intel_cqm_events_group = {
+	.name = "events",
+	.attrs = intel_cqm_events_attr,
+};
+
+PMU_FORMAT_ATTR(event, "config:0-7");
+static struct attribute *intel_cqm_formats_attr[] = {
+	&format_attr_event.attr,
+	NULL,
+};
+
+static struct attribute_group intel_cqm_format_group = {
+	.name = "format",
+	.attrs = intel_cqm_formats_attr,
+};
+
+static const struct attribute_group *intel_cqm_attr_groups[] = {
+	&intel_cqm_events_group,
+	&intel_cqm_format_group,
+	NULL,
+};
+
+static struct pmu intel_cqm_pmu = {
+	.hrtimer_interval_ms = CQM_DEFAULT_ROTATION_PERIOD,
+	.attr_groups	     = intel_cqm_attr_groups,
+	.task_ctx_nr	     = perf_sw_context,
+	.event_init	     = intel_cqm_event_init,
+	.add		     = intel_cqm_event_add,
+	.del		     = intel_cqm_event_stop,
+	.start		     = intel_cqm_event_start,
+	.stop		     = intel_cqm_event_stop,
+	.read		     = intel_cqm_event_read,
+};
+
+static inline void cqm_pick_event_reader(int cpu)
+{
+	u16 pkg_id = topology_physical_package_id(cpu);
+	/* XXX: lock, check if rotation cpu is online, maybe */
+	/*
+	 * Pick a reader if there isn't one already.
+	 */
+	if (cqm_pkgs_data[pkg_id]->rotation_cpu != -1)
+		cqm_pkgs_data[pkg_id]->rotation_cpu = cpu;
+}
+
+static void intel_cqm_cpu_starting(unsigned int cpu)
+{
+	struct intel_pqr_state *state = &per_cpu(pqr_state, cpu);
+	struct cpuinfo_x86 *c = &cpu_data(cpu);
+	u16 pkg_id = topology_physical_package_id(cpu);
+
+	state->rmid = 0;
+	state->closid = 0;
+
+	/* XXX: lock */
+	/* XXX: Make sure this case is handled when hotplug happens. */
+	WARN_ON(c->x86_cache_max_rmid != cqm_pkgs_data[pkg_id]->max_rmid);
+	WARN_ON(c->x86_cache_occ_scale != cqm_l3_scale);
+}
+
+static void intel_cqm_cpu_exit(unsigned int cpu)
+{
+	/*
+	 * Is @cpu a designated cqm reader?
+	 */
+	u16 pkg_id = topology_physical_package_id(cpu);
+
+	if (cqm_pkgs_data[pkg_id]->rotation_cpu != cpu)
+		return;
+	/* XXX: do remove unused packages */
+	cqm_pkgs_data[pkg_id]->rotation_cpu = cpumask_any_but(
+		topology_core_cpumask(cpu), cpu);
+}
+
+static int intel_cqm_cpu_notifier(struct notifier_block *nb,
+				  unsigned long action, void *hcpu)
+{
+	unsigned int cpu  = (unsigned long)hcpu;
+
+	switch (action & ~CPU_TASKS_FROZEN) {
+	case CPU_DOWN_PREPARE:
+		intel_cqm_cpu_exit(cpu);
+		break;
+	case CPU_STARTING:
+		pkg_data_init_cpu(cpu);
+		intel_cqm_cpu_starting(cpu);
+		cqm_pick_event_reader(cpu);
+		break;
+	}
+
+	return NOTIFY_OK;
+}
+
+static const struct x86_cpu_id intel_cqm_match[] = {
+	{ .vendor = X86_VENDOR_INTEL, .feature = X86_FEATURE_CQM_OCCUP_LLC },
+	{}
+};
+
+static int __init intel_cqm_init(void)
+{
+	char *str, scale[20];
+	int i, cpu, ret = 0, min_max_rmid = 0;
+
+	if (!x86_match_cpu(intel_cqm_match))
+		return -ENODEV;
+
+	cqm_l3_scale = boot_cpu_data.x86_cache_occ_scale;
+	if (WARN_ON(cqm_l3_scale == 0))
+		cqm_l3_scale = 1;
+
+	cqm_pkgs_data = kmalloc(
+		sizeof(struct pkg_data *) * topology_max_packages(),
+		GFP_KERNEL);
+	if (!cqm_pkgs_data)
+		return -ENOMEM;
+
+	for (i = 0; i < topology_max_packages(); i++)
+		cqm_pkgs_data[i] = NULL;
+
+	/*
+	 * It's possible that not all resources support the same number
+	 * of RMIDs. Instead of making scheduling much more complicated
+	 * (where we have to match a task's RMID to a cpu that supports
+	 * that many RMIDs) just find the minimum RMIDs supported across
+	 * all cpus.
+	 *
+	 * Also, check that the scales match on all cpus.
+	 */
+	cpu_notifier_register_begin();
+
+	/* XXX: assert all cpus in pkg have same nr rmids (they should). */
+	for_each_online_cpu(cpu) {
+		ret = pkg_data_init_cpu(cpu);
+		if  (ret)
+			goto error;
+	}
+
+	/* Select the minimum of the maximum rmids to use as limit for
+	 * threshold. XXX: per-package threshold.
+	 */
+	cqm_pkg_id_for_each_online(i) {
+		if (min_max_rmid < cqm_pkgs_data[i]->max_rmid)
+			min_max_rmid = cqm_pkgs_data[i]->max_rmid;
+		intel_cqm_setup_pkg_prmid_pools(i);
+	}
+
+	/*
+	 * A reasonable upper limit on the max threshold is the number
+	 * of lines tagged per RMID if all RMIDs have the same number of
+	 * lines tagged in the LLC.
+	 *
+	 * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC.
+	 */
+	__intel_cqm_max_threshold =
+		boot_cpu_data.x86_cache_size * 1024 / (min_max_rmid + 1);
+
+	snprintf(scale, sizeof(scale), "%u", cqm_l3_scale);
+	str = kstrdup(scale, GFP_KERNEL);
+	if (!str) {
+		ret = -ENOMEM;
+		goto error;
+	}
+
+	event_attr_intel_cqm_llc_scale.event_str = str;
+
+	for_each_online_cpu(i) {
+		intel_cqm_cpu_starting(i);
+		cqm_pick_event_reader(i);
+	}
+
+	__perf_cpu_notifier(intel_cqm_cpu_notifier);
+
+	ret = perf_pmu_register(&intel_cqm_pmu, "intel_cqm", -1);
+	if (ret)
+		goto error;
+
+	cpu_notifier_register_done();
+
+	pr_info("Intel CQM monitoring enabled with at least %u rmids per package.\n",
+		min_max_rmid + 1);
+
+	return ret;
+
+error:
+	pr_err("Intel CQM perf registration failed: %d\n", ret);
+	cpu_notifier_register_done();
+
+	return ret;
+}
+
+device_initcall(intel_cqm_init);
diff --git a/arch/x86/events/intel/cqm.h b/arch/x86/events/intel/cqm.h
index 06964cd..08623b5 100644
--- a/arch/x86/events/intel/cqm.h
+++ b/arch/x86/events/intel/cqm.h
@@ -41,9 +41,71 @@ struct prmid {
 };
 
 /*
+ * struct pkg_data: Per-package CQM data.
+ * @max_rmid:			Max rmid valid for cpus in this package.
+ * @prmids_by_rmid:		Utility mapping between rmid values and prmids.
+ *				XXX: Make it an array of prmids.
+ * @free_prmid_pool:		Free prmids.
+ * @pkg_data_mutex:		Hold for stability when modifying pmonrs
+ *				hierarchy.
+ * @pkg_data_lock:		Hold to protect variables that may be accessed
+ *				during process scheduling. The locks for all
+ *				packages must be held when modifying the monr
+ *				hierarchy.
+ * @rotation_cpu:               CPU to run @rotation_work on, it must be in the
+ *                              package associated to this instance of pkg_data.
+ */
+struct pkg_data {
+	u32			max_rmid;
+	/* Quick map from rmids to prmids. */
+	struct prmid		**prmids_by_rmid;
+
+	/*
+	 * Pools of prmids used in rotation logic.
+	 */
+	struct list_head	free_prmids_pool;
+
+	struct mutex		pkg_data_mutex;
+	raw_spinlock_t		pkg_data_lock;
+
+	int			rotation_cpu;
+};
+
+extern struct pkg_data **cqm_pkgs_data;
+
+static inline u16 __cqm_pkgs_data_next_online(u16 pkg_id)
+{
+	while (!cqm_pkgs_data[++pkg_id] && pkg_id < topology_max_packages())
+		;
+	return pkg_id;
+}
+
+static inline u16 __cqm_pkgs_data_first_online(void)
+{
+	if (cqm_pkgs_data[0])
+		return 0;
+	return __cqm_pkgs_data_next_online(0);
+}
+
+/* Iterate for each online pkgs data */
+#define cqm_pkg_id_for_each_online(pkg_id__) \
+	for (pkg_id__ = __cqm_pkgs_data_first_online(); \
+	     pkg_id__ < topology_max_packages(); \
+	     pkg_id__ = __cqm_pkgs_data_next_online(pkg_id__))
+
+#define __pkg_data(pmonr, member) cqm_pkgs_data[pmonr->pkg_id]->member
+
+/*
  * Time between execution of rotation logic. The frequency of execution does
  * not affect the rate at which RMIDs are recycled, except by the delay by the
  * delay updating the prmid's and their pools.
  * The rotation period is stored in pmu->hrtimer_interval_ms.
  */
 #define CQM_DEFAULT_ROTATION_PERIOD 1200	/* ms */
+
+/*
+ * __intel_cqm_max_threshold provides an upper bound on the threshold,
+ * and is measured in bytes because it's exposed to userland.
+ * It's units are bytes must be scaled by cqm_l3_scale to obtain cache lines.
+ */
+static unsigned int __intel_cqm_max_threshold;
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 1417d3b..02b8e24 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -118,6 +118,13 @@ struct hw_perf_event {
 			/* for tp_event->class */
 			struct list_head	tp_list;
 		};
+#ifdef CONFIG_INTEL_RDT
+		struct { /* intel_cqm */
+			void			*cqm_monr;
+			struct list_head	cqm_event_group_entry;
+			struct list_head	cqm_event_groups_entry;
+		};
+#endif
 		struct { /* itrace */
 			int			itrace_started;
 		};
-- 
2.8.0.rc3.226.g39d4020

[toc] | [prev] | [next] | [standalone]

#1403133 — Re: [PATCH v2 06/32] perf/x86/intel/cqm: add per-package RMIDs, data and locks

On Wed, 11 May 2016, David Carrillo-Cisneros wrote:
> +/* Init cqm pkg_data for @cpu 's package. */
> +static int pkg_data_init_cpu(int cpu)
> +{
> +	struct pkg_data *pkg_data;
> +	struct cpuinfo_x86 *c = &cpu_data(cpu);
> +	u16 pkg_id = topology_physical_package_id(cpu);
> +
> +	if (cqm_pkgs_data[pkg_id])
> +		return 0;
> +
> +
> +	pkg_data = kmalloc_node(sizeof(struct pkg_data),
> +				GFP_KERNEL, cpu_to_node(cpu));

kzalloc_node() please

> +	if (!pkg_data)
> +		return -ENOMEM;
> +
> +	pkg_data->max_rmid = c->x86_cache_max_rmid;
> +
> +	/* Does hardware has more rmids than this driver can handle? */
> +	if (WARN_ON(pkg_data->max_rmid >= INVALID_RMID))
> +		pkg_data->max_rmid = INVALID_RMID - 1;
> +
> +	if (c->x86_cache_occ_scale != cqm_l3_scale) {
> +		pr_err("Multiple LLC scale values, disabling\n");
> +		kfree(pkg_data);
> +		return -EINVAL;
> +	}

Please move this check before the allocation. 

> +	pkg_data->prmids_by_rmid = kmalloc_node(
> +		sizeof(struct prmid *) * (1 + pkg_data->max_rmid),
> +		GFP_KERNEL, cpu_to_node(cpu));

kzalloc_node()

> +
> +	if (!pkg_data) {
> +		kfree(pkg_data);

Huch? You alloc pkg_data->prmids_by_rmid and then check pkg_data, which is
guaranteed to be non NULL here.

> +		return -ENOMEM;
> +	}
> +
> +	INIT_LIST_HEAD(&pkg_data->free_prmids_pool);
> +
> +	mutex_init(&pkg_data->pkg_data_mutex);
> +	raw_spin_lock_init(&pkg_data->pkg_data_lock);
> +
> +	/* XXX: Chose randomly*/

Please add coherent comments, e.g.:

       /*
        * We should select the rotation_cpu randomly in the package,
        * because ..... Use the current cpu for now.
	*/

> +	pkg_data->rotation_cpu = cpu;
> +
> +	cqm_pkgs_data[pkg_id] = pkg_data;
> +	return 0;
> +}
> +
> +static int intel_cqm_setup_pkg_prmid_pools(u16 pkg_id)
> +{
> +	int r;
> +	unsigned long flags;
> +	struct prmid *prmid;
> +	struct pkg_data *pkg_data = cqm_pkgs_data[pkg_id];

This variable ordering is really hard to read.

	struct pkg_data *pkg_data = cqm_pkgs_data[pkg_id];
	struct prmid *prmid;
	unsigned long flags;
	int r;

Parses faster, but that's my personal taste and the least of my worries with
this patch.

> +
> +	if (!__valid_pkg_id(pkg_id))
> +		return -EINVAL;
> +
> +	for (r = 0; r <= pkg_data->max_rmid; r++) {
> +
> +		prmid = kmalloc_node(sizeof(struct prmid), GFP_KERNEL,
> +				     cpu_to_node(pkg_data->rotation_cpu));
> +		if (!prmid)
> +			goto fail;
> +
> +		atomic64_set(&prmid->last_read_value, 0L);
> +		atomic64_set(&prmid->last_read_time, 0L);
> +		INIT_LIST_HEAD(&prmid->pool_entry);
> +		prmid->rmid = r;
> +
> +		/* Lock needed if called during CPU hotplug. */
> +		raw_spin_lock_irqsave_nested(
> +			&pkg_data->pkg_data_lock, flags, pkg_id);
> +		pkg_data->prmids_by_rmid[r] = prmid;

So this adds the prmid to the array, but it does not link it into the free
pool list. So the fail path is completely bogus.

> +		/* RMID 0 is special and makes the root of rmid hierarchy. */

This comment does not make any sense here.

> +		raw_spin_unlock_irqrestore(&pkg_data->pkg_data_lock, flags);
> +	}
> +	return 0;
> +fail:
> +	while (!list_empty(&pkg_data->free_prmids_pool)) {
> +		prmid = list_first_entry(&pkg_data->free_prmids_pool,
> +					 struct prmid, pool_entry);
> +		list_del(&prmid->pool_entry);
> +		kfree(pkg_data->prmids_by_rmid[prmid->rmid]);
> +		kfree(prmid);
> +	}
> +	return -ENOMEM;

Please split out the fail path into a seperate function so it can be reused
for error handling and module removal.

> +/*
> + * Determine if @a and @b measure the same set of tasks.
> + *
> + * If @a and @b measure the same set of tasks then we want to share a
> + * single RMID.
> + */
> +static bool __match_event(struct perf_event *a, struct perf_event *b)

Can you please split out the event support and just keep the infrastructure
setup in this patch for review purposes? We can fold stuff together once we
are done with this.

> +static inline void cqm_pick_event_reader(int cpu)
> +{
> +	u16 pkg_id = topology_physical_package_id(cpu);
> +	/* XXX: lock, check if rotation cpu is online, maybe */

So I assume 'XXX' means: FIXME or such. 

1) If you have no idea whether you need the lock then you better make your
   mind up before posting.

2) @cpu is online as this is called from CPU_STARTING resp. from the
   init code.

So please can you fix this stuff and/or remove crap comments like the above.

> +	/*
> +	 * Pick a reader if there isn't one already.
> +	 */
> +	if (cqm_pkgs_data[pkg_id]->rotation_cpu != -1)
> +		cqm_pkgs_data[pkg_id]->rotation_cpu = cpu;

This does not do what the comment says. If rotation_cpu is -1, i.e. not
assigned then it stays that way and if its assigned it is overwritten
unconditionally.

> +}
> +
> +static void intel_cqm_cpu_starting(unsigned int cpu)
> +{
> +	struct intel_pqr_state *state = &per_cpu(pqr_state, cpu);
> +	struct cpuinfo_x86 *c = &cpu_data(cpu);
> +	u16 pkg_id = topology_physical_package_id(cpu);
> +
> +	state->rmid = 0;
> +	state->closid = 0;

That's a pointless exercise. If the cpu was never in use then this is 0. If
the cpu goes down, then this better is cleaned up before it vanishes.

> +
> +	/* XXX: lock */

Sigh.

> +	/* XXX: Make sure this case is handled when hotplug happens. */
> +	WARN_ON(c->x86_cache_max_rmid != cqm_pkgs_data[pkg_id]->max_rmid);
> +	WARN_ON(c->x86_cache_occ_scale != cqm_l3_scale);

If your code depends on the consistency then you better fix this now. No point
in emitting a warning with a well known stack trace and then let the code fall
over somewhere else.

The proper thing to do here is to do the check and mark the package unusable
if the check fails. That's all we can do here.

> +}
> +
> +static void intel_cqm_cpu_exit(unsigned int cpu)
> +{
> +	/*
> +	 * Is @cpu a designated cqm reader?
> +	 */
> +	u16 pkg_id = topology_physical_package_id(cpu);
> +
> +	if (cqm_pkgs_data[pkg_id]->rotation_cpu != cpu)
> +		return;
> +	/* XXX: do remove unused packages */

XXX: No. Leave allocated memory around and be done with it. Especially as you
must handle CPU_DOWN_FAILED ....

> +	cqm_pkgs_data[pkg_id]->rotation_cpu = cpumask_any_but(
> +		topology_core_cpumask(cpu), cpu);
> +}
> +
> +static int intel_cqm_cpu_notifier(struct notifier_block *nb,
> +				  unsigned long action, void *hcpu)
> +{
> +	unsigned int cpu  = (unsigned long)hcpu;
> +
> +	switch (action & ~CPU_TASKS_FROZEN) {
> +	case CPU_DOWN_PREPARE:
> +		intel_cqm_cpu_exit(cpu);
> +		break;

Misses  CPU_DOWN_FAILED handling

> +	case CPU_STARTING:
> +		pkg_data_init_cpu(cpu);

This definitely never saw any testing. You cannot call pkg_data_init_cpu()
from here. This code is called with interrupts disabled on the upcoming cpu.

The allocation stuff must happen before this in UP_PREPARE and fail properly
if the allocations fails. Otherwise you dereference a NULL pointer in
cqm_pick_event_reader() ...

> +		intel_cqm_cpu_starting(cpu);
> +		cqm_pick_event_reader(cpu);
> +		break;
> +	}
> +
> +	return NOTIFY_OK;
> +}
> +
> +static const struct x86_cpu_id intel_cqm_match[] = {
> +	{ .vendor = X86_VENDOR_INTEL, .feature = X86_FEATURE_CQM_OCCUP_LLC },
> +	{}
> +};
> +
> +static int __init intel_cqm_init(void)
> +{
> +	char *str, scale[20];
> +	int i, cpu, ret = 0, min_max_rmid = 0;
> +
> +	if (!x86_match_cpu(intel_cqm_match))
> +		return -ENODEV;
> +
> +	cqm_l3_scale = boot_cpu_data.x86_cache_occ_scale;
> +	if (WARN_ON(cqm_l3_scale == 0))

There is no point in using a WARN_ON is a well known call chain.

> +		cqm_l3_scale = 1;
> +
> +	cqm_pkgs_data = kmalloc(
> +		sizeof(struct pkg_data *) * topology_max_packages(),
> +		GFP_KERNEL);
> +	if (!cqm_pkgs_data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < topology_max_packages(); i++)
> +		cqm_pkgs_data[i] = NULL;

kzalloc() is your friend.

> +	/*
> +	 * It's possible that not all resources support the same number
> +	 * of RMIDs. Instead of making scheduling much more complicated
> +	 * (where we have to match a task's RMID to a cpu that supports
> +	 * that many RMIDs) just find the minimum RMIDs supported across
> +	 * all cpus.
> +	 *
> +	 * Also, check that the scales match on all cpus.
> +	 */
> +	cpu_notifier_register_begin();
> +
> +	/* XXX: assert all cpus in pkg have same nr rmids (they should). */

Your commentry is really annoying. This XXX crap is just telling me that this
whole thing is half baken.

> +	for_each_online_cpu(cpu) {
> +		ret = pkg_data_init_cpu(cpu);
> +		if  (ret)
> +			goto error;
> +	}
> +
> +	/* Select the minimum of the maximum rmids to use as limit for
> +	 * threshold. XXX: per-package threshold.
> +	 */
> +	cqm_pkg_id_for_each_online(i) {
> +		if (min_max_rmid < cqm_pkgs_data[i]->max_rmid)
> +			min_max_rmid = cqm_pkgs_data[i]->max_rmid;
> +		intel_cqm_setup_pkg_prmid_pools(i);

So if intel_cqm_setup_pkg_prmid_pools() fails, we happily proceed, right?

> +	/*
> +	 * A reasonable upper limit on the max threshold is the number
> +	 * of lines tagged per RMID if all RMIDs have the same number of
> +	 * lines tagged in the LLC.
> +	 *
> +	 * For a 35MB LLC and 56 RMIDs, this is ~1.8% of the LLC.
> +	 */
> +	__intel_cqm_max_threshold =
> +		boot_cpu_data.x86_cache_size * 1024 / (min_max_rmid + 1);
> +
> +	snprintf(scale, sizeof(scale), "%u", cqm_l3_scale);
> +	str = kstrdup(scale, GFP_KERNEL);
> +	if (!str) {
> +		ret = -ENOMEM;

Sure, we have no memory and leak the already allocated stuff. This wants
proper error handling. And by proper error handling I don't mean

error1:
error2:
....
errroN:

style at the end of this function. This wants to be a proper rollback which
can also be used for module unload.

> +		goto error;
> +	}

> +	event_attr_intel_cqm_llc_scale.event_str = str;
> +
> +	for_each_online_cpu(i) {
> +		intel_cqm_cpu_starting(i);
> +		cqm_pick_event_reader(i);
> +	}
> +
> +	__perf_cpu_notifier(intel_cqm_cpu_notifier);
> +
> +	ret = perf_pmu_register(&intel_cqm_pmu, "intel_cqm", -1);
> +	if (ret)
> +		goto error;
> +
> +	cpu_notifier_register_done();
> +
> +	pr_info("Intel CQM monitoring enabled with at least %u rmids per package.\n",
> +		min_max_rmid + 1);
> +
> +	return ret;
> +
> +error:
> +	pr_err("Intel CQM perf registration failed: %d\n", ret);
> +	cpu_notifier_register_done();
> +
> +	return ret;
> +}
> +
> +device_initcall(intel_cqm_init);

Please make this a modular driver from the very beginning.

Thanks,

	tglx

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web