x86: Intel Cache Allocation Technology support

arch/x86/include/asm/cacheqe.h new file +10 -0

+
+}
+
+static inline void cacheqe_sched_out(struct task_struct *task)
+{
+	unsigned int l, h;
+
+	if (!cqe_genable)
+		return;
+

arch/x86/kernel/cpu/cacheqe.c new file +9 -0

+
+/* ccmap maintains 1:1 mapping between CLOSid and cbm.*/
+
+static struct closcbm_map *ccmap;
+static struct cacheqe_subsys_info *cqess_info;
+
+char hsw_brandstrs[5][64] = {
+			"Intel(R) Xeon(R)  CPU E5-2658 v3  @  2.20GHz",
+			"Intel(R) Xeon(R)  CPU E5-2648L v3  @  1.80GHz",

arch/x86/kernel/cpu/cacheqe.c new file +11 -0

+
+	for (i = 0; i < 5; i++)
+		if (!strcmp(hsw_brandstrs[i], c->x86_model_id)) {
+			c->x86_cqe_closs = 4;
+			c->x86_cqe_cbmlength = 20;
+			return true;
+		}
+
+	return false;
+
+}

arch/x86/kernel/cpu/cacheqe.c new file +6 -0

+ * Called with the cqe_group_mutex held.
+ */
+
+static int cqe_alloc_closid(struct cacheqe *cq)
+{
+	unsigned int tempid;

arch/x86/kernel/cpu/cacheqe.c new file +21 -0

+static int validate_cbm(struct cacheqe *cq, unsigned long cbmvalue)
+{
+	struct cacheqe *par, *c;
+	struct cgroup_subsys_state *css;
+
+	if (!cbm_minbits(cbmvalue) || !cbm_iscontiguous(cbmvalue)) {
+		pr_info("CQE error: minimum bits not set or non contiguous mask\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Needs to be a subset of its parent.
+	 */
+	par = parent_cqe(cq);
+
+	if (!bitmap_subset(&cbmvalue, par->cbm, MAX_CBM_LENGTH))
+		return -EINVAL;
+
+	rcu_read_lock();
+
+	/*

arch/x86/kernel/cpu/cacheqe.c new file +15 -0

+static bool cbm_search(unsigned long cbm, int *closid)
+{
+
+	int maxid = boot_cpu_data.x86_cqe_closs;
+	unsigned int i;
+
+	for (i = 0; i < maxid; i++)
+		if (bitmap_equal(&cbm, &ccmap[i].cbm, MAX_CBM_LENGTH)) {
+			*closid = i;
+			return true;
+		}
+
+	return false;
+
+}

arch/x86/kernel/cpu/common.c +1 -0

index 4b4f78c..a9b277a 100644
@@ -633,6 +633,27 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 		c->x86_capability[9] = ebx;
 	}
 
+/* Additional Intel-defined flags: level 0x00000010 */

arch/x86/kernel/cpu/common.c +2 -0

+		Say N if unsure.
+
 config PROC_PID_CPUSET
 	bool "Include legacy /proc/<pid>/cpuset file"
 	depends on CPUSETS

kernel/sched/core.c +0 -0

index 240157c..afa2897 100644

kernel/sched/core.c +0 -0

 	perf_event_task_sched_in(prev, current);
 	finish_lock_switch(rq, prev);
 	finish_arch_post_lock_switch();

kernel/sched/sched.h +0 -0

index 24156c84..79b9ff6 100644

arch/x86/include/asm/cacheqe.h line 22

Quoted 4 lines · Vikas Shivappa

arch/x86/include/asm/cacheqe.h new file +4 -0

+/* maximum possible cbm length */
+#define MAX_CBM_LENGTH                 32
+
+#define IA32_CBMMAX_MASK(x)            (0xffffffff & (~((u64)(1 << x) - 1)))

Unused define.

Review

Quoted 3 lines

(quoted diff) +2 -0

+
+#define IA32_CBM_MASK                          0xffffffff

(~0U) ?

arch/x86/include/asm/cacheqe.h line 5

Quoted 6 lines

arch/x86/include/asm/cacheqe.h new file +5 -0

@@ -0,0 +1,144 @@
+static inline void cacheqe_sched_in(struct task_struct *task)
+{
+	struct cacheqe *cq;
+	unsigned int clos;
+	unsigned int l, h;

These variable names suck. You say in the comment below:

write the PQR for the proc.

So what is it now? clos or pqr or what?

IA32_PQR_ASSOC is the MSR name and bit 0-9 is RMID and bit 32-63 is COS

But you create random names just because it makes it harder to map the code to the SDM or is there some other sensible reason I'm missing here?

arch/x86/include/asm/cacheqe.h line 85

Quoted 3 lines

arch/x86/include/asm/cacheqe.h new file +3 -0

+
+	if (!cqe_genable)
+		return;

This wants to be a static key.

Review line 9

Quoted 2 lines

(quoted diff) +1 -0

+	rdmsr(IA32_PQR_ASSOC, l, h);

Why on earth do we want to read an MSR on every context switch? What's wrong with having

DEFINE_PER_CPU(u64, cacheqe_pqr);

	u64 next_pqr, cur_pqr = this_cpu_read(cacheqe_pqr);

	cq = task_cacheqe(task);
	next_pqr = cq ? cq->pqr : 0;

	if (next_pqr != cur_pqr) {
	   	wrmsrl(IA32_PQR_ASSOC, next_pqr);

		this_cpu_write(cacheqe_pqr, next_pqr);
	}

That saves the whole idiocity of reading MSRs on schedule in and schedule out. In fact it makes the schedule_out part completely superfluous.

arch/x86/include/asm/cacheqe.h line 101

Quoted 13 lines

arch/x86/include/asm/cacheqe.h new file +13 -0

+	rcu_read_lock();
+	cq = task_cacheqe(task);
+
+	if (cq == NULL || cq->clos == h) {
+		rcu_read_unlock();
+		return;
+	}
+
+	clos = cq->clos;
+
+	/*
+	 * After finding the cacheqe of the task , write the PQR for the proc.
+	 * We are assuming the current core is the one its scheduled to.

You are in the middle of the scheduler. So what needs to be assumed here?

arch/x86/include/asm/cacheqe.h line 102

Quoted 1 line

arch/x86/include/asm/cacheqe.h new file +1 -0

+	 * In unified scheduling , write the PQR each time.

What the heck is unified scheduling?

arch/x86/include/asm/cacheqe.h line 108

Quoted 6 lines

arch/x86/include/asm/cacheqe.h new file +6 -0

+	 */
+	wrmsr(IA32_PQR_ASSOC, l, clos);
+	rcu_read_unlock();
+
+	CQE_DEBUG(("schedule in clos :0x%x,task cpu:%u, currcpu: %u,pid:%u\n",
+		       clos, task_cpu(task), smp_processor_id(), task->pid));

A printk in the guts of the sched_switch function? Ever heard about trace points?

Quoted 10 lines

arch/x86/include/asm/cacheqe.h new file +10 -0

+
+}
+
+static inline void cacheqe_sched_out(struct task_struct *task)
+{
+	unsigned int l, h;
+
+	if (!cqe_genable)
+		return;
+

arch/x86/include/asm/cacheqe.h line 128

Quoted 10 lines

arch/x86/include/asm/cacheqe.h new file +10 -0

+	rdmsr(IA32_PQR_ASSOC, l, h);
+
+	if (h == 0)
+		return;
+
+	/*
+	  *After finding the cacheqe of the task , write the PQR for the proc.
+	 * We are assuming the current core is the one its scheduled to.
+	 * Write zero when scheduling out so that we get a more accurate
+	 * cache allocation.

This comment makes no sense at all. But well, this does not make sense anyway.

arch/x86/kernel/cpu/Makefile CFLAGS_REMOVE_common.o = -pg · line 13

Quoted 3 lines

arch/x86/kernel/cpu/Makefile +3 -0

+ifdef CONFIG_CACHEQE_DEBUG
+CFLAGS_cacheqe.o := -DDEBUG
+endif

Certainly NOT!

arch/x86/kernel/cpu/cacheqe.c line 3

Quoted 6 lines

arch/x86/kernel/cpu/cacheqe.c new file +4 -0

+++ b/arch/x86/kernel/cpu/cacheqe.c

@@ -0,0 +1,487 @@
+
+/*
+ *  kernel/cacheqe.c

What's the point of the file name here? Especially if the file is not there at all.

arch/x86/kernel/cpu/cacheqe.c line 27

Quoted 6 lines

arch/x86/kernel/cpu/cacheqe.c new file +6 -0

+#include <asm/cacheqe.h>
+
+struct cacheqe root_cqe_group;
+static DEFINE_MUTEX(cqe_group_mutex);
+
+bool cqe_genable;

This wants to be readmostly, if it's a global, but we really want a static key for this.

Quoted 9 lines

arch/x86/kernel/cpu/cacheqe.c new file +9 -0

+
+/* ccmap maintains 1:1 mapping between CLOSid and cbm.*/
+
+static struct closcbm_map *ccmap;
+static struct cacheqe_subsys_info *cqess_info;
+
+char hsw_brandstrs[5][64] = {
+			"Intel(R) Xeon(R)  CPU E5-2658 v3  @  2.20GHz",
+			"Intel(R) Xeon(R)  CPU E5-2648L v3  @  1.80GHz",

arch/x86/kernel/cpu/cacheqe.c line 46

Quoted 10 lines

arch/x86/kernel/cpu/cacheqe.c new file +10 -0

+			"Intel(R) Xeon(R)  CPU E5-2628L v3  @  2.00GHz",
+			"Intel(R) Xeon(R)  CPU E5-2618L v3  @  2.30GHz",
+			"Intel(R) Xeon(R)  CPU E5-2608L v3  @  2.00GHz"
+};
+
+#define cacheqe_for_each_child(child_cq, pos_css, parent_cq)		\
+	css_for_each_child((pos_css),	\
+	&(parent_cq)->css)
+
+#if CONFIG_CACHEQE_DEBUG

We really do NOT need another config option for this. See above.

arch/x86/kernel/cpu/cacheqe.c line 48

Quoted 1 line

arch/x86/kernel/cpu/cacheqe.c new file +1 -0

+/*DUMP the closid-cbm map.*/

Wow that comment is really informative.

arch/x86/kernel/cpu/cacheqe.c line 79

Quoted 12 lines

arch/x86/kernel/cpu/cacheqe.c new file +12 -0

+static inline bool cqe_enabled(struct cpuinfo_x86 *c)
+{
+
+	int i;
+
+	if (cpu_has(c, X86_FEATURE_CQE_L3))
+		return true;
+
+	/*
+	 * Hard code the checks and values for HSW SKUs.
+	 * Unfortunately! have to check against only these brand name strings.
+	 */

You must be kidding.

Quoted 16 lines

arch/x86/kernel/cpu/cacheqe.c new file +15 -0

+
+	for (i = 0; i < 5; i++)
+		if (!strcmp(hsw_brandstrs[i], c->x86_model_id)) {
+			c->x86_cqe_closs = 4;
+			c->x86_cqe_cbmlength = 20;
+			return true;
+		}
+
+	return false;
+
+}
+
+
+static int __init cqe_late_init(void)
+{

arch/x86/kernel/cpu/cacheqe.c line 100

Quoted 6 lines

arch/x86/kernel/cpu/cacheqe.c new file +6 -0

+
+	struct cpuinfo_x86 *c = &boot_cpu_data;
+	size_t sizeb;
+	int maxid = boot_cpu_data.x86_cqe_closs;
+
+	cqe_genable = false;

Right, we need to initialize bss storage.

arch/x86/kernel/cpu/cacheqe.c line 105

Quoted 6 lines

arch/x86/kernel/cpu/cacheqe.c new file +5 -0

+
+	/*
+	 * Need the cqe_genable hint helps decide if the
+	 * kernel has enabled cache allocation.
+	 */

What hint?

arch/x86/kernel/cpu/cacheqe.c line 116

Quoted 10 lines

arch/x86/kernel/cpu/cacheqe.c new file +10 -0

+	if (!cqe_enabled(c)) {
+
+		root_cqe_group.css.ss->disabled = 1;
+		return -ENODEV;
+
+	} else {
+
+		cqess_info =
+				kzalloc(sizeof(struct cacheqe_subsys_info),
+						 GFP_KERNEL);

Can you add a few more newlines so the code gets better readable?

arch/x86/kernel/cpu/cacheqe.c line 123

Quoted 7 lines

arch/x86/kernel/cpu/cacheqe.c new file +7 -0

+
+		if (!cqess_info)
+			return -ENOMEM;
+
+		sizeb = BITS_TO_LONGS(c->x86_cqe_closs) * sizeof(long);
+		cqess_info->closmap =
+				kzalloc(sizeb, GFP_KERNEL);

Sigh.

arch/x86/kernel/cpu/cacheqe.c line 134

Quoted 10 lines

arch/x86/kernel/cpu/cacheqe.c new file +10 -0

+		if (!cqess_info->closmap) {
+			kfree(cqess_info);
+			return -ENOMEM;
+		}
+
+		sizeb = maxid * sizeof(struct closcbm_map);
+		ccmap = kzalloc(sizeb, GFP_KERNEL);
+
+		if (!ccmap)
+			return -ENOMEM;

Leaks closmap and cqess_info.

arch/x86/kernel/cpu/cacheqe.c line 151

Quoted 3 lines

arch/x86/kernel/cpu/cacheqe.c new file +2 -0

+		barrier();
+		cqe_genable = true;

What's the exact point of that barrier?

Review line 98

Quoted 2 lines

(quoted diff) +2 -0

+}
+

Stray newline. checkpatch.pl is optional, right?

arch/x86/kernel/cpu/cacheqe.c line 165

Quoted 5 lines

arch/x86/kernel/cpu/cacheqe.c new file +4 -0

+late_initcall(cqe_late_init);
+
+/*
+ * Allocates a new closid from unused list of closids.

There is no unused list.

Quoted 10 lines

arch/x86/kernel/cpu/cacheqe.c new file +10 -0

+ * Called with the cqe_group_mutex held.
+ */
+
+static int cqe_alloc_closid(struct cacheqe *cq)
+{
+	unsigned int tempid;
+	unsigned int maxid;
+	int err;
+
+	maxid = boot_cpu_data.x86_cqe_closs;

arch/x86/kernel/cpu/cacheqe.c line 181

Quoted 6 lines

arch/x86/kernel/cpu/cacheqe.c new file +6 -0

+
+	tempid = find_next_zero_bit(cqess_info->closmap, maxid, 0);
+
+	if (tempid == maxid) {
+		err = -ENOSPC;
+		goto closidallocfail;

What's the point of the goto? What's wrong with 'return -ENOSPC;' ?

arch/x86/kernel/cpu/cacheqe.c line 188

Quoted 7 lines

arch/x86/kernel/cpu/cacheqe.c new file +7 -0

+	}
+
+	set_bit(tempid, cqess_info->closmap);
+	ccmap[tempid].ref++;
+	cq->clos = tempid;
+
+	pr_debug("cqe : Allocated a directory.closid:%u\n", cq->clos);

And of course we have no idea whatfor this closid got allocated. Your debug info is just useless.

arch/x86/kernel/cpu/cacheqe.c line 207

Quoted 7 lines

arch/x86/kernel/cpu/cacheqe.c new file +6 -0

+static void cqe_free_closid(struct cacheqe *cq)
+{
+
+	pr_debug("cqe :Freeing closid:%u\n", cq->clos);
+
+	ccmap[cq->clos].ref--;

This wants a sanity check to catch ref == 0.

arch/x86/kernel/cpu/cacheqe.c line 223

Quoted 9 lines

arch/x86/kernel/cpu/cacheqe.c new file +8 -0

+/* Create a new cacheqe cgroup.*/
+static struct cgroup_subsys_state *
+cqe_css_alloc(struct cgroup_subsys_state *parent_css)
+{
+	struct cacheqe *parent = css_cacheqe(parent_css);
+	struct cacheqe *cq;
+
+	/* This is the call before the feature is detected */

Huch? We know at early boot that this is available. So why do you need this? The first call to this should never happen before the whole thing is initialized. If it happens, it's a design failure.

arch/x86/kernel/cpu/cacheqe.c line 231

Quoted 8 lines

arch/x86/kernel/cpu/cacheqe.c new file +8 -0

+	if (!parent) {
+		root_cqe_group.clos = 0;
+		return &root_cqe_group.css;
+	}
+
+	/* To check if cqe is enabled.*/
+	if (!cqe_genable)
+		return ERR_PTR(-ENODEV);

So we first check for !parent and return &root_cqe_group.css and later we return an error pointer? Really consistent behaviour.

arch/x86/kernel/cpu/cacheqe.c line 255

Quoted 2 lines

arch/x86/kernel/cpu/cacheqe.c new file +1 -0

+/* Destroy an existing CAT cgroup.*/

If you would spend some time on providing real comments instead of documenting the obvious ...

arch/x86/kernel/cpu/cacheqe.c line 267

Quoted 12 lines

arch/x86/kernel/cpu/cacheqe.c new file +12 -0

+static void cqe_css_free(struct cgroup_subsys_state *css)
+{
+	struct cacheqe *cq = css_cacheqe(css);
+	int len = boot_cpu_data.x86_cqe_cbmlength;
+
+	pr_debug("cqe : In cacheqe_css_free\n");
+
+	mutex_lock(&cqe_group_mutex);
+
+	/* Reset the CBM for the cgroup.Should be all 1s by default !*/
+
+	wrmsrl(CQECBMMSR(cq->clos), ((1 << len) - 1));

So that's writing to IA32_L3_MASK_n, right? Pretty obvious name:
CQECBMMSR.

And the write should set ALL bits to 1 not just the enabled ones if I understand the SDM correctly and what you said in your own comment above.

Aside of that what guarantees that all references to this L3_MASK are gone? You allow the sharing of COS ids. Your whole refcounting scheme is a trainwreck. And when the COS id is not longer in use, there is no point in writing the MSR at all. It does not matter whats in there if nothing ever uses it.

arch/x86/kernel/cpu/cacheqe.c line 278

Quoted 12 lines

arch/x86/kernel/cpu/cacheqe.c new file +11 -0

+	cqe_free_closid(cq);
+	kfree(cq);
+
+	mutex_unlock(&cqe_group_mutex);
+
+}
+
+/*
+ * Called during do_exit() syscall during a task exit.
+ * This assumes that the thread is running on the current
+ * cpu.

Your assumptions about running on current cpu are interesting. On which CPU is a task supposed to run, when it is on a CPU? Surely on some other cpu, right?

arch/x86/kernel/cpu/cacheqe.c line 286

Quoted 8 lines

arch/x86/kernel/cpu/cacheqe.c new file +8 -0

+ */
+
+static void cqe_exit(struct cgroup_subsys_state *css,
+			    struct cgroup_subsys_state *old_css,
+			    struct task_struct *task)
+{
+
+	cacheqe_sched_out(task);

And whats the point here? Nothing at all. You need to clean the associated data and then the exit schedule will clean up the PQR automatically.

arch/x86/kernel/cpu/cacheqe.c line 292

Quoted 6 lines

arch/x86/kernel/cpu/cacheqe.c new file +6 -0

+
+}
+
+static inline bool cbm_minbits(unsigned long var)
+{
+

Your supply of useless newlines is endless.

arch/x86/kernel/cpu/cacheqe.c line 303

Quoted 11 lines

arch/x86/kernel/cpu/cacheqe.c new file +11 -0

+	unsigned long i;
+
+	/*Minimum of 2 bits must be set.*/
+
+	i = var & (var - 1);
+	if (!i || !var)
+		return false;
+
+	return true;
+
+}

What's wrong with using bitmap functions consistently? So this test would simply become:

bitmap_weight(map, nrbits) < 2

arch/x86/kernel/cpu/cacheqe.c line 310

Quoted 7 lines

arch/x86/kernel/cpu/cacheqe.c new file +7 -0

+
+/*
+ * Tests if only contiguous bits are set.
+ */
+
+static inline bool cbm_iscontiguous(unsigned long var)
+{

This one here can be implemented with existing bitmap functions as well.

Would be too simple and too obvious to understand, right?

arch/x86/kernel/cpu/cacheqe.c line 339

Quoted 5 lines

arch/x86/kernel/cpu/cacheqe.c new file +5 -0

+static int cqe_cbm_read(struct seq_file *m, void *v)
+{
+	struct cacheqe *cq = css_cacheqe(seq_css(m));
+
+	pr_debug("cqe : In cqe_cqemode_read\n");

We really need a debug printk for a seqfile read function, right?

arch/x86/kernel/cpu/cacheqe.c line 340

Quoted 1 line

arch/x86/kernel/cpu/cacheqe.c new file +1 -0

+	seq_printf(m, "0x%x\n", (unsigned int)*(cq->cbm));

seq_bitmap()

Quoted 21 lines

arch/x86/kernel/cpu/cacheqe.c new file +21 -0

+static int validate_cbm(struct cacheqe *cq, unsigned long cbmvalue)
+{
+	struct cacheqe *par, *c;
+	struct cgroup_subsys_state *css;
+
+	if (!cbm_minbits(cbmvalue) || !cbm_iscontiguous(cbmvalue)) {
+		pr_info("CQE error: minimum bits not set or non contiguous mask\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Needs to be a subset of its parent.
+	 */
+	par = parent_cqe(cq);
+
+	if (!bitmap_subset(&cbmvalue, par->cbm, MAX_CBM_LENGTH))
+		return -EINVAL;
+
+	rcu_read_lock();
+
+	/*

arch/x86/kernel/cpu/cacheqe.c line 376

Quoted 10 lines

arch/x86/kernel/cpu/cacheqe.c new file +10 -0

+	 * Each of children should be a subset of the mask.
+	 */
+
+	cacheqe_for_each_child(c, css, cq) {
+		c = css_cacheqe(css);
+		if (!bitmap_subset(c->cbm, &cbmvalue, MAX_CBM_LENGTH)) {
+			pr_debug("cqe : Children's cbm not a subset\n");
+			return -EINVAL;
+		}
+	}

So now you have 3 error exits here. One with pr_info, one with out and one with pr_debug. And of course the most obvious to figure out has a pr_info. Useful.

Quoted 15 lines

arch/x86/kernel/cpu/cacheqe.c new file +15 -0

+static bool cbm_search(unsigned long cbm, int *closid)
+{
+
+	int maxid = boot_cpu_data.x86_cqe_closs;
+	unsigned int i;
+
+	for (i = 0; i < maxid; i++)
+		if (bitmap_equal(&cbm, &ccmap[i].cbm, MAX_CBM_LENGTH)) {
+			*closid = i;
+			return true;
+		}
+
+	return false;
+
+}

arch/x86/kernel/cpu/cacheqe.c line 408

Quoted 10 lines

arch/x86/kernel/cpu/cacheqe.c new file +10 -0

+
+static int cqe_cbm_write(struct cgroup_subsys_state *css,
+				 struct cftype *cft, u64 cbmvalue)
+{
+	struct cacheqe *cq = css_cacheqe(css);
+	ssize_t err = 0;
+	unsigned long cbm;
+	unsigned int closid;
+
+	pr_debug("cqe : In cqe_cbm_write\n");

Groan.

arch/x86/kernel/cpu/cacheqe.c line 417

Quoted 8 lines

arch/x86/kernel/cpu/cacheqe.c new file +8 -0

+	if (!cqe_genable)
+		return -ENODEV;
+
+	if (cq == &root_cqe_group || !cq)
+		return -EPERM;
+
+	/*
+	* Need global mutex as cbm write may allocate the closid.

You need to protect against changes on all ends, not only allocating a closid.

Quoted 9 lines

arch/x86/kernel/cpu/common.c +1 -0

index 4b4f78c..a9b277a 100644
@@ -633,6 +633,27 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 		c->x86_capability[9] = ebx;
 	}
 
+/* Additional Intel-defined flags: level 0x00000010 */

arch/x86/kernel/cpu/common.c void get_cpu_cap(struct cpuinfo_x86 *c) · line 646

Quoted 10 lines

arch/x86/kernel/cpu/common.c +10 -0

+	if (c->cpuid_level >= 0x00000010) {
+		u32 eax, ebx, ecx, edx;
+
+		cpuid_count(0x00000010, 0, &eax, &ebx, &ecx, &edx);
+
+		c->x86_capability[10] = ebx;
+
+		if (cpu_has(c, X86_FEATURE_CQE_L3)) {
+
+			u32 eax, ebx, ecx, edx;

If you add another indentation level, you can define the same variables once more.

arch/x86/kernel/cpu/common.c void get_cpu_cap(struct cpuinfo_x86 *c)

Quoted 7 lines

arch/x86/kernel/cpu/common.c +7 -0

+
+			cpuid_count(0x00000010, 1, &eax, &ebx, &ecx, &edx);
+
+			c->x86_cqe_closs = (edx & 0xffff) + 1;
+			c->x86_cqe_cbmlength = (eax & 0xf) + 1;
+config CACHEQE_DEBUG
+  bool "Cache QoS Enforcement cgroup subsystem debug"

This needs to go away. The debugging you provide is beyond useless. We want proper tracepoints for that not random debug printks with no value.

arch/x86/kernel/cpu/common.c void get_cpu_cap(struct cpuinfo_x86 *c) · line 659

Quoted 6 lines

arch/x86/kernel/cpu/common.c +6 -0

+  depends on X86 || X86_64
+  help
+    This option provides framework to allocate Cache cache lines when
+    applications fill cache.
+    This can be used by users to configure how much cache that can be
+    allocated to different PIDs.Enables debug

Copy and paste is wonderful, right?

Quoted 10 lines

arch/x86/kernel/cpu/common.c +2 -0

+		Say N if unsure.
+
 config PROC_PID_CPUSET
 	bool "Include legacy /proc/<pid>/cpuset file"
 	depends on CPUSETS

kernel/sched/core.c +0 -0

index 240157c..afa2897 100644
@@ -2215,7 +2215,7 @@ prepare_task_switch(struct rq *rq, struct task_struct *prev,

kernel/sched/core.c prepare_task_switch(struct rq *rq, struct task_struct *prev, · line 2218

Quoted 5 lines

kernel/sched/core.c +1 -1

 	perf_event_task_sched_out(prev, next);
 	fire_sched_out_preempt_notifiers(prev, next);
 	prepare_lock_switch(rq, next);
-	prepare_arch_switch(next);
+	prepare_arch_switch(prev);

Great. You just broke all architectures which implement prepare_arch_switch(). Congratulations!

kernel/sched/core.c static void finish_task_switch(struct rq *rq, struct task_struct *prev) · line 2257

Quoted 9 lines

kernel/sched/core.c +1 -1

 }
 
 /**

@@ -2254,7 +2254,7 @@ static void finish_task_switch(struct rq *rq, struct task_struct *prev)
 	 */
 	prev_state = prev->state;
 	vtime_task_switch(prev);
-	finish_arch_switch(prev);
+	finish_arch_switch(current);

Ditto.

Quoted 8 lines

kernel/sched/core.c +0 -0

 	perf_event_task_sched_in(prev, current);
 	finish_lock_switch(rq, prev);
 	finish_arch_post_lock_switch();

kernel/sched/sched.h +0 -0

index 24156c84..79b9ff6 100644

kernel/sched/sched.h static inline int task_on_rq_migrating(struct task_struct *p) · line 971

Quoted 7 lines

kernel/sched/sched.h +4 -0

@@ -965,12 +965,36 @@ static inline int task_on_rq_migrating(struct task_struct *p)
 	return p->on_rq == TASK_ON_RQ_MIGRATING;
 }
 
+#ifdef CONFIG_X86_64
+#ifdef CONFIG_CGROUP_CACHEQE
+
+#include <asm/cacheqe.h>

We surely not put x86 specific nonsense in the core scheduler code. Did you ever come up with the idea to look how this is used by other architectures? Obviously not.

I have no idea how Matts reviewed tag got onto this masterpiece of trainwreck engineering and I better dont want to know.

Thanks,

tglx

Quoted 20 lines

arch/x86/kernel/cpu/cacheqe.c line 46

Quoted 13 lines

arch/x86/kernel/cpu/cacheqe.c new file +13 -0

+char hsw_brandstrs[5][64] = {
+			"Intel(R) Xeon(R)  CPU E5-2658 v3  @  2.20GHz",
+			"Intel(R) Xeon(R)  CPU E5-2648L v3  @  1.80GHz",
+			"Intel(R) Xeon(R)  CPU E5-2628L v3  @  2.00GHz",
+			"Intel(R) Xeon(R)  CPU E5-2618L v3  @  2.30GHz",
+			"Intel(R) Xeon(R)  CPU E5-2608L v3  @  2.00GHz"
+};
+
+#define cacheqe_for_each_child(child_cq, pos_css, parent_cq)		\
+	css_for_each_child((pos_css),	\
+	&(parent_cq)->css)
+
+#if CONFIG_CACHEQE_DEBUG

We really do NOT need another config option for this. See above.

Quoted 1 line

+/*DUMP the closid-cbm map.*/

Wow that comment is really informative.

Quoted 14 lines

arch/x86/kernel/cpu/cacheqe.c line 79

Quoted 12 lines

arch/x86/kernel/cpu/cacheqe.c new file +12 -0

+static inline bool cqe_enabled(struct cpuinfo_x86 *c)
+{
+
+	int i;
+
+	if (cpu_has(c, X86_FEATURE_CQE_L3))
+		return true;
+
+	/*
+	 * Hard code the checks and values for HSW SKUs.
+	 * Unfortunately! have to check against only these brand name strings.
+	 */

You must be kidding.

No. Will have a microcode version check as well in next patch after thats confirmed from h/w team

Quoted 35 lines · Vikas Shivappa

Quoted 20 lines

arch/x86/kernel/cpu/cacheqe.c line 46

Quoted 13 lines

arch/x86/kernel/cpu/cacheqe.c new file +13 -0

+char hsw_brandstrs[5][64] = {
+			"Intel(R) Xeon(R)  CPU E5-2658 v3  @  2.20GHz",
+			"Intel(R) Xeon(R)  CPU E5-2648L v3  @  1.80GHz",
+			"Intel(R) Xeon(R)  CPU E5-2628L v3  @  2.00GHz",
+			"Intel(R) Xeon(R)  CPU E5-2618L v3  @  2.30GHz",
+			"Intel(R) Xeon(R)  CPU E5-2608L v3  @  2.00GHz"
+};
+
+#define cacheqe_for_each_child(child_cq, pos_css, parent_cq)		\
+	css_for_each_child((pos_css),	\
+	&(parent_cq)->css)
+
+#if CONFIG_CACHEQE_DEBUG

We really do NOT need another config option for this. See above.

Quoted 1 line

+/*DUMP the closid-cbm map.*/

Wow that comment is really informative.

Quoted 14 lines

arch/x86/kernel/cpu/cacheqe.c line 79

Quoted 12 lines

arch/x86/kernel/cpu/cacheqe.c new file +12 -0

+static inline bool cqe_enabled(struct cpuinfo_x86 *c)
+{
+
+	int i;
+
+	if (cpu_has(c, X86_FEATURE_CQE_L3))
+		return true;
+
+	/*
+	 * Hard code the checks and values for HSW SKUs.
+	 * Unfortunately! have to check against only these brand name strings.
+	 */

You must be kidding.

Quoted 2 lines

No. Will have a microcode version check as well in next patch after thats confirmed from h/w team

Checking random brand strings? Please don't tell me those are not really immutable either...

And what happens with newer models appearing? Add more brand strings? Lovely stuff, that.

Well, since you're talking to the h/w team: can they give you some immutable bit somewhere which you can check instead of looking at brand strings? This'll be a sane solution, actually.

Signature · 5 lines

Regards/Gruss,
   Boris.

Sent from a fat crate under my desk. Formatting is fine.
--

Quoted 13 lines · Thomas Gleixner

Review

Quoted 1 line · Vikas Shivappa

(quoted diff) +1 -0

+	rdmsr(IA32_PQR_ASSOC, l, h);

Why on earth do we want to read an MSR on every context switch? What's wrong with having

DEFINE_PER_CPU(u64, cacheqe_pqr);

	u64 next_pqr, cur_pqr = this_cpu_read(cacheqe_pqr);

	cq = task_cacheqe(task);
	next_pqr = cq ? cq->pqr : 0;

Quoted 2 lines

	if (next_pqr != cur_pqr) {
	   	wrmsrl(IA32_PQR_ASSOC, next_pqr);

when cqm(cache monitoring) and cqe are both running together , cant corrupt the low 32 bits, the rdmsr was a fix for that.
If there are better alternatives at low cost , certainly acceptable

Quoted 13 lines

arch/x86/kernel/cpu/cacheqe.c line 223

Quoted 8 lines

arch/x86/kernel/cpu/cacheqe.c new file +8 -0

+/* Create a new cacheqe cgroup.*/
+static struct cgroup_subsys_state *
+cqe_css_alloc(struct cgroup_subsys_state *parent_css)
+{
+	struct cacheqe *parent = css_cacheqe(parent_css);
+	struct cacheqe *cq;
+
+	/* This is the call before the feature is detected */

Huch? We know at early boot that this is available. So why do you need this? The first call to this should never happen before the whole thing is initialized. If it happens, it's a design failure.

Quoted 11 lines

arch/x86/kernel/cpu/cacheqe.c line 231

Quoted 8 lines

arch/x86/kernel/cpu/cacheqe.c new file +8 -0

+	if (!parent) {
+		root_cqe_group.clos = 0;
+		return &root_cqe_group.css;
+	}
+
+	/* To check if cqe is enabled.*/
+	if (!cqe_genable)
+		return ERR_PTR(-ENODEV);

So we first check for !parent and return &root_cqe_group.css and later we return an error pointer? Really consistent behaviour.

this is due to how the cgroup init works.

Quoted 13 lines · Thomas Gleixner

Review

Quoted 1 line · Vikas Shivappa

(quoted diff) +1 -0

+	rdmsr(IA32_PQR_ASSOC, l, h);

Why on earth do we want to read an MSR on every context switch? What's wrong with having

DEFINE_PER_CPU(u64, cacheqe_pqr);

	u64 next_pqr, cur_pqr = this_cpu_read(cacheqe_pqr);

	cq = task_cacheqe(task);
	next_pqr = cq ? cq->pqr : 0;

Quoted 2 lines

	if (next_pqr != cur_pqr) {
	   	wrmsrl(IA32_PQR_ASSOC, next_pqr);

when cqm(cache monitoring) and cqe are both running together , cant corrupt the low 32 bits, the rdmsr was a fix for that. If there are better alternatives at low cost , certainly acceptable

Quoted 12 lines

arch/x86/kernel/cpu/cacheqe.c line 223

Quoted 7 lines

arch/x86/kernel/cpu/cacheqe.c new file +7 -0

+/* Create a new cacheqe cgroup.*/ static struct 
+cgroup_subsys_state * cqe_css_alloc(struct cgroup_subsys_state 
+*parent_css) {
+	struct cacheqe *parent = css_cacheqe(parent_css);
+	struct cacheqe *cq;
+
+	/* This is the call before the feature is detected */

Huch? We know at early boot that this is available. So why do you need this? The first call to this should never happen before the whole thing is initialized. If it happens, it's a design failure.

Quoted 11 lines

arch/x86/kernel/cpu/cacheqe.c line 231

Quoted 8 lines

arch/x86/kernel/cpu/cacheqe.c new file +8 -0

+	if (!parent) {
+		root_cqe_group.clos = 0;
+		return &root_cqe_group.css;
+	}
+
+	/* To check if cqe is enabled.*/
+	if (!cqe_genable)
+		return ERR_PTR(-ENODEV);

So we first check for !parent and return &root_cqe_group.css and later we return an error pointer? Really consistent behaviour.

this is due to how the cgroup init works.

arch/x86/kernel/cpu/cacheqe.c line 151

Quoted 2 lines

arch/x86/kernel/cpu/cacheqe.c new file +2 -0

+		barrier();
+		cqe_genable = true;

What's the exact point of that barrier?

arch/x86/kernel/cpu/cacheqe.c line 310

Quoted 7 lines

arch/x86/kernel/cpu/cacheqe.c new file +7 -0

+
+/*
+ * Tests if only contiguous bits are set.
+ */
+
+static inline bool cbm_iscontiguous(unsigned long var)
+{

This one here can be implemented with existing bitmap functions as well.

Something like this?

	first_bit = find_next_bit(map, nr_bits, -1);
	zero_bit = find_next_zero_bit(map, nr_bits, first_bit);

	if (find_next_bit(map, nr_bits, zero_bit) < nr_bits)
		return -EINVAL;	/* non-contiguous bits */

arch/x86/include/asm/cacheqe.h new file +9 -0

+
+}
+
+static inline void cacheqe_sched_out(struct task_struct *task) {
+	unsigned int l, h;
+
+	if (!cqe_genable)
+		return;
+

arch/x86/kernel/cpu/cacheqe.c new file +9 -0

+
+/* ccmap maintains 1:1 mapping between CLOSid and cbm.*/
+
+static struct closcbm_map *ccmap;
+static struct cacheqe_subsys_info *cqess_info;
+
+char hsw_brandstrs[5][64] = {
+			"Intel(R) Xeon(R)  CPU E5-2658 v3  @  2.20GHz",
+			"Intel(R) Xeon(R)  CPU E5-2648L v3  @  1.80GHz",

arch/x86/kernel/cpu/cacheqe.c new file +10 -0

+
+	for (i = 0; i < 5; i++)
+		if (!strcmp(hsw_brandstrs[i], c->x86_model_id)) {
+			c->x86_cqe_closs = 4;
+			c->x86_cqe_cbmlength = 20;
+			return true;
+		}
+
+	return false;
+

arch/x86/kernel/cpu/cacheqe.c new file +5 -0

+ * Called with the cqe_group_mutex held.
+ */
+
+static int cqe_alloc_closid(struct cacheqe *cq) {
+	unsigned int tempid;

arch/x86/kernel/cpu/cacheqe.c new file +20 -0

+static int validate_cbm(struct cacheqe *cq, unsigned long cbmvalue) {
+	struct cacheqe *par, *c;
+	struct cgroup_subsys_state *css;
+
+	if (!cbm_minbits(cbmvalue) || !cbm_iscontiguous(cbmvalue)) {
+		pr_info("CQE error: minimum bits not set or non contiguous mask\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Needs to be a subset of its parent.
+	 */
+	par = parent_cqe(cq);
+
+	if (!bitmap_subset(&cbmvalue, par->cbm, MAX_CBM_LENGTH))
+		return -EINVAL;
+
+	rcu_read_lock();
+
+	/*

arch/x86/kernel/cpu/cacheqe.c new file +13 -0

+static bool cbm_search(unsigned long cbm, int *closid) {
+
+	int maxid = boot_cpu_data.x86_cqe_closs;
+	unsigned int i;
+
+	for (i = 0; i < maxid; i++)
+		if (bitmap_equal(&cbm, &ccmap[i].cbm, MAX_CBM_LENGTH)) {
+			*closid = i;
+			return true;
+		}
+
+	return false;
+

(quoted diff) +0 -0

b/arch/x86/kernel/cpu/common.c index 4b4f78c..a9b277a 100644

arch/x86/kernel/cpu/common.c +1 -0

@@ -633,6 +633,27 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 		c->x86_capability[9] = ebx;
 	}
 
+/* Additional Intel-defined flags: level 0x00000010 */

arch/x86/kernel/cpu/common.c +2 -0

+		Say N if unsure.
+
 config PROC_PID_CPUSET
 	bool "Include legacy /proc/<pid>/cpuset file"
 	depends on CPUSETS

kernel/sched/core.c index renamed from kernel/sched/core.c +0 -0

240157c..afa2897 100644

kernel/sched/core.c +0 -0

 	perf_event_task_sched_in(prev, current);
 	finish_lock_switch(rq, prev);
 	finish_arch_post_lock_switch();

kernel/sched/sched.h index renamed from kernel/sched/sched.h +0 -0

24156c84..79b9ff6 100644

arch/x86/include/asm/cacheqe.h new file +9 -0

+
+}
+
+static inline void cacheqe_sched_out(struct task_struct *task) {
+	unsigned int l, h;
+
+	if (!cqe_genable)
+		return;
+

arch/x86/kernel/cpu/cacheqe.c new file +9 -0

+
+/* ccmap maintains 1:1 mapping between CLOSid and cbm.*/
+
+static struct closcbm_map *ccmap;
+static struct cacheqe_subsys_info *cqess_info;
+
+char hsw_brandstrs[5][64] = {
+			"Intel(R) Xeon(R)  CPU E5-2658 v3  @  2.20GHz",
+			"Intel(R) Xeon(R)  CPU E5-2648L v3  @  1.80GHz",

arch/x86/kernel/cpu/cacheqe.c new file +10 -0

+
+	for (i = 0; i < 5; i++)
+		if (!strcmp(hsw_brandstrs[i], c->x86_model_id)) {
+			c->x86_cqe_closs = 4;
+			c->x86_cqe_cbmlength = 20;
+			return true;
+		}
+
+	return false;
+

arch/x86/kernel/cpu/cacheqe.c new file +5 -0

+ * Called with the cqe_group_mutex held.
+ */
+
+static int cqe_alloc_closid(struct cacheqe *cq) {
+	unsigned int tempid;

arch/x86/kernel/cpu/cacheqe.c new file +20 -0

+static int validate_cbm(struct cacheqe *cq, unsigned long cbmvalue) {
+	struct cacheqe *par, *c;
+	struct cgroup_subsys_state *css;
+
+	if (!cbm_minbits(cbmvalue) || !cbm_iscontiguous(cbmvalue)) {
+		pr_info("CQE error: minimum bits not set or non contiguous mask\n");
+		return -EINVAL;
+	}
+
+	/*
+	 * Needs to be a subset of its parent.
+	 */
+	par = parent_cqe(cq);
+
+	if (!bitmap_subset(&cbmvalue, par->cbm, MAX_CBM_LENGTH))
+		return -EINVAL;
+
+	rcu_read_lock();
+
+	/*

arch/x86/kernel/cpu/cacheqe.c new file +13 -0

+static bool cbm_search(unsigned long cbm, int *closid) {
+
+	int maxid = boot_cpu_data.x86_cqe_closs;
+	unsigned int i;
+
+	for (i = 0; i < maxid; i++)
+		if (bitmap_equal(&cbm, &ccmap[i].cbm, MAX_CBM_LENGTH)) {
+			*closid = i;
+			return true;
+		}
+
+	return false;
+

(quoted diff) +0 -0

b/arch/x86/kernel/cpu/common.c index 4b4f78c..a9b277a 100644

arch/x86/kernel/cpu/common.c +1 -0

@@ -633,6 +633,27 @@ void get_cpu_cap(struct cpuinfo_x86 *c)
 		c->x86_capability[9] = ebx;
 	}
 
+/* Additional Intel-defined flags: level 0x00000010 */