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

Groups > linux.kernel > #1553317 > unrolled thread

[PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes

Back to article view | Back to linux.kernel

Contents

  [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-06 23:30 +0100
    [PATCH 10/12] perf/core,x86/cqm: Add read for Cgroup events,per pkg reads. Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-06 23:30 +0100
    [PATCH 06/12] x86/cqm: Add cgroup hierarchical monitoring support Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-06 23:30 +0100
      Re: [PATCH 06/12] x86/cqm: Add cgroup hierarchical monitoring  support Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 15:10 +0100
    [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-06 23:30 +0100
      Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 13:20 +0100
        Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare Peter Zijlstra <peterz@infradead.org> - 2017-01-17 13:40 +0100
        Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-18 03:20 +0100
      Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 14:50 +0100
        Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-17 21:30 +0100
          Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 22:40 +0100
      Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare Peter Zijlstra <peterz@infradead.org> - 2017-01-17 16:30 +0100
        Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-17 21:30 +0100
    [PATCH 04/12] x86/cqm: Add Per pkg rmid support Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-06 23:30 +0100
      Re: [PATCH 04/12] x86/cqm: Add Per pkg rmid support\ Thomas Gleixner <tglx@linutronix.de> - 2017-01-16 19:50 +0100
        Re: [PATCH 04/12] x86/cqm: Add Per pkg rmid support\ Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-17 20:20 +0100
    [PATCH 03/12] x86/rdt: Add rdt common/cqm compile option Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-06 23:30 +0100
      Re: [PATCH 03/12] x86/rdt: Add rdt common/cqm compile option Thomas Gleixner <tglx@linutronix.de> - 2017-01-16 19:10 +0100
        Re: [PATCH 03/12] x86/rdt: Add rdt common/cqm compile option Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-17 18:30 +0100
    [PATCH 11/12] perf/stat: fix bug in handling events in error state Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-06 23:30 +0100
    [PATCH 07/12] x86/rdt,cqm: Scheduling support update Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-06 23:30 +0100
      Re: [PATCH 07/12] x86/rdt,cqm: Scheduling support update Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-17 23:30 +0100
      Re: [PATCH 07/12] x86/rdt,cqm: Scheduling support update Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 23:50 +0100
    [PATCH 01/12] Documentation, x86/cqm: Intel Resource Monitoring Documentation Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-06 23:30 +0100
    Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Thomas Gleixner <tglx@linutronix.de> - 2017-01-17 18:40 +0100
      Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-18 03:50 +0100
        Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Thomas Gleixner <tglx@linutronix.de> - 2017-01-18 10:00 +0100
          Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Peter Zijlstra <peterz@infradead.org> - 2017-01-18 11:10 +0100
            Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-19 21:10 +0100
          Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-18 20:50 +0100
          RE: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes "Yu, Fenghua" <fenghua.yu@intel.com> - 2017-01-18 22:20 +0100
          Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes David Carrillo-Cisneros <davidcc@google.com> - 2017-01-18 22:20 +0100
            Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Thomas Gleixner <tglx@linutronix.de> - 2017-01-19 18:50 +0100
              Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes David Carrillo-Cisneros <davidcc@google.com> - 2017-01-20 08:50 +0100
                Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Thomas Gleixner <tglx@linutronix.de> - 2017-01-20 09:40 +0100
                  Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes David Carrillo-Cisneros <davidcc@google.com> - 2017-01-20 21:30 +0100
          Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes David Carrillo-Cisneros <davidcc@google.com> - 2017-01-19 03:20 +0100
            Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes David Carrillo-Cisneros <davidcc@google.com> - 2017-01-19 18:30 +0100
              Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Thomas Gleixner <tglx@linutronix.de> - 2017-01-19 19:50 +0100
          Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-19 03:30 +0100
          Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Stephane Eranian <eranian@google.com> - 2017-01-19 07:50 +0100
            Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Thomas Gleixner <tglx@linutronix.de> - 2017-01-19 19:50 +0100
          Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Vikas Shivappa <vikas.shivappa@linux.intel.com> - 2017-01-20 03:40 +0100
            Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes David Carrillo-Cisneros <davidcc@google.com> - 2017-01-20 09:00 +0100
              Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Thomas Gleixner <tglx@linutronix.de> - 2017-01-20 15:50 +0100
                Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes David Carrillo-Cisneros <davidcc@google.com> - 2017-01-20 21:20 +0100
                  Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-20 22:10 +0100
                    Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes David Carrillo-Cisneros <davidcc@google.com> - 2017-01-20 22:50 +0100
                      Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-21 01:00 +0100
                  Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Thomas Gleixner <tglx@linutronix.de> - 2017-01-23 11:20 +0100
                    Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Peter Zijlstra <peterz@infradead.org> - 2017-01-23 12:40 +0100
              Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Shivappa Vikas <vikas.shivappa@intel.com> - 2017-01-20 21:50 +0100
            Re: [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes Stephane Eranian <eranian@google.com> - 2017-01-20 20:40 +0100

Page 1 of 3  [1] 2 3  Next page →

#1553317 — [PATCH 00/12] Cqm2: Intel Cache quality monitoring fixes

Resending version 5 with updated send list. Sorry for the spam.

Cqm(cache quality monitoring) is part of Intel RDT(resource director
technology) which enables monitoring and controlling of processor shared
resources via MSR interface.

The current upstream cqm(Cache monitoring) has major issues which make
the feature almost unusable which this series tries to fix and also
address Thomas comments on previous versions of the cqm2 patch series to
better document/organize what we are trying to fix.

	Changes in V5
- Based on Peterz feedback, removed the file interface in perf_event
cgroup to start and stop continuous monitoring.
- Based on Andi's feedback and references David has sent a patch optimizing
the perf overhead as a seperate patch which is generic and not cqm
specific.

This is a continuation of patch series David(davidcc@google.com)
previously posted and hence its based on his patches and is also trying
to fix the same issues. Patches apply on 4.10-rc2 

Below are the issues and the fixes we attempt-

- Issue(1): Inaccurate data for per package data, systemwide. Just prints
zeros or arbitrary numbers.

Fix: Patches fix this by just throwing an error if the mode is not supported. 
The modes supported is task monitoring and cgroup monitoring. 
Also the per package
data for say socket x is returned with the -C <cpu on socketx> -G cgrpy option.
The systemwide data can be looked up by monitoring root cgroup.

- Issue(2): RMIDs are global and dont scale with more packages and hence
also run out of RMIDs very soon.

Fix: Support per pkg RMIDs hence scale better with more
packages, and get more RMIDs to use and use when needed (ie when tasks
are actually scheduled on the package).

- Issue(3): Cgroup monitoring is not complete. No hierarchical monitoring
support, inconsistent or wrong data seen when monitoring cgroup.

Fix: cgroup monitoring support added. 
Patch adds full cgroup monitoring support. Can monitor different cgroups
in the same hierarchy together and separately. And can also monitor a
task and the cgroup which the task belongs.

- Issue(4): Lot of inconsistent data is seen currently when we monitor different
kind of events like cgroup and task events *together*.

Fix: Patch adds support to be
able to monitor a cgroup x and as task p1 with in a cgroup x and also
monitor different cgroup and tasks together.

- Issue(5): CAT and cqm/mbm write the same PQR_ASSOC_MSR seperately
Fix: Integrate the sched in code and write the PQR_MSR only once every switch_to 

- Issue(6): RMID recycling leads to inaccurate data and complicates the
code and increases the code foot print. Currently, it almost makes the
feature *unusable* as we only see zeroes and inconsistent data once we
run out of RMIDs in the life time of a systemboot. The only way to get
right numbers is to reboot the system once we run out of RMIDs.

Root cause: Recycling steals an RMID from an existing event x and gives
it to an other event y. However due to the nature of monitoring
llc_occupancy we may miss tracking an unknown(possibly large) part of
cache fills at the time when event does not have RMID. Hence the user
ends up with inaccurate data for both events x and y and the inaccuracy
is arbitrary and cannot be measured.  Even if an event x gets another
RMID very soon after loosing the previous RMID, we still miss all the
occupancy data that was tied to the previous RMID which means we cannot
get accurate data even when for most of the time event has an RMID.
There is no way to guarantee accurate results with recycling and data is
inaccurate by arbitrary degree. The fact that an event can loose an RMID
anytime complicates a lot of code in sched_in, init, count, read. It
also complicates mbm as we may loose the RMID anytime and hence need to
keep a history of all the old counts.

Fix: Recycling is removed based on Tony's idea originally that its
introducing a lot of code, failing to provide accurate data and hence
questionable benefits. Because inspite of several attempts to improve
the recycling there is no way to guarantee accurate data as explained
above and the incorrectness is of arbitrary degree(where we cant say for
    ex: the data is off by x% ). As a fix we introduce per-pkg RMIDs to
mitigate the scarcity of RMIDs to a large extent - this is because RMIDs
are plenty - about 2 to 4 per logical processor/SMT thread on each
package. So on a 2 socket BDW system with say 44 logical processors/SMT
threads we have 176 RMIDs on each package (a total of 2x176 = 352
    RMIDs). Also cgroup is fully supported and hence many threads like
all threads in one VM/container can be grouped which use just one RMID.
The RMIDs scale with the number of sockets. If we still run out of RMIDs
perf read throws an error because we are not able to monitor as we run
out of limited h/w resource.

This may be better unlike recycling(even with a better version than the
one upstream)where the user thinks events are being monitored but they
actually are not monitored for arbitrary amount of time hence resulting
in inaccurate data of arbitrary degree. The inaccurate data defeats the
purpose of RDT whose goal is to provide a consistent system behaviour by
giving the ability to monitor and control processor resources in an
accurate and reliable fashion.  The fix instead helps provide accurate
data and for large extent mitigates the RMID scarcity.

Whats working now (unit tested):
Task monitoring, cgroup hierarchical monitoring, monitor multiple
cgroups, cgroup and task in same cgroup, 
per pkg rmids, error on read.

TBD : 
- Most of MBM is working but will need updates to hierarchical
monitoring and other new feature related changes we introduce. 

Below is a list of patches and what each patch fixes, Each commit
message also gives details on what the patch actually fixes among the
bunch:

[PATCH 02/12] x86/cqm: Remove cqm recycling/conflict handling

Before the patch: Users sees only zeros or wrong data once we run out of
RMIDs.
After: User would see either correct data or an error that we run out of
RMIDs.

[PATCH 03/12] x86/rdt: Add rdt common/cqm compile option
[PATCH 04/12] x86/cqm: Add Per pkg rmid support

Before patch: RMIds are global.
Tests: Available RMIDs increase by x times where x is # of packages.
Adds LAZY RMID alloc - RMIDs are alloced during first sched in

[PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare
[PATCH 06/12] x86/cqm: Add cgroup hierarchical monitoring support
[PATCH 07/12] x86/rdt,cqm: Scheduling support update

Before patch: cgroup monitoring not supported fully.
After: cgroup monitoring is fully supported including hierarchical
monitoring.

[PATCH 08/12] x86/cqm: Add support for monitoring task and cgroup

Before patch: cgroup and task could not be monitored together and would
result in a lot of inconsistent data.
After : Can monitor task and cgroup together and also supports
monitoring a task within a cgroup and the cgroup together.

[PATCH 9/12] x86/cqm: Add RMID reuse

Before patch: Once RMID is used , its never used again.
After: We reuse the RMIDs which are freed. User can specify NOLAZY RMID
allocation and open fails if we fail to get all RMIDs at open.

[PATCH 10/12] perf/core,x86/cqm: Add read for Cgroup events,per pkg
[PATCH 11/12] perf/stat: fix bug in handling events in error state
[PATCH 12/12] perf/stat: revamp read error handling, snapshot and

Patches 1/12 - 9/12 Add all the features but the data is not visible to
the perf/core nor the perf user mode. The 11-12 fix these and make the
data availabe to the perf user mode.

[toc] | [next] | [standalone]

#1553319 — [PATCH 10/12] perf/core,x86/cqm: Add read for Cgroup events,per pkg reads.

For cqm cgroup events, the events can be read even if the event was not
active on the cpu on which the event is being read. This is because the
RMIDs are per package and hence if we read the llc_occupancy value on a
cpu x, we are really reading the occupancy for the package where cpu x
belongs.

This patch adds a PERF_INACTIVE_CPU_READ_PKG to indicate this behaviour
of cqm and also changes the perf/core to still call the reads even when
the event is inactive on the cpu for cgroup events. The task events have
event->cpu as -1 and hence it does not apply for task events.

Tests: perf stat -C <cpux> would not return a count before this patch to
the perf/core.  After this patch the count of the package is returned to
the perf/core. We still dont see the count in the perf user mode - that
is fixed in next patches.

Patch is based on David Carrillo-Cisneros <davidcc@google.com> patches
in cqm2 series.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/events/intel/cqm.c             | 31 ++++++++++++++++++++++++-------
 arch/x86/include/asm/intel_rdt_common.h |  2 +-
 include/linux/perf_event.h              | 19 ++++++++++++++++---
 kernel/events/core.c                    | 16 ++++++++++++----
 4 files changed, 53 insertions(+), 15 deletions(-)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index 92efe12..3f5860c 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -111,6 +111,13 @@ bool __rmid_valid(u32 rmid)
 	return true;
 }
 
+static inline bool __rmid_valid_raw(u32 rmid)
+{
+	if (rmid > cqm_max_rmid)
+		return false;
+	return true;
+}
+
 static u64 __rmid_read(u32 rmid)
 {
 	u64 val;
@@ -884,16 +891,16 @@ static u64 intel_cqm_event_count(struct perf_event *event)
 	return __perf_event_count(event);
 }
 
-void alloc_needed_pkg_rmid(u32 *cqm_rmid)
+u32 alloc_needed_pkg_rmid(u32 *cqm_rmid)
 {
 	unsigned long flags;
 	u32 rmid;
 
 	if (WARN_ON(!cqm_rmid))
-		return;
+		return -EINVAL;
 
 	if (cqm_rmid == cqm_rootcginfo.rmid || cqm_rmid[pkg_id])
-		return;
+		return 0;
 
 	raw_spin_lock_irqsave(&cache_lock, flags);
 
@@ -902,6 +909,8 @@ void alloc_needed_pkg_rmid(u32 *cqm_rmid)
 		cqm_rmid[pkg_id] = rmid;
 
 	raw_spin_unlock_irqrestore(&cache_lock, flags);
+
+	return rmid;
 }
 
 static void intel_cqm_event_start(struct perf_event *event, int mode)
@@ -913,10 +922,8 @@ static void intel_cqm_event_start(struct perf_event *event, int mode)
 
 	event->hw.cqm_state &= ~PERF_HES_STOPPED;
 
-	if (is_task_event(event)) {
-		alloc_needed_pkg_rmid(event->hw.cqm_rmid);
+	if (is_task_event(event))
 		state->next_task_rmid = event->hw.cqm_rmid[pkg_id];
-	}
 }
 
 static void intel_cqm_event_stop(struct perf_event *event, int mode)
@@ -932,10 +939,19 @@ static void intel_cqm_event_stop(struct perf_event *event, int mode)
 
 static int intel_cqm_event_add(struct perf_event *event, int mode)
 {
+	u32 rmid;
+
 	event->hw.cqm_state = PERF_HES_STOPPED;
 
-	if ((mode & PERF_EF_START))
+	/*
+	 * If Lazy RMID alloc fails indicate the error to the user.
+	*/
+	if ((mode & PERF_EF_START)) {
+		rmid = alloc_needed_pkg_rmid(event->hw.cqm_rmid);
+		if (!__rmid_valid_raw(rmid))
+			return -EINVAL;
 		intel_cqm_event_start(event, mode);
+	}
 
 	return 0;
 }
@@ -1048,6 +1064,7 @@ static int intel_cqm_event_init(struct perf_event *event)
 	 * cgroup hierarchies.
 	 */
 	event->event_caps |= PERF_EV_CAP_CGROUP_NO_RECURSION;
+	event->event_caps |= PERF_EV_CAP_INACTIVE_CPU_READ_PKG;
 
 	mutex_lock(&cache_mutex);
 
diff --git a/arch/x86/include/asm/intel_rdt_common.h b/arch/x86/include/asm/intel_rdt_common.h
index 544acaa..fcaaaeb 100644
--- a/arch/x86/include/asm/intel_rdt_common.h
+++ b/arch/x86/include/asm/intel_rdt_common.h
@@ -27,7 +27,7 @@ struct intel_pqr_state {
 
 u32 __get_rmid(int domain);
 bool __rmid_valid(u32 rmid);
-void alloc_needed_pkg_rmid(u32 *cqm_rmid);
+u32 alloc_needed_pkg_rmid(u32 *cqm_rmid);
 struct cgrp_cqm_info *cqminfo_from_tsk(struct task_struct *tsk);
 
 extern struct cgrp_cqm_info cqm_rootcginfo;
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 410642a..adfddec 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -525,10 +525,13 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
  * PERF_EV_CAP_CGROUP_NO_RECURSION: A cgroup event that handles its own
  * cgroup scoping. It does not need to be enabled for all of its descendants
  * cgroups.
+ * PERF_EV_CAP_INACTIVE_CPU_READ_PKG: A cgroup event where we can read
+ * the package count on any cpu on the pkg even if inactive.
  */
-#define PERF_EV_CAP_SOFTWARE		BIT(0)
-#define PERF_EV_CAP_READ_ACTIVE_PKG	BIT(1)
-#define PERF_EV_CAP_CGROUP_NO_RECURSION	BIT(2)
+#define PERF_EV_CAP_SOFTWARE                    BIT(0)
+#define PERF_EV_CAP_READ_ACTIVE_PKG             BIT(1)
+#define PERF_EV_CAP_CGROUP_NO_RECURSION         BIT(2)
+#define PERF_EV_CAP_INACTIVE_CPU_READ_PKG       BIT(3)
 
 #define SWEVENT_HLIST_BITS		8
 #define SWEVENT_HLIST_SIZE		(1 << SWEVENT_HLIST_BITS)
@@ -722,6 +725,16 @@ struct perf_event {
 #endif /* CONFIG_PERF_EVENTS */
 };
 
+#ifdef CONFIG_PERF_EVENTS
+static inline bool __perf_can_read_inactive(struct perf_event *event)
+{
+	if ((event->group_caps & PERF_EV_CAP_INACTIVE_CPU_READ_PKG))
+		return true;
+
+	return false;
+}
+#endif
+
 /**
  * struct perf_event_context - event context structure
  *
diff --git a/kernel/events/core.c b/kernel/events/core.c
index 229f611..e71ca66 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -3443,9 +3443,13 @@ struct perf_read_data {
 
 static int find_cpu_to_read(struct perf_event *event, int local_cpu)
 {
+	bool active = event->state == PERF_EVENT_STATE_ACTIVE;
 	int event_cpu = event->oncpu;
 	u16 local_pkg, event_pkg;
 
+	if (__perf_can_read_inactive(event) && !active)
+		event_cpu = event->cpu;
+
 	if (event->group_caps & PERF_EV_CAP_READ_ACTIVE_PKG) {
 		event_pkg =  topology_physical_package_id(event_cpu);
 		local_pkg =  topology_physical_package_id(local_cpu);
@@ -3467,6 +3471,7 @@ static void __perf_event_read(void *info)
 	struct perf_event_context *ctx = event->ctx;
 	struct perf_cpu_context *cpuctx = __get_cpu_context(ctx);
 	struct pmu *pmu = event->pmu;
+	bool read_inactive = __perf_can_read_inactive(event);
 
 	/*
 	 * If this is a task context, we need to check whether it is
@@ -3475,7 +3480,7 @@ static void __perf_event_read(void *info)
 	 * event->count would have been updated to a recent sample
 	 * when the event was scheduled out.
 	 */
-	if (ctx->task && cpuctx->task_ctx != ctx)
+	if (ctx->task && cpuctx->task_ctx != ctx && !read_inactive)
 		return;
 
 	raw_spin_lock(&ctx->lock);
@@ -3485,7 +3490,7 @@ static void __perf_event_read(void *info)
 	}
 
 	update_event_times(event);
-	if (event->state != PERF_EVENT_STATE_ACTIVE)
+	if (ctx->task && cpuctx->task_ctx != ctx && !read_inactive)
 		goto unlock;
 
 	if (!data->group) {
@@ -3500,7 +3505,8 @@ static void __perf_event_read(void *info)
 
 	list_for_each_entry(sub, &event->sibling_list, group_entry) {
 		update_event_times(sub);
-		if (sub->state == PERF_EVENT_STATE_ACTIVE) {
+		if (sub->state == PERF_EVENT_STATE_ACTIVE ||
+		    __perf_can_read_inactive(sub)) {
 			/*
 			 * Use sibling's PMU rather than @event's since
 			 * sibling could be on different (eg: software) PMU.
@@ -3578,13 +3584,15 @@ u64 perf_event_read_local(struct perf_event *event)
 
 static int perf_event_read(struct perf_event *event, bool group)
 {
+	bool active = event->state == PERF_EVENT_STATE_ACTIVE;
 	int ret = 0, cpu_to_read, local_cpu;
 
 	/*
 	 * If event is enabled and currently active on a CPU, update the
 	 * value in the event structure:
 	 */
-	if (event->state == PERF_EVENT_STATE_ACTIVE) {
+	if (active || __perf_can_read_inactive(event)) {
+
 		struct perf_read_data data = {
 			.event = event,
 			.group = group,
-- 
1.9.1

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

#1553321 — [PATCH 06/12] x86/cqm: Add cgroup hierarchical monitoring support

From: David Carrillo-Cisneros <davidcc@google.com>

Patch adds support for monitoring cgroup hierarchy. The
arch_info that was introduced in the perf_cgroup is used to maintain the
cgroup related rmid and hierarchy information.

Since cgroup supports hierarchical monitoring, a cgroup is always
monitoring for some ancestor. By default root is always monitored with
RMID 0 and hence when any cgroup is first created it always reports data
to the root.  mfa or 'monitor for ancestor' is used to keep track of
this information.  Basically which ancestor the cgroup is actually
monitoring for or has to report the data to.

By default, all cgroup's mfa points to root.
1.event init: When ever a new cgroup x would start to be monitored,
the mfa of the monitored cgroup's descendants point towards the cgroup
x.
2.switch_to: task finds the cgroup its associated with and if the
cgroup itself is being monitored cgroup uses its own rmid(a) else it uses
the rmid of the mfa(b).
3.read: During the read call, the cgroup x just adds the
counts of its descendants who had cgroup x as mfa and were also
monitored(To count the scenario (a) in switch_to).

Locking: cgroup traversal: rcu_readlock.  cgroup->arch_info: css_alloc,
css_free, event terminate, init hold mutex.

Tests: Cgroup monitoring should work. Monitoring multiple cgroups in the
same hierarchy works. monitoring cgroup and a task within same cgroup
doesnt work yet.

Patch modified/refactored by Vikas Shivappa
<vikas.shivappa@linux.intel.com> to support recycling removal.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/events/intel/cqm.c             | 227 +++++++++++++++++++++++++++-----
 arch/x86/include/asm/intel_rdt_common.h |  64 +++++++++
 2 files changed, 257 insertions(+), 34 deletions(-)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index a9bd7bd..c6479ae 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -85,6 +85,7 @@ struct sample {
 static cpumask_t cqm_cpumask;
 
 struct pkg_data **cqm_pkgs_data;
+struct cgrp_cqm_info cqm_rootcginfo;
 
 #define RMID_VAL_ERROR		(1ULL << 63)
 #define RMID_VAL_UNAVAIL	(1ULL << 62)
@@ -193,6 +194,11 @@ static void __put_rmid(u32 rmid, int domain)
 	list_add_tail(&entry->list, &cqm_pkgs_data[domain]->cqm_rmid_limbo_lru);
 }
 
+static bool is_task_event(struct perf_event *e)
+{
+	return (e->attach_state & PERF_ATTACH_TASK);
+}
+
 static void cqm_cleanup(void)
 {
 	int i;
@@ -209,7 +215,6 @@ static void cqm_cleanup(void)
 	kfree(cqm_pkgs_data);
 }
 
-
 /*
  * Determine if @a and @b measure the same set of tasks.
  *
@@ -224,20 +229,18 @@ static bool __match_event(struct perf_event *a, struct perf_event *b)
 		return false;
 
 #ifdef CONFIG_CGROUP_PERF
-	if (a->cgrp != b->cgrp)
-		return false;
-#endif
-
-	/* If not task event, we're machine wide */
-	if (!(b->attach_state & PERF_ATTACH_TASK))
+	if ((is_cgroup_event(a) && is_cgroup_event(b)) &&
+		(a->cgrp == b->cgrp))
 		return true;
+#endif
 
 	/*
 	 * Events that target same task are placed into the same cache group.
 	 * Mark it as a multi event group, so that we update ->count
 	 * for every event rather than just the group leader later.
 	 */
-	if (a->hw.target == b->hw.target) {
+	if ((is_task_event(a) && is_task_event(b)) &&
+		(a->hw.target == b->hw.target)) {
 		b->hw.is_group_event = true;
 		return true;
 	}
@@ -365,6 +368,63 @@ static void init_mbm_sample(u32 *rmid, u32 evt_type)
 	on_each_cpu_mask(&cqm_cpumask, __intel_mbm_event_init, &rr, 1);
 }
 
+static inline void cqm_enable_mon(struct cgrp_cqm_info *cqm_info, u32 *rmid)
+{
+	if (rmid != NULL) {
+		cqm_info->mon_enabled = true;
+		cqm_info->rmid = rmid;
+	} else {
+		cqm_info->mon_enabled = false;
+		cqm_info->rmid = NULL;
+	}
+}
+
+static void cqm_assign_hier_rmid(struct cgroup_subsys_state *rcss, u32 *rmid)
+{
+	struct cgrp_cqm_info *ccqm_info, *rcqm_info;
+	struct cgroup_subsys_state *pos_css;
+
+	rcu_read_lock();
+
+	rcqm_info = css_to_cqm_info(rcss);
+
+	/* Enable or disable monitoring based on rmid.*/
+	cqm_enable_mon(rcqm_info, rmid);
+
+	pos_css = css_next_descendant_pre(rcss, rcss);
+	while (pos_css) {
+		ccqm_info = css_to_cqm_info(pos_css);
+
+		/*
+		 * Monitoring is being enabled.
+		 * Update the descendents to monitor for you, unless
+		 * they were already monitoring for a descendent of yours.
+		 */
+		if (rmid && (rcqm_info->level > ccqm_info->mfa->level))
+			ccqm_info->mfa = rcqm_info;
+
+		/*
+		 * Monitoring is being disabled.
+		 * Update the descendents who were monitoring for you
+		 * to monitor for the ancestor you were monitoring.
+		 */
+		if (!rmid && (ccqm_info->mfa == rcqm_info))
+			ccqm_info->mfa = rcqm_info->mfa;
+		pos_css = css_next_descendant_pre(pos_css, rcss);
+	}
+	rcu_read_unlock();
+}
+
+static int cqm_assign_rmid(struct perf_event *event, u32 *rmid)
+{
+#ifdef CONFIG_CGROUP_PERF
+	if (is_cgroup_event(event)) {
+		cqm_assign_hier_rmid(&event->cgrp->css, rmid);
+	}
+#endif
+	return 0;
+}
+
 /*
  * Find a group and setup RMID.
  *
@@ -402,11 +462,14 @@ static int intel_cqm_setup_event(struct perf_event *event,
 	return 0;
 }
 
+static u64 cqm_read_subtree(struct perf_event *event, struct rmid_read *rr);
+
 static void intel_cqm_event_read(struct perf_event *event)
 {
-	unsigned long flags;
-	u32 rmid;
-	u64 val;
+	struct rmid_read rr = {
+		.evt_type = event->attr.config,
+		.value = ATOMIC64_INIT(0),
+	};
 
 	/*
 	 * Task events are handled by intel_cqm_event_count().
@@ -414,26 +477,9 @@ static void intel_cqm_event_read(struct perf_event *event)
 	if (event->cpu == -1)
 		return;
 
-	raw_spin_lock_irqsave(&cache_lock, flags);
-	rmid = event->hw.cqm_rmid[pkg_id];
-
-	if (!__rmid_valid(rmid))
-		goto out;
-
-	if (is_mbm_event(event->attr.config))
-		val = rmid_read_mbm(rmid, event->attr.config);
-	else
-		val = __rmid_read(rmid);
-
-	/*
-	 * Ignore this reading on error states and do not update the value.
-	 */
-	if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
-		goto out;
+	rr.rmid = ACCESS_ONCE(event->hw.cqm_rmid);
 
-	local64_set(&event->count, val);
-out:
-	raw_spin_unlock_irqrestore(&cache_lock, flags);
+	cqm_read_subtree(event, &rr);
 }
 
 static void __intel_cqm_event_count(void *info)
@@ -545,6 +591,55 @@ static void mbm_hrtimer_init(void)
 	}
 }
 
+static void cqm_mask_call_local(struct rmid_read *rr)
+{
+	if (is_mbm_event(rr->evt_type))
+		__intel_mbm_event_count(rr);
+	else
+		__intel_cqm_event_count(rr);
+}
+
+static inline void
+	delta_local(struct perf_event *event, struct rmid_read *rr, u32 *rmid)
+{
+	atomic64_set(&rr->value, 0);
+	rr->rmid = ACCESS_ONCE(rmid);
+
+	cqm_mask_call_local(rr);
+	local64_add(atomic64_read(&rr->value), &event->count);
+}
+
+/*
+ * Since cgroup follows hierarchy, add the count of
+ *  the descendents who were being monitored as well.
+ */
+static u64 cqm_read_subtree(struct perf_event *event, struct rmid_read *rr)
+{
+#ifdef CONFIG_CGROUP_PERF
+
+	struct cgroup_subsys_state *rcss, *pos_css;
+	struct cgrp_cqm_info *ccqm_info;
+
+	cqm_mask_call_local(rr);
+	local64_set(&event->count, atomic64_read(&(rr->value)));
+
+	if (is_task_event(event))
+		return __perf_event_count(event);
+
+	rcu_read_lock();
+	rcss = &event->cgrp->css;
+	css_for_each_descendant_pre(pos_css, rcss) {
+		ccqm_info = (css_to_cqm_info(pos_css));
+
+		/* Add the descendent 'monitored cgroup' counts */
+		if (pos_css != rcss && ccqm_info->mon_enabled)
+			delta_local(event, rr, ccqm_info->rmid);
+	}
+	rcu_read_unlock();
+#endif
+	return __perf_event_count(event);
+}
+
 static u64 intel_cqm_event_count(struct perf_event *event)
 {
 	struct rmid_read rr = {
@@ -603,7 +698,7 @@ void alloc_needed_pkg_rmid(u32 *cqm_rmid)
 	if (WARN_ON(!cqm_rmid))
 		return;
 
-	if (cqm_rmid[pkg_id])
+	if (cqm_rmid == cqm_rootcginfo.rmid || cqm_rmid[pkg_id])
 		return;
 
 	raw_spin_lock_irqsave(&cache_lock, flags);
@@ -661,9 +756,11 @@ static int intel_cqm_event_add(struct perf_event *event, int mode)
 			__put_rmid(rmid[d], d);
 	}
 	kfree(event->hw.cqm_rmid);
+	cqm_assign_rmid(event, NULL);
 	list_del(&event->hw.cqm_groups_entry);
 }
-static void intel_cqm_event_destroy(struct perf_event *event)
+
+static void intel_cqm_event_terminate(struct perf_event *event)
 {
 	struct perf_event *group_other = NULL;
 	unsigned long flags;
@@ -917,6 +1014,7 @@ static int intel_cqm_event_init(struct perf_event *event)
 	.attr_groups	     = intel_cqm_attr_groups,
 	.task_ctx_nr	     = perf_sw_context,
 	.event_init	     = intel_cqm_event_init,
+	.event_terminate     = intel_cqm_event_terminate,
 	.add		     = intel_cqm_event_add,
 	.del		     = intel_cqm_event_stop,
 	.start		     = intel_cqm_event_start,
@@ -924,12 +1022,67 @@ static int intel_cqm_event_init(struct perf_event *event)
 	.read		     = intel_cqm_event_read,
 	.count		     = intel_cqm_event_count,
 };
+
 #ifdef CONFIG_CGROUP_PERF
 int perf_cgroup_arch_css_alloc(struct cgroup_subsys_state *parent_css,
 				      struct cgroup_subsys_state *new_css)
-{}
+{
+	struct cgrp_cqm_info *cqm_info, *pcqm_info;
+	struct perf_cgroup *new_cgrp;
+
+	if (!parent_css) {
+		cqm_rootcginfo.level = 0;
+
+		cqm_rootcginfo.mon_enabled = true;
+		cqm_rootcginfo.cont_mon = true;
+		cqm_rootcginfo.mfa = NULL;
+		INIT_LIST_HEAD(&cqm_rootcginfo.tskmon_rlist);
+
+		if (new_css) {
+			new_cgrp = css_to_perf_cgroup(new_css);
+			new_cgrp->arch_info = &cqm_rootcginfo;
+		}
+		return 0;
+	}
+
+	mutex_lock(&cache_mutex);
+
+	new_cgrp = css_to_perf_cgroup(new_css);
+
+	cqm_info = kzalloc(sizeof(struct cgrp_cqm_info), GFP_KERNEL);
+	if (!cqm_info) {
+		mutex_unlock(&cache_mutex);
+		return -ENOMEM;
+	}
+
+	pcqm_info = (css_to_cqm_info(parent_css));
+	cqm_info->level = pcqm_info->level + 1;
+	cqm_info->rmid = pcqm_info->rmid;
+
+	cqm_info->cont_mon = false;
+	cqm_info->mon_enabled = false;
+	INIT_LIST_HEAD(&cqm_info->tskmon_rlist);
+	if (!pcqm_info->mfa)
+		cqm_info->mfa = pcqm_info;
+	else
+		cqm_info->mfa = pcqm_info->mfa;
+
+	new_cgrp->arch_info = cqm_info;
+	mutex_unlock(&cache_mutex);
+
+	return 0;
+}
+
 void perf_cgroup_arch_css_free(struct cgroup_subsys_state *css)
-{}
+{
+	struct perf_cgroup *cgrp = css_to_perf_cgroup(css);
+
+	mutex_lock(&cache_mutex);
+	kfree(cgrp_to_cqm_info(cgrp));
+	cgrp->arch_info = NULL;
+	mutex_unlock(&cache_mutex);
+}
+
 void perf_cgroup_arch_attach(struct cgroup_taskset *tset)
 {}
 int perf_cgroup_arch_can_attach(struct cgroup_taskset *tset)
@@ -1053,6 +1206,12 @@ static int pkg_data_init_cpu(int cpu)
 	entry = __rmid_entry(0, curr_pkgid);
 	list_del(&entry->list);
 
+	cqm_rootcginfo.rmid = kzalloc(sizeof(u32) * cqm_socket_max, GFP_KERNEL);
+	if (!cqm_rootcginfo.rmid) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
 	return 0;
 fail:
 	kfree(ccqm_rmid_ptrs);
diff --git a/arch/x86/include/asm/intel_rdt_common.h b/arch/x86/include/asm/intel_rdt_common.h
index b31081b..e11ed5e 100644
--- a/arch/x86/include/asm/intel_rdt_common.h
+++ b/arch/x86/include/asm/intel_rdt_common.h
@@ -24,4 +24,68 @@ struct intel_pqr_state {
 
 DECLARE_PER_CPU(struct intel_pqr_state, pqr_state);
 
+/**
+ * struct cgrp_cqm_info - perf_event cgroup metadata for cqm
+ * @cont_mon     Continuous monitoring flag
+ * @mon_enabled  Whether monitoring is enabled
+ * @level        Level in the cgroup tree. Root is level 0.
+ * @rmid        The rmids of the cgroup.
+ * @mfa          'Monitoring for ancestor' points to the cqm_info
+ *  of the ancestor the cgroup is monitoring for. 'Monitoring for ancestor'
+ *  means you will use an ancestors RMID at sched_in if you are
+ *  not monitoring yourself.
+ *
+ *  Due to the hierarchical nature of cgroups, every cgroup just
+ *  monitors for the 'nearest monitored ancestor' at all times.
+ *  Since root cgroup is always monitored, all descendents
+ *  at boot time monitor for root and hence all mfa points to root except
+ *  for root->mfa which is NULL.
+ *  1. RMID setup: When cgroup x start monitoring:
+ *    for each descendent y, if y's mfa->level < x->level, then
+ *    y->mfa = x. (Where level of root node = 0...)
+ *  2. sched_in: During sched_in for x
+ *    if (x->mon_enabled) choose x->rmid
+ *    else choose x->mfa->rmid.
+ *  3. read: for each descendent of cgroup x
+ *     if (x->monitored) count += rmid_read(x->rmid).
+ *  4. evt_destroy: for each descendent y of x, if (y->mfa == x) then
+ *     y->mfa = x->mfa. Meaning if any descendent was monitoring for x,
+ *     set that descendent to monitor for the cgroup which x was monitoring for.
+ *
+ * @tskmon_rlist List of tasks being monitored in the cgroup
+ *  When a task which belongs to a cgroup x is being monitored, it always uses
+ *  its own task->rmid even if cgroup x is monitored during sched_in.
+ *  To account for the counts of such tasks, cgroup keeps this list
+ *  and parses it during read.
+ *
+ *  Perf handles hierarchy for other events, but because RMIDs are per pkg
+ *  this is handled here.
+*/
+struct cgrp_cqm_info {
+	bool cont_mon;
+	bool mon_enabled;
+	int level;
+	u32 *rmid;
+	struct cgrp_cqm_info *mfa;
+	struct list_head tskmon_rlist;
+};
+
+struct tsk_rmid_entry {
+	u32 *rmid;
+	struct list_head list;
+};
+
+#ifdef CONFIG_CGROUP_PERF
+
+# 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_))
+
+#else
+
+# define css_to_perf_cgroup(css_) NULL
+# define cgrp_to_cqm_info(cgrp_) NULL
+# define css_to_cqm_info(css_) NULL
+
+#endif
 #endif /* _ASM_X86_INTEL_RDT_COMMON_H */
-- 
1.9.1

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

#1560608 — Re: [PATCH 06/12] x86/cqm: Add cgroup hierarchical monitoring support

On Fri, 6 Jan 2017, Vikas Shivappa wrote:
> From: David Carrillo-Cisneros <davidcc@google.com>
> 
> Patch adds support for monitoring cgroup hierarchy. The
> arch_info that was introduced in the perf_cgroup is used to maintain the
> cgroup related rmid and hierarchy information.
> 
> Since cgroup supports hierarchical monitoring, a cgroup is always
> monitoring for some ancestor. By default root is always monitored with
> RMID 0 and hence when any cgroup is first created it always reports data
> to the root.  mfa or 'monitor for ancestor' is used to keep track of
> this information.  Basically which ancestor the cgroup is actually
> monitoring for or has to report the data to.
> 
> By default, all cgroup's mfa points to root.
> 1.event init: When ever a new cgroup x would start to be monitored,
> the mfa of the monitored cgroup's descendants point towards the cgroup
> x.
> 2.switch_to: task finds the cgroup its associated with and if the
> cgroup itself is being monitored cgroup uses its own rmid(a) else it uses
> the rmid of the mfa(b).
> 3.read: During the read call, the cgroup x just adds the
> counts of its descendants who had cgroup x as mfa and were also
> monitored(To count the scenario (a) in switch_to).
> 
> Locking: cgroup traversal: rcu_readlock.  cgroup->arch_info: css_alloc,
> css_free, event terminate, init hold mutex.

Again: Locking must be documented in the code not in a changelog. And the
documentation of lokcing wants to be elaborate not a sloppy unparseable
blurb.

> Tests: Cgroup monitoring should work. Monitoring multiple cgroups in the

should work is really a great test.....

> same hierarchy works. monitoring cgroup and a task within same cgroup
> doesnt work yet.
> 
> Patch modified/refactored by Vikas Shivappa
> <vikas.shivappa@linux.intel.com> to support recycling removal.

Preserving Signed-off-by tags is not in your book, right?

> Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
>  
>  struct pkg_data **cqm_pkgs_data;
> +struct cgrp_cqm_info cqm_rootcginfo;

And this is global because?

>  #define RMID_VAL_ERROR		(1ULL << 63)
>  #define RMID_VAL_UNAVAIL	(1ULL << 62)
> @@ -193,6 +194,11 @@ static void __put_rmid(u32 rmid, int domain)
>  	list_add_tail(&entry->list, &cqm_pkgs_data[domain]->cqm_rmid_limbo_lru);
>  }
>  
> +static bool is_task_event(struct perf_event *e)

inline

> +{
> +	return (e->attach_state & PERF_ATTACH_TASK);
> +}

And if at all this should go into a core perf header. It's not at all CQM
specific.

> +static inline void cqm_enable_mon(struct cgrp_cqm_info *cqm_info, u32 *rmid)
> +{
> +	if (rmid != NULL) {
> +		cqm_info->mon_enabled = true;
> +		cqm_info->rmid = rmid;
> +	} else {
> +		cqm_info->mon_enabled = false;
> +		cqm_info->rmid = NULL;
> +	}

The function truly implements what the function name suggests. Really
intuitive - NOT!

>  
> +static u64 cqm_read_subtree(struct perf_event *event, struct rmid_read *rr);

Forward declaration should be on top of the file and not at some random
place in the middle of the code.

> +
>  static void intel_cqm_event_read(struct perf_event *event)
>  {
> -	unsigned long flags;
> -	u32 rmid;
> -	u64 val;
> +	struct rmid_read rr = {
> +		.evt_type = event->attr.config,
> +		.value = ATOMIC64_INIT(0),
> +	};
>  
>  	/*
>  	 * Task events are handled by intel_cqm_event_count().
> @@ -414,26 +477,9 @@ static void intel_cqm_event_read(struct perf_event *event)
>  	if (event->cpu == -1)
>  		return;
>  
> -	raw_spin_lock_irqsave(&cache_lock, flags);
> -	rmid = event->hw.cqm_rmid[pkg_id];
> -
> -	if (!__rmid_valid(rmid))
> -		goto out;
> -
> -	if (is_mbm_event(event->attr.config))
> -		val = rmid_read_mbm(rmid, event->attr.config);
> -	else
> -		val = __rmid_read(rmid);
> -
> -	/*
> -	 * Ignore this reading on error states and do not update the value.
> -	 */
> -	if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
> -		goto out;
> +	rr.rmid = ACCESS_ONCE(event->hw.cqm_rmid);
>  
> -	local64_set(&event->count, val);
> -out:
> -	raw_spin_unlock_irqrestore(&cache_lock, flags);
> +	cqm_read_subtree(event, &rr);

So why isn't the implementation right here? That's just another pointless
indirection in the code to make review and reading harder.

>  }
>  
>  static void __intel_cqm_event_count(void *info)
> @@ -545,6 +591,55 @@ static void mbm_hrtimer_init(void)
>  	}
>  }
>  
> +static void cqm_mask_call_local(struct rmid_read *rr)

I have a hard time to understand the name of this function. Where the heck
is a mask or a call involved here?

> +{
> +	if (is_mbm_event(rr->evt_type))
> +		__intel_mbm_event_count(rr);
> +	else
> +		__intel_cqm_event_count(rr);
> +}
> +
> +static inline void
> +	delta_local(struct perf_event *event, struct rmid_read *rr, u32 *rmid)

Hell no. We either do

static inline void
delta_local(struct perf_event *event, struct rmid_read *rr, u32 *rmid)

or

static inline void delta_local(struct perf_event *event, struct rmid_read *rr,
       	      	   	       u32 *rmid)

but not this completely unparseable crap.

Aside of that what is delta_local? I cannot see any delta here. And that
rmid_read pointer is confusing as hell. What's the point of it?

You clear the value and set the rmid. So the only value you preserve is the
event type and that one is in the event and can be reinitialized locally.

> +{
> +	atomic64_set(&rr->value, 0);
> +	rr->rmid = ACCESS_ONCE(rmid);

What's the purpose of this access once? Voodoo programming is the only
reasonable explanation I came up with.

> +
> +	cqm_mask_call_local(rr);
> +	local64_add(atomic64_read(&rr->value), &event->count);
> +}
> +
> +/*
> + * Since cgroup follows hierarchy, add the count of
> + *  the descendents who were being monitored as well.
> + */
> +static u64 cqm_read_subtree(struct perf_event *event, struct rmid_read *rr)
> +{
> +#ifdef CONFIG_CGROUP_PERF
> +
> +	struct cgroup_subsys_state *rcss, *pos_css;
> +	struct cgrp_cqm_info *ccqm_info;
> +
> +	cqm_mask_call_local(rr);
> +	local64_set(&event->count, atomic64_read(&(rr->value)));
> +
> +	if (is_task_event(event))
> +		return __perf_event_count(event);

And what happens for non task and non cgroup, aka. CPU wide events?

> +
> +	rcu_read_lock();
> +	rcss = &event->cgrp->css;
> +	css_for_each_descendant_pre(pos_css, rcss) {
> +		ccqm_info = (css_to_cqm_info(pos_css));
> +
> +		/* Add the descendent 'monitored cgroup' counts */
> +		if (pos_css != rcss && ccqm_info->mon_enabled)
> +			delta_local(event, rr, ccqm_info->rmid);
> +	}
> +	rcu_read_unlock();
> +#endif
> +	return __perf_event_count(event);

Oh well. Yet another function which does something different than the
function name suggests. It does not read the subtree, it reads either the
task or the cgroup thingy.

> -static void intel_cqm_event_destroy(struct perf_event *event)
> +
> +static void intel_cqm_event_terminate(struct perf_event *event)

Ah. So now that function gets reused. Not sure whether that works better
than before, but at least the compiler warning is gone. Progress!

>  {
>  	struct perf_event *group_other = NULL;
>  	unsigned long flags;
> @@ -917,6 +1014,7 @@ static int intel_cqm_event_init(struct perf_event *event)
>  	.attr_groups	     = intel_cqm_attr_groups,
>  	.task_ctx_nr	     = perf_sw_context,
>  	.event_init	     = intel_cqm_event_init,
> +	.event_terminate     = intel_cqm_event_terminate,
>  	.add		     = intel_cqm_event_add,
>  	.del		     = intel_cqm_event_stop,
>  	.start		     = intel_cqm_event_start,
> @@ -924,12 +1022,67 @@ static int intel_cqm_event_init(struct perf_event *event)
>  	.read		     = intel_cqm_event_read,
>  	.count		     = intel_cqm_event_count,
>  };
> +
>  #ifdef CONFIG_CGROUP_PERF
>  int perf_cgroup_arch_css_alloc(struct cgroup_subsys_state *parent_css,
>  				      struct cgroup_subsys_state *new_css)
> -{}
> +{
> +	struct cgrp_cqm_info *cqm_info, *pcqm_info;
> +	struct perf_cgroup *new_cgrp;
> +
> +	if (!parent_css) {
> +		cqm_rootcginfo.level = 0;
> +
> +		cqm_rootcginfo.mon_enabled = true;
> +		cqm_rootcginfo.cont_mon = true;
> +		cqm_rootcginfo.mfa = NULL;
> +		INIT_LIST_HEAD(&cqm_rootcginfo.tskmon_rlist);
> +
> +		if (new_css) {
> +			new_cgrp = css_to_perf_cgroup(new_css);
> +			new_cgrp->arch_info = &cqm_rootcginfo;
> +		}
> +		return 0;
> +	}
> +
> +	mutex_lock(&cache_mutex);
> +
> +	new_cgrp = css_to_perf_cgroup(new_css);
> +
> +	cqm_info = kzalloc(sizeof(struct cgrp_cqm_info), GFP_KERNEL);
> +	if (!cqm_info) {
> +		mutex_unlock(&cache_mutex);
> +		return -ENOMEM;
> +	}
> +
> +	pcqm_info = (css_to_cqm_info(parent_css));

The extra brackets are necessary to make it more readable, right?

> +	cqm_info->level = pcqm_info->level + 1;
> +	cqm_info->rmid = pcqm_info->rmid;
> +
> +	cqm_info->cont_mon = false;
> +	cqm_info->mon_enabled = false;
> +	INIT_LIST_HEAD(&cqm_info->tskmon_rlist);
> +	if (!pcqm_info->mfa)
> +		cqm_info->mfa = pcqm_info;
> +	else
> +		cqm_info->mfa = pcqm_info->mfa;

Comments would really confuse the reader here.

> +
> +	new_cgrp->arch_info = cqm_info;
> +	mutex_unlock(&cache_mutex);
> +
> +	return 0;
> +}
> +
>  void perf_cgroup_arch_css_free(struct cgroup_subsys_state *css)
> -{}
> +{
> +	struct perf_cgroup *cgrp = css_to_perf_cgroup(css);
> +
> +	mutex_lock(&cache_mutex);
> +	kfree(cgrp_to_cqm_info(cgrp));
> +	cgrp->arch_info = NULL;

So the two lines above deal both with cgrp->arch_info. Why do you need that
extra conversion macro for the kfree() call?

> +	mutex_unlock(&cache_mutex);
> +}
> +
>  void perf_cgroup_arch_attach(struct cgroup_taskset *tset)
>  {}
>  int perf_cgroup_arch_can_attach(struct cgroup_taskset *tset)
> @@ -1053,6 +1206,12 @@ static int pkg_data_init_cpu(int cpu)
>  	entry = __rmid_entry(0, curr_pkgid);
>  	list_del(&entry->list);
>  
> +	cqm_rootcginfo.rmid = kzalloc(sizeof(u32) * cqm_socket_max, GFP_KERNEL);
> +	if (!cqm_rootcginfo.rmid) {
> +		ret = -ENOMEM;
> +		goto fail;
> +	}

Brilliant. So this allocates the cqm_rootcginfo.rmid array for each package
over and over. That's just a memory leak on boot, but when hotplugging a
full socket _AFTER_ initialization this is going to replace the previously
used and RMID populated array with a zeroed out one.

Why would a global allocation be placed into a per cpu data setup function?
Just because there happened to be a nice empty spot to hack it in?

> +
>  	return 0;
>  fail:
>  	kfree(ccqm_rmid_ptrs);
> diff --git a/arch/x86/include/asm/intel_rdt_common.h b/arch/x86/include/asm/intel_rdt_common.h
> index b31081b..e11ed5e 100644
> --- a/arch/x86/include/asm/intel_rdt_common.h
> +++ b/arch/x86/include/asm/intel_rdt_common.h
> @@ -24,4 +24,68 @@ struct intel_pqr_state {
>  
>  DECLARE_PER_CPU(struct intel_pqr_state, pqr_state);
>  
> +/**
> + * struct cgrp_cqm_info - perf_event cgroup metadata for cqm
> + * @cont_mon     Continuous monitoring flag

Please check the syntax for kernel doc struct members ...

> + * @mon_enabled  Whether monitoring is enabled
> + * @level        Level in the cgroup tree. Root is level 0.
> + * @rmid        The rmids of the cgroup.
> + * @mfa          'Monitoring for ancestor' points to the cqm_info
> + *  of the ancestor the cgroup is monitoring for. 'Monitoring for ancestor'
> + *  means you will use an ancestors RMID at sched_in if you are
> + *  not monitoring yourself.
> + *
> + *  Due to the hierarchical nature of cgroups, every cgroup just
> + *  monitors for the 'nearest monitored ancestor' at all times.
> + *  Since root cgroup is always monitored, all descendents
> + *  at boot time monitor for root and hence all mfa points to root except
> + *  for root->mfa which is NULL.
> + *  1. RMID setup: When cgroup x start monitoring:
> + *    for each descendent y, if y's mfa->level < x->level, then
> + *    y->mfa = x. (Where level of root node = 0...)
> + *  2. sched_in: During sched_in for x
> + *    if (x->mon_enabled) choose x->rmid
> + *    else choose x->mfa->rmid.
> + *  3. read: for each descendent of cgroup x
> + *     if (x->monitored) count += rmid_read(x->rmid).
> + *  4. evt_destroy: for each descendent y of x, if (y->mfa == x) then
> + *     y->mfa = x->mfa. Meaning if any descendent was monitoring for x,
> + *     set that descendent to monitor for the cgroup which x was monitoring for.

That mfa member is not the right place for this extensive
documentation. Put it somewhere in the code where the whole machinery is
described.

> + * @tskmon_rlist List of tasks being monitored in the cgroup
> + *  When a task which belongs to a cgroup x is being monitored, it always uses
> + *  its own task->rmid even if cgroup x is monitored during sched_in.
> + *  To account for the counts of such tasks, cgroup keeps this list
> + *  and parses it during read.
> + *
> + *  Perf handles hierarchy for other events, but because RMIDs are per pkg
> + *  this is handled here.
> +*/
> +struct cgrp_cqm_info {
> +	bool cont_mon;
> +	bool mon_enabled;
> +	int level;
> +	u32 *rmid;
> +	struct cgrp_cqm_info *mfa;
> +	struct list_head tskmon_rlist;
> +};
> +
> +struct tsk_rmid_entry {
> +	u32 *rmid;
> +	struct list_head list;
> +};

Make the struct members tabular please for readability sake.

Thanks,

	tglx

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

#1553326 — [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare

From: David Carrillo-Cisneros <davidcc@google.com>

cgroup hierarchy monitoring is not supported currently. This patch
builds all the necessary datastructures, cgroup APIs like alloc, free
etc and necessary quirks for supporting cgroup hierarchy monitoring in
later patches.

- Introduce a architecture specific data structure arch_info in
perf_cgroup to keep track of RMIDs and cgroup hierarchical monitoring.
- perf sched_in calls all the cgroup ancestors when a cgroup is
scheduled in. This will not work with cqm as we have a common per pkg
rmid associated with one task and hence cannot write different RMIds
into the MSR for each event. cqm driver enables a flag
PERF_EV_CGROUP_NO_RECURSION which indicates the perf to not call all
ancestor cgroups for each event and let the driver handle the hierarchy
monitoring for cgroup.
- Introduce event_terminate as event_destroy is called after cgrp is
disassociated from the event to support rmid handling of the cgroup.
This helps cqm clean up the cqm specific arch_info.
- Add the cgroup APIs for alloc,free,attach and can_attach

The above framework will be used to build different cgroup features in
later patches.

Tests: Same as before. Cgroup still doesnt work but we did the prep to
get it to work

Patch modified/refactored by Vikas Shivappa
<vikas.shivappa@linux.intel.com> to support recycling removal.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/events/intel/cqm.c       | 19 ++++++++++++++++++-
 arch/x86/include/asm/perf_event.h | 27 +++++++++++++++++++++++++++
 include/linux/perf_event.h        | 32 ++++++++++++++++++++++++++++++++
 kernel/events/core.c              | 28 +++++++++++++++++++++++++++-
 4 files changed, 104 insertions(+), 2 deletions(-)

diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
index 68fd1da..a9bd7bd 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -741,7 +741,13 @@ static int intel_cqm_event_init(struct perf_event *event)
 	INIT_LIST_HEAD(&event->hw.cqm_group_entry);
 	INIT_LIST_HEAD(&event->hw.cqm_groups_entry);
 
-	event->destroy = intel_cqm_event_destroy;
+	/*
+	 * CQM driver handles cgroup recursion and since only noe
+	 * RMID can be programmed at the time in each core, then
+	 * it is incompatible with the way generic code handles
+	 * cgroup hierarchies.
+	 */
+	event->event_caps |= PERF_EV_CAP_CGROUP_NO_RECURSION;
 
 	mutex_lock(&cache_mutex);
 
@@ -918,6 +924,17 @@ static int intel_cqm_event_init(struct perf_event *event)
 	.read		     = intel_cqm_event_read,
 	.count		     = intel_cqm_event_count,
 };
+#ifdef CONFIG_CGROUP_PERF
+int perf_cgroup_arch_css_alloc(struct cgroup_subsys_state *parent_css,
+				      struct cgroup_subsys_state *new_css)
+{}
+void perf_cgroup_arch_css_free(struct cgroup_subsys_state *css)
+{}
+void perf_cgroup_arch_attach(struct cgroup_taskset *tset)
+{}
+int perf_cgroup_arch_can_attach(struct cgroup_taskset *tset)
+{}
+#endif
 
 static inline void cqm_pick_event_reader(int cpu)
 {
diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
index f353061..f38c7f0 100644
--- a/arch/x86/include/asm/perf_event.h
+++ b/arch/x86/include/asm/perf_event.h
@@ -299,4 +299,31 @@ 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_INTEL_RDT_M
+#ifdef CONFIG_CGROUP_PERF
+
+#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_free	perf_cgroup_arch_css_free
+
+void perf_cgroup_arch_css_free(struct cgroup_subsys_state *css);
+
+#define perf_cgroup_arch_attach		perf_cgroup_arch_attach
+
+void perf_cgroup_arch_attach(struct cgroup_taskset *tset);
+
+#define perf_cgroup_arch_can_attach	perf_cgroup_arch_can_attach
+
+int perf_cgroup_arch_can_attach(struct cgroup_taskset *tset);
+
+#endif
+
+#endif
 #endif /* _ASM_X86_PERF_EVENT_H */
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index a8f4749..410642a 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -300,6 +300,12 @@ struct pmu {
 	int (*event_init)		(struct perf_event *event);
 
 	/*
+	 * Terminate the event for this PMU. Optional complement for a
+	 * successful event_init. Called before the event fields are tear down.
+	 */
+	void (*event_terminate)		(struct perf_event *event);
+
+	/*
 	 * Notification that the event was mapped or unmapped.  Called
 	 * in the context of the mapping task.
 	 */
@@ -516,9 +522,13 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
  * PERF_EV_CAP_SOFTWARE: Is a software event.
  * PERF_EV_CAP_READ_ACTIVE_PKG: A CPU event (or cgroup event) that can be read
  * from any CPU in the package where it is active.
+ * PERF_EV_CAP_CGROUP_NO_RECURSION: A cgroup event that handles its own
+ * cgroup scoping. It does not need to be enabled for all of its descendants
+ * cgroups.
  */
 #define PERF_EV_CAP_SOFTWARE		BIT(0)
 #define PERF_EV_CAP_READ_ACTIVE_PKG	BIT(1)
+#define PERF_EV_CAP_CGROUP_NO_RECURSION	BIT(2)
 
 #define SWEVENT_HLIST_BITS		8
 #define SWEVENT_HLIST_SIZE		(1 << SWEVENT_HLIST_BITS)
@@ -823,6 +833,8 @@ struct perf_cgroup_info {
 };
 
 struct perf_cgroup {
+	/* Architecture specific information. */
+	void				 *arch_info;
 	struct cgroup_subsys_state	css;
 	struct perf_cgroup_info	__percpu *info;
 };
@@ -844,6 +856,7 @@ struct perf_cgroup {
 
 #ifdef CONFIG_PERF_EVENTS
 
+extern int is_cgroup_event(struct perf_event *event);
 extern void *perf_aux_output_begin(struct perf_output_handle *handle,
 				   struct perf_event *event);
 extern void perf_aux_output_end(struct perf_output_handle *handle,
@@ -1387,4 +1400,23 @@ ssize_t perf_event_sysfs_show(struct device *dev, struct device_attribute *attr,
 #define perf_event_exit_cpu	NULL
 #endif
 
+/*
+ * Hooks for architecture specific extensions for perf_cgroup.
+ */
+#ifndef perf_cgroup_arch_css_alloc
+#define perf_cgroup_arch_css_alloc(parent_css, new_css) 0
+#endif
+
+#ifndef perf_cgroup_arch_css_free
+#define perf_cgroup_arch_css_free(css) do { } while (0)
+#endif
+
+#ifndef perf_cgroup_arch_attach
+#define perf_cgroup_arch_attach(tskset) do { } while (0)
+#endif
+
+#ifndef perf_cgroup_arch_can_attach
+#define perf_cgroup_arch_can_attach(tskset) 0
+#endif
+
 #endif /* _LINUX_PERF_EVENT_H */
diff --git a/kernel/events/core.c b/kernel/events/core.c
index ab15509..229f611 100644
--- a/kernel/events/core.c
+++ b/kernel/events/core.c
@@ -590,6 +590,9 @@ static inline u64 perf_event_clock(struct perf_event *event)
 	if (!cpuctx->cgrp)
 		return false;
 
+	if (event->event_caps & PERF_EV_CAP_CGROUP_NO_RECURSION)
+		return cpuctx->cgrp->css.cgroup == event->cgrp->css.cgroup;
+
 	/*
 	 * Cgroup scoping is recursive.  An event enabled for a cgroup is
 	 * also enabled for all its descendant cgroups.  If @cpuctx's
@@ -606,7 +609,7 @@ static inline void perf_detach_cgroup(struct perf_event *event)
 	event->cgrp = NULL;
 }
 
-static inline int is_cgroup_event(struct perf_event *event)
+int is_cgroup_event(struct perf_event *event)
 {
 	return event->cgrp != NULL;
 }
@@ -4019,6 +4022,9 @@ static void _free_event(struct perf_event *event)
 		mutex_unlock(&event->mmap_mutex);
 	}
 
+	if (event->pmu->event_terminate)
+		event->pmu->event_terminate(event);
+
 	if (is_cgroup_event(event))
 		perf_detach_cgroup(event);
 
@@ -9246,6 +9252,8 @@ static void account_event(struct perf_event *event)
 	exclusive_event_destroy(event);
 
 err_pmu:
+	if (event->pmu->event_terminate)
+		event->pmu->event_terminate(event);
 	if (event->destroy)
 		event->destroy(event);
 	module_put(pmu->module);
@@ -10748,6 +10756,7 @@ static int __init perf_event_sysfs_init(void)
 perf_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
 {
 	struct perf_cgroup *jc;
+	int ret;
 
 	jc = kzalloc(sizeof(*jc), GFP_KERNEL);
 	if (!jc)
@@ -10759,6 +10768,12 @@ static int __init perf_event_sysfs_init(void)
 		return ERR_PTR(-ENOMEM);
 	}
 
+	jc->arch_info = NULL;
+
+	ret = perf_cgroup_arch_css_alloc(parent_css, &jc->css);
+	if (ret)
+		return ERR_PTR(ret);
+
 	return &jc->css;
 }
 
@@ -10766,6 +10781,8 @@ static void perf_cgroup_css_free(struct cgroup_subsys_state *css)
 {
 	struct perf_cgroup *jc = container_of(css, struct perf_cgroup, css);
 
+	perf_cgroup_arch_css_free(css);
+
 	free_percpu(jc->info);
 	kfree(jc);
 }
@@ -10786,11 +10803,20 @@ static void perf_cgroup_attach(struct cgroup_taskset *tset)
 
 	cgroup_taskset_for_each(task, css, tset)
 		task_function_call(task, __perf_cgroup_move, task);
+
+	perf_cgroup_arch_attach(tset);
+}
+
+static int perf_cgroup_can_attach(struct cgroup_taskset *tset)
+{
+	return perf_cgroup_arch_can_attach(tset);
 }
 
+
 struct cgroup_subsys perf_event_cgrp_subsys = {
 	.css_alloc	= perf_cgroup_css_alloc,
 	.css_free	= perf_cgroup_css_free,
+	.can_attach	= perf_cgroup_can_attach,
 	.attach		= perf_cgroup_attach,
 };
 #endif /* CONFIG_CGROUP_PERF */
-- 
1.9.1

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

#1560539 — Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare

On Fri, 6 Jan 2017, Vikas Shivappa wrote:

> From: David Carrillo-Cisneros <davidcc@google.com>
> 
> cgroup hierarchy monitoring is not supported currently. This patch
> builds all the necessary datastructures, cgroup APIs like alloc, free
> etc and necessary quirks for supporting cgroup hierarchy monitoring in
> later patches.
> 
> - Introduce a architecture specific data structure arch_info in
> perf_cgroup to keep track of RMIDs and cgroup hierarchical monitoring.
> - perf sched_in calls all the cgroup ancestors when a cgroup is
> scheduled in. This will not work with cqm as we have a common per pkg
> rmid associated with one task and hence cannot write different RMIds
> into the MSR for each event. cqm driver enables a flag
> PERF_EV_CGROUP_NO_RECURSION which indicates the perf to not call all
> ancestor cgroups for each event and let the driver handle the hierarchy
> monitoring for cgroup.
> - Introduce event_terminate as event_destroy is called after cgrp is
> disassociated from the event to support rmid handling of the cgroup.
> This helps cqm clean up the cqm specific arch_info.
> - Add the cgroup APIs for alloc,free,attach and can_attach
> 
> The above framework will be used to build different cgroup features in
> later patches.

That's not a framework. It's a hodgepodge of core and x86 specific changes.

I'm not even trying to review it as a whole, simply because such changes
want to be split into several preparatory changes in the core which provide
the 'framework' parts and then an actual user in the architecture
code. I'll give some general feedback whatsoever.

> Tests: Same as before. Cgroup still doesnt work but we did the prep to
> get it to work

Oh well. What tests are the same as before? This information is just there
to take room in the changelog, right?

> Patch modified/refactored by Vikas Shivappa
> <vikas.shivappa@linux.intel.com> to support recycling removal.
> 
> Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>

So this patch is from David, but where is Davids Signed-off-by?

> ---
>  arch/x86/events/intel/cqm.c       | 19 ++++++++++++++++++-
>  arch/x86/include/asm/perf_event.h | 27 +++++++++++++++++++++++++++
>  include/linux/perf_event.h        | 32 ++++++++++++++++++++++++++++++++
>  kernel/events/core.c              | 28 +++++++++++++++++++++++++++-
>  4 files changed, 104 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
> index 68fd1da..a9bd7bd 100644
> --- a/arch/x86/events/intel/cqm.c
> +++ b/arch/x86/events/intel/cqm.c
> @@ -741,7 +741,13 @@ static int intel_cqm_event_init(struct perf_event *event)
>  	INIT_LIST_HEAD(&event->hw.cqm_group_entry);
>  	INIT_LIST_HEAD(&event->hw.cqm_groups_entry);
>  
> -	event->destroy = intel_cqm_event_destroy;
> +	/*
> +	 * CQM driver handles cgroup recursion and since only noe
> +	 * RMID can be programmed at the time in each core, then
> +	 * it is incompatible with the way generic code handles
> +	 * cgroup hierarchies.
> +	 */
> +	event->event_caps |= PERF_EV_CAP_CGROUP_NO_RECURSION;
>  
>  	mutex_lock(&cache_mutex);
>  
> @@ -918,6 +924,17 @@ static int intel_cqm_event_init(struct perf_event *event)
>  	.read		     = intel_cqm_event_read,
>  	.count		     = intel_cqm_event_count,
>  };
> +#ifdef CONFIG_CGROUP_PERF
> +int perf_cgroup_arch_css_alloc(struct cgroup_subsys_state *parent_css,
> +				      struct cgroup_subsys_state *new_css)
> +{}
> +void perf_cgroup_arch_css_free(struct cgroup_subsys_state *css)
> +{}
> +void perf_cgroup_arch_attach(struct cgroup_taskset *tset)
> +{}
> +int perf_cgroup_arch_can_attach(struct cgroup_taskset *tset)
> +{}
> +#endif

What the heck is this for? It does not even compile because
perf_cgroup_arch_css_alloc() and perf_cgroup_arch_can_attach() are empty
functions.

Crap, crap, crap.

>  static inline void cqm_pick_event_reader(int cpu)
>  {
> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
> index f353061..f38c7f0 100644
> --- a/arch/x86/include/asm/perf_event.h
> +++ b/arch/x86/include/asm/perf_event.h
> @@ -299,4 +299,31 @@ 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_INTEL_RDT_M
> +#ifdef CONFIG_CGROUP_PERF
> +
> +#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_free	perf_cgroup_arch_css_free
> +
> +void perf_cgroup_arch_css_free(struct cgroup_subsys_state *css);
> +
> +#define perf_cgroup_arch_attach		perf_cgroup_arch_attach
> +
> +void perf_cgroup_arch_attach(struct cgroup_taskset *tset);
> +
> +#define perf_cgroup_arch_can_attach	perf_cgroup_arch_can_attach
> +
> +int perf_cgroup_arch_can_attach(struct cgroup_taskset *tset);
> +
> +#endif
> +
> +#endif
>  #endif /* _ASM_X86_PERF_EVENT_H */

How the heck is one supposed to figure out which endif is belonging to
what? Random new lines are not helping for that.

Aside of that the double ifdef is horrible and this really is not at even
remotely a framework. It's hardcoded crap to serve that CQM mess. Nothing
else can ever use it. So don't pretend it to be a 'framework'. 

> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
> index a8f4749..410642a 100644
> --- a/include/linux/perf_event.h
> +++ b/include/linux/perf_event.h
> @@ -300,6 +300,12 @@ struct pmu {
>  	int (*event_init)		(struct perf_event *event);
>  
>  	/*
> +	 * Terminate the event for this PMU. Optional complement for a
> +	 * successful event_init. Called before the event fields are tear down.
> +	 */
> +	void (*event_terminate)		(struct perf_event *event);

And why does this need to be a PMU callback. It's called right before
perf_cgroup_detach(). Why does it need extra treatment and cannot be done
from the cgroup muck?

> +
> +	/*
>  	 * Notification that the event was mapped or unmapped.  Called
>  	 * in the context of the mapping task.
>  	 */
> @@ -516,9 +522,13 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
>   * PERF_EV_CAP_SOFTWARE: Is a software event.
>   * PERF_EV_CAP_READ_ACTIVE_PKG: A CPU event (or cgroup event) that can be read
>   * from any CPU in the package where it is active.
> + * PERF_EV_CAP_CGROUP_NO_RECURSION: A cgroup event that handles its own
> + * cgroup scoping. It does not need to be enabled for all of its descendants
> + * cgroups.
>   */
>  #define PERF_EV_CAP_SOFTWARE		BIT(0)
>  #define PERF_EV_CAP_READ_ACTIVE_PKG	BIT(1)
> +#define PERF_EV_CAP_CGROUP_NO_RECURSION	BIT(2)
>  
>  #define SWEVENT_HLIST_BITS		8
>  #define SWEVENT_HLIST_SIZE		(1 << SWEVENT_HLIST_BITS)
> @@ -823,6 +833,8 @@ struct perf_cgroup_info {
>  };
>  
>  struct perf_cgroup {
> +	/* Architecture specific information. */

That's a really useful comment.

> +	void				 *arch_info;
>  	struct cgroup_subsys_state	css;
>  	struct perf_cgroup_info	__percpu *info;
>  };
> @@ -844,6 +856,7 @@ struct perf_cgroup {
>  
>  #ifdef CONFIG_PERF_EVENTS
>  
> +extern int is_cgroup_event(struct perf_event *event);
>  extern void *perf_aux_output_begin(struct perf_output_handle *handle,
>  				   struct perf_event *event);
>  extern void perf_aux_output_end(struct perf_output_handle *handle,
> @@ -1387,4 +1400,23 @@ ssize_t perf_event_sysfs_show(struct device *dev, struct device_attribute *attr,
>  #define perf_event_exit_cpu	NULL
>  #endif
>  
> +/*
> + * Hooks for architecture specific extensions for perf_cgroup.

No. That's not architecture specific. That's CQM specific hackery.

> + */
> +#ifndef perf_cgroup_arch_css_alloc
> +#define perf_cgroup_arch_css_alloc(parent_css, new_css) 0
> +#endif

I really hate this define style. That can be solved nicely with weak
functions which avoid all this define and ifdeffery mess.

> +#ifndef perf_cgroup_arch_css_free
> +#define perf_cgroup_arch_css_free(css) do { } while (0)
> +#endif
> +
> +#ifndef perf_cgroup_arch_attach
> +#define perf_cgroup_arch_attach(tskset) do { } while (0)
> +#endif
> +
> +#ifndef perf_cgroup_arch_can_attach
> +#define perf_cgroup_arch_can_attach(tskset) 0

This one is exceptionally stupid. Here is the use case:

> +static int perf_cgroup_can_attach(struct cgroup_taskset *tset)
> +{
> +     return perf_cgroup_arch_can_attach(tset);
>  }
>
> +
>  struct cgroup_subsys perf_event_cgrp_subsys = {
>       .css_alloc      = perf_cgroup_css_alloc,
>       .css_free       = perf_cgroup_css_free,
> +     .can_attach     = perf_cgroup_can_attach,
>       .attach         = perf_cgroup_attach,
>  };

So you need a extra function for calling a stub macro if it just can
be done by assigning the real function (if required) to the callback
pointer and otherwise leave it NULL.

But all of this is moot because this 'arch framework' is just crap because
it's in reality a CQM extension of the core code, which is not going to
happen.

> +#endif
> +
>  #endif /* _LINUX_PERF_EVENT_H */
> diff --git a/kernel/events/core.c b/kernel/events/core.c
> index ab15509..229f611 100644
> --- a/kernel/events/core.c
> +++ b/kernel/events/core.c
> @@ -590,6 +590,9 @@ static inline u64 perf_event_clock(struct perf_event *event)
>  	if (!cpuctx->cgrp)
>  		return false;
>  
> +	if (event->event_caps & PERF_EV_CAP_CGROUP_NO_RECURSION)
> +		return cpuctx->cgrp->css.cgroup == event->cgrp->css.cgroup;
> +

Comments explaining what this does are overrated, right?

>  	/*
>  	 * Cgroup scoping is recursive.  An event enabled for a cgroup is
>  	 * also enabled for all its descendant cgroups.  If @cpuctx's
> @@ -606,7 +609,7 @@ static inline void perf_detach_cgroup(struct perf_event *event)
>  	event->cgrp = NULL;
>  }
>  
> -static inline int is_cgroup_event(struct perf_event *event)
> +int is_cgroup_event(struct perf_event *event)

So this is made global because there is no actual user outside of the core.

>  {
>  	return event->cgrp != NULL;
>  }
> @@ -4019,6 +4022,9 @@ static void _free_event(struct perf_event *event)
>  		mutex_unlock(&event->mmap_mutex);
>  	}
>  
> +	if (event->pmu->event_terminate)
> +		event->pmu->event_terminate(event);
> +
>  	if (is_cgroup_event(event))
>  		perf_detach_cgroup(event);
>  
> @@ -9246,6 +9252,8 @@ static void account_event(struct perf_event *event)
>  	exclusive_event_destroy(event);
>  
>  err_pmu:
> +	if (event->pmu->event_terminate)
> +		event->pmu->event_terminate(event);
>  	if (event->destroy)
>  		event->destroy(event);
>  	module_put(pmu->module);
> @@ -10748,6 +10756,7 @@ static int __init perf_event_sysfs_init(void)
>  perf_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
>  {
>  	struct perf_cgroup *jc;
> +	int ret;
>  
>  	jc = kzalloc(sizeof(*jc), GFP_KERNEL);
>  	if (!jc)
> @@ -10759,6 +10768,12 @@ static int __init perf_event_sysfs_init(void)
>  		return ERR_PTR(-ENOMEM);
>  	}
>  
> +	jc->arch_info = NULL;

Never trust kzalloc to zero out a data structure correctly!

> +
> +	ret = perf_cgroup_arch_css_alloc(parent_css, &jc->css);
> +	if (ret)
> +		return ERR_PTR(ret);

And then leak the allocated memory in case of error.

Another wonderful piece of trainwreck engineering.

As I said above: Split this into bits and pieces and provide a proper
justification for each of the items you add to the core: terminate,
PERF_EV_CAP_CGROUP_NO_RECURSION.

Then sit down and come up with a solution which allows to make use of the
cgroup core extensions for more than a single instance of a particular
piece of x86 perf hardware.

And please provide changelogs which explain WHY all of this is necessary,
not just the WHAT.

Thanks,

	tglx

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

#1560559 — Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare

On Tue, Jan 17, 2017 at 01:11:50PM +0100, Thomas Gleixner wrote:
> On Fri, 6 Jan 2017, Vikas Shivappa wrote:
> 
> > From: David Carrillo-Cisneros <davidcc@google.com>
> > 
> > cgroup hierarchy monitoring is not supported currently. This patch
> > builds all the necessary datastructures, cgroup APIs like alloc, free
> > etc and necessary quirks for supporting cgroup hierarchy monitoring in
> > later patches.
> > 
> > - Introduce a architecture specific data structure arch_info in
> > perf_cgroup to keep track of RMIDs and cgroup hierarchical monitoring.
> > - perf sched_in calls all the cgroup ancestors when a cgroup is
> > scheduled in. This will not work with cqm as we have a common per pkg
> > rmid associated with one task and hence cannot write different RMIds
> > into the MSR for each event. cqm driver enables a flag
> > PERF_EV_CGROUP_NO_RECURSION which indicates the perf to not call all
> > ancestor cgroups for each event and let the driver handle the hierarchy
> > monitoring for cgroup.
> > - Introduce event_terminate as event_destroy is called after cgrp is
> > disassociated from the event to support rmid handling of the cgroup.
> > This helps cqm clean up the cqm specific arch_info.
> > - Add the cgroup APIs for alloc,free,attach and can_attach
> > 
> > The above framework will be used to build different cgroup features in
> > later patches.
> 
> That's not a framework. It's a hodgepodge of core and x86 specific changes.

Trainwreck comes to mind. It completely fails to describe semantics of
the hacks and how they would preserve the cgroup invariants.

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

#1561225 — Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare

On Tue, 17 Jan 2017, Thomas Gleixner wrote:

> On Fri, 6 Jan 2017, Vikas Shivappa wrote:
>
>> From: David Carrillo-Cisneros <davidcc@google.com>
>>
>> cgroup hierarchy monitoring is not supported currently. This patch
>> builds all the necessary datastructures, cgroup APIs like alloc, free
>> etc and necessary quirks for supporting cgroup hierarchy monitoring in
>> later patches.
>>
>> - Introduce a architecture specific data structure arch_info in
>> perf_cgroup to keep track of RMIDs and cgroup hierarchical monitoring.
>> - perf sched_in calls all the cgroup ancestors when a cgroup is
>> scheduled in. This will not work with cqm as we have a common per pkg
>> rmid associated with one task and hence cannot write different RMIds
>> into the MSR for each event. cqm driver enables a flag
>> PERF_EV_CGROUP_NO_RECURSION which indicates the perf to not call all
>> ancestor cgroups for each event and let the driver handle the hierarchy
>> monitoring for cgroup.
>> - Introduce event_terminate as event_destroy is called after cgrp is
>> disassociated from the event to support rmid handling of the cgroup.
>> This helps cqm clean up the cqm specific arch_info.
>> - Add the cgroup APIs for alloc,free,attach and can_attach
>>
>> The above framework will be used to build different cgroup features in
>> later patches.
>
> That's not a framework. It's a hodgepodge of core and x86 specific changes.
>
> I'm not even trying to review it as a whole, simply because such changes
> want to be split into several preparatory changes in the core which provide
> the 'framework' parts and then an actual user in the architecture
> code. I'll give some general feedback whatsoever.

Will split them into multiple patches.

>
>> Tests: Same as before. Cgroup still doesnt work but we did the prep to
>> get it to work
>
> Oh well. What tests are the same as before? This information is just there
> to take room in the changelog, right?

Will fix the logs

>
>> Patch modified/refactored by Vikas Shivappa
>> <vikas.shivappa@linux.intel.com> to support recycling removal.
>>
>> Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
>
> So this patch is from David, but where is Davids Signed-off-by?
>
>> ---
>>  arch/x86/events/intel/cqm.c       | 19 ++++++++++++++++++-
>>  arch/x86/include/asm/perf_event.h | 27 +++++++++++++++++++++++++++
>>  include/linux/perf_event.h        | 32 ++++++++++++++++++++++++++++++++
>>  kernel/events/core.c              | 28 +++++++++++++++++++++++++++-
>>  4 files changed, 104 insertions(+), 2 deletions(-)
>>
>> diff --git a/arch/x86/events/intel/cqm.c b/arch/x86/events/intel/cqm.c
>> index 68fd1da..a9bd7bd 100644
>> --- a/arch/x86/events/intel/cqm.c
>> +++ b/arch/x86/events/intel/cqm.c
>> @@ -741,7 +741,13 @@ static int intel_cqm_event_init(struct perf_event *event)
>>  	INIT_LIST_HEAD(&event->hw.cqm_group_entry);
>>  	INIT_LIST_HEAD(&event->hw.cqm_groups_entry);
>>
>> -	event->destroy = intel_cqm_event_destroy;
>> +	/*
>> +	 * CQM driver handles cgroup recursion and since only noe
>> +	 * RMID can be programmed at the time in each core, then
>> +	 * it is incompatible with the way generic code handles
>> +	 * cgroup hierarchies.
>> +	 */
>> +	event->event_caps |= PERF_EV_CAP_CGROUP_NO_RECURSION;
>>
>>  	mutex_lock(&cache_mutex);
>>
>> @@ -918,6 +924,17 @@ static int intel_cqm_event_init(struct perf_event *event)
>>  	.read		     = intel_cqm_event_read,
>>  	.count		     = intel_cqm_event_count,
>>  };
>> +#ifdef CONFIG_CGROUP_PERF
>> +int perf_cgroup_arch_css_alloc(struct cgroup_subsys_state *parent_css,
>> +				      struct cgroup_subsys_state *new_css)
>> +{}
>> +void perf_cgroup_arch_css_free(struct cgroup_subsys_state *css)
>> +{}
>> +void perf_cgroup_arch_attach(struct cgroup_taskset *tset)
>> +{}
>> +int perf_cgroup_arch_can_attach(struct cgroup_taskset *tset)
>> +{}
>> +#endif
>
> What the heck is this for? It does not even compile because
> perf_cgroup_arch_css_alloc() and perf_cgroup_arch_can_attach() are empty
> functions.
>
> Crap, crap, crap.
>
>>  static inline void cqm_pick_event_reader(int cpu)
>>  {
>> diff --git a/arch/x86/include/asm/perf_event.h b/arch/x86/include/asm/perf_event.h
>> index f353061..f38c7f0 100644
>> --- a/arch/x86/include/asm/perf_event.h
>> +++ b/arch/x86/include/asm/perf_event.h
>> @@ -299,4 +299,31 @@ 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_INTEL_RDT_M
>> +#ifdef CONFIG_CGROUP_PERF
>> +
>> +#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_free	perf_cgroup_arch_css_free
>> +
>> +void perf_cgroup_arch_css_free(struct cgroup_subsys_state *css);
>> +
>> +#define perf_cgroup_arch_attach		perf_cgroup_arch_attach
>> +
>> +void perf_cgroup_arch_attach(struct cgroup_taskset *tset);
>> +
>> +#define perf_cgroup_arch_can_attach	perf_cgroup_arch_can_attach
>> +
>> +int perf_cgroup_arch_can_attach(struct cgroup_taskset *tset);
>> +
>> +#endif
>> +
>> +#endif
>>  #endif /* _ASM_X86_PERF_EVENT_H */
>
> How the heck is one supposed to figure out which endif is belonging to
> what? Random new lines are not helping for that.

Will add comments to indicate which ifdef the endif belongs

>
> Aside of that the double ifdef is horrible and this really is not at even
> remotely a framework. It's hardcoded crap to serve that CQM mess. Nothing
> else can ever use it. So don't pretend it to be a 'framework'.
>
>> diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
>> index a8f4749..410642a 100644
>> --- a/include/linux/perf_event.h
>> +++ b/include/linux/perf_event.h
>> @@ -300,6 +300,12 @@ struct pmu {
>>  	int (*event_init)		(struct perf_event *event);
>>
>>  	/*
>> +	 * Terminate the event for this PMU. Optional complement for a
>> +	 * successful event_init. Called before the event fields are tear down.
>> +	 */
>> +	void (*event_terminate)		(struct perf_event *event);
>
> And why does this need to be a PMU callback. It's called right before
> perf_cgroup_detach(). Why does it need extra treatment and cannot be done
> from the cgroup muck?

Will remove the terminate and do this in the cgroup api itself.

>
>> +
>> +	/*
>>  	 * Notification that the event was mapped or unmapped.  Called
>>  	 * in the context of the mapping task.
>>  	 */
>> @@ -516,9 +522,13 @@ typedef void (*perf_overflow_handler_t)(struct perf_event *,
>>   * PERF_EV_CAP_SOFTWARE: Is a software event.
>>   * PERF_EV_CAP_READ_ACTIVE_PKG: A CPU event (or cgroup event) that can be read
>>   * from any CPU in the package where it is active.
>> + * PERF_EV_CAP_CGROUP_NO_RECURSION: A cgroup event that handles its own
>> + * cgroup scoping. It does not need to be enabled for all of its descendants
>> + * cgroups.
>>   */
>>  #define PERF_EV_CAP_SOFTWARE		BIT(0)
>>  #define PERF_EV_CAP_READ_ACTIVE_PKG	BIT(1)
>> +#define PERF_EV_CAP_CGROUP_NO_RECURSION	BIT(2)
>>
>>  #define SWEVENT_HLIST_BITS		8
>>  #define SWEVENT_HLIST_SIZE		(1 << SWEVENT_HLIST_BITS)
>> @@ -823,6 +833,8 @@ struct perf_cgroup_info {
>>  };
>>
>>  struct perf_cgroup {
>> +	/* Architecture specific information. */
>
> That's a really useful comment.
>
>> +	void				 *arch_info;
>>  	struct cgroup_subsys_state	css;
>>  	struct perf_cgroup_info	__percpu *info;
>>  };
>> @@ -844,6 +856,7 @@ struct perf_cgroup {
>>
>>  #ifdef CONFIG_PERF_EVENTS
>>
>> +extern int is_cgroup_event(struct perf_event *event);
>>  extern void *perf_aux_output_begin(struct perf_output_handle *handle,
>>  				   struct perf_event *event);
>>  extern void perf_aux_output_end(struct perf_output_handle *handle,
>> @@ -1387,4 +1400,23 @@ ssize_t perf_event_sysfs_show(struct device *dev, struct device_attribute *attr,
>>  #define perf_event_exit_cpu	NULL
>>  #endif
>>
>> +/*
>> + * Hooks for architecture specific extensions for perf_cgroup.
>
> No. That's not architecture specific. That's CQM specific hackery.
>
>> + */
>> +#ifndef perf_cgroup_arch_css_alloc
>> +#define perf_cgroup_arch_css_alloc(parent_css, new_css) 0
>> +#endif
>
> I really hate this define style. That can be solved nicely with weak
> functions which avoid all this define and ifdeffery mess.
>
>> +#ifndef perf_cgroup_arch_css_free
>> +#define perf_cgroup_arch_css_free(css) do { } while (0)
>> +#endif
>> +
>> +#ifndef perf_cgroup_arch_attach
>> +#define perf_cgroup_arch_attach(tskset) do { } while (0)
>> +#endif
>> +
>> +#ifndef perf_cgroup_arch_can_attach
>> +#define perf_cgroup_arch_can_attach(tskset) 0
>
> This one is exceptionally stupid. Here is the use case:

Will fix and use the weak functions.

>
>> +static int perf_cgroup_can_attach(struct cgroup_taskset *tset)
>> +{
>> +     return perf_cgroup_arch_can_attach(tset);
>>  }
>>
>> +
>>  struct cgroup_subsys perf_event_cgrp_subsys = {
>>       .css_alloc      = perf_cgroup_css_alloc,
>>       .css_free       = perf_cgroup_css_free,
>> +     .can_attach     = perf_cgroup_can_attach,
>>       .attach         = perf_cgroup_attach,
>>  };
>
> So you need a extra function for calling a stub macro if it just can
> be done by assigning the real function (if required) to the callback
> pointer and otherwise leave it NULL.
>
> But all of this is moot because this 'arch framework' is just crap because
> it's in reality a CQM extension of the core code, which is not going to
> happen.
>
>> +#endif
>> +
>>  #endif /* _LINUX_PERF_EVENT_H */
>> diff --git a/kernel/events/core.c b/kernel/events/core.c
>> index ab15509..229f611 100644
>> --- a/kernel/events/core.c
>> +++ b/kernel/events/core.c
>> @@ -590,6 +590,9 @@ static inline u64 perf_event_clock(struct perf_event *event)
>>  	if (!cpuctx->cgrp)
>>  		return false;
>>
>> +	if (event->event_caps & PERF_EV_CAP_CGROUP_NO_RECURSION)
>> +		return cpuctx->cgrp->css.cgroup == event->cgrp->css.cgroup;
>> +
>
> Comments explaining what this does are overrated, right?

Will fix. Add a comment.

perf sched_in calls the cgroup ancestors during a cgroup event sched in. But the 
cqm events are package wide having mapped to RMIDs. Since only RMID can be 
written to the PQR_ASSOC we need to handle this seperately. The 
EV_CAP_NO_RECURSION event_caps indicates to not call all the ancestor groups.

>
>>  	/*
>>  	 * Cgroup scoping is recursive.  An event enabled for a cgroup is
>>  	 * also enabled for all its descendant cgroups.  If @cpuctx's
>> @@ -606,7 +609,7 @@ static inline void perf_detach_cgroup(struct perf_event *event)
>>  	event->cgrp = NULL;
>>  }
>>
>> -static inline int is_cgroup_event(struct perf_event *event)
>> +int is_cgroup_event(struct perf_event *event)
>
> So this is made global because there is no actual user outside of the core.

Will fix

>
>>  {
>>  	return event->cgrp != NULL;
>>  }
>> @@ -4019,6 +4022,9 @@ static void _free_event(struct perf_event *event)
>>  		mutex_unlock(&event->mmap_mutex);
>>  	}
>>
>> +	if (event->pmu->event_terminate)
>> +		event->pmu->event_terminate(event);
>> +
>>  	if (is_cgroup_event(event))
>>  		perf_detach_cgroup(event);
>>
>> @@ -9246,6 +9252,8 @@ static void account_event(struct perf_event *event)
>>  	exclusive_event_destroy(event);
>>
>>  err_pmu:
>> +	if (event->pmu->event_terminate)
>> +		event->pmu->event_terminate(event);
>>  	if (event->destroy)
>>  		event->destroy(event);
>>  	module_put(pmu->module);
>> @@ -10748,6 +10756,7 @@ static int __init perf_event_sysfs_init(void)
>>  perf_cgroup_css_alloc(struct cgroup_subsys_state *parent_css)
>>  {
>>  	struct perf_cgroup *jc;
>> +	int ret;
>>
>>  	jc = kzalloc(sizeof(*jc), GFP_KERNEL);
>>  	if (!jc)
>> @@ -10759,6 +10768,12 @@ static int __init perf_event_sysfs_init(void)
>>  		return ERR_PTR(-ENOMEM);
>>  	}
>>
>> +	jc->arch_info = NULL;
>
> Never trust kzalloc to zero out a data structure correctly!
>
>> +
>> +	ret = perf_cgroup_arch_css_alloc(parent_css, &jc->css);
>> +	if (ret)
>> +		return ERR_PTR(ret);
>
> And then leak the allocated memory in case of error.
>
> Another wonderful piece of trainwreck engineering.
>
> As I said above: Split this into bits and pieces and provide a proper
> justification for each of the items you add to the core: terminate,
> PERF_EV_CAP_CGROUP_NO_RECURSION.
>
> Then sit down and come up with a solution which allows to make use of the
> cgroup core extensions for more than a single instance of a particular
> piece of x86 perf hardware.

Will fix. Can split the patches (can remove the terminate..) and then add 
generic extensions which can be used.

cqm would need a way to maintain cgroup specific information as shown in the 
struct below because of the way h/w RMIDs are designed:

-the RMIDs are per package meaning the count(for mbm) and occupancy (for cqm) 
cannot be updated per cpu on when a task is scheduled.
-and we cannot track two events in a hierarchy at the same as we cannot write 
two RMIDs.

Because of this we maintain the mfa as shown below to keep track of which 
ancestor the cgroup(or the task) needs to report data to.

  * struct cgrp_cqm_info - perf_event cgroup metadata for cqm
  * @mon_enabled  Whether monitoring is enabled
  * @level        Level in the cgroup tree. Root is level 0.
  * @rmid        The rmids of the cgroup.
  * @mfa          'Monitoring for ancestor' points to the cqm_info
  *  of the ancestor the cgroup is monitoring for. 'Monitoring for ancestor'
  *  means you will use an ancestors RMID at sched_in if you are
  *  not monitoring yourself.
  *
  *  Due to the hierarchical nature of cgroups, every cgroup just
  *  monitors for the 'nearest monitored ancestor' at all times.
  *  Since root cgroup is always monitored, all descendents
  *  at boot time monitor for root and hence all mfa points to root except
  *  for root->mfa which is NULL.
  *  1. RMID setup: When cgroup x start monitoring:
  *    for each descendent y, if y's mfa->level < x->level, then
  *    y->mfa = x. (Where level of root node = 0...)
  *  2. sched_in: During sched_in for x
  *    if (x->mon_enabled) choose x->rmid
  *    else choose x->mfa->rmid.
  *  3. read: for each descendent of cgroup x
  *     if (x->monitored) count += rmid_read(x->rmid).
  *  4. evt_destroy: for each descendent y of x, if (y->mfa == x) then
  *     y->mfa = x->mfa. Meaning if any descendent was monitoring for x,
  *     set that descendent to monitor for the cgroup which x was monitoring for.
  *
  * @tskmon_rlist List of tasks being monitored in the cgroup
  *  When a task which belongs to a cgroup x is being monitored, it always uses
  *  its own task->rmid even if cgroup x is monitored during sched_in.
  *  To account for the counts of such tasks, cgroup keeps this list
  *  and parses it during read.

Thanks,
Vikas

>
> And please provide changelogs which explain WHY all of this is necessary,
> not just the WHAT.
>
> Thanks,
>
> 	tglx
>
>

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

#1560588 — Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare

On Fri, 6 Jan 2017, Vikas Shivappa wrote:
> @@ -741,7 +741,13 @@ static int intel_cqm_event_init(struct perf_event *event)
>  	INIT_LIST_HEAD(&event->hw.cqm_group_entry);
>  	INIT_LIST_HEAD(&event->hw.cqm_groups_entry);
>  
> -	event->destroy = intel_cqm_event_destroy;

I missed this in the first round, but tripped over it when looking at one
of the follow up patches.

How is that supposed to work?

1) intel_cqm_event_destroy() is still in the code and unused which emits a
   compiler warning, but that can obviously be ignored for a good measure.

2) How would any testing of this mess actually work?

   Not all all. Nothing ever tears down an event. So you just leave
   everything hanging around probably with dangling pointers left and
   right.

So now the 'Tests: Same as before.' in the so called changelog makes sense:

   'Same as before' means: Completely untested and broken.

Thanks,

	tglx

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

#1560991 — Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare

On Tue, 17 Jan 2017, Thomas Gleixner wrote:

> On Fri, 6 Jan 2017, Vikas Shivappa wrote:
>> @@ -741,7 +741,13 @@ static int intel_cqm_event_init(struct perf_event *event)
>>  	INIT_LIST_HEAD(&event->hw.cqm_group_entry);
>>  	INIT_LIST_HEAD(&event->hw.cqm_groups_entry);
>>
>> -	event->destroy = intel_cqm_event_destroy;
>
> I missed this in the first round, but tripped over it when looking at one
> of the follow up patches.
>
> How is that supposed to work?
>
> 1) intel_cqm_event_destroy() is still in the code and unused which emits a
>   compiler warning, but that can obviously be ignored for a good measure.
>
> 2) How would any testing of this mess actually work?
>
>   Not all all. Nothing ever tears down an event. So you just leave
>   everything hanging around probably with dangling pointers left and
>   right.

The terminate is defined in next patch. Will fix this as we dont need all this 
new api and the cgroup cqm specific structures can be freed with cgroup hooks 
instead of creating this new one.

Thanks,
Vikas

>
> So now the 'Tests: Same as before.' in the so called changelog makes sense:
>
>   'Same as before' means: Completely untested and broken.
>
> Thanks,
>
> 	tglx
>
>

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

#1561041 — Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare

On Tue, 17 Jan 2017, Shivappa Vikas wrote:
> On Tue, 17 Jan 2017, Thomas Gleixner wrote:
> 
> > On Fri, 6 Jan 2017, Vikas Shivappa wrote:
> > > @@ -741,7 +741,13 @@ static int intel_cqm_event_init(struct perf_event
> > > *event)
> > >  	INIT_LIST_HEAD(&event->hw.cqm_group_entry);
> > >  	INIT_LIST_HEAD(&event->hw.cqm_groups_entry);
> > > 
> > > -	event->destroy = intel_cqm_event_destroy;
> > 
> > I missed this in the first round, but tripped over it when looking at one
> > of the follow up patches.
> > 
> > How is that supposed to work?
> > 
> > 1) intel_cqm_event_destroy() is still in the code and unused which emits a
> >   compiler warning, but that can obviously be ignored for a good measure.
> > 
> > 2) How would any testing of this mess actually work?
> > 
> >   Not all all. Nothing ever tears down an event. So you just leave
> >   everything hanging around probably with dangling pointers left and
> >   right.
> 
> The terminate is defined in next patch.

I know and that does not make it any better. It's broken, end of story.

Thanks,

	tglx

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

#1560705 — Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare

On Fri, Jan 06, 2017 at 01:59:58PM -0800, Vikas Shivappa wrote:
> - Introduce event_terminate as event_destroy is called after cgrp is
> disassociated from the event to support rmid handling of the cgroup.
> This helps cqm clean up the cqm specific arch_info.

You've not even tried to audit the code to see if you can either move
the existing ->destroy() invocation or the perf_detach_cgroup() one,
have you?

Minimal APIs are a good thing, don't expand unless you absolutely have
to.

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

#1560990 — Re: [PATCH 05/12] x86/cqm,perf/core: Cgroup support prepare

On Tue, 17 Jan 2017, Peter Zijlstra wrote:

> On Fri, Jan 06, 2017 at 01:59:58PM -0800, Vikas Shivappa wrote:
>> - Introduce event_terminate as event_destroy is called after cgrp is
>> disassociated from the event to support rmid handling of the cgroup.
>> This helps cqm clean up the cqm specific arch_info.
>
> You've not even tried to audit the code to see if you can either move
> the existing ->destroy() invocation or the perf_detach_cgroup() one,
> have you?
>
> Minimal APIs are a good thing, don't expand unless you absolutely have
> to.

yes , the perf_detach_cgroup can just do the job.. Thanks for pointing. Will 
fix.

>

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

#1553327 — [PATCH 04/12] x86/cqm: Add Per pkg rmid support

The RMID is currently global and this extends it to per pkg rmid. The
h/w provides a set of RMIDs on each package and the same task can hence
be associated with different RMIDs on each package.

Patch introduces a new cqm_pkgs_data to keep track of the per package
free list, limbo list and other locking structures.
The corresponding rmid structures in the perf_event is
changed to hold an array of u32 RMIDs instead of a single u32.

The RMIDs are not assigned at the time of event creation and are
assigned in lazy mode at the first sched_in time for a task, thereby
rmid is never allocated if a task is not scheduled on a package. This
helps better usage of RMIDs and its scales with the increasing
sockets/packages.

Locking:
event list - perf init and terminate hold mutex. spin lock is held to
gaurd from the mbm hrtimer.
per pkg free and limbo list - global spin lock. Used by
get_rmid,put_rmid, perf start, terminate

Tests: RMIDs available increase by x times where x is number of sockets
and the usage is dynamic so we save more.

Patch is based on David Carrillo-Cisneros <davidcc@google.com> patches
in cqm2 series.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 arch/x86/events/intel/cqm.c | 340 ++++++++++++++++++++++++--------------------
 arch/x86/events/intel/cqm.h |  37 +++++
 include/linux/perf_event.h  |   2 +-
 3 files changed, 226 insertions(+), 153 deletions(-)
 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 7c37a25..68fd1da 100644
--- a/arch/x86/events/intel/cqm.c
+++ b/arch/x86/events/intel/cqm.c
@@ -11,6 +11,7 @@
 #include <asm/cpu_device_id.h>
 #include <asm/intel_rdt_common.h>
 #include "../perf_event.h"
+#include "cqm.h"
 
 #define MSR_IA32_QM_CTR		0x0c8e
 #define MSR_IA32_QM_EVTSEL	0x0c8d
@@ -25,7 +26,7 @@
 static u32 cqm_max_rmid = -1;
 static unsigned int cqm_l3_scale; /* supposedly cacheline size */
 static bool cqm_enabled, mbm_enabled;
-unsigned int mbm_socket_max;
+unsigned int cqm_socket_max;
 
 /*
  * The cached intel_pqr_state is strictly per CPU and can never be
@@ -83,6 +84,8 @@ struct sample {
  */
 static cpumask_t cqm_cpumask;
 
+struct pkg_data **cqm_pkgs_data;
+
 #define RMID_VAL_ERROR		(1ULL << 63)
 #define RMID_VAL_UNAVAIL	(1ULL << 62)
 
@@ -142,50 +145,11 @@ struct cqm_rmid_entry {
 	unsigned long queue_time;
 };
 
-/*
- * cqm_rmid_free_lru - A least recently used list of RMIDs.
- *
- * Oldest entry at the head, newest (most recently used) entry at the
- * tail. This list is never traversed, it's only used to keep track of
- * the lru order. That is, we only pick entries of the head or insert
- * them on the tail.
- *
- * All entries on the list are 'free', and their RMIDs are not currently
- * in use. To mark an RMID as in use, remove its entry from the lru
- * list.
- *
- *
- * cqm_rmid_limbo_lru - list of currently unused but (potentially) dirty RMIDs.
- *
- * This list is contains RMIDs that no one is currently using but that
- * may have a non-zero occupancy value associated with them. The
- * rotation worker moves RMIDs from the limbo list to the free list once
- * the occupancy value drops below __intel_cqm_threshold.
- *
- * Both lists are protected by cache_mutex.
- */
-static LIST_HEAD(cqm_rmid_free_lru);
-static LIST_HEAD(cqm_rmid_limbo_lru);
-
-/*
- * We use a simple array of pointers so that we can lookup a struct
- * cqm_rmid_entry in O(1). This alleviates the callers of __get_rmid()
- * and __put_rmid() from having to worry about dealing with struct
- * cqm_rmid_entry - they just deal with rmids, i.e. integers.
- *
- * Once this array is initialized it is read-only. No locks are required
- * to access it.
- *
- * All entries for all RMIDs can be looked up in the this array at all
- * times.
- */
-static struct cqm_rmid_entry **cqm_rmid_ptrs;
-
-static inline struct cqm_rmid_entry *__rmid_entry(u32 rmid)
+static inline struct cqm_rmid_entry *__rmid_entry(u32 rmid, int domain)
 {
 	struct cqm_rmid_entry *entry;
 
-	entry = cqm_rmid_ptrs[rmid];
+	entry = &cqm_pkgs_data[domain]->cqm_rmid_ptrs[rmid];
 	WARN_ON(entry->rmid != rmid);
 
 	return entry;
@@ -196,91 +160,56 @@ static inline struct cqm_rmid_entry *__rmid_entry(u32 rmid)
  *
  * We expect to be called with cache_mutex held.
  */
-static u32 __get_rmid(void)
+static u32 __get_rmid(int domain)
 {
+	struct list_head *cqm_flist;
 	struct cqm_rmid_entry *entry;
 
-	lockdep_assert_held(&cache_mutex);
+	lockdep_assert_held(&cache_lock);
 
-	if (list_empty(&cqm_rmid_free_lru))
+	cqm_flist = &cqm_pkgs_data[domain]->cqm_rmid_free_lru;
+
+	if (list_empty(cqm_flist))
 		return INVALID_RMID;
 
-	entry = list_first_entry(&cqm_rmid_free_lru, struct cqm_rmid_entry, list);
+	entry = list_first_entry(cqm_flist, struct cqm_rmid_entry, list);
 	list_del(&entry->list);
 
 	return entry->rmid;
 }
 
-static void __put_rmid(u32 rmid)
+static void __put_rmid(u32 rmid, int domain)
 {
 	struct cqm_rmid_entry *entry;
 
-	lockdep_assert_held(&cache_mutex);
+	lockdep_assert_held(&cache_lock);
 
-	WARN_ON(!__rmid_valid(rmid));
-	entry = __rmid_entry(rmid);
+	WARN_ON(!rmid);
+	entry = __rmid_entry(rmid, domain);
 
 	entry->queue_time = jiffies;
 	entry->state = RMID_DIRTY;
 
-	list_add_tail(&entry->list, &cqm_rmid_limbo_lru);
+	list_add_tail(&entry->list, &cqm_pkgs_data[domain]->cqm_rmid_limbo_lru);
 }
 
 static void cqm_cleanup(void)
 {
 	int i;
 
-	if (!cqm_rmid_ptrs)
+	if (!cqm_pkgs_data)
 		return;
 
-	for (i = 0; i < cqm_max_rmid; i++)
-		kfree(cqm_rmid_ptrs[i]);
-
-	kfree(cqm_rmid_ptrs);
-	cqm_rmid_ptrs = NULL;
-	cqm_enabled = false;
-}
-
-static int intel_cqm_setup_rmid_cache(void)
-{
-	struct cqm_rmid_entry *entry;
-	unsigned int nr_rmids;
-	int r = 0;
-
-	nr_rmids = cqm_max_rmid + 1;
-	cqm_rmid_ptrs = kzalloc(sizeof(struct cqm_rmid_entry *) *
-				nr_rmids, GFP_KERNEL);
-	if (!cqm_rmid_ptrs)
-		return -ENOMEM;
-
-	for (; r <= cqm_max_rmid; r++) {
-		struct cqm_rmid_entry *entry;
-
-		entry = kmalloc(sizeof(*entry), GFP_KERNEL);
-		if (!entry)
-			goto fail;
-
-		INIT_LIST_HEAD(&entry->list);
-		entry->rmid = r;
-		cqm_rmid_ptrs[r] = entry;
-
-		list_add_tail(&entry->list, &cqm_rmid_free_lru);
+	for (i = 0; i < cqm_socket_max; i++) {
+		if (cqm_pkgs_data[i]) {
+			kfree(cqm_pkgs_data[i]->cqm_rmid_ptrs);
+			kfree(cqm_pkgs_data[i]);
+		}
 	}
-
-	/*
-	 * RMID 0 is special and is always allocated. It's used for all
-	 * tasks that are not monitored.
-	 */
-	entry = __rmid_entry(0);
-	list_del(&entry->list);
-
-	return 0;
-
-fail:
-	cqm_cleanup();
-	return -ENOMEM;
+	kfree(cqm_pkgs_data);
 }
 
+
 /*
  * Determine if @a and @b measure the same set of tasks.
  *
@@ -333,13 +262,13 @@ static inline struct perf_cgroup *event_to_cgroup(struct perf_event *event)
 #endif
 
 struct rmid_read {
-	u32 rmid;
+	u32 *rmid;
 	u32 evt_type;
 	atomic64_t value;
 };
 
 static void __intel_cqm_event_count(void *info);
-static void init_mbm_sample(u32 rmid, u32 evt_type);
+static void init_mbm_sample(u32 *rmid, u32 evt_type);
 static void __intel_mbm_event_count(void *info);
 
 static bool is_cqm_event(int e)
@@ -420,10 +349,11 @@ static void __intel_mbm_event_init(void *info)
 {
 	struct rmid_read *rr = info;
 
-	update_sample(rr->rmid, rr->evt_type, 1);
+	if (__rmid_valid(rr->rmid[pkg_id]))
+		update_sample(rr->rmid[pkg_id], rr->evt_type, 1);
 }
 
-static void init_mbm_sample(u32 rmid, u32 evt_type)
+static void init_mbm_sample(u32 *rmid, u32 evt_type)
 {
 	struct rmid_read rr = {
 		.rmid = rmid,
@@ -444,7 +374,7 @@ static int intel_cqm_setup_event(struct perf_event *event,
 				  struct perf_event **group)
 {
 	struct perf_event *iter;
-	u32 rmid;
+	u32 *rmid, sizet;
 
 	event->hw.is_group_event = false;
 	list_for_each_entry(iter, &cache_groups, hw.cqm_groups_entry) {
@@ -454,24 +384,20 @@ static int intel_cqm_setup_event(struct perf_event *event,
 			/* All tasks in a group share an RMID */
 			event->hw.cqm_rmid = rmid;
 			*group = iter;
-			if (is_mbm_event(event->attr.config) && __rmid_valid(rmid))
+			if (is_mbm_event(event->attr.config))
 				init_mbm_sample(rmid, event->attr.config);
 			return 0;
 		}
-
-	}
-
-	rmid = __get_rmid();
-
-	if (!__rmid_valid(rmid)) {
-		pr_info("out of RMIDs\n");
-		return -EINVAL;
 	}
 
-	if (is_mbm_event(event->attr.config) && __rmid_valid(rmid))
-		init_mbm_sample(rmid, event->attr.config);
-
-	event->hw.cqm_rmid = rmid;
+	/*
+	 * RMIDs are allocated in LAZY mode by default only when
+	 * tasks monitored are scheduled in.
+	 */
+	sizet = sizeof(u32) * cqm_socket_max;
+	event->hw.cqm_rmid = kzalloc(sizet, GFP_KERNEL);
+	if (!event->hw.cqm_rmid)
+		return -ENOMEM;
 
 	return 0;
 }
@@ -489,7 +415,7 @@ static void intel_cqm_event_read(struct perf_event *event)
 		return;
 
 	raw_spin_lock_irqsave(&cache_lock, flags);
-	rmid = event->hw.cqm_rmid;
+	rmid = event->hw.cqm_rmid[pkg_id];
 
 	if (!__rmid_valid(rmid))
 		goto out;
@@ -515,12 +441,12 @@ static void __intel_cqm_event_count(void *info)
 	struct rmid_read *rr = info;
 	u64 val;
 
-	val = __rmid_read(rr->rmid);
-
-	if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
-		return;
-
-	atomic64_add(val, &rr->value);
+	if (__rmid_valid(rr->rmid[pkg_id])) {
+		val = __rmid_read(rr->rmid[pkg_id]);
+		if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
+			return;
+		atomic64_add(val, &rr->value);
+	}
 }
 
 static inline bool cqm_group_leader(struct perf_event *event)
@@ -533,10 +459,12 @@ static void __intel_mbm_event_count(void *info)
 	struct rmid_read *rr = info;
 	u64 val;
 
-	val = rmid_read_mbm(rr->rmid, rr->evt_type);
-	if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
-		return;
-	atomic64_add(val, &rr->value);
+	if (__rmid_valid(rr->rmid[pkg_id])) {
+		val = rmid_read_mbm(rr->rmid[pkg_id], rr->evt_type);
+		if (val & (RMID_VAL_ERROR | RMID_VAL_UNAVAIL))
+			return;
+		atomic64_add(val, &rr->value);
+	}
 }
 
 static enum hrtimer_restart mbm_hrtimer_handle(struct hrtimer *hrtimer)
@@ -559,7 +487,7 @@ static enum hrtimer_restart mbm_hrtimer_handle(struct hrtimer *hrtimer)
 	}
 
 	list_for_each_entry(iter, &cache_groups, hw.cqm_groups_entry) {
-		grp_rmid = iter->hw.cqm_rmid;
+		grp_rmid = iter->hw.cqm_rmid[pkg_id];
 		if (!__rmid_valid(grp_rmid))
 			continue;
 		if (is_mbm_event(iter->attr.config))
@@ -572,7 +500,7 @@ static enum hrtimer_restart mbm_hrtimer_handle(struct hrtimer *hrtimer)
 			if (!iter1->hw.is_group_event)
 				break;
 			if (is_mbm_event(iter1->attr.config))
-				update_sample(iter1->hw.cqm_rmid,
+				update_sample(iter1->hw.cqm_rmid[pkg_id],
 					      iter1->attr.config, 0);
 		}
 	}
@@ -610,7 +538,7 @@ static void mbm_hrtimer_init(void)
 	struct hrtimer *hr;
 	int i;
 
-	for (i = 0; i < mbm_socket_max; i++) {
+	for (i = 0; i < cqm_socket_max; i++) {
 		hr = &mbm_timers[i];
 		hrtimer_init(hr, CLOCK_MONOTONIC, HRTIMER_MODE_REL);
 		hr->function = mbm_hrtimer_handle;
@@ -667,16 +595,39 @@ static u64 intel_cqm_event_count(struct perf_event *event)
 	return __perf_event_count(event);
 }
 
+void alloc_needed_pkg_rmid(u32 *cqm_rmid)
+{
+	unsigned long flags;
+	u32 rmid;
+
+	if (WARN_ON(!cqm_rmid))
+		return;
+
+	if (cqm_rmid[pkg_id])
+		return;
+
+	raw_spin_lock_irqsave(&cache_lock, flags);
+
+	rmid = __get_rmid(pkg_id);
+	if (__rmid_valid(rmid))
+		cqm_rmid[pkg_id] = rmid;
+
+	raw_spin_unlock_irqrestore(&cache_lock, flags);
+}
+
 static void intel_cqm_event_start(struct perf_event *event, int mode)
 {
 	struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
-	u32 rmid = event->hw.cqm_rmid;
+	u32 rmid;
 
 	if (!(event->hw.cqm_state & PERF_HES_STOPPED))
 		return;
 
 	event->hw.cqm_state &= ~PERF_HES_STOPPED;
 
+	alloc_needed_pkg_rmid(event->hw.cqm_rmid);
+
+	rmid = event->hw.cqm_rmid[pkg_id];
 	state->rmid = rmid;
 	wrmsr(MSR_IA32_PQR_ASSOC, rmid, state->closid);
 }
@@ -691,22 +642,27 @@ static void intel_cqm_event_stop(struct perf_event *event, int mode)
 
 static int intel_cqm_event_add(struct perf_event *event, int mode)
 {
-	unsigned long flags;
-	u32 rmid;
-
-	raw_spin_lock_irqsave(&cache_lock, flags);
-
 	event->hw.cqm_state = PERF_HES_STOPPED;
-	rmid = event->hw.cqm_rmid;
 
-	if (__rmid_valid(rmid) && (mode & PERF_EF_START))
+	if ((mode & PERF_EF_START))
 		intel_cqm_event_start(event, mode);
 
-	raw_spin_unlock_irqrestore(&cache_lock, flags);
-
 	return 0;
 }
 
+static inline void
+	cqm_event_free_rmid(struct perf_event *event)
+{
+	u32 *rmid = event->hw.cqm_rmid;
+	int d;
+
+	for (d = 0; d < cqm_socket_max; d++) {
+		if (__rmid_valid(rmid[d]))
+			__put_rmid(rmid[d], d);
+	}
+	kfree(event->hw.cqm_rmid);
+	list_del(&event->hw.cqm_groups_entry);
+}
 static void intel_cqm_event_destroy(struct perf_event *event)
 {
 	struct perf_event *group_other = NULL;
@@ -737,16 +693,11 @@ static void intel_cqm_event_destroy(struct perf_event *event)
 		 * If there was a group_other, make that leader, otherwise
 		 * destroy the group and return the RMID.
 		 */
-		if (group_other) {
+		if (group_other)
 			list_replace(&event->hw.cqm_groups_entry,
 				     &group_other->hw.cqm_groups_entry);
-		} else {
-			u32 rmid = event->hw.cqm_rmid;
-
-			if (__rmid_valid(rmid))
-				__put_rmid(rmid);
-			list_del(&event->hw.cqm_groups_entry);
-		}
+		else
+			cqm_event_free_rmid(event);
 	}
 
 	raw_spin_unlock_irqrestore(&cache_lock, flags);
@@ -794,7 +745,7 @@ static int intel_cqm_event_init(struct perf_event *event)
 
 	mutex_lock(&cache_mutex);
 
-	/* Will also set rmid, return error on RMID not being available*/
+	/* Delay allocating RMIDs */
 	if (intel_cqm_setup_event(event, &group)) {
 		ret = -EINVAL;
 		goto out;
@@ -1036,12 +987,95 @@ static void mbm_cleanup(void)
 	{}
 };
 
+static int pkg_data_init_cpu(int cpu)
+{
+	struct cqm_rmid_entry *ccqm_rmid_ptrs = NULL, *entry = NULL;
+	int curr_pkgid = topology_physical_package_id(cpu);
+	struct pkg_data *pkg_data = NULL;
+	int i = 0, nr_rmids, ret = 0;
+
+	if (cqm_pkgs_data[curr_pkgid])
+		return 0;
+
+	pkg_data = kzalloc_node(sizeof(struct pkg_data),
+				GFP_KERNEL, cpu_to_node(cpu));
+	if (!pkg_data)
+		return -ENOMEM;
+
+	INIT_LIST_HEAD(&pkg_data->cqm_rmid_free_lru);
+	INIT_LIST_HEAD(&pkg_data->cqm_rmid_limbo_lru);
+
+	mutex_init(&pkg_data->pkg_data_mutex);
+	raw_spin_lock_init(&pkg_data->pkg_data_lock);
+
+	pkg_data->rmid_work_cpu = cpu;
+
+	nr_rmids = cqm_max_rmid + 1;
+	ccqm_rmid_ptrs = kzalloc(sizeof(struct cqm_rmid_entry) *
+			   nr_rmids, GFP_KERNEL);
+	if (!ccqm_rmid_ptrs) {
+		ret = -ENOMEM;
+		goto fail;
+	}
+
+	for (; i <= cqm_max_rmid; i++) {
+		entry = &ccqm_rmid_ptrs[i];
+		INIT_LIST_HEAD(&entry->list);
+		entry->rmid = i;
+
+		list_add_tail(&entry->list, &pkg_data->cqm_rmid_free_lru);
+	}
+
+	pkg_data->cqm_rmid_ptrs = ccqm_rmid_ptrs;
+	cqm_pkgs_data[curr_pkgid] = pkg_data;
+
+	/*
+	 * RMID 0 is special and is always allocated. It's used for all
+	 * tasks that are not monitored.
+	 */
+	entry = __rmid_entry(0, curr_pkgid);
+	list_del(&entry->list);
+
+	return 0;
+fail:
+	kfree(ccqm_rmid_ptrs);
+	ccqm_rmid_ptrs = NULL;
+	kfree(pkg_data);
+	pkg_data = NULL;
+	cqm_pkgs_data[curr_pkgid] = NULL;
+	return ret;
+}
+
+static int cqm_init_pkgs_data(void)
+{
+	int i, cpu, ret = 0;
+
+	cqm_pkgs_data = kzalloc(
+		sizeof(struct pkg_data *) * cqm_socket_max,
+		GFP_KERNEL);
+	if (!cqm_pkgs_data)
+		return -ENOMEM;
+
+	for (i = 0; i < cqm_socket_max; i++)
+		cqm_pkgs_data[i] = NULL;
+
+	for_each_online_cpu(cpu) {
+		ret = pkg_data_init_cpu(cpu);
+		if (ret)
+			goto fail;
+	}
+
+	return 0;
+fail:
+	cqm_cleanup();
+	return ret;
+}
+
 static int intel_mbm_init(void)
 {
 	int ret = 0, array_size, maxid = cqm_max_rmid + 1;
 
-	mbm_socket_max = topology_max_packages();
-	array_size = sizeof(struct sample) * maxid * mbm_socket_max;
+	array_size = sizeof(struct sample) * maxid * cqm_socket_max;
 	mbm_local = kmalloc(array_size, GFP_KERNEL);
 	if (!mbm_local)
 		return -ENOMEM;
@@ -1052,7 +1086,7 @@ static int intel_mbm_init(void)
 		goto out;
 	}
 
-	array_size = sizeof(struct hrtimer) * mbm_socket_max;
+	array_size = sizeof(struct hrtimer) * cqm_socket_max;
 	mbm_timers = kmalloc(array_size, GFP_KERNEL);
 	if (!mbm_timers) {
 		ret = -ENOMEM;
@@ -1128,7 +1162,8 @@ static int __init intel_cqm_init(void)
 
 	event_attr_intel_cqm_llc_scale.event_str = str;
 
-	ret = intel_cqm_setup_rmid_cache();
+	cqm_socket_max = topology_max_packages();
+	ret = cqm_init_pkgs_data();
 	if (ret)
 		goto out;
 
@@ -1171,6 +1206,7 @@ static int __init intel_cqm_init(void)
 	if (ret) {
 		kfree(str);
 		cqm_cleanup();
+		cqm_enabled = false;
 		mbm_cleanup();
 	}
 
diff --git a/arch/x86/events/intel/cqm.h b/arch/x86/events/intel/cqm.h
new file mode 100644
index 0000000..4415497
--- /dev/null
+++ b/arch/x86/events/intel/cqm.h
@@ -0,0 +1,37 @@
+#ifndef _ASM_X86_CQM_H
+#define _ASM_X86_CQM_H
+
+#ifdef CONFIG_INTEL_RDT_M
+
+#include <linux/perf_event.h>
+
+/**
+ * struct pkg_data - cqm per package(socket) meta data
+ * @cqm_rmid_free_lru    A least recently used list of free RMIDs
+ *     These RMIDs are guaranteed to have an occupancy less than the
+ * threshold occupancy
+ * @cqm_rmid_limbo_lru       list of currently unused but (potentially)
+ *     dirty RMIDs.
+ *     This list contains RMIDs that no one is currently using but that
+ *     may have a occupancy value > __intel_cqm_threshold. User can change
+ *     the threshold occupancy value.
+ * @cqm_rmid_entry - The entry in the limbo and free lists.
+ * @delayed_work - Work to reuse the RMIDs that have been freed.
+ * @rmid_work_cpu - The cpu on the package on which work is scheduled.
+ */
+struct pkg_data {
+	struct list_head	cqm_rmid_free_lru;
+	struct list_head	cqm_rmid_limbo_lru;
+
+	struct cqm_rmid_entry	*cqm_rmid_ptrs;
+
+	struct mutex		pkg_data_mutex;
+	raw_spinlock_t		pkg_data_lock;
+
+	struct delayed_work	intel_cqm_rmid_work;
+	atomic_t		reuse_scheduled;
+
+	int			rmid_work_cpu;
+};
+#endif
+#endif
diff --git a/include/linux/perf_event.h b/include/linux/perf_event.h
index 4741ecd..a8f4749 100644
--- a/include/linux/perf_event.h
+++ b/include/linux/perf_event.h
@@ -141,7 +141,7 @@ struct hw_perf_event {
 		};
 		struct { /* intel_cqm */
 			int			cqm_state;
-			u32			cqm_rmid;
+			u32			*cqm_rmid;
 			int			is_group_event;
 			struct list_head	cqm_events_entry;
 			struct list_head	cqm_groups_entry;
-- 
1.9.1

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

#1559981 — Re: [PATCH 04/12] x86/cqm: Add Per pkg rmid support\

On Fri, 6 Jan 2017, Vikas Shivappa wrote:

> Subject : [PATCH 04/12] x86/cqm: Add Per pkg rmid support

Can you please be a bit more careful about the subject lines. 'Per' wants
to be 'per' and pkg really can be written as package. There is no point in
cryptic abbreviations for no value. Finaly RMID wants to be all uppercase
as you use it in the text below.

> Patch introduces a new cqm_pkgs_data to keep track of the per package

Again. We know that this is a patch.....

> free list, limbo list and other locking structures.

So free list and limbo list are locking structures, interesting.

> The corresponding rmid structures in the perf_event is

s/structures/structure/

> changed to hold an array of u32 RMIDs instead of a single u32.
> 
> The RMIDs are not assigned at the time of event creation and are
> assigned in lazy mode at the first sched_in time for a task, thereby
> rmid is never allocated if a task is not scheduled on a package. This
> helps better usage of RMIDs and its scales with the increasing
> sockets/packages.
> 
> Locking:
> event list - perf init and terminate hold mutex. spin lock is held to
> gaurd from the mbm hrtimer.
> per pkg free and limbo list - global spin lock. Used by
> get_rmid,put_rmid, perf start, terminate

Locking documentation wants to be in the code not in some random changelog.

> Tests: RMIDs available increase by x times where x is number of sockets
> and the usage is dynamic so we save more.

What means: 'Tests:'? Is there a test in this patch? If yes, I seem to be
missing it.

> Patch is based on David Carrillo-Cisneros <davidcc@google.com> patches
> in cqm2 series.

We document such attributions with a tag:

Originally-From: David Carrillo-Cisneros <davidcc@google.com>

That way tools can pick it up.

>  static cpumask_t cqm_cpumask;
>  
> +struct pkg_data **cqm_pkgs_data;

Why is this global? There is no user outside of cqm.c AFAICT.

> -/*
> - * We use a simple array of pointers so that we can lookup a struct
> - * cqm_rmid_entry in O(1). This alleviates the callers of __get_rmid()
> - * and __put_rmid() from having to worry about dealing with struct
> - * cqm_rmid_entry - they just deal with rmids, i.e. integers.
> - *
> - * Once this array is initialized it is read-only. No locks are required
> - * to access it.
> - *
> - * All entries for all RMIDs can be looked up in the this array at all
> - * times.

So this comment was actually valuable. Sure, it does not match the new
implementation, but the basic principle is still the same. The comment
wants to be updated and not just removed.

>   *
>   * We expect to be called with cache_mutex held.

You rename cache_mutex to cache lock, but updating the comments is
optional, right?

> -static u32 __get_rmid(void)
> +static u32 __get_rmid(int domain)

  unsigned int domain please. The domain cannot be negative.

>  {
> +	struct list_head *cqm_flist;
>  	struct cqm_rmid_entry *entry;

Please keep the variables as a reverse fir tree:

>  	struct cqm_rmid_entry *entry;
> +	struct list_head *cqm_flist;

>  
> -	lockdep_assert_held(&cache_mutex);
> +	lockdep_assert_held(&cache_lock);
 
> -static void __put_rmid(u32 rmid)
> +static void __put_rmid(u32 rmid, int domain)
>  {
>  	struct cqm_rmid_entry *entry;
>  
> -	lockdep_assert_held(&cache_mutex);
> +	lockdep_assert_held(&cache_lock);
>  
> -	WARN_ON(!__rmid_valid(rmid));
> -	entry = __rmid_entry(rmid);
> +	WARN_ON(!rmid);

What's wrong with __rmid_valid() ?

> +	entry = __rmid_entry(rmid, domain);
>  
> +	kfree(cqm_pkgs_data);
>  }
>  
> +

Random white space noise.

>  static bool is_cqm_event(int e)
> @@ -420,10 +349,11 @@ static void __intel_mbm_event_init(void *info)
>  {
>  	struct rmid_read *rr = info;
>  
> -	update_sample(rr->rmid, rr->evt_type, 1);
> +	if (__rmid_valid(rr->rmid[pkg_id]))

Where the heck is pkg_id coming from?

Ahh:

#define pkg_id  topology_physical_package_id(smp_processor_id())

That's crap. It's existing crap, but nevertheless crap.

First of all it's irritating as 'pkg_id' looks like a variable and in this
case it would be at least file global, which makes no sense at all.

Secondly, we really want to move this to the logical package id as that is
actually guaranteing that the package id is smaller than
topology_max_packages(). The physical package id has no such guarantee.

And then please keep this local so its simple to read and parse:

    	 unsigned int pkgid = topology_logical_package_id(smp_processor_id());

> +		update_sample(rr->rmid[pkg_id], rr->evt_type, 1);
>  }

> @@ -444,7 +374,7 @@ static int intel_cqm_setup_event(struct perf_event *event,
>  				  struct perf_event **group)
>  {
>  	struct perf_event *iter;
> -	u32 rmid;
> +	u32 *rmid, sizet;

What's wrong with size? It's not confusable with size_t, right?

> +void alloc_needed_pkg_rmid(u32 *cqm_rmid)

Again: Why is this global?

And what's the meaning of needed in the function name? If the RMID wouldn't
be needed then the function would not be called, right?

> +{
> +	unsigned long flags;
> +	u32 rmid;
> +
> +	if (WARN_ON(!cqm_rmid))
> +		return;
> +
> +	if (cqm_rmid[pkg_id])
> +		return;
> +
> +	raw_spin_lock_irqsave(&cache_lock, flags);
> +
> +	rmid = __get_rmid(pkg_id);
> +	if (__rmid_valid(rmid))
> +		cqm_rmid[pkg_id] = rmid;
> +
> +	raw_spin_unlock_irqrestore(&cache_lock, flags);
> +}
> +
>  static void intel_cqm_event_start(struct perf_event *event, int mode)
>  {
>  	struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
> -	u32 rmid = event->hw.cqm_rmid;
> +	u32 rmid;
>  
>  	if (!(event->hw.cqm_state & PERF_HES_STOPPED))
>  		return;
>  
>  	event->hw.cqm_state &= ~PERF_HES_STOPPED;
>  
> +	alloc_needed_pkg_rmid(event->hw.cqm_rmid);
> +
> +	rmid = event->hw.cqm_rmid[pkg_id];

The allocation can fail. What's in event->hw.cqm_rmid[pkg_id] then? Zero or
what? And why is any of those values fine?

Can you please add comments so reviewers and readers do not have to figure
out every substantial detail themself?

>  	state->rmid = rmid;
>  	wrmsr(MSR_IA32_PQR_ASSOC, rmid, state->closid);

> +static inline void
> +	cqm_event_free_rmid(struct perf_event *event)

Yet another coding style variant from your unlimited supply of coding
horrors.

> +{
> +	u32 *rmid = event->hw.cqm_rmid;
> +	int d;
> +
> +	for (d = 0; d < cqm_socket_max; d++) {
> +		if (__rmid_valid(rmid[d]))
> +			__put_rmid(rmid[d], d);
> +	}
> +	kfree(event->hw.cqm_rmid);
> +	list_del(&event->hw.cqm_groups_entry);
> +}
>  static void intel_cqm_event_destroy(struct perf_event *event)
>  {
>  	struct perf_event *group_other = NULL;
> @@ -737,16 +693,11 @@ static void intel_cqm_event_destroy(struct perf_event *event)
>  		 * If there was a group_other, make that leader, otherwise
>  		 * destroy the group and return the RMID.
>  		 */
> -		if (group_other) {
> +		if (group_other)
>  			list_replace(&event->hw.cqm_groups_entry,
>  				     &group_other->hw.cqm_groups_entry);

Please keep the brackets. See:

http://lkml.kernel.org/r/alpine.DEB.2.20.1609101416420.32361@nanos

> -		} else {
> -			u32 rmid = event->hw.cqm_rmid;
> -
> -			if (__rmid_valid(rmid))
> -				__put_rmid(rmid);
> -			list_del(&event->hw.cqm_groups_entry);
> -		}
> +		else
> +			cqm_event_free_rmid(event);
>  	}
>  
>  	raw_spin_unlock_irqrestore(&cache_lock, flags);

> +static int pkg_data_init_cpu(int cpu)

cpus are unsigned int

> +{
> +	struct cqm_rmid_entry *ccqm_rmid_ptrs = NULL, *entry = NULL;

What are the pointer intializations for?

> +	int curr_pkgid = topology_physical_package_id(cpu);

Please use logical packages.

> +	struct pkg_data *pkg_data = NULL;
> +	int i = 0, nr_rmids, ret = 0;

Crap. 'i' is used in the for() loop below and therefor initialized exactly
there and not at some random other place. ret is a completely pointless
variable and the initialization is even more pointless. There is exactly
one code path using it (fail) and you can just return -ENOMEM from there.

> +	if (cqm_pkgs_data[curr_pkgid])
> +		return 0;
> +
> +	pkg_data = kzalloc_node(sizeof(struct pkg_data),
> +				GFP_KERNEL, cpu_to_node(cpu));
> +	if (!pkg_data)
> +		return -ENOMEM;
> +
> +	INIT_LIST_HEAD(&pkg_data->cqm_rmid_free_lru);
> +	INIT_LIST_HEAD(&pkg_data->cqm_rmid_limbo_lru);
> +
> +	mutex_init(&pkg_data->pkg_data_mutex);
> +	raw_spin_lock_init(&pkg_data->pkg_data_lock);
> +
> +	pkg_data->rmid_work_cpu = cpu;
> +
> +	nr_rmids = cqm_max_rmid + 1;
> +	ccqm_rmid_ptrs = kzalloc(sizeof(struct cqm_rmid_entry) *
> +			   nr_rmids, GFP_KERNEL);
> +	if (!ccqm_rmid_ptrs) {
> +		ret = -ENOMEM;
> +		goto fail;
> +	}
> +
> +	for (; i <= cqm_max_rmid; i++) {
> +		entry = &ccqm_rmid_ptrs[i];
> +		INIT_LIST_HEAD(&entry->list);
> +		entry->rmid = i;
> +
> +		list_add_tail(&entry->list, &pkg_data->cqm_rmid_free_lru);
> +	}
> +
> +	pkg_data->cqm_rmid_ptrs = ccqm_rmid_ptrs;
> +	cqm_pkgs_data[curr_pkgid] = pkg_data;
> +
> +	/*
> +	 * RMID 0 is special and is always allocated. It's used for all
> +	 * tasks that are not monitored.
> +	 */
> +	entry = __rmid_entry(0, curr_pkgid);
> +	list_del(&entry->list);
> +
> +	return 0;
> +fail:
> +	kfree(ccqm_rmid_ptrs);
> +	ccqm_rmid_ptrs = NULL;

And clearing the local variable has which value?

> +	kfree(pkg_data);
> +	pkg_data = NULL;
> +	cqm_pkgs_data[curr_pkgid] = NULL;

It never got set, so why do you need to clear it? Just because you do not
trust the compiler?

> +	return ret;
> +}
> +
> +static int cqm_init_pkgs_data(void)
> +{
> +	int i, cpu, ret = 0;

Again, pointless 0 initialization.

> +	cqm_pkgs_data = kzalloc(
> +		sizeof(struct pkg_data *) * cqm_socket_max,
> +		GFP_KERNEL);

Using a simple local variable to calculate size first would spare this new
variant of coding style horror.

> +	if (!cqm_pkgs_data)
> +		return -ENOMEM;
> +
> +	for (i = 0; i < cqm_socket_max; i++)
> +		cqm_pkgs_data[i] = NULL;

Surely kzalloc() is not reliable enough, so make sure that the pointers are
really NULL. Brilliant stuff.

> +
> +	for_each_online_cpu(cpu) {
> +		ret = pkg_data_init_cpu(cpu);
> +		if (ret)
> +			goto fail;
> +	}

And that's protected against CPU hotplug in which way?

Aside of that. What allocates the package data for packages which come
online _AFTER_ this function is called? Nothing AFAICT.

What you really need is a CPU hotplug callback for the prepare stage, which
is doing the setup of this race free and also handling packages which come
online after this.

> diff --git a/arch/x86/events/intel/cqm.h b/arch/x86/events/intel/cqm.h
> new file mode 100644
> index 0000000..4415497
> --- /dev/null
> +++ b/arch/x86/events/intel/cqm.h
> @@ -0,0 +1,37 @@
> +#ifndef _ASM_X86_CQM_H
> +#define _ASM_X86_CQM_H
> +
> +#ifdef CONFIG_INTEL_RDT_M
> +
> +#include <linux/perf_event.h>
> +
> +/**
> + * struct pkg_data - cqm per package(socket) meta data
> + * @cqm_rmid_free_lru    A least recently used list of free RMIDs
> + *     These RMIDs are guaranteed to have an occupancy less than the
> + * threshold occupancy

You certainly did not even try to run kernel doc on this.

    @var:  Explanation

is the proper format. Can you spot the difference?

Also the multi line comments are horribly formatted:

   @var:       	    This is a multiline commend which is necessary because
   		    it needs a lot of text......

> + * @cqm_rmid_entry - The entry in the limbo and free lists.

And this is incorrect as well. You are not even trying to make stuff
consistently wrong.

> + * @delayed_work - Work to reuse the RMIDs that have been freed.
> + * @rmid_work_cpu - The cpu on the package on which work is scheduled.

Also the formatting wants to be tabular:

 * @var:		Text
 * @long_var:		Text
 * @really_long_var:	Text

Sigh,

	tglx

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

#1560946 — Re: [PATCH 04/12] x86/cqm: Add Per pkg rmid support\

On Mon, 16 Jan 2017, Thomas Gleixner wrote:

> On Fri, 6 Jan 2017, Vikas Shivappa wrote:
>
>> Subject : [PATCH 04/12] x86/cqm: Add Per pkg rmid support
>
> Can you please be a bit more careful about the subject lines. 'Per' wants
> to be 'per' and pkg really can be written as package. There is no point in
> cryptic abbreviations for no value. Finaly RMID wants to be all uppercase
> as you use it in the text below.

Will fix and write more readable names.

>
>> Patch introduces a new cqm_pkgs_data to keep track of the per package
>
> Again. We know that this is a patch.....
>
>> free list, limbo list and other locking structures.
>
> So free list and limbo list are locking structures, interesting.
>
>> The corresponding rmid structures in the perf_event is
>
> s/structures/structure/

Will fix

>
>> changed to hold an array of u32 RMIDs instead of a single u32.
>>
>> The RMIDs are not assigned at the time of event creation and are
>> assigned in lazy mode at the first sched_in time for a task, thereby
>> rmid is never allocated if a task is not scheduled on a package. This
>> helps better usage of RMIDs and its scales with the increasing
>> sockets/packages.
>>
>> Locking:
>> event list - perf init and terminate hold mutex. spin lock is held to
>> gaurd from the mbm hrtimer.
>> per pkg free and limbo list - global spin lock. Used by
>> get_rmid,put_rmid, perf start, terminate
>
> Locking documentation wants to be in the code not in some random changelog.

Will add this to the code where i do the locking.

>
>> Tests: RMIDs available increase by x times where x is number of sockets
>> and the usage is dynamic so we save more.
>
> What means: 'Tests:'? Is there a test in this patch? If yes, I seem to be
> missing it.

Patch does not include a test case. Will add a description of the actual tests. 
Will that work ?

In this case , the test was done as below:

On a dual socket bdw system
For testing , force the max_rmid to x (prefreably less #)
Run x threads which a affinitized to each socket.
-This gives valid data with the per package patch.

>
>> Patch is based on David Carrillo-Cisneros <davidcc@google.com> patches
>> in cqm2 series.
>
> We document such attributions with a tag:
>
> Originally-From: David Carrillo-Cisneros <davidcc@google.com>
>
> That way tools can pick it up.

Ok , will fix this on all patches rather than the confusing 'based on' in the 
change log on all the patches.

>
>>  static cpumask_t cqm_cpumask;
>>
>> +struct pkg_data **cqm_pkgs_data;
>
> Why is this global? There is no user outside of cqm.c AFAICT.

will fix

>
>> -/*
>> - * We use a simple array of pointers so that we can lookup a struct
>> - * cqm_rmid_entry in O(1). This alleviates the callers of __get_rmid()
>> - * and __put_rmid() from having to worry about dealing with struct
>> - * cqm_rmid_entry - they just deal with rmids, i.e. integers.
>> - *
>> - * Once this array is initialized it is read-only. No locks are required
>> - * to access it.
>> - *
>> - * All entries for all RMIDs can be looked up in the this array at all
>> - * times.
>
> So this comment was actually valuable. Sure, it does not match the new
> implementation, but the basic principle is still the same. The comment
> wants to be updated and not just removed.
>
>>   *
>>   * We expect to be called with cache_mutex held.
>
> You rename cache_mutex to cache lock, but updating the comments is
> optional, right?
>
>> -static u32 __get_rmid(void)
>> +static u32 __get_rmid(int domain)
>
>  unsigned int domain please. The domain cannot be negative.
>
>>  {
>> +	struct list_head *cqm_flist;
>>  	struct cqm_rmid_entry *entry;
>
> Please keep the variables as a reverse fir tree:
>
>>  	struct cqm_rmid_entry *entry;
>> +	struct list_head *cqm_flist;
>
>>
>> -	lockdep_assert_held(&cache_mutex);
>> +	lockdep_assert_held(&cache_lock);
>
>> -static void __put_rmid(u32 rmid)
>> +static void __put_rmid(u32 rmid, int domain)
>>  {
>>  	struct cqm_rmid_entry *entry;
>>
>> -	lockdep_assert_held(&cache_mutex);
>> +	lockdep_assert_held(&cache_lock);
>>
>> -	WARN_ON(!__rmid_valid(rmid));
>> -	entry = __rmid_entry(rmid);
>> +	WARN_ON(!rmid);
>
> What's wrong with __rmid_valid() ?
>
>> +	entry = __rmid_entry(rmid, domain);
>>
>> +	kfree(cqm_pkgs_data);
>>  }
>>
>> +
>
> Random white space noise.
>

Will fix with respect to all comments above

>>  static bool is_cqm_event(int e)
>> @@ -420,10 +349,11 @@ static void __intel_mbm_event_init(void *info)
>>  {
>>  	struct rmid_read *rr = info;
>>
>> -	update_sample(rr->rmid, rr->evt_type, 1);
>> +	if (__rmid_valid(rr->rmid[pkg_id]))
>
> Where the heck is pkg_id coming from?
>
> Ahh:
>
> #define pkg_id  topology_physical_package_id(smp_processor_id())
>
> That's crap. It's existing crap, but nevertheless crap.
>
> First of all it's irritating as 'pkg_id' looks like a variable and in this
> case it would be at least file global, which makes no sense at all.
>
> Secondly, we really want to move this to the logical package id as that is
> actually guaranteing that the package id is smaller than
> topology_max_packages(). The physical package id has no such guarantee.
>
> And then please keep this local so its simple to read and parse:

Ok , this pkg_id was from the mbm patch. Will change this and fix, probably 
have a seperate patch to first change this one as its already used in 
upstream.

>
>    	 unsigned int pkgid = topology_logical_package_id(smp_processor_id());
>
>> +		update_sample(rr->rmid[pkg_id], rr->evt_type, 1);
>>  }
>
>> @@ -444,7 +374,7 @@ static int intel_cqm_setup_event(struct perf_event *event,
>>  				  struct perf_event **group)
>>  {
>>  	struct perf_event *iter;
>> -	u32 rmid;
>> +	u32 *rmid, sizet;
>
> What's wrong with size? It's not confusable with size_t, right?
>
>> +void alloc_needed_pkg_rmid(u32 *cqm_rmid)
>
> Again: Why is this global?
>
> And what's the meaning of needed in the function name? If the RMID wouldn't
> be needed then the function would not be called, right?

THis is related to the other problem you commented a lot that I put 
structures/functions and used them later in a seperate patch (hence compiler 
warnings .. and a whole bunch of such issues ). This i used later -

i will better organize the patches to fix all these.

>
>> +{
>> +	unsigned long flags;
>> +	u32 rmid;
>> +
>> +	if (WARN_ON(!cqm_rmid))
>> +		return;
>> +
>> +	if (cqm_rmid[pkg_id])
>> +		return;
>> +
>> +	raw_spin_lock_irqsave(&cache_lock, flags);
>> +
>> +	rmid = __get_rmid(pkg_id);
>> +	if (__rmid_valid(rmid))
>> +		cqm_rmid[pkg_id] = rmid;
>> +
>> +	raw_spin_unlock_irqrestore(&cache_lock, flags);
>> +}
>> +
>>  static void intel_cqm_event_start(struct perf_event *event, int mode)
>>  {
>>  	struct intel_pqr_state *state = this_cpu_ptr(&pqr_state);
>> -	u32 rmid = event->hw.cqm_rmid;
>> +	u32 rmid;
>>
>>  	if (!(event->hw.cqm_state & PERF_HES_STOPPED))
>>  		return;
>>
>>  	event->hw.cqm_state &= ~PERF_HES_STOPPED;
>>
>> +	alloc_needed_pkg_rmid(event->hw.cqm_rmid);
>> +
>> +	rmid = event->hw.cqm_rmid[pkg_id];
>
> The allocation can fail. What's in event->hw.cqm_rmid[pkg_id] then? Zero or
> what? And why is any of those values fine?
>
> Can you please add comments so reviewers and readers do not have to figure
> out every substantial detail themself?

If the allocation fails , it will have zero which is the default. And zero is 
the default RMID for all threads. A later patch indicates this as an error to 
the user if the RMID wasnt available. Will comment this.

>
>>  	state->rmid = rmid;
>>  	wrmsr(MSR_IA32_PQR_ASSOC, rmid, state->closid);
>
>> +static inline void
>> +	cqm_event_free_rmid(struct perf_event *event)
>
> Yet another coding style variant from your unlimited supply of coding
> horrors.
>
>> +{
>> +	u32 *rmid = event->hw.cqm_rmid;
>> +	int d;
>> +
>> +	for (d = 0; d < cqm_socket_max; d++) {
>> +		if (__rmid_valid(rmid[d]))
>> +			__put_rmid(rmid[d], d);
>> +	}
>> +	kfree(event->hw.cqm_rmid);
>> +	list_del(&event->hw.cqm_groups_entry);
>> +}
>>  static void intel_cqm_event_destroy(struct perf_event *event)
>>  {
>>  	struct perf_event *group_other = NULL;
>> @@ -737,16 +693,11 @@ static void intel_cqm_event_destroy(struct perf_event *event)
>>  		 * If there was a group_other, make that leader, otherwise
>>  		 * destroy the group and return the RMID.
>>  		 */
>> -		if (group_other) {
>> +		if (group_other)
>>  			list_replace(&event->hw.cqm_groups_entry,
>>  				     &group_other->hw.cqm_groups_entry);
>
> Please keep the brackets. See:
>
> http://lkml.kernel.org/r/alpine.DEB.2.20.1609101416420.32361@nanos
>
>> -		} else {
>> -			u32 rmid = event->hw.cqm_rmid;
>> -
>> -			if (__rmid_valid(rmid))
>> -				__put_rmid(rmid);
>> -			list_del(&event->hw.cqm_groups_entry);
>> -		}
>> +		else
>> +			cqm_event_free_rmid(event);
>>  	}
>>
>>  	raw_spin_unlock_irqrestore(&cache_lock, flags);
>
>> +static int pkg_data_init_cpu(int cpu)
>
> cpus are unsigned int
>
>> +{
>> +	struct cqm_rmid_entry *ccqm_rmid_ptrs = NULL, *entry = NULL;
>
> What are the pointer intializations for?
>
>> +	int curr_pkgid = topology_physical_package_id(cpu);
>
> Please use logical packages.

Will fix this and all coding style errors pointed.

>
>> +	struct pkg_data *pkg_data = NULL;
>> +	int i = 0, nr_rmids, ret = 0;
>
> Crap. 'i' is used in the for() loop below and therefor initialized exactly
> there and not at some random other place. ret is a completely pointless
> variable and the initialization is even more pointless. There is exactly
> one code path using it (fail) and you can just return -ENOMEM from there.
>
>> +	if (cqm_pkgs_data[curr_pkgid])
>> +		return 0;
>> +
>> +	pkg_data = kzalloc_node(sizeof(struct pkg_data),
>> +				GFP_KERNEL, cpu_to_node(cpu));
>> +	if (!pkg_data)
>> +		return -ENOMEM;
>> +
>> +	INIT_LIST_HEAD(&pkg_data->cqm_rmid_free_lru);
>> +	INIT_LIST_HEAD(&pkg_data->cqm_rmid_limbo_lru);
>> +
>> +	mutex_init(&pkg_data->pkg_data_mutex);
>> +	raw_spin_lock_init(&pkg_data->pkg_data_lock);
>> +
>> +	pkg_data->rmid_work_cpu = cpu;
>> +
>> +	nr_rmids = cqm_max_rmid + 1;
>> +	ccqm_rmid_ptrs = kzalloc(sizeof(struct cqm_rmid_entry) *
>> +			   nr_rmids, GFP_KERNEL);
>> +	if (!ccqm_rmid_ptrs) {
>> +		ret = -ENOMEM;
>> +		goto fail;
>> +	}
>> +
>> +	for (; i <= cqm_max_rmid; i++) {
>> +		entry = &ccqm_rmid_ptrs[i];
>> +		INIT_LIST_HEAD(&entry->list);
>> +		entry->rmid = i;
>> +
>> +		list_add_tail(&entry->list, &pkg_data->cqm_rmid_free_lru);
>> +	}
>> +
>> +	pkg_data->cqm_rmid_ptrs = ccqm_rmid_ptrs;
>> +	cqm_pkgs_data[curr_pkgid] = pkg_data;
>> +
>> +	/*
>> +	 * RMID 0 is special and is always allocated. It's used for all
>> +	 * tasks that are not monitored.
>> +	 */
>> +	entry = __rmid_entry(0, curr_pkgid);
>> +	list_del(&entry->list);
>> +
>> +	return 0;
>> +fail:
>> +	kfree(ccqm_rmid_ptrs);
>> +	ccqm_rmid_ptrs = NULL;
>
> And clearing the local variable has which value?
>
>> +	kfree(pkg_data);
>> +	pkg_data = NULL;
>> +	cqm_pkgs_data[curr_pkgid] = NULL;
>
> It never got set, so why do you need to clear it? Just because you do not
> trust the compiler?
>
>> +	return ret;
>> +}
>> +
>> +static int cqm_init_pkgs_data(void)
>> +{
>> +	int i, cpu, ret = 0;
>
> Again, pointless 0 initialization.
>
>> +	cqm_pkgs_data = kzalloc(
>> +		sizeof(struct pkg_data *) * cqm_socket_max,
>> +		GFP_KERNEL);
>
> Using a simple local variable to calculate size first would spare this new
> variant of coding style horror.
>
>> +	if (!cqm_pkgs_data)
>> +		return -ENOMEM;
>> +
>> +	for (i = 0; i < cqm_socket_max; i++)
>> +		cqm_pkgs_data[i] = NULL;
>
> Surely kzalloc() is not reliable enough, so make sure that the pointers are
> really NULL. Brilliant stuff.

Will fix all the initialization and repeated zeroing indicated.

>
>> +
>> +	for_each_online_cpu(cpu) {
>> +		ret = pkg_data_init_cpu(cpu);
>> +		if (ret)
>> +			goto fail;
>> +	}
>
> And that's protected against CPU hotplug in which way?
>
> Aside of that. What allocates the package data for packages which come
> online _AFTER_ this function is called? Nothing AFAICT.
>
> What you really need is a CPU hotplug callback for the prepare stage, which
> is doing the setup of this race free and also handling packages which come
> online after this.

Will fix the init and protecting this in cpu hotplug (get_online_cpus()).

>
>> diff --git a/arch/x86/events/intel/cqm.h b/arch/x86/events/intel/cqm.h
>> new file mode 100644
>> index 0000000..4415497
>> --- /dev/null
>> +++ b/arch/x86/events/intel/cqm.h
>> @@ -0,0 +1,37 @@
>> +#ifndef _ASM_X86_CQM_H
>> +#define _ASM_X86_CQM_H
>> +
>> +#ifdef CONFIG_INTEL_RDT_M
>> +
>> +#include <linux/perf_event.h>
>> +
>> +/**
>> + * struct pkg_data - cqm per package(socket) meta data
>> + * @cqm_rmid_free_lru    A least recently used list of free RMIDs
>> + *     These RMIDs are guaranteed to have an occupancy less than the
>> + * threshold occupancy
>
> You certainly did not even try to run kernel doc on this.
>
>    @var:  Explanation
>
> is the proper format. Can you spot the difference?
>
> Also the multi line comments are horribly formatted:
>
>   @var:       	    This is a multiline commend which is necessary because
>   		    it needs a lot of text......
>
>> + * @cqm_rmid_entry - The entry in the limbo and free lists.
>
> And this is incorrect as well. You are not even trying to make stuff
> consistently wrong.
>
>> + * @delayed_work - Work to reuse the RMIDs that have been freed.
>> + * @rmid_work_cpu - The cpu on the package on which work is scheduled.
>
> Also the formatting wants to be tabular:
>
> * @var:		Text
> * @long_var:		Text
> * @really_long_var:	Text

Will fix the comment formats.

Thanks,
Vikas



>
> Sigh,
>
> 	tglx
>

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

#1553341 — [PATCH 03/12] x86/rdt: Add rdt common/cqm compile option

Add a compile option INTEL_RDT which enables common code for all
RDT(Resource director technology) and a specific INTEL_RDT_M which
enables code for RDT monitoring. CQM(cache quality monitoring) and
mbm(memory b/w monitoring) are part of Intel RDT monitoring.

Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>

Conflicts:
	arch/x86/Kconfig
---
 arch/x86/Kconfig               | 17 +++++++++++++++++
 arch/x86/events/intel/Makefile |  3 ++-
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index e487493..b2f4b24 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -412,11 +412,28 @@ config GOLDFISH
        def_bool y
        depends on X86_GOLDFISH
 
+config INTEL_RDT
+	bool
+
+config INTEL_RDT_M
+	bool "Intel Resource Director Technology Monitoring support"
+	default n
+	depends on X86 && CPU_SUP_INTEL
+	select INTEL_RDT
+	help
+	  Select to enable resource monitoring which is a sub-feature of
+	  Intel Resource Director Technology(RDT). More information about
+	  RDT can be found in the Intel x86 Architecture Software
+	  Developer Manual.
+
+	  Say N if unsure.
+
 config INTEL_RDT_A
 	bool "Intel Resource Director Technology Allocation support"
 	default n
 	depends on X86 && CPU_SUP_INTEL
 	select KERNFS
+	select INTEL_RDT
 	help
 	  Select to enable resource allocation which is a sub-feature of
 	  Intel Resource Director Technology(RDT). More information about
diff --git a/arch/x86/events/intel/Makefile b/arch/x86/events/intel/Makefile
index 06c2baa..2e002a5 100644
--- a/arch/x86/events/intel/Makefile
+++ b/arch/x86/events/intel/Makefile
@@ -1,4 +1,5 @@
-obj-$(CONFIG_CPU_SUP_INTEL)		+= core.o bts.o cqm.o
+obj-$(CONFIG_CPU_SUP_INTEL)		+= core.o bts.o
+obj-$(CONFIG_INTEL_RDT_M)		+= cqm.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-perf.o
-- 
1.9.1

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

#1559958 — Re: [PATCH 03/12] x86/rdt: Add rdt common/cqm compile option

On Fri, 6 Jan 2017, Vikas Shivappa wrote:
> Add a compile option INTEL_RDT which enables common code for all
> RDT(Resource director technology) and a specific INTEL_RDT_M which
> enables code for RDT monitoring. CQM(cache quality monitoring) and
> mbm(memory b/w monitoring) are part of Intel RDT monitoring.

If we handle this with its own config option, can you please make this
thing modular? There is no point to have this compiled in and wasting
memory if it's not supported by the CPU.

Thanks,

	tglx

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

#1560835 — Re: [PATCH 03/12] x86/rdt: Add rdt common/cqm compile option

On Mon, 16 Jan 2017, Thomas Gleixner wrote:

> On Fri, 6 Jan 2017, Vikas Shivappa wrote:
>> Add a compile option INTEL_RDT which enables common code for all
>> RDT(Resource director technology) and a specific INTEL_RDT_M which
>> enables code for RDT monitoring. CQM(cache quality monitoring) and
>> mbm(memory b/w monitoring) are part of Intel RDT monitoring.
>
> If we handle this with its own config option, can you please make this
> thing modular? There is no point to have this compiled in and wasting
> memory if it's not supported by the CPU.

will fix, can change this  as a module...

Thanks,
Vikas

>
> Thanks,
>
> 	tglx
>

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

#1553343 — [PATCH 11/12] perf/stat: fix bug in handling events in error state

From: Stephane Eranian <eranian@google.com>

When an event is in error state, read() returns 0
instead of sizeof() buffer. In certain modes, such
as interval printing, ignoring the 0 return value
may cause bogus count deltas to be computed and
thus invalid results printed.

this patch fixes this problem by modifying read_counters()
to mark the event as not scaled (scaled = -1) to force
the printout routine to show <NOT COUNTED>.

Signed-off-by: Stephane Eranian <eranian@google.com>
Signed-off-by: Vikas Shivappa <vikas.shivappa@linux.intel.com>
---
 tools/perf/builtin-stat.c | 12 +++++++++---
 tools/perf/util/evsel.c   |  4 ++--
 2 files changed, 11 insertions(+), 5 deletions(-)

diff --git a/tools/perf/builtin-stat.c b/tools/perf/builtin-stat.c
index a02f2e9..9f109b9 100644
--- a/tools/perf/builtin-stat.c
+++ b/tools/perf/builtin-stat.c
@@ -310,8 +310,12 @@ static int read_counter(struct perf_evsel *counter)
 			struct perf_counts_values *count;
 
 			count = perf_counts(counter->counts, cpu, thread);
-			if (perf_evsel__read(counter, cpu, thread, count))
+			if (perf_evsel__read(counter, cpu, thread, count)) {
+				counter->counts->scaled = -1;
+				perf_counts(counter->counts, cpu, thread)->ena = 0;
+				perf_counts(counter->counts, cpu, thread)->run = 0;
 				return -1;
+			}
 
 			if (STAT_RECORD) {
 				if (perf_evsel__write_stat_event(counter, cpu, thread, count)) {
@@ -336,12 +340,14 @@ static int read_counter(struct perf_evsel *counter)
 static void read_counters(void)
 {
 	struct perf_evsel *counter;
+	int ret;
 
 	evlist__for_each_entry(evsel_list, counter) {
-		if (read_counter(counter))
+		ret = read_counter(counter);
+		if (ret)
 			pr_debug("failed to read counter %s\n", counter->name);
 
-		if (perf_stat_process_counter(&stat_config, counter))
+		if (ret == 0 && perf_stat_process_counter(&stat_config, counter))
 			pr_warning("failed to process counter %s\n", counter->name);
 	}
 }
diff --git a/tools/perf/util/evsel.c b/tools/perf/util/evsel.c
index 04e536a..7aa10e3 100644
--- a/tools/perf/util/evsel.c
+++ b/tools/perf/util/evsel.c
@@ -1232,7 +1232,7 @@ int perf_evsel__read(struct perf_evsel *evsel, int cpu, int thread,
 	if (FD(evsel, cpu, thread) < 0)
 		return -EINVAL;
 
-	if (readn(FD(evsel, cpu, thread), count, sizeof(*count)) < 0)
+	if (readn(FD(evsel, cpu, thread), count, sizeof(*count)) <= 0)
 		return -errno;
 
 	return 0;
@@ -1250,7 +1250,7 @@ int __perf_evsel__read_on_cpu(struct perf_evsel *evsel,
 	if (evsel->counts == NULL && perf_evsel__alloc_counts(evsel, cpu + 1, thread + 1) < 0)
 		return -ENOMEM;
 
-	if (readn(FD(evsel, cpu, thread), &count, nv * sizeof(u64)) < 0)
+	if (readn(FD(evsel, cpu, thread), &count, nv * sizeof(u64)) <= 0)
 		return -errno;
 
 	perf_evsel__compute_deltas(evsel, cpu, thread, &count);
-- 
1.9.1

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

Page 1 of 3  [1] 2 3  Next page →

Back to top | Article view | linux.kernel

csiph-web